Fix/project scoped provider skills - #6661
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
vouch:unvouched
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
There was a problem hiding this comment.
Effect Service Conventions review: one finding — a new service capability is declared as an optional interface member and feature-detected at runtime, with a production fallback that exists only for partial test doubles. Everything else (namespace imports from effect/* subpaths, environment-based dependency acquisition in the drivers, no ManagedRuntime/runPromise in service code, Layer.succeed limited to test seams, error mapping reusing the existing typed persistence errors) matches the conventions.
Posted via Macroscope — Effect Service Conventions
There was a problem hiding this comment.
Effect Service Conventions review: one finding — a new service capability is declared as an optional interface member and feature-detected at runtime, with a production fallback that exists only for partial test doubles. Everything else (namespace imports from effect/* subpaths, environment-based dependency acquisition in the drivers, no ManagedRuntime/runPromise in service code, Layer.succeed limited to test seams, error mapping reusing the existing typed persistence errors) matches the conventions.
Posted via Macroscope — Effect Service Conventions
ApprovabilityVerdict: Skipped Macroscope did not run approvability analysis for this PR. Macroscope could not determine whether this PR modifies its approvability configuration, so the PR was not approved automatically. A PR that may change the rules that govern approval is never approved automatically. Not approved because:
|
abc0ffe to
9233756
Compare
There was a problem hiding this comment.
One finding: two ProjectionSnapshotQuery test doubles added for skill workspace resolution still implement only getShellSnapshot, which is no longer the method the production path calls.
Posted via Macroscope — Effect Service Conventions
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
39abc3f to
5447a24
Compare
There was a problem hiding this comment.
One finding: the new Grok inspect helper puts bare Error values in an Effect failure channel instead of a Schema.TaggedErrorClass with structured attributes. Everything else in scope (new SkillDiscovery / SkillWorkspaceCwds / OpenCodeCommands modules, driver wiring, contract additions, and the updated ProjectionSnapshotQuery shape/test doubles) follows the service, import, and dependency-acquisition conventions.
Posted via Macroscope — Effect Service Conventions
There was a problem hiding this comment.
One finding: the / composer menu still builds its skill: entries from the unfiltered provider inventory, so the cross-workspace leak this PR fixes for $ remains on the slash path (and that read is no longer covered by the memo deps).
Posted via Macroscope — UI Consistency
- Add shared skill discovery for Codex, Grok, and OpenCode - Include project hierarchy, compatibility roots, and collision precedence - Expose discovered skills in provider snapshots with focused tests
- discover provider skills across registered projects and worktrees - add Cursor support and align Grok and OpenCode roots with their harnesses - use a lightweight workspace projection query and cover multi-workspace behavior
Macroscope flagged four issues on the skill discovery work: - `getActiveWorkspaceCwds` was an optional service member, so `resolveSkillWorkspaceCwds` feature-detected it and otherwise fell back to the full shell hydration the method exists to avoid. It is now a required member; the fallback and the test-side non-null assertion are gone, and the projection test doubles declare it. - `normalizePosixSkillWorkspacePath` rewrote every backslash to `/`, so `/projects/foo\bar` and `/projects/foo/bar` shared a skill bag. A backslash is a legal POSIX filename character and now stays in the segment; Windows forms are already routed to the Windows normalizer. - `normalizeWindowsSkillWorkspacePath` let `..` pop the drive letter or UNC share root, so `C:\..\project` normalized to `project` and never matched the server's `sourceCwd`. The root is now a floor. - `discoverClaudeSkills` resolved a relative `CLAUDE_CONFIG_DIR` against the first workspace only, hiding later workspaces' user skills. Each workspace now contributes its resolved config root, deduplicated. Model: Claude Opus 5, harness: Claude Code. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Follow-up review found two problems with the previous commit. Macroscope: the `emptySkillProjectionLayer` doubles in `ProviderRegistry.test.ts` and `ProviderInstanceRegistryLive.test.ts` still stubbed only `getShellSnapshot`, so once the fallback was removed the probes called an undefined method and `Effect.ignoreCause` swallowed the defect — the path was silently dead. Both now stub `getActiveWorkspaceCwds`, as does the `Layer.mock` in `server.test.ts`. Bugbot: resolving a relative `CLAUDE_CONFIG_DIR` per workspace added 每 workspace's config dir as an untagged user root, and user skills are the baseline for every chat — so one project's config skills leaked into another's picker, and same-named entries collapsed under the shared user inventory key. A single shared config dir (the common case) stays untagged and global; when workspaces resolve to different dirs, each root is tagged with the cwd whose CLI would actually load it. Model: Claude Opus 5, harness: Claude Code. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Grok's $ and / pickers only scanned filesystem skill roots, so bundled skills and harness features never appeared. Query grok inspect --json for the skill inventory and capture ACP available_commands_update for the live slash menu, with filesystem fallback when inspect is missing. Made-with: Grok 4.6 (grok-build)
74a51cb to
9100794
Compare
|
|
||
| const adoptPendingAvailableCommands = (sessionId: string) => | ||
| Effect.gen(function* () { | ||
| const pending = yield* Ref.getAndSet(pendingAvailableCommandsRef, new Map()); |
There was a problem hiding this comment.
🟡 Medium acp/AcpSessionRuntime.ts:385
getAvailableCommands can remain stale after startup: an available_commands_update received between Ref.getAndSet(pendingAvailableCommandsRef, new Map()) and setting startStateRef to Started is buffered after the drain and is never adopted. Coordinate the pending-map drain with the startup-state transition atomically, or perform a second drain after transitioning to Started.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/provider/acp/AcpSessionRuntime.ts around line 385:
`getAvailableCommands` can remain stale after startup: an `available_commands_update` received between `Ref.getAndSet(pendingAvailableCommandsRef, new Map())` and setting `startStateRef` to `Started` is buffered after the drain and is never adopted. Coordinate the pending-map drain with the startup-state transition atomically, or perform a second drain after transitioning to `Started`.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 536153c. Configure here.
| const next = new Map(pending); | ||
| next.set(notification.sessionId, commands); | ||
| return next; | ||
| }); |
There was a problem hiding this comment.
ACP command adopt race window
Medium Severity
Fresh-start available_commands_update handling buffers into pendingAvailableCommandsRef while status is still Starting, and start() drains that map once via getAndSet before flipping to Started. A matching update that lands after the drain but before Started is written goes back into the pending map and is never adopted, so availableCommandsRef can keep a stale or empty menu. That regresses the old path, which wrote straight into availableCommandsRef during startup.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 536153c. Configure here.
2 participants
What Changed
Project skills in the
$picker were effectively environment-global. This PR makes skill discovery multi-workspace on the server, tags each project skill with where it came from, and filters the picker to the active chat checkout on web and mobile.Server
SkillDiscoverywalks project skill roots (git-root → cwd ancestors, compatibility dirs, collision precedence) instead of only the process cwd.SkillWorkspaceCwds/ProjectionSnapshotQuery.getActiveWorkspaceCwdscollect every registered project root and active worktree so status probes discover across the environment.sourceCwd.ServerProviderSkillgains optionalsourceCwd(user/global skills omit it).Shared / contracts
@t3tools/shared/providerSkills: path normalization +filterProviderSkillsForWorkspaceso server stamps and client paths compare cleanly (trailing separators,./.., mixed slashes, Windows forms).worktreePath ?? project.workspaceRoot, and may passprojectRootso worktree chats still see project-root-tagged skills before a re-probe re-tags under the worktree.Clients
ChatComposerand mobileThreadComposer/ new-task flow scope the$picker to the active checkout.Docs
docs/internals/providers.mdanddocs/internals/project-scoped-skills.htmldescribing the model and the accepted interim payload cost (full multi-workspace skill union on provider snapshots until a scopedlistSkills(cwd)RPC might land later).Why
With multiple projects or worktrees open, the
$skill picker showed every project skill from every open workspace. That is wrong for isolation: skills from project A should not appear when chatting in project B (or a different worktree), while user/global skills should still be available everywhere.Discovery had to expand first (probes only saw one cwd), then the wire format needed a stable workspace tag (
sourceCwd), then clients needed a shared filter so web and mobile stay consistent. Filtering only on the client without multi-cwd discovery would still miss skills that were never scanned; tagging without filtering would still dump the full union into the picker.Payload growth with open workspaces is accepted as an interim cost documented in internals; do not collapse by skill name across workspaces.
UI Changes
Behavior change only in the composer
$skill picker (and mobile new-task flow using the same filter):sourceCwdmatches the active chat checkout (worktreePath ?? project.workspaceRoot, with optional project-root fallback).No layout or animation redesign. Screenshots not attached; happy to add if useful for review.
Checklist
Note
High Risk
Touches every provider probe, the provider snapshot contract, projection reads, and composer pickers. Wrong tagging or filtering would leak or hide skills/commands across projects.
Overview
Scopes
$skills and/slash commands to the active chat checkout instead of dumping every open workspace’s inventory into the picker.Server: probes now scan all active project roots and worktrees via
getActiveWorkspaceCwds, then stamp project entries withsourceCwd. SharedSkillDiscoverywalks git-root → cwd ancestors with harness-specific roots. Claude/Codex/Cursor/Grok/OpenCode all use this; Grok prefersinspect+ ACP menus, OpenCode queriesdebug config/ SDKcommand.list. User/global/bundled items stay untagged.Clients: web
ChatComposerand mobileThreadComposer/ new-task flow filter withfilterProviderSkillsForWorkspace/filterProviderSlashCommandsForWorkspace(worktreePath ?? project root, with a project-root fallback for worktree chats). Timeline labels still use the full inventory.Also fixes Android skill-chip colors by parsing
#RRGGBBAA(uniwind) instead of Android#AARRGGBB. Snapshots still ship the full multi-workspace union until a scoped list RPC exists.Reviewed by Cursor Bugbot for commit 536153c. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Add project-scoped provider skill and slash command discovery across workspaces
sourceCwdgetActiveWorkspaceCwdstoProjectionSnapshotQueryto supply active project roots and thread worktrees; all provider drivers now require this service and pass resolvedskillCwdsinto their status checkscheckClaudeProviderStatus,checkCodexProviderStatus,checkCursorProviderStatus,checkGrokProviderStatus, andcheckOpenCodeProviderStatusto accept an options object withskillCwdsandenvironmentfilterProviderSkillsForWorkspace,filterProviderSlashCommandsForWorkspace) and applies them in webChatComposer, mobileThreadComposer, andNewTaskFlowProviderso$and/pickers show only skills/commands for the current checkoutavailable_commands_updatecapture inAcpSessionRuntime(viagetAvailableCommands) and routes recognized/name argssubmissions tosession.commandinOpenCodeAdapterClaudeDriverEnv,CodexDriverEnv,CursorDriverEnv,GrokDriverEnv, andOpenCodeDriverEnvnow requireProjectionSnapshotQuery; all test layers updated with stubs.ServerProviderSkillandServerProviderSlashCommandschemas gain optionalsourceCwd.flattenOpenCodeSkillsis removed.packages/client-runtimeexport path changes from./providerSkillsto./providerSkillDisplay.Macroscope summarized 536153c.