fix(provider): resolve skills and slash commands against the workspace root - #7882
fix(provider): resolve skills and slash commands against the workspace root#7882CurlyPeter wants to merge 1 commit into
Conversation
…e root Both composer pickers are empty or incomplete in a packaged desktop build. `DesktopEnvironment` sets the server's cwd to the user's home directory when packaged (`isPackaged ? homeDirectory : appRoot`). `ServerProvider.skills` and `ServerProvider.slashCommands` are produced once per provider instance against that single cwd, but both are project-scoped: - `ClaudeSkills` scans `<configDir>/skills` and `<cwd>/.claude/skills`, so the "project" root resolves to `~/.claude/skills` for every project. A user who keeps no skills there sees none at all, and `$` reports "No skills found". - the CLI's init handshake reports project-scoped slash commands, so `/` lists only the built-ins. Measured on one real workspace: the CLI reports 140 commands from the project root versus 48 from a neutral directory. A provider is a machine-level installation in this model — version, auth and models genuinely are per-machine — so rather than making the snapshot project-aware this adds a workspace-scoped surface beside it: - `ProviderInstance` gains optional `discoverSkillsForCwd` and `discoverSlashCommandsForCwd`. Optional, so the other drivers are untouched. - `ProviderRegistry` gains matching `*ForInstance` methods that follow the existing `getProviderMaintenanceCapabilitiesForInstance` delegation pattern and resolve empty when a driver cannot enumerate per directory. - new `providers.workspaceSkills` RPC resolves `thread.worktreePath ?? project.workspaceRoot` — the same idiom `assets.createUrl` already uses — and is mapped to orchestration:read. - the composer prefers the workspace result for `$` and `/`, falling back to the snapshot, so an older server behaves exactly as before. The RPC is keyed on the project with an optional thread, because the composer is frequently on a draft thread that has no persisted row yet, and that is precisely when the pickers matter. `ClaudeDriver`'s capabilities cache is now keyed by cwd (capacity 16) instead of holding a single entry probed against the server's cwd. `makeClaudeCapabilitiesCacheKey` already included cwd and `ClaudeHome.test.ts` already asserted that two cwds produce two keys, so this completes what the key was designed for. Binary path and resolved HOME are constant within one instance's scope, so keying on cwd preserves the isolation the composite key provided. Cost is one CLI probe per workspace per existing 5-minute TTL: measured ~1.6s cold, ~20ms warm. Skills are deliberately still read from the filesystem rather than the init handshake: `init.skills` carries bare names, while `ServerProviderSkill` requires `path` and displays `description`, which only the scan can supply.
|
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.
One finding: the composer's workspaceSkills prop receives an already-fallback-resolved value, which can make the $ picker show skills for a different provider instance than the one the composer selected. Details inline.
Posted via Macroscope — UI Consistency
| interactionMode={interactionMode} | ||
| lockedProvider={lockedProvider} | ||
| providerStatuses={providerStatuses as ServerProvider[]} | ||
| workspaceSkills={activeSkills} |
There was a problem hiding this comment.
activeSkills already falls back to activeProviderStatus?.skills, so what arrives as workspaceSkills here is often the machine-scoped snapshot of ChatView's resolved instance — and since composerSkills prefers any non-empty workspaceSkills over selectedProviderStatus?.skills, it overrides the composer's own selection.
The two resolutions can disagree: ChatView matches providerStatuses by id only, while ChatComposer additionally filters on enabled && isAvailable plus locked driver/continuation group. With a persisted-but-disabled selection, the workspace RPC returns [] (instance not live) and the $ picker then lists the disabled instance's snapshot skills instead of the instance that will run the turn.
Suggest passing the raw query result and letting ChatComposer own the fallback, the same way workspaceSlashCommands is passed (activeSkills can stay as-is for MessagesTimeline).
| workspaceSkills={activeSkills} | |
| workspaceSkills={workspaceCapabilitiesQuery.data?.skills} |
Posted via Macroscope — UI Consistency
| // An unknown thread id is not an error: a draft thread has no row. | ||
| const worktreePath = input.threadId | ||
| ? yield* projectionSnapshotQuery.getThreadShellById(input.threadId).pipe( | ||
| Effect.map((thread) => |
There was a problem hiding this comment.
🟡 Medium src/ws.ts:1964
A request can name project A with a thread belonging to project B, and this handler then uses B's worktreePath for the project-scoped skill and slash-command discovery result. The threadId lookup must verify thread.value.projectId === input.projectId and ignore or reject mismatches.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/ws.ts around line 1964:
A request can name project A with a thread belonging to project B, and this handler then uses B's `worktreePath` for the project-scoped skill and slash-command discovery result. The `threadId` lookup must verify `thread.value.projectId === input.projectId` and ignore or reject mismatches.
| Effect.map((thread) => | ||
| Option.isSome(thread) ? thread.value.worktreePath : null, | ||
| ), | ||
| Effect.orElseSucceed(() => null), |
There was a problem hiding this comment.
🟡 Medium src/ws.ts:1967
A projection failure for an existing thread is converted to null, so providersWorkspaceSkills silently scans project.value.workspaceRoot and returns incorrect or incomplete results instead of reporting the RPC error. Remove Effect.orElseSucceed(() => null) so only an actual Option.none() selects the project-root fallback.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/ws.ts around line 1967:
A projection failure for an existing thread is converted to `null`, so `providersWorkspaceSkills` silently scans `project.value.workspaceRoot` and returns incorrect or incomplete results instead of reporting the RPC error. Remove `Effect.orElseSucceed(() => null)` so only an actual `Option.none()` selects the project-root fallback.
| description: command.description ?? command.input?.hint ?? "Run provider command", | ||
| })); | ||
| const query = composerTrigger.query.trim().toLowerCase(); | ||
| const skillItems = (selectedProviderStatus?.skills ?? []) |
There was a problem hiding this comment.
🟡 Medium chat/ChatComposer.tsx:1152
The / slash-command menu builds skillItems from selectedProviderStatus.skills, so workspace-only skills available through composerSkills are missing there and stale machine-cwd skills can be shown instead. Use composerSkills for this list, as the $ trigger already does.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/web/src/components/chat/ChatComposer.tsx around line 1152:
The `/` slash-command menu builds `skillItems` from `selectedProviderStatus.skills`, so workspace-only skills available through `composerSkills` are missing there and stale machine-cwd skills can be shown instead. Use `composerSkills` for this list, as the `$` trigger already does.
There was a problem hiding this comment.
Reviewed the new provider capability service surface (ProviderInstance.discoverSkillsForCwd / discoverSlashCommandsForCwd, ProviderRegistry.discoverSkillsForInstance / discoverSlashCommandsForInstance, the providers.workspaceSkills RPC and the client atom family) against the Effect service conventions.
Imports, Effect.fn usage, dependency acquisition (yield* FileSystem.FileSystem / Path.Path inside the driver scope), error modelling at the RPC boundary and the atom family reusing the shared connectionAtomRuntime all look consistent with the existing patterns — no service-injection, hidden-runtime, or error-shape violations found.
Two findings on change discipline, noted inline.
Posted via Macroscope — Effect Service Conventions
| type ProviderSnapshotSettings, | ||
| } from "../providerUpdateSettings.ts"; | ||
| import { makeClaudeCapabilitiesCacheKey, makeClaudeContinuationGroupKey } from "./ClaudeHome.ts"; | ||
| import { makeClaudeContinuationGroupKey } from "./ClaudeHome.ts"; |
There was a problem hiding this comment.
Dropping this import leaves makeClaudeCapabilitiesCacheKey in ClaudeHome.ts with no production caller — the only remaining references are in ClaudeHome.test.ts. Suggest deleting the helper and its now-obsolete test cases along with the cache-key change so no dead pre-refactor path is retained.
Posted via Macroscope — Effect Service Conventions
| ), | ||
| { "rpc.aggregate": "workspace" }, | ||
| ), | ||
| [WS_METHODS.providersWorkspaceSkills]: (input) => |
There was a problem hiding this comment.
This is new backend behavior (workspace-root resolution where a persisted thread's worktree overrides the project root, an unknown project failing as OrchestrationGetSnapshotError, an unknown thread tolerated, and the two registry discovery calls fanned out against the resolved root) and it ships without focused tests — only existing registry mocks were extended.
Consider adding a focused case for the handler in apps/server/src/server.test.ts (the Layer.mock(ProviderRegistry.ProviderRegistry) seam there already supports overriding discoverSkillsForInstance / discoverSlashCommandsForInstance) covering draft-thread vs. worktree root resolution and the unknown-project error, plus a case in apps/server/src/provider/Layers/ProviderRegistry.test.ts for the empty-array fallback when the instance is not live or its driver omits discoverSkillsForCwd.
Posted via Macroscope — Effect Service Conventions
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 3 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want fixes drafted automatically? Bugbot Autofix can create code changes for findings. A team admin can enable Autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 8e69659. Configure here.
| skill.description ?? | ||
| (skill.scope ? `${skill.scope} skill` : "Run provider skill"), | ||
| }), | ||
| ); |
There was a problem hiding this comment.
Slash menu skills still machine-scoped
High Severity
The / picker still builds skillItems from selectedProviderStatus?.skills (machine-scoped snapshot) while the $ path correctly uses composerSkills. In packaged builds that leaves project skills missing from / search even though workspace discovery succeeded, so the two triggers disagree after this fix.
Reviewed by Cursor Bugbot for commit 8e69659. Configure here.
| const activeSkills = | ||
| workspaceCapabilitiesQuery.data?.skills ?? | ||
| activeProviderStatus?.skills ?? | ||
| EMPTY_PROVIDER_SKILLS; |
There was a problem hiding this comment.
Empty skills skip snapshot fallback
Medium Severity
activeSkills uses nullish coalescing on data?.skills, so a successful empty array from drivers without discoverSkillsForCwd replaces the snapshot. That contradicts the registry contract and the slash-command path, which only adopts workspace data when length > 0. Codex snapshot skills are cleared for the timeline once the RPC returns.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 8e69659. Configure here.
| // and the project root is the right answer for it anyway. | ||
| ...(activeServerThread ? { threadId: activeServerThread.id } : {}), | ||
| }, | ||
| }) |
There was a problem hiding this comment.
Workspace query uses wrong instance
Medium Severity
The new workspace RPC is keyed on activeProviderInstanceId, which does not apply the composer’s enabled/available and locked-provider filters used by selectedInstanceId. When those diverge, the pickers can show another instance’s skills or slash commands while labeling them for the instance that will actually send the turn.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 8e69659. Configure here.
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:
|
1 participant
What changed
Both composer pickers are empty or incomplete in a packaged desktop build. This resolves skills and slash commands against the workspace root instead of the server's single global cwd.
Why
DesktopEnvironment.tssets the backend cwd to the user's home directory when packaged:ServerProvider.skillsandServerProvider.slashCommandsare produced once per provider instance against that one cwd — but both are project-scoped:ClaudeSkillsscans<configDir>/skillsand<cwd>/.claude/skills. With cwd pinned to$HOME, the "project" root resolves to~/.claude/skillsfor every project, so a user who keeps no skills there sees none at all and$reports "No skills found". It reports the mis-scoped scan accurately; it is not a UI bug./lists only the built-ins. Measured on one real workspace: the CLI reports 140 commands from the project root versus 48 from a neutral directory.Running sessions are unaffected — they get the right scope through the CLI's own settings resolution — so this is a discovery gap, not a loss of capability.
Approach
A provider is a machine-level installation in this model, and version/auth/models genuinely are per-machine. So rather than making the snapshot project-aware, this adds a workspace-scoped surface beside it:
ProviderInstancegains optionaldiscoverSkillsForCwd/discoverSlashCommandsForCwd. Optional, so the other four drivers are untouched.ProviderRegistrygains matching*ForInstancemethods, following the existinggetProviderMaintenanceCapabilitiesForInstancedelegation pattern, resolving empty when a driver cannot enumerate per directory.providers.workspaceSkillsRPC resolvesthread.worktreePath ?? project.workspaceRoot— the same idiomassets.createUrlalready uses — mapped toorchestration:read.$and/, falling back to the snapshot, so an older server behaves exactly as today.It is keyed on the project with an optional thread, because the composer is frequently on a draft thread with no persisted row, and that is precisely when the pickers matter.
The cache
ClaudeDriver's capabilities cache is now keyed by cwd (capacity 16) rather than holding a single entry probed against the server's cwd.This completes something already designed for:
makeClaudeCapabilitiesCacheKeyalready includes cwd, andClaudeHome.test.tsalready asserts that two cwds produce two keys — the driver simply never varied it. Binary path and resolved HOME are constant within one instance's scope, so keying on cwd preserves the isolation the composite key provided.Cost: one CLI probe per workspace per the existing 5-minute TTL. Measured ~1.6 s cold, ~20 ms warm.
Deliberately not done
Skills are still read from the filesystem rather than
init.skills. The handshake carries bare names, whileServerProviderSkillrequirespathand displaysdescription, which only the scan can supply — the rationale inClaudeSkills.ts's own docstring still holds.Testing
contracts258,server2625,web2696 — all passing on this branch; typecheck,fmt --checkand lint clean. The new RPC is covered by the existing "declares exactly one scope for every RPC in the server group" test.Verified against a live server: the RPC returns 92 skills (all with descriptions) and 140 slash commands for a workspace where the snapshot returned 0 and 48.
No UI layout changes — the pickers render exactly as before, with populated data.
Happy to split this or move it to an issue-first discussion if you'd prefer; I read CONTRIBUTING and this is larger than a one-liner, though it is a single change with no scope expansion.
Note
Medium Risk
Adds a new read RPC that scans a project workspace and may spawn the Claude CLI, plus cwd-keyed capability caching. Fallback to the machine snapshot keeps older servers working, but probe/cache behavior is now per workspace.
Overview
Fixes empty/incomplete
$and/pickers in packaged desktop builds, where the provider snapshot was scanned against$HOMEand missed project.claude/skillsand CLI project commands.Adds
providers.workspaceSkills(orchestration read). It resolvesthread.worktreePath ?? project.workspaceRootso draft threads still work, then returns workspace skills plus slash commands. The composer prefers that result and falls back to the machine-scoped snapshot.Claude-only optional
discoverSkillsForCwd/discoverSlashCommandsForCwdon the instance; other drivers omit them. Skills stay a cheap filesystem scan. Slash commands reuse the capabilities probe cache, now keyed by cwd (capacity 16, 5-minute TTL) instead of a single server-cwd entry.Reviewed by Cursor Bugbot for commit 8e69659. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Resolve skills and slash commands against workspace root via new
providers.workspaceSkillsRPCproviders.workspaceSkillsRPC that derives the workspace root from a thread's worktree path or the project root, then returns project-scoped skills and slash commands for a given provider instanceClaudeDrivernow probes and caches capabilities per working directory (cwd) with a capacity-16 LRU, and exposesdiscoverSkillsForCwd/discoverSlashCommandsForCwdon theProviderInstanceinterfaceProviderRegistryforwards discovery calls to the live instance, returning empty arrays when unsupported or not liveChatView,ChatComposer) prefers workspace-scoped results for the$and/menus, falling back to the machine-scoped snapshot when unavailableClaudeDriver.createis now keyed bycwdinstead of a single global key; any code that relied on the oldmakeClaudeCapabilitiesCacheKeysingleton cache will no longer share state across workspaces📊 Macroscope summarized 8e69659. 12 files reviewed, 4 issues evaluated, 1 issue filtered, 3 comments posted
🗂️ Filtered Issues
apps/web/src/components/chat/ChatComposer.tsx — 1 comment posted, 2 evaluated, 1 filtered
composerSkillstreats an explicitly returned emptyworkspaceSkillsarray as unavailable and falls back toselectedProviderStatus.skills. When a workspace legitimately has no skills, the$picker and editor therefore show the machine-cwd skills instead of no workspace skills, reintroducing the incorrect-scope behavior this change is meant to fix. Use nullish availability rather thanlength > 0so only an absent result falls back. [ Already posted ]