Expose project provider skills in chat#3982
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}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
ApprovabilityVerdict: Needs human review 1 blocking correctness issue found. This PR introduces substantial new functionality (project skill discovery, new RPC endpoint, CLI process spawning) with multiple unresolved review comments identifying medium-severity bugs including potential process accumulation, stale cache issues, and configuration mismatches. You can customize Macroscope's approvability policy. Learn more. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 917ea83630
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const commandResult = yield* spawnAndCollect( | ||
| binaryPath, | ||
| ChildProcess.make(spawnCommand.command, spawnCommand.args, { | ||
| cwd: input.cwd, | ||
| ...(input.environment ? { env: input.environment } : {}), | ||
| shell: spawnCommand.shell, | ||
| }), | ||
| ).pipe( |
There was a problem hiding this comment.
When grok inspect hangs or is interrupted, this spawn is only attached to whatever ambient Scope happens to be running the RPC/adapter call; GrokDriver.listSkills exposes it as a normal service method and does not create a per-call scope. In that case the 10s timeout can return an error while the child process remains tied to a longer-lived provider/server scope, so repeated $ menu opens or sends with $skill tokens can accumulate stuck grok inspect processes until shutdown. Wrap the spawned spawnAndCollect probe in its own Effect.scoped before applying the timeout.
Useful? React with 👍 / 👎.
| if (input.cachedSkills?.targetKey === input.targetKey) { | ||
| return input.cachedSkills.skills; |
There was a problem hiding this comment.
When the current cwd-aware skill query fails (for example, grok inspect exits non-zero), discoveredSkills is null and discoveryUnsupported is false, so this branch still returns the previous successful cache for the same target. That makes the menu offer stale skills while the provider is currently unable to rediscover them; selecting one then sends a $skill token that Grok re-checks at send time and fails, blocking the turn instead of surfacing the discovery failure in the composer.
Useful? React with 👍 / 👎.
| environmentId, | ||
| instanceId: selectedProviderStatus?.instanceId ?? null, | ||
| cwd: gitCwd, | ||
| enabled: composerTriggerKind === "skill", |
There was a problem hiding this comment.
When the user types a shell/env-var token such as $PATH or $NODE_ENV, detectComposerTrigger still marks the token as a skill, so this enables server.listProviderSkills and spawns provider CLI probes even though the Grok send path explicitly filters uppercase env-var-style tokens. In prompts or docs involving env vars, merely placing the cursor after $PATH opens the skill menu and can run grok inspect; gate this query with the same env-var-style check or wait until the token can plausibly be a skill.
Useful? React with 👍 / 👎.
917ea83 to
ecda648
Compare
| return yield* probeCodexAppServerProvider({ | ||
| binaryPath: effectiveConfig.binaryPath, | ||
| homePath: effectiveConfig.homePath, | ||
| cwd, | ||
| customModels: [], | ||
| environment: processEnv, | ||
| includeModels: false, | ||
| }).pipe( |
There was a problem hiding this comment.
🟡 Medium Drivers/CodexDriver.ts:173
listSkills calls probeCodexAppServerProvider without passing effectiveConfig.launchArgs, so instances that require custom Codex launch arguments (e.g. --config flags) are probed with a different app-server configuration than their normal snapshot and sessions. Skill discovery can return the wrong skill set or fail even though the provider itself works. Pass launchArgs: resolveCodexLaunchArgs(effectiveConfig.launchArgs, processEnv) as the status probe already does.
| return yield* probeCodexAppServerProvider({ | |
| binaryPath: effectiveConfig.binaryPath, | |
| homePath: effectiveConfig.homePath, | |
| cwd, | |
| customModels: [], | |
| environment: processEnv, | |
| includeModels: false, | |
| }).pipe( | |
| return yield* probeCodexAppServerProvider({ | |
| binaryPath: effectiveConfig.binaryPath, | |
| homePath: effectiveConfig.homePath, | |
| launchArgs: resolveCodexLaunchArgs(effectiveConfig.launchArgs, processEnv), | |
| cwd, | |
| customModels: [], | |
| environment: processEnv, | |
| includeModels: false, | |
| }).pipe( |
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/provider/Drivers/CodexDriver.ts around lines 173-180:
`listSkills` calls `probeCodexAppServerProvider` without passing `effectiveConfig.launchArgs`, so instances that require custom Codex launch arguments (e.g. `--config` flags) are probed with a different app-server configuration than their normal snapshot and sessions. Skill discovery can return the wrong skill set or fail even though the provider itself works. Pass `launchArgs: resolveCodexLaunchArgs(effectiveConfig.launchArgs, processEnv)` as the status probe already does.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit ecda648. Configure here.
| customModels: [], | ||
| environment: processEnv, | ||
| includeModels: false, | ||
| }).pipe( |
There was a problem hiding this comment.
Codex skills omit launch args
Medium Severity
The CodexDriver.listSkills function omits launchArgs when probing the app server, unlike checkCodexProviderStatus and the Codex adapter. This causes configured Codex launch arguments (including T3CODE_CODEX_LAUNCH_ARGS) to be ignored during skill discovery, which can lead to skills being missed or mis-resolved in the Composer's $ menu.
Reviewed by Cursor Bugbot for commit ecda648. Configure here.
| return undefined; | ||
| } | ||
| return yield* instance.listSkills(input.cwd); | ||
| }); |
There was a problem hiding this comment.
Live-sub miss treated unsupported
Medium Severity
ProviderRegistry.listSkills only consults liveSubsRef and returns undefined when the instance is absent there. The RPC maps that to ServerProviderSkillsUnsupportedError, so the composer caches non–cwd-aware snapshot skills even for Codex/Grok during startup or instance rebuild while syncLiveSources has not yet written the new subscription map.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit ecda648. Configure here.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ecda6483c4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return yield* probeCodexAppServerProvider({ | ||
| binaryPath: effectiveConfig.binaryPath, | ||
| homePath: effectiveConfig.homePath, | ||
| cwd, | ||
| customModels: [], | ||
| environment: processEnv, | ||
| includeModels: false, |
There was a problem hiding this comment.
When a Codex instance is configured with launchArgs or T3CODE_CODEX_LAUNCH_ARGS (for example --strict-config, --config, or feature flags), status checks and real sessions resolve and pass those args, but this new server.listProviderSkills path starts a plain app-server without them. In that configuration the $ menu can discover a different skill set or fail even though the provider itself works with the configured app-server; pass the resolved launch args into probeCodexAppServerProvider here as the other Codex app-server callers do.
Useful? React with 👍 / 👎.
| [WS_METHODS.serverProbe, AuthOrchestrationReadScope], | ||
| [WS_METHODS.serverGetConfig, AuthOrchestrationReadScope], | ||
| [WS_METHODS.serverRefreshProviders, AuthOrchestrationOperateScope], | ||
| [WS_METHODS.serverListProviderSkills, AuthOrchestrationReadScope], |
There was a problem hiding this comment.
For sessions that only have orchestration:read, this new RPC still reaches the handler that calls providerRegistry.listSkills, and the new driver implementations start provider subprocesses (codex app-server or grok inspect) in the requested cwd. Comparable provider probing via serverRefreshProviders is already orchestration:operate, so leaving this as read-only lets restricted clients launch local provider processes through the skill menu; require the operate scope here as well.
Useful? React with 👍 / 👎.
1 participant
Summary
listSkillscapability, RPC, client query, and composer pipelinegrok --cwd <cwd> inspect --json$skillreferences to Grok native/skillsyntax only at the ACP boundary while retaining$skillin stored and rendered messagesThis supersedes #3901. The replacement generalizes the project-skill feature across Codex and Grok without adding a second filesystem scanner or provider-specific web/RPC paths.
Validation
pnpm exec vp checkpnpm exec vp run typecheck$skillchip storage, and successful native skill execution through ACP$skillregression checkNote
Medium Risk
Touches turn submission and spawns provider CLI probes on skill menu use; Grok prompt rewriting could mis-handle edge-case tokens if discovery lists overlap env var names.
Overview
Adds cwd-aware provider skill discovery end-to-end: optional
listSkillson provider instances, registry delegation,server.listProviderSkillsWebSocket RPC with unsupported/error types, and a client query wired into the composer when the$skill menu opens.Codex lists skills via an exported, skills-only
probeCodexAppServerProvider(includeModels: false) with probe timeouts. Grok addsgrok --cwd <cwd> inspect --jsonparsing (GrokSkillDiscovery) and uses it from the driver and adapter.Composer UX resolves skills per environment/instance/cwd with keyed caching and snapshot fallback only when discovery is unsupported; the Lexical editor refreshes skill chip labels without rewriting full editor state during in-flight probes.
Grok send path rewrites enabled
$skilltokens to/skillonly in the ACP prompt (stored UI text unchanged), skips discovery for prompts without skill tokens or all-caps env-style tokens, and commits model switches before skill discovery so a failed list does not roll back the model.Reviewed by Cursor Bugbot for commit ecda648. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Expose provider skill discovery in the chat composer via a new
server.listProviderSkillsRPCserver.listProviderSkillsWebSocket RPC that queries the active provider (Codex or Grok) for its available skills given aninstanceIdandcwd, returningServerProviderSkillsUnsupportedErrorwhen a provider doesn't support skill listing.listSkillson Codex and Grok provider drivers: Codex probes the app server binary; Grok runsgrok inspect --json. Both return empty lists when disabled and surface timeouts asProviderDriverError.$review→/review) in the prompt before sending, skipping environment-variable-style tokens like$PATH. Discovery errors surface as typedProviderAdapterRequestError.Macroscope summarized ecda648.