Repository navigation
feat(codex): preserve bounded custom prompts - #912
Conversation
|
Warning Review limit reached
Next review available in: 41 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (29)
📝 WalkthroughWalkthroughAdds bounded Codex custom-prompt discovery and catalog integration, identity-aware prompt/Skill resolution, partial catalog reconciliation, and capability-identity persistence through the web slash-command pipeline. ChangesCodex prompt compatibility flow
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
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 |
fbb453c to
bd089fa
Compare
bd089fa to
ade6e8b
Compare
|
@coderabbitai full review |
✅ Action performedFull review finished. Your plan includes PR reviews subject to rate limits. More reviews will be available in 46 minutes. |
![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 (2)
apps/server/src/transport/provider-catalog.ts (1)
58-67: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCentralize the diagnostics cap instead of duplicating the
100literal.
codex-catalog-service.tsindependently hardcodes.slice(0, 100)twice for the same concept. DefiningPROVIDER_CATALOG_MAX_DIAGNOSTICShere only locally means three independent sources of truth for the same cap; consider hoisting it into@mcode/contractsalongsidePROVIDER_CATALOG_MAX_ENTRIESand friends, and importing it in both files.🤖 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 58 - 67, Move PROVIDER_CATALOG_MAX_DIAGNOSTICS from provider-catalog.ts into `@mcode/contracts` alongside the existing catalog limits, then import and reuse that shared constant in both appendDiagnostic and the two codex-catalog-service.ts slice calls, removing the duplicated literal 100.apps/server/src/services/codex-catalog-service.ts (1)
423-429: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueConsider catching
promptRefreshto prevent masking catalog errors.If
this.customPromptService.refresh()were to reject,await promptRefreshin thecatchblock below would throw the exact same error. This would bypass the generation of the fallback catalog snapshot and potentially mask the originalclient.listSkillserror.While prompt discovery may currently suppress its own errors, appending a
.catchguarantees that the main catalog fallback safely completes.♻️ Proposed fix
- const promptRefresh = this.customPromptService.refresh(); + const promptRefresh = this.customPromptService.refresh().catch((err) => ({ + prompts: [], + diagnostics: [{ severity: "warning", code: "source-unavailable", message: String(err) }], + available: false, + }));🤖 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 423 - 429, Update the promptRefresh initialization in the catalog refresh method to catch and suppress refresh failures, ensuring the promise resolves safely. Keep the existing Promise.all flow and fallback catalog handling unchanged so client.listSkills or listPlugins errors still reach the original catch path without being masked by awaiting promptRefresh.
🤖 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-custom-prompt-service.ts`:
- Around line 258-271: Update the non-ENOENT catch block in the prompt-loading
method to preserve existing diagnostics collected before fileSystem.entries()
failed, appending the source-unavailable warning instead of replacing the
diagnostics array. Keep already-collected prompts and the available: false
result unchanged.
---
Nitpick comments:
In `@apps/server/src/services/codex-catalog-service.ts`:
- Around line 423-429: Update the promptRefresh initialization in the catalog
refresh method to catch and suppress refresh failures, ensuring the promise
resolves safely. Keep the existing Promise.all flow and fallback catalog
handling unchanged so client.listSkills or listPlugins errors still reach the
original catch path without being masked by awaiting promptRefresh.
In `@apps/server/src/transport/provider-catalog.ts`:
- Around line 58-67: Move PROVIDER_CATALOG_MAX_DIAGNOSTICS from
provider-catalog.ts into `@mcode/contracts` alongside the existing catalog limits,
then import and reuse that shared constant in both appendDiagnostic and the two
codex-catalog-service.ts slice calls, removing the duplicated literal 100.
🪄 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: 7fc08744-a087-4d0b-9c3e-44b10314e2d8
📒 Files selected for processing (29)
apps/server/src/container.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-prompt.tsapps/server/src/providers/codex/codex-provider.tsapps/server/src/services/__tests__/codex-catalog-service.test.tsapps/server/src/services/__tests__/codex-custom-prompt-service.test.tsapps/server/src/services/codex-catalog-service.tsapps/server/src/services/codex-custom-prompt-service.tsapps/server/src/services/provider-catalog-service.test.tsapps/server/src/services/provider-catalog-service.tsapps/server/src/services/skill-service.test.tsapps/server/src/services/skill-service.tsapps/server/src/services/skill-watcher-service.test.tsapps/server/src/services/skill-watcher-service.tsapps/server/src/transport/provider-catalog.test.tsapps/server/src/transport/provider-catalog.tsapps/server/src/transport/ws-router.tsapps/server/src/transport/ws-router.validation.test.tsapps/web/src/__tests__/slash-command-node.test.tsapps/web/src/__tests__/slash-command.test.tsapps/web/src/components/chat/Composer.tsxapps/web/src/components/chat/lexical/SlashCommandNode.tsxapps/web/src/components/chat/lexical/SlashCommandPlugin.tsxapps/web/src/components/chat/lexical/cursor-utils.tsapps/web/src/components/chat/useSlashCommand.tspackages/contracts/src/models/__tests__/mention.test.tspackages/contracts/src/models/mention.ts
💤 Files with no reviewable changes (1)
- apps/server/src/services/skill-watcher-service.ts
| } catch (error) { | ||
| if ((error as NodeJS.ErrnoException).code === "ENOENT") { | ||
| return { prompts: [], diagnostics: [], available: true }; | ||
| } | ||
| return { | ||
| prompts, | ||
| diagnostics: [{ | ||
| severity: "warning", | ||
| code: "source-unavailable", | ||
| message: "The effective Codex custom prompt directory could not be read.", | ||
| }], | ||
| available: false, | ||
| }; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Directory-level failure discards already-collected per-file diagnostics.
If fileSystem.entries() throws after some files were already processed (and diagnosed), prompts gathered so far are preserved but the earlier diagnostics entries are replaced wholesale by a single source-unavailable diagnostic, losing per-file context for files already inspected in this refresh.
🩹 Proposed fix to preserve earlier diagnostics
return {
prompts,
- diagnostics: [{
- severity: "warning",
- code: "source-unavailable",
- message: "The effective Codex custom prompt directory could not be read.",
- }],
+ diagnostics: [
+ ...diagnostics,
+ {
+ severity: "warning",
+ code: "source-unavailable",
+ message: "The effective Codex custom prompt directory could not be read.",
+ },
+ ].slice(0, CODEX_CUSTOM_PROMPT_MAX_DIAGNOSTICS),
available: false,
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } catch (error) { | |
| if ((error as NodeJS.ErrnoException).code === "ENOENT") { | |
| return { prompts: [], diagnostics: [], available: true }; | |
| } | |
| return { | |
| prompts, | |
| diagnostics: [{ | |
| severity: "warning", | |
| code: "source-unavailable", | |
| message: "The effective Codex custom prompt directory could not be read.", | |
| }], | |
| available: false, | |
| }; | |
| } | |
| } catch (error) { | |
| if ((error as NodeJS.ErrnoException).code === "ENOENT") { | |
| return { prompts: [], diagnostics: [], available: true }; | |
| } | |
| return { | |
| prompts, | |
| diagnostics: [ | |
| ...diagnostics, | |
| { | |
| severity: "warning", | |
| code: "source-unavailable", | |
| message: "The effective Codex custom prompt directory could not be read.", | |
| }, | |
| ].slice(0, CODEX_CUSTOM_PROMPT_MAX_DIAGNOSTICS), | |
| available: false, | |
| }; | |
| } |
🤖 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-custom-prompt-service.ts` around lines 258 -
271, Update the non-ENOENT catch block in the prompt-loading method to preserve
existing diagnostics collected before fileSystem.entries() failed, appending the
source-unavailable warning instead of replacing the diagnostics array. Keep
already-collected prompts and the available: false result unchanged.
1 participant
What
Add first-class support for Codex custom prompts from
CODEX_HOME/prompts, with bounded loading and source-aware invocation.Why
Mcode previously mixed Codex custom prompts into its general skill scan. That lost prompt identity, delayed picker updates, and could invoke a same-named skill or project command through the wrong path.
Key Changes
.mdprompt files from the same Codex home used to launch the provider, with entry, file-count, file-size, and diagnostic limits./prompts:*before manual invocation and apply the existing prompt argument expansion.~/.codex/promptsfrom the general skill scanner and watcher.Verification
TurnFileTrackertest reached its five-second timeout under full-suite load, then passed in a 45-test focused rerun.Closes #896