Repository navigation
fix(daemon): unify recovery hints and preserve log paths - #139227
Conversation
|
🦞👀 Pull request received. I will update this pull request when review starts. ClawSweeper review completeClawSweeper finished reviewing this revision. The review result is being finalized. |
|
Codex review: blocked before merge. Reviewed September 5, 2026, 1:48 PM ET / 17:48 UTC. ClawSweeper reviewWhat this changesPreserves actual macOS log paths in recovery messages, shares status and doctor guidance, removes unused runtime-flag handling, and corrects documented LaunchAgent permissions. Merge readiness⛔ Blocked before merge - 1 item remains This remains a useful fix for a current diagnostic defect, with no blocking findings. The earlier migration-proof warning is unsupported: the patch changes neither stored representations nor log-writing behavior. Priority: P2 Review scores
Verification
How this fits togetherDaemon diagnostics turn native service state and resolved log paths into status output and doctor recovery guidance. These messages help operators inspect and recover their Gateway service. flowchart TD
A[Native service state] --> C[Status and doctor checks]
B[Profile and environment] --> C
B --> D[Canonical log paths]
C --> E[Shared recovery hints]
D --> E
D --> F[Lifecycle log writer]
E --> G[Operator diagnostics]
Before merge
Agent review detailsSecurityNone. PR surfaceSource -71, Tests -35, Docs 0. Total -106 across 16 files. View PR surface stats
Review metrics
Stored data modelPersistent data-model change detected: Root-cause clusterRelationship: Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Technical reviewBest possible solution: Keep log-path ownership in the existing resolvers and share recovery text while preserving each caller’s eligibility and policy decisions. Do we have a high-confidence way to reproduce the issue? Yes. Current-main source deterministically rewrites a literal backslash after resolving the written filename, matching the supplied macOS before/after evidence; this review did not execute the reproduction. Is this the best way to solve the issue? Yes. Removing the display-only rewrite repairs the failing boundary directly; changing the writer or migrating existing logs would unnecessarily alter correct behavior. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 3e4becea5e2c. LabelsLabel changes:
Label justifications:
EvidenceWhat I checked:
Likely related people:
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (1 earlier review cycle)
|
|
ClawSweeper compatibility checklist: no migration or upgrade conversion applies. |
|
Merged via squash.
|
* commit '925fb0c3353ce98f1c8d6d1f2c27bbb8472af095': fix(daemon): unify recovery hints and preserve log paths (openclaw#139227)
…9227) * fix(daemon): preserve log paths in shared recovery hints * docs(daemon): correct LaunchAgent file permission guidance
Closes #139226
Additional instructions
MUST: Keep Allow edits from maintainers enabled for this PR so maintainers
can help update the branch when needed.
What Problem This Solves
Fixes an issue where operators inspecting macOS Gateway recovery hints are directed to a nonexistent restart log when their state-directory name contains a literal backslash. The log is written successfully, but the advertised path replaces the backslash with a directory separator.
Why This Change Was Made
Recovery hints must display the path supplied by the log owner. This removes a display-only transformation introduced for a mocked Windows-path test, while retaining the existing producer behavior for native and cross-platform paths.
The same cleanup centralizes repeated GUI-session and stopped-service detail lines in the daemon hint owner. Status and doctor retain their own eligibility checks, headings, profile/container command context, installation restrictions, crash-loop guidance, and ordering. It also removes
missingSupervisionbranches left behind after the native producer switched to stopped/unknown/GUI-session state in #123961; current source and thev2026.9.1tag no longer produce that field.The daemon reference also corrects an existing file-mode claim: LaunchAgent plists use
0644, while the generated environment file (0600) and wrapper (0700) remain private. This documents existing behavior; it does not change permissions.Production changes are −71 lines; tests are −35 lines. There are no changes to configuration, schemas, service definitions, log writers, retention, or lifecycle authority.
User Impact
macOS recovery hints now identify the actual log file when a POSIX path contains a backslash. Existing status and doctor guidance—including systemd restart-limit recovery and the deliberate exit-78 exception—retains its text and behavior.
Evidence
Current head:
7c8a7424fbed0b6db6223d004a064da397888819. All 15 files from the reviewed source commit remain byte-identical; the successor only corrects the documented LaunchAgent file modes. Final-head source review is clean. Hosted CI passed, including the final CI gate; its preflight confirmed a merge of this exact head into base84b79be0d1f5556df5f4d131525c7a04f3b81db1.The reproduction ran on macOS 27.0 with Node v26.8.1 against source base
0e6b033d4bdb0ce6750f24fb9044bb524ddd344e. It used the actual lifecycle log writer and hint helper in a temporary fixture:<fixture>/state\literal/logs/gateway-restart.log<fixture>/state/literal/logs/gateway-restart.log<fixture>/state\literal/logs/gateway-restart.logsrc/daemon/restart-logs.test.tsfailed on the original source for this exact path mismatch and passes after the fix.402e3ce1b717c84a79832689ea75e1e2c5e7c1fa. The one-line documentation follow-up passed documentation checks, independent source review, and its own fresh mandatory P2 review. The direct review of final head7c8a7424fbed0b6db6223d004a064da397888819is also clean; hosted CI passed on that exact head.The filesystem and assembled-rendering proof did not start, stop, install, or restart a live service. No operator configuration or state was used. The changed classifier requires core/core-test validation; no package or lazy-import boundary changed, so no full build was run.
The completed ClawSweeper review covers this exact head and reports no correctness or security findings and no Rank-up moves. Its sole checklist item is an automated data-model compatibility flag.
ClawSweeper compatibility checklist: no migration or upgrade conversion applies.
src/commands/doctor-format.tsformats transient recovery guidance; it does not read or write persisted data. The removed optionalGatewayServiceRuntime.missingSupervisionfield has no current orv2026.9.1native producer, persisted reader, or public SDK input. Its replacement stopped/unknown/GUI-session states were already established by #123961. This PR changes no schema, stored representation, writer, retention, migration, or service definition. Current and stable producer/consumer inspection, the 312 passing tests, and the 1,944 rendering comparison rows support that unchanged compatibility contract.