Repository navigation
feat: serve Codex Skills from a lazy app-server catalog - #904
Conversation
📝 WalkthroughWalkthroughCodex Skills now come from a lazy, catalog-only app-server connection with per-workspace caching, refresh notifications, diagnostics, and freshness metadata. Codex provider and WebSocket catalog flows use this service, while legacy Codex filesystem discovery is removed. ChangesCodex catalog flow
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant WsRouter
participant CodexCatalogService
participant CodexCatalogClient
participant CodexAppServer
Client->>WsRouter: provider.catalog(workspaceId)
WsRouter->>CodexCatalogService: refresh(cwd)
CodexCatalogService->>CodexCatalogClient: start or reuse client
CodexCatalogClient->>CodexAppServer: initialize catalog-only session
CodexCatalogService->>CodexCatalogClient: listSkills(cwd)
CodexCatalogClient-->>CodexCatalogService: skills result
CodexCatalogService-->>WsRouter: skills, diagnostics, freshness
WsRouter-->>Client: provider catalog snapshot
Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
apps/server/src/transport/provider-catalog.ts (1)
47-48: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd one-line docstrings.
As per coding guidelines, add one-line JSDoc/TSDoc docstrings to all exported interfaces and their properties. Consider adding brief descriptions for these new fields to explain their operational purpose in the snapshot.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/server/src/transport/provider-catalog.ts` around lines 47 - 48, Add one-line TSDoc comments to the exported interface and each property shown in provider-catalog.ts, including diagnostics and freshness; briefly describe each field’s operational purpose in the snapshot while preserving the existing types and optionality.Source: Coding guidelines
apps/server/src/transport/ws-router.ts (1)
1001-1007: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse explicit types for uninitialized variables.
As per coding guidelines, use explicit types to make the code self-documenting and to prevent uninitialized variables from implicitly evaluating to
anybefore assignment.Consider explicitly defining the types, for example:
let diagnostics: readonly ProviderCatalogDiagnostic[] | undefined; let freshness: ProviderCatalogFreshness | undefined;(You may need to import these types if they aren't already available in this file.)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/server/src/transport/ws-router.ts` around lines 1001 - 1007, Declare the uninitialized diagnostics and freshness variables in the provider catalog flow with explicit union types matching the catalog’s diagnostic and freshness types plus undefined. Import the types if needed, while preserving their existing assignments from codexCatalogService.refresh(cwd).Source: Coding guidelines
apps/server/src/services/skill-service.ts (1)
413-437: 📐 Maintainability & Code Quality | 🔵 TrivialDocstring doesn't mention the new codex-specific bypass.
filterAndDedup's docstring only describes generic per-filtered-set dedup, but theproviderId === "codex"branch (435-437) now skips that cleanup entirely and returns native items unfiltered by name collision. Worth a one-line addition explaining why codex bypasses the collision cleanup (native path identity is authoritative, not name-based dedup).* Deduplication is intentionally scoped to the filtered set so that a skill * named "deploy" can independently exist for both claude and codex. + * For codex, native catalog items bypass this cleanup entirely: their path + * identity from the app-server is authoritative, so same-name native Skills + * (e.g. user vs project scope) are preserved rather than deduped by name. */🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/server/src/services/skill-service.ts` around lines 413 - 437, Update the docstring for filterAndDedup to document the providerId === "codex" behavior: nativeFiltered items are returned alongside the deduplicated entries without name-based collision cleanup because native path identity is authoritative.apps/server/src/services/codex-catalog-service.ts (1)
155-160: 🚀 Performance & Scalability | 🔵 TrivialOptional: no in-flight request coalescing per cwd in
refresh().Concurrent
refresh(cwd)calls for the samecwd(e.g. two composer mounts polling skills around the same time) each trigger an independentclient.listSkills()RPC rather than sharing one in-flight request.acquireClient()already coalesces client startup viastartPromise; the same pattern could dedupe concurrentrefreshContextcalls per key.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/server/src/services/codex-catalog-service.ts` around lines 155 - 160, Add per-cwd in-flight request coalescing to refresh() so concurrent calls share one refreshContext(cwd, false) promise and trigger only one listSkills RPC. Reuse the existing key normalization/context handling, store the promise while pending, and remove it after completion so later refreshes execute normally.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/server/src/services/codex-catalog-service.ts`:
- Around line 250-277: Update acquireClient to check and return the existing
startPromise before checking client?.isAlive, ensuring concurrent callers await
startup and handshake completion. Preserve the current alive-client fast path
for cases with no in-flight start.
---
Nitpick comments:
In `@apps/server/src/services/codex-catalog-service.ts`:
- Around line 155-160: Add per-cwd in-flight request coalescing to refresh() so
concurrent calls share one refreshContext(cwd, false) promise and trigger only
one listSkills RPC. Reuse the existing key normalization/context handling, store
the promise while pending, and remove it after completion so later refreshes
execute normally.
In `@apps/server/src/services/skill-service.ts`:
- Around line 413-437: Update the docstring for filterAndDedup to document the
providerId === "codex" behavior: nativeFiltered items are returned alongside the
deduplicated entries without name-based collision cleanup because native path
identity is authoritative.
In `@apps/server/src/transport/provider-catalog.ts`:
- Around line 47-48: Add one-line TSDoc comments to the exported interface and
each property shown in provider-catalog.ts, including diagnostics and freshness;
briefly describe each field’s operational purpose in the snapshot while
preserving the existing types and optionality.
In `@apps/server/src/transport/ws-router.ts`:
- Around line 1001-1007: Declare the uninitialized diagnostics and freshness
variables in the provider catalog flow with explicit union types matching the
catalog’s diagnostic and freshness types plus undefined. Import the types if
needed, while preserving their existing assignments from
codexCatalogService.refresh(cwd).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: be1dfcc3-70cb-4b1f-898c-7ba42b7329bd
📒 Files selected for processing (15)
apps/server/src/container.tsapps/server/src/index.tsapps/server/src/providers/codex/__tests__/codex-app-server-handshake.test.tsapps/server/src/providers/codex/__tests__/codex-provider-first-turn.test.tsapps/server/src/providers/codex/__tests__/codex-provider-permission.test.tsapps/server/src/providers/codex/__tests__/codex-provider-subagent-turn.test.tsapps/server/src/providers/codex/codex-app-server.tsapps/server/src/providers/codex/codex-provider.tsapps/server/src/services/__tests__/codex-catalog-service.test.tsapps/server/src/services/codex-catalog-service.tsapps/server/src/services/skill-service.test.tsapps/server/src/services/skill-service.tsapps/server/src/transport/provider-catalog.tsapps/server/src/transport/ws-router.tsapps/server/src/transport/ws-router.validation.test.ts
| private async acquireClient(cwd?: string): Promise<CodexCatalogClient> { | ||
| if (this.client?.isAlive) return this.client; | ||
| if (this.startPromise) return this.startPromise; | ||
|
|
||
| const settings = this.settingsService.get(); | ||
| const client = this.clientFactory.create({ | ||
| cliPath: settings.provider.cli.codex || "codex", | ||
| workingDirectory: cwd ?? process.cwd(), | ||
| approvalPolicy: "never", | ||
| catalogOnly: true, | ||
| jobObject: this.jobObject, | ||
| getSpawnEnv: () => this.envService.getEnv(), | ||
| }); | ||
| this.client = client; | ||
| client.on("notification", (notification: unknown) => { | ||
| this.handleNotification(notification); | ||
| }); | ||
| this.startPromise = client.start().then(() => client); | ||
| try { | ||
| return await this.startPromise; | ||
| } catch (error) { | ||
| if (this.client === client) this.client = null; | ||
| await client.kill().catch(() => undefined); | ||
| throw error; | ||
| } finally { | ||
| this.startPromise = null; | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Race: isAlive checked before startPromise, can return a not-yet-handshaked client.
CodexAppServer._isAlive flips to true right after spawn succeeds but before runHandshake() completes (see codex-app-server.ts start()). Because acquireClient() checks this.client?.isAlive before this.startPromise, a second concurrent refresh()/refreshContext() call landing in that window returns the client immediately and can fire listSkills() before the initialize/initialized handshake finishes.
🔒️ Reorder checks so an in-flight start is always awaited first
private async acquireClient(cwd?: string): Promise<CodexCatalogClient> {
- if (this.client?.isAlive) return this.client;
- if (this.startPromise) return this.startPromise;
+ if (this.startPromise) return this.startPromise;
+ if (this.client?.isAlive) return this.client;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/server/src/services/codex-catalog-service.ts` around lines 250 - 277,
Update acquireClient to check and return the existing startPromise before
checking client?.isAlive, ensuring concurrent callers await startup and
handshake completion. Preserve the current alive-client fast path for cases with
no in-flight start.
1 participant
What
Serve Codex Skills from one lazy provider-wide app-server catalog before thread creation.
Why
Mcode needs provider-authoritative Skill discovery without starting or coupling to a thread app-server.
Key Changes
skills/changedin the background.Closes #892
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by CodeRabbit
New Features
Bug Fixes