Local skills and commands: codex impl#3788
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 Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
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 Effect service convention issue found in the new provider project-capabilities error. See the inline comment.
Posted via Macroscope — Effect Service Conventions
| detail: Schema.String, | ||
| cause: Schema.optional(Schema.Defect()), | ||
| }, | ||
| ) { | ||
| override get message(): string { | ||
| return `Provider '${this.provider}' failed to list project capabilities for instance '${this.instanceId}' in '${this.cwd}': ${this.detail}`; |
There was a problem hiding this comment.
detail merely copies cause.detail/cause.message (via describeCapabilityProbeFailure inside makeProviderProjectCapabilitiesError), and the message getter is then built from this.detail. The error conventions forbid adding a detail field that copies cause.message and using it to construct the wrapper message; the message must be derived exclusively from stable structural attributes (provider, instanceId, cwd), while the underlying failure is preserved only as cause.
Drop detail and derive the message from the structural fields:
{
provider: Schema.String,
instanceId: Schema.String,
cwd: Schema.String,
- detail: Schema.String,
cause: Schema.optional(Schema.Defect()),
},
) {
override get message(): string {
- return `Provider '${this.provider}' failed to list project capabilities for instance '${this.instanceId}' in '${this.cwd}': ${this.detail}`;
+ return `Provider '${this.provider}' failed to list project capabilities for instance '${this.instanceId}' in '${this.cwd}'.`;
}
}Then simplify makeProviderProjectCapabilitiesError to construct the error directly with cause and remove describeCapabilityProbeFailure.
Posted via Macroscope — Effect Service Conventions
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
lmtr0
force-pushed
the
local-skills-and-commands/codex
branch
from
5c85692 to
091e39e
Compare
github-actions
Bot
added
size:XL
| )?.skills ?? [], | ||
| [selectedEnvironmentServerConfig, selectedModel?.instanceId], | ||
| ); | ||
| const selectedProviderCapabilities = useProviderProjectCapabilities({ |
There was a problem hiding this comment.
🟡 Medium threads/new-task-flow-provider.tsx:384
When workspaceMode is switched back to "local", the provider capabilities query still uses the previously selected selectedWorktreePath instead of the project's workspaceRoot, so the composer shows skills and slash-commands for the old worktree rather than the local project. setWorkspaceMode preserves selectedWorktreePath when updating the draft, so switching to local doesn't clear it, and the query at line 387 passes selectedWorktreePath || selectedProject?.workspaceRoot, which picks up the stale worktree path. Consider passing workspaceMode === "worktree" ? selectedWorktreePath : selectedProject?.workspaceRoot so capabilities match the active workspace.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/mobile/src/features/threads/new-task-flow-provider.tsx around line 384:
When `workspaceMode` is switched back to `"local"`, the provider capabilities query still uses the previously selected `selectedWorktreePath` instead of the project's `workspaceRoot`, so the composer shows skills and slash-commands for the old worktree rather than the local project. `setWorkspaceMode` preserves `selectedWorktreePath` when updating the draft, so switching to local doesn't clear it, and the query at line 387 passes `selectedWorktreePath || selectedProject?.workspaceRoot`, which picks up the stale worktree path. Consider passing `workspaceMode === "worktree" ? selectedWorktreePath : selectedProject?.workspaceRoot` so capabilities match the active workspace.
| ? provider | ||
| : null; | ||
| }, [queryTarget, target.providerInstanceId, target.providers]); | ||
| useEffect(() => { |
There was a problem hiding this comment.
🟡 Medium state/queries.ts:162
After calling refresh(), every subsequent fetch for that target keeps forceReload: true applied indefinitely, so the cache is bypassed on every later render/remount and the live capability probe re-runs repeatedly. refresh() sets forceReloadTargetKey to targetKey, but the cleanup effect at lines 162–169 only clears it when the target changes or queryTarget becomes null — never after the forced reload succeeds. The effect needs a condition that resets forceReloadTargetKey once the forced query is no longer pending.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/mobile/src/state/queries.ts around line 162:
After calling `refresh()`, every subsequent fetch for that target keeps `forceReload: true` applied indefinitely, so the cache is bypassed on every later render/remount and the live capability probe re-runs repeatedly. `refresh()` sets `forceReloadTargetKey` to `targetKey`, but the cleanup effect at lines 162–169 only clears it when the target changes or `queryTarget` becomes `null` — never after the forced reload succeeds. The effect needs a condition that resets `forceReloadTargetKey` once the forced query is no longer pending.
ApprovabilityVerdict: Needs human review 5 blocking correctness issues found. This PR introduces new project-scoped provider capabilities - a substantial feature addition with new RPC endpoints, caching infrastructure, and changed runtime behavior for skill/command resolution. Multiple unresolved review comments identify bugs including stale worktree paths, missing fallbacks on RPC failure, and accidental removal of the /model command. You can customize Macroscope's approvability policy. Learn more. |
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 091e39e. Configure here.
| }; | ||
| } | ||
|
|
||
| export function useProviderProjectCapabilities(target: { |
There was a problem hiding this comment.
Missing cwd shows wrong skills
Low Severity
When cwd is missing or blank, useProviderProjectCapabilities skips the project RPC but still falls back to the provider snapshot’s skills and slashCommands. Those snapshot lists come from Codex status probes against process.cwd(), not the user’s project or worktree, so composer hints can show the wrong project’s skills.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 091e39e. Configure here.
| const snapshotCapabilities = useMemo(() => { | ||
| const provider = | ||
| target.providers?.find((candidate) => candidate.instanceId === target.providerInstanceId) ?? | ||
| null; | ||
| return queryTarget === null && | ||
| provider !== null && | ||
| isProviderProjectCapabilitiesProviderQueryable(provider) | ||
| ? provider | ||
| : null; | ||
| }, [queryTarget, target.providerInstanceId, target.providers]); |
There was a problem hiding this comment.
🟡 Medium state/queries.ts:251
When the project-scoped capabilities RPC fails, useProviderProjectCapabilities returns EMPTY_PROVIDER_SKILLS and EMPTY_PROVIDER_SLASH_COMMANDS instead of falling back to the provider's existing skills and slashCommands. This means a transient RPC error (transport failure, unsupported cwd, etc.) makes all / commands and skills disappear, even though the provider snapshot still has them.
The snapshotCapabilities fallback is only applied when queryTarget === null, so any non-null query that results in a Failure drops the snapshot data. Consider applying the snapshot fallback whenever result.data is absent, not only when there is no active query.
| const snapshotCapabilities = useMemo(() => { | |
| const provider = | |
| target.providers?.find((candidate) => candidate.instanceId === target.providerInstanceId) ?? | |
| null; | |
| return queryTarget === null && | |
| provider !== null && | |
| isProviderProjectCapabilitiesProviderQueryable(provider) | |
| ? provider | |
| : null; | |
| }, [queryTarget, target.providerInstanceId, target.providers]); | |
| const snapshotCapabilities = useMemo(() => { | |
| const provider = | |
| target.providers?.find((candidate) => candidate.instanceId === target.providerInstanceId) ?? | |
| null; | |
| return provider !== null && isProviderProjectCapabilitiesProviderQueryable(provider) | |
| ? provider | |
| : null; | |
| }, [queryTarget, target.providerInstanceId, target.providers]); |
Also found in 1 other location(s)
apps/mobile/src/features/threads/ThreadDetailScreen.tsx:276
useProviderProjectCapabilitiesatThreadDetailScreenline 276 replaces the previous directserverConfig.providers[].skillsfallback with an RPC-backed lookup that returns[]on the first project-capabilities failure. BecauseThreadFeednow receivesselectedProviderCapabilities.skills, any transient RPC error or unsupportedprojectCapabilitiesresponse strips all known skill metadata from the feed, so existing skill references in messages stop rendering as skills even thoughprops.serverConfigstill has the provider's advertisedskills.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/web/src/state/queries.ts around lines 251-260:
When the project-scoped capabilities RPC fails, `useProviderProjectCapabilities` returns `EMPTY_PROVIDER_SKILLS` and `EMPTY_PROVIDER_SLASH_COMMANDS` instead of falling back to the provider's existing `skills` and `slashCommands`. This means a transient RPC error (transport failure, unsupported cwd, etc.) makes all `/` commands and skills disappear, even though the provider snapshot still has them.
The `snapshotCapabilities` fallback is only applied when `queryTarget === null`, so any non-null query that results in a `Failure` drops the snapshot data. Consider applying the snapshot fallback whenever `result.data` is absent, not only when there is no active query.
Also found in 1 other location(s):
- apps/mobile/src/features/threads/ThreadDetailScreen.tsx:276 -- `useProviderProjectCapabilities` at `ThreadDetailScreen` line 276 replaces the previous direct `serverConfig.providers[].skills` fallback with an RPC-backed lookup that returns `[]` on the first project-capabilities failure. Because `ThreadFeed` now receives `selectedProviderCapabilities.skills`, any transient RPC error or unsupported `projectCapabilities` response strips all known skill metadata from the feed, so existing skill references in messages stop rendering as skills even though `props.serverConfig` still has the provider's advertised `skills`.
| const providerSnapshot = (yield* Ref.get(providersRef)).find( | ||
| (provider) => provider.instanceId === input.providerInstanceId, | ||
| ); | ||
| if ( |
There was a problem hiding this comment.
🟡 Medium Layers/ProviderRegistry.ts:501
listProjectCapabilities only gates on installed, enabled, and availability, so an unauthenticated-but-installed instance still reaches instance.composerCapabilities.list. The live probe raises ProviderProjectCapabilitiesError instead of returning empty capabilities, so composer capability lookups fail for every unauthenticated instance until the user logs in. Consider adding an auth/readiness guard (e.g. checking providerSnapshot.auth !== "authenticated") before calling the live probe, or document why unauthenticated instances should be allowed to reach it.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/provider/Layers/ProviderRegistry.ts around line 501:
`listProjectCapabilities` only gates on `installed`, `enabled`, and `availability`, so an unauthenticated-but-installed instance still reaches `instance.composerCapabilities.list`. The live probe raises `ProviderProjectCapabilitiesError` instead of returning empty capabilities, so composer capability lookups fail for every unauthenticated instance until the user logs in. Consider adding an auth/readiness guard (e.g. checking `providerSnapshot.auth !== "authenticated"`) before calling the live probe, or document why unauthenticated instances should be allowed to reach it.
There was a problem hiding this comment.
🟡 Medium threads/ThreadComposer.tsx:354
The built-in /model slash command was removed from allBuiltIn in composerMenuItems, so typing /m or /mo in the composer now returns No matching commands. instead of surfacing /model for autocompletion. This regresses a core composer command on mobile — users can no longer discover or switch models via the slash menu. Consider re-adding the /model entry to allBuiltIn.
const allBuiltIn = [
+ {
+ id: "cmd:model",
+ type: "slash-command" as const,
+ command: "model",
+ label: "/model",
+ description: "Switch model",
+ },
{
id: "cmd:plan",
type: "slash-command" as const,
command: "plan",
label: "/plan",
description: "Switch to plan mode",
},🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/mobile/src/features/threads/ThreadComposer.tsx around lines 354-360:
The built-in `/model` slash command was removed from `allBuiltIn` in `composerMenuItems`, so typing `/m` or `/mo` in the composer now returns `No matching commands.` instead of surfacing `/model` for autocompletion. This regresses a core composer command on mobile — users can no longer discover or switch models via the slash menu. Consider re-adding the `/model` entry to `allBuiltIn`.
1 participant
Important
This is the Codex implementation adapter. To make the PR small and easy to review I have created different branches for the other adapters. This branch contains all the codex code and #3787 contains the shared implementation. If they both get merged, I'll create PRs for claude code, opencode, and cursor which I already have implemented, but I would rather not spam the repository with tons of PRs.
Why
Same as #3787
What Changed
Adds codex provider implementation for project-scoped skills and slash-commands discovery.
Code Changes
Testing
vp run typecheckvp checkUI Changes
Same as the parent branch
Checklist
Note
Medium Risk
Composer menus and skill highlighting now depend on Codex app-server subprocess probes per cwd; failures surface to users instead of silent empty lists, and first-load latency is visible despite caching.
Overview
Adds Codex as the first driver with live project-scoped skill discovery, wired through the shared
providers.projectCapabilitiesRPC and composer hooks from the parent work.On the server, CodexDriver exposes
composerCapabilities.list, which spawns the Codex app-server for the requestcwd, callsskills/list, and returns skills with empty slash commands. Results are cached per cwd for 30s (capacity 64), withforceReloadbypassing the cache. Probes run under an 8s timeout and map failures toProviderProjectCapabilitiesError/ServerProviderProjectCapabilitiesError.listCodexProjectSkillsis extracted inCodexProvider.ts(with exportedparseCodexSkillsListResponse). ProviderRegistry gainslistProjectCapabilities, returning empty arrays for disabled, unavailable, or drivers withoutcomposerCapabilities, and surfacing probe errors for enabled Codex instances.Web and mobile composers stop using static provider snapshot skills/commands for
/and$menus. They useuseProviderProjectCapabilities(environment, instance, thread/worktreecwd), show loading while the RPC is pending, and pass errors intoComposerCommandPopover. The command popover can appear even with zero matches; mobile drops the built-in/modelentry from the slash menu. Thread feedskillsfor markdown highlighting follow the same capability source.Reviewed by Cursor Bugbot for commit 091e39e. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Add provider project capabilities (skills and slash commands) via Codex RPC endpoint
providers.projectCapabilitiesWebSocket RPC that fetches project-scoped skills and slash commands for a given provider instance andcwd, backed byProviderRegistry.listProjectCapabilities.listCodexProjectSkillsinCodexDriver, which spawns the Codex app-server, runs theskills/listprotocol, and caches results percwdwith a 30-second TTL and optionalforceReload.useProviderProjectCapabilitieshooks (web and mobile) that query the new RPC, exposeskills,slashCommands,isPending,error, and arefreshfunction with snapshot fallback when querying is unavailable.ChatComposer,ThreadComposer, and timeline/feed views, replacing static provider-snapshot lookups with live, environment- and cwd-scoped data.ExactNonBlankString,ServerProviderProjectCapabilitiesInput/Result/Errorcontract schemas with strict non-blankcwdvalidation and an 8-second probe timeout.📊 Macroscope summarized 5c85692. 27 files reviewed, 0 issues evaluated, 0 issues filtered, 0 comments posted
🗂️ Filtered Issues
No issues evaluated.