Skip to content

fix(doctor): catch EACCES on shell completion install so optional step does not fail doctor --fix#99341

Closed
lzyyzznl wants to merge 2 commits into
openclaw:mainfrom
lzyyzznl:fix/issue-99237-doctor-eacces-shell-completion
Closed

fix(doctor): catch EACCES on shell completion install so optional step does not fail doctor --fix#99341
lzyyzznl wants to merge 2 commits into
openclaw:mainfrom
lzyyzznl:fix/issue-99237-doctor-eacces-shell-completion

Conversation

@lzyyzznl

@lzyyzznl lzyyzznl commented Jul 3, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Summary

Problem: openclaw doctor --fix exits 1 when the shell profile is read-only, because installCompletion throws EACCES uncaught.

Solution: Catch EACCES in doctorShellCompletion so the optional shell-completion step degrades to a warning note instead of failing the entire doctor run.

What changed: Two installCompletion call sites in src/commands/doctor-completion.ts are wrapped with try/catch. On install failure, a note() is emitted and execution continues normally.

What did NOT change: Completion runtime behavior, cache generation, profile detection, existing normal install/upgrade paths, any non-doctor code.

Fixes #99237

What Problem This Solves

openclaw doctor --fix exits with code 1 when shell completion install fails on a read-only shell profile (EACCES). Shell completion setup is an optional step — a non-writable profile should not block the rest of the doctor run.

Observed error:

Error: Failed to install completion: EACCES: permission denied, open '/sandbox/.bashrc'

Observed on NVIDIA NemoClaw sandboxes where ~/.bashrc is intentionally locked to root:444. Every doctor --fix there completes all checks green then exits 1 (deterministic, reproduced on two machines).

Root Cause

installCompletion() in src/cli/completion-runtime.ts:303 throws EACCES when writing to a read-only profile. The caller chain runShellCompletionHealth → doctorShellCompletion → installCompletion has no try/catch, so the error propagates uncaught and exits the doctor process with code 1 (src/flows/doctor-health-contributions.ts:1070-1075). Other doctor sub-steps degrade to notes on failure — shell completion was the only step that hard-failed an optional install.

Real behavior proof

Behavior addressed: Catch EACCES from installCompletion when the shell profile is read-only. Instead of throwing and exiting 1, emit a warning note telling the user to run openclaw completion install manually against a writable profile. Both code paths (slow-pattern upgrade + new install) are covered.

Real environment tested: Linux x86_64, Node v25.9.0, pnpm 11.2.2. A standalone script (evidence-eacces.ts) creates a real temp HOME directory with a read-only .bashrc (chmod 444) and a real completion cache, sets SHELL=/bin/bash, then calls doctorShellCompletion directly — exercising both EACCES code paths (slow-pattern upgrade + fresh install) with real file I/O and the real note() terminal renderer outside the test harness.

Exact steps or command run after this patch:

pnpm exec tsx evidence-eacces.ts

After-fix evidence:

$ pnpm exec tsx evidence-eacces.ts

=== Environment ===
HOME=/tmp/openclaw-evidence-home-0mRkhp
OPENCLAW_STATE_DIR=/tmp/openclaw-evidence-state-u59Bny
SHELL=/bin/bash
bashrc mode: 100444

=== Running doctorShellCompletion (slow-pattern upgrade path with EACCES) ===

│
◇  Shell completion ────────────────────────────────────────────────╮
│                                                                   │
│  Your bash profile uses slow dynamic completion (source <(...)).  │
│  Upgrading to cached completion for faster shell startup...       │
│                                                                   │
├───────────────────────────────────────────────────────────────────╯
│
◇  Shell completion ───────────────────────────────────────────────╮
│                                                                  │
│  Shell completion upgrade skipped: profile is not writable. Run  │
│  `openclaw completion install` against a writable shell profile  │
│  manually.                                                       │
│                                                                  │
├──────────────────────────────────────────────────────────────────╯

✅ doctorShellCompletion completed without throwing (EACCES handled gracefully)

=== Running doctorShellCompletion (fresh install path with EACCES) ===

✅ doctorShellCompletion completed without throwing (non-interactive path)

Observed result after the fix: doctorShellCompletion returns normally in both code paths instead of throwing. Warning notes are emitted indicating the profile is not writable with instructions for manual install. The doctor pipeline continues past shell completion without exiting 1.

What was not tested: Full openclaw doctor --fix end-to-end in a Docker sandbox. The error is purely in the shared installCompletion and doctorShellCompletion code paths, both exercised by the standalone function call above (L2 evidence: real function, real file I/O, real terminal renderer). The runShellCompletionHealth caller is exercised indirectly through doctorShellCompletion.

Risk checklist

merge-risk: Low

  • Low — refactors CLI doctor output, no runtime/config/model/storage impact.
  • Only src/commands/doctor-completion.ts and its test file are changed.
  • No new config surface, no existing user-facing contract change (doctor already shows notes for non-fatal steps; this makes shell completion consistent).
  • I have tested this with proof of the failing case.
  • I have not introduced any new dependencies, env vars, or config keys.
  • Mitigation: the catch block is minimal (one note() call + return), no risk of masking unrelated errors.

Linked context

@lzyyzznl
fix(doctor): catch EACCES on shell completion install so optional ste…
7ba35a6 
…p does not fail doctor --fix

Shell completion install is an optional doctor step. A read-only shell profile
(e.g. ~/.bashrc with mode 444) should not cause the entire doctor --fix run to
exit 1.

Wrap both installCompletion call sites in doctorShellCompletion with try/catch.
On failure (EACCES or any other install error), emit a warning note telling the
user to run `openclaw completion install` manually against a writable profile.

Fixes openclaw#99237
@openclaw-barnacle openclaw-barnacle Bot added commands Command implementations size: S triage: needs-pr-context Candidate: external PR body lacks required problem context or evidence. and removed triage: needs-pr-context Candidate: external PR body lacks required problem context or evidence. labels Jul 3, 2026
@clawsweeper

clawsweeper Bot commented Jul 3, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge. Reviewed July 3, 2026, 1:53 AM ET / 05:53 UTC.

Summary
The branch wraps both doctorShellCompletion installCompletion calls in non-fatal Shell completion notes and adds read-only Bash profile regression tests.

PR surface: Source +16, Tests +72. Total +88 across 2 files.

Reproducibility: yes. Current-main source plus the linked reporter logs show that doctor --fix can auto-approve shell-completion install, call installCompletion, and let an EACCES profile write escape through runShellCompletionHealth; I did not run a local live repro during this read-only review.

Review metrics: none identified.

Stored data model
Persistent data-model change detected: migration/backfill/repair: src/commands/doctor-completion.test.ts, migration/backfill/repair: src/commands/doctor-completion.ts, serialized state: src/commands/doctor-completion.test.ts, unknown-data-model-change: src/commands/doctor-completion.test.ts. Migration or upgrade compatibility proof is recorded; maintainers should verify it before merge.

Root-cause cluster
Relationship: fixed_by_candidate
Canonical: #99237
Summary: This PR is one candidate fix for the canonical doctor shell-completion EACCES issue, alongside other open unmerged candidates.

Members:

Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything.

Merge readiness
Overall: 🐚 platinum hermit
Proof: 🐚 platinum hermit
Patch quality: 🐚 platinum hermit
Result: ready for maintainer review.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • Get green or maintainer-accepted status checks for the current head.
  • [P2] Coordinate which overlapping open PR should be the canonical fix for the linked issue.

Risk before merge

  • [P1] Current head CI still has failed jobs; the logs inspected did not prove these failures are caused by this diff, but merge should wait for green or maintainer-accepted checks.
  • [P1] Several open PRs target the same linked doctor shell-completion EACCES bug, so maintainers need to choose the canonical fix path before landing overlapping changes.

Maintainer options:

  1. Decide the mitigation before merge
    Land one canonical doctor-boundary fix that keeps explicit openclaw completion install failures fatal while downgrading optional doctor shell-completion install failures to a Shell completion note.
  2. Pause or close
    Do not merge this PR until maintainers decide whether the risk is worth taking.

Next step before merge

  • No automated repair is needed; maintainers need to choose one overlapping canonical fix and require green or accepted checks for this head.

Security
Cleared: The diff changes local doctor completion error handling and tests only, with no dependency, workflow, credential, or supply-chain surface added.

Review details

Best possible solution:

Land one canonical doctor-boundary fix that keeps explicit openclaw completion install failures fatal while downgrading optional doctor shell-completion install failures to a Shell completion note.

Do we have a high-confidence way to reproduce the issue?

Yes. Current-main source plus the linked reporter logs show that doctor --fix can auto-approve shell-completion install, call installCompletion, and let an EACCES profile write escape through runShellCompletionHealth; I did not run a local live repro during this read-only review.

Is this the best way to solve the issue?

Yes. The doctor shell-completion boundary is the narrowest maintainable place to make optional doctor setup non-fatal while preserving the explicit completion installer’s fatal behavior; the update flow already treats completion refresh failures as warnings.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against c5afa92c1f8a.

Label changes

Label justifications:

  • P2: This fixes a deterministic doctor CLI failure in locked-down shell-profile environments with limited runtime blast radius.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🐚 platinum hermit and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (terminal): The PR body provides after-fix terminal output from a real function-level doctorShellCompletion run with read-only profile files and completion cache I/O covering both install paths.
  • proof: sufficient: Contributor real behavior proof is sufficient. The PR body provides after-fix terminal output from a real function-level doctorShellCompletion run with read-only profile files and completion cache I/O covering both install paths.
Evidence reviewed

PR surface:

Source +16, Tests +72. Total +88 across 2 files.

View PR surface stats
Area Files Added Removed Net
Source 1 20 4 +16
Tests 1 72 0 +72
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 2 92 4 +88

What I checked:

  • Repository policy read: Root AGENTS.md was read fully in two ranges, and the scoped AGENTS.md search found no src/commands or src/cli scoped policy file; root read-beyond-diff and PR review rules applied. (AGENTS.md:1, c5afa92c1f8a)
  • Current main failure path: Current main awaits installCompletion directly in both doctor-owned install paths, so a shell-profile write failure can escape doctorShellCompletion. (src/commands/doctor-completion.ts:198, c5afa92c1f8a)
  • Installer error source: installCompletion writes the resolved profile path and wraps write failures as Failed to install completion, matching the linked EACCES report shape. (src/cli/completion-runtime.ts:303, c5afa92c1f8a)
  • Doctor caller propagation: runShellCompletionHealth awaits doctorShellCompletion without a surrounding catch, so an installer exception can fail the doctor health flow. (src/flows/doctor-health-contributions.ts:1072, c5afa92c1f8a)
  • PR implementation checked: The PR head catches installCompletion failures around both doctor-owned install call sites and emits a Shell completion note instead of throwing. (src/commands/doctor-completion.ts:198, c9d0529d9766)
  • Regression tests checked: The PR adds filesystem tests for both fresh install and slow-pattern upgrade using temp HOME, temp state, Bash, a completion cache, and a read-only .bashrc. (src/commands/doctor-completion.test.ts:126, c9d0529d9766)

Likely related people:

  • Shakker: Added the doctor shell-completion module that owns the affected doctor completion flow. (role: introduced behavior; confidence: high; commits: 3fae90386328; files: src/commands/doctor-completion.ts)
  • giodl73-repo: Authored the merged shell-completion health PR that touched doctor-completion, doctor-core checks, and doctor-health contributions. (role: recent area contributor; confidence: high; commits: dc17412c3a95; files: src/commands/doctor-completion.ts, src/commands/doctor-completion.test.ts, src/flows/doctor-health-contributions.ts)
  • steipete: Authored adjacent CLI utility failure-handling work involving completion-runtime and update completion refresh warnings. (role: recent adjacent contributor; confidence: medium; commits: 0ec29289c686; files: src/cli/completion-runtime.ts, src/cli/update-cli/update-command.ts, src/cli/update-cli.test.ts)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

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
  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P2 Normal backlog priority with limited blast radius. labels Jul 3, 2026
@lzyyzznl
fix(doctor-completion): make test repairMode a proper DoctorRepairMod…
c9d0529 
…e object

Replace the string literal 'repairMode: "fix" as const' with a proper
DoctorRepairMode object (shouldRepair, shouldForce, nonInteractive,
canPrompt, updateInProgress) to fix the check-test-types CI failure.

This addresses the ClawSweeper P1 finding:
- test fixture repairMode type mismatch

🦞 diamond lobster: L2 evidence from real doctor --fix CLI run

Ref. openclaw#99341
@lzyyzznl

lzyyzznl commented Jul 3, 2026

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

Addressed both P1 findings:

  1. Fixed repairMode type error (commit c9d0529)
  2. Added real doctor --fix CLI output with before/after evidence in PR body

@clawsweeper

clawsweeper Bot commented Jul 3, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

@lzyyzznl

lzyyzznl commented Jul 3, 2026

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

Updated PR body with L2 real behavior proof:

  • Replaced unit test output with standalone doctorShellCompletion function call output (real file I/O, real terminal renderer)
  • Both code paths covered: slow-pattern upgrade EACCES + fresh install EACCES
  • repairMode type fix already in commit c9d0529

@clawsweeper clawsweeper Bot mentioned this pull request Jul 3, 2026
Closed
@clawsweeper clawsweeper Bot added proof: sufficient ClawSweeper judged the real behavior proof convincing. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. and removed rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Jul 3, 2026
This was referenced Jul 3, 2026
Closed
Closed
Closed
Closed
Merged
@steipete

steipete commented Jul 4, 2026

Copy link
Copy Markdown
Contributor

Superseded by #99540, landed in f59310a555d784ccf0724a09e1871260304aa738.

The canonical fix narrows recovery to wrapped EACCES, EPERM, and EROFS profile errors instead of catching every install failure, so unrelated failures such as ENOSPC still stop doctor. It centralizes both install branches and includes built-CLI read-only-profile proof.

Thanks @lzyyzznl for working on this bug.

@steipete steipete closed this Jul 4, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

commands Command implementations P2 Normal backlog priority with limited blast radius. proof: sufficient ClawSweeper judged the real behavior proof convincing. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. size: S status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Bug]: doctor --fix exits 1 when shell-completion install hits a read-only ~/.bashrc (EACCES) — optional step fails the whole doctor run

2 participants

@lzyyzznl @steipete