[codex] Fix doctor completion cache plugin loading#76235
Conversation
|
Codex review: found issues before merge. Reviewed July 5, 2026, 8:57 AM ET / 12:57 UTC. Summary PR surface: Source +20, Tests +39, Docs +1, Other +9. Total +69 across 7 files. Reproducibility: yes. by source inspection. Current main loads plugin commands for completion unless the skip env is set, while PR head routes several user-facing cache writes through Review metrics: 1 noteworthy metric.
Stored data model Root-cause cluster Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Risk before merge
Maintainer options:
Next step before merge
Security Review findings
Review detailsBest possible solution: Keep the explicit mode API, but restrict Do we have a high-confidence way to reproduce the issue? Yes, by source inspection. Current main loads plugin commands for completion unless the skip env is set, while PR head routes several user-facing cache writes through Is this the best way to solve the issue? No, not yet. The explicit mode split is the maintainable shape, but the current caller mapping and cache-existence check are too broad for plugin-command compatibility. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against e29448df08e2. Label changesLabel changes:
Label justifications:
Evidence reviewedPR surface: Source +20, Tests +39, Docs +1, Other +9. Total +69 across 7 files. View PR surface stats
Acceptance criteria:
What I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
|
|
The bug remains real, but this patch is not landable as-is on current
Best fix: recreate this narrowly on current |
ad61912 to
50db7c9
Compare
|
Rewritten around an explicit completion-cache generation mode instead of an ambient skip variable.
Proof:
The exact reviewed head is |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 31705ccff6
ℹ️ 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".
| options: CompletionCacheGenerationOptions, | ||
| ): Promise<boolean> { | ||
| const shell = options.shell ?? resolveShellFromEnv(); | ||
| const cacheExists = await completionCacheExists(shell, binName); |
There was a problem hiding this comment.
Regenerate full caches over core-only caches
When a doctor/update repair has already written a core-only completion cache, later onboarding paths pass generationMode: "full" but this helper still treats any existing cache file as sufficient. In that scenario setupWizardShellCompletion installs or accepts the stale core-only cache, so plugin CLI commands remain missing even though the user-facing/onboarding path requested a full completion cache; force regeneration for full mode or record enough metadata to distinguish cache modes.
Useful? React with 👍 / 👎.
31705cc to
84fa2d9
Compare
There was a problem hiding this comment.
💡 Codex Review
openclaw/src/commands/doctor-completion.ts
Line 300 in 84fa2d9
When callers omit options.shell, this helper checks only the detected shell above, but then spawns completion --write-state without --shell; the completion command treats that as writing every shell cache. In the new core-only update/repair callers, fixing one missing shell cache can therefore overwrite existing full caches for other shells with plugin-free scripts, causing users in those shells to lose plugin command completion. Pass the resolved shell into generateCompletionCache for these ensure writes.
ℹ️ 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 cacheGenerated = await ensureCompletionCacheExists(CLI_NAME); | ||
| const cacheGenerated = await ensureCompletionCacheExists(CLI_NAME, generationOptions); |
There was a problem hiding this comment.
Use full cache generation for prompted installs
When an interactive update finds no profile installed and the user accepts the Enable ... shell completion prompt, this reuses the core-only repair options. That mode sets OPENCLAW_COMPLETION_SKIP_PLUGIN_COMMANDS=1, and the completion command skips plugin CLI registration under that env var, so a newly installed completion profile omits every plugin command until the user knows to manually rebuild the cache. Keep core-only for repair-only paths, but generate a full cache for this initial install path.
Useful? React with 👍 / 👎.
84fa2d9 to
c327f7d
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c327f7d1c3
ℹ️ 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".
|
|
||
| if (shouldInstall) { | ||
| const generated = await generateCompletionCache(); | ||
| const generated = await generateCompletionCache({ generationMode: "core-only" }); |
There was a problem hiding this comment.
Generate full caches for doctor installs
When a user has no completion profile and accepts the interactive openclaw doctor prompt, this now writes the cache with generationMode: "core-only". That sets OPENCLAW_COMPLETION_SKIP_PLUGIN_COMMANDS=1, and src/cli/completion-cli.ts skips registerPluginCliCommandsFromValidatedConfig under that guard, so the newly installed profile points at a cache missing every plugin CLI command until the user manually rebuilds it. Keep the core-only mode for automatic doctor repairs, but use a full cache for this explicit install prompt.
Useful? React with 👍 / 👎.
c327f7d to
5b29853
Compare
5b29853 to
a34278d
Compare
|
Landed as 17c0ad8. The rewrite makes completion-cache generation explicit: doctor/update repair use |
* fix(completion): make cache generation mode explicit * test(completion): type spawned cache options * fix(completion): require explicit cache binary name
Summary
Fixes the Windows
openclaw doctorshell-completion cache path observed in the maintainer workshop: doctor was spawningopenclaw completion --write-statewithout the plugin-command skip guard, so completion cache generation could load plugin CLI commands and trigger bundled runtime dependency work during doctor.The update path already sets
OPENCLAW_COMPLETION_SKIP_PLUGIN_COMMANDS=1for this same cache write. This applies the same guard to doctor-triggered completion cache writes.Validation
pnpm exec vitest run src/commands/doctor-completion.test.ts src/cli/completion-cli.write-state.test.tspnpm exec oxlint src/commands/doctor-completion.ts src/commands/doctor-completion.test.ts