fix: allow a separate supervisor bootstrap log directory - #150
TheAngryPit wants to merge 1 commit into
Conversation
|
Codex review: needs real behavior proof before merge. Reviewed October 2, 2026, 3:25 PM ET / 19:25 UTC (Revision 3). ClawSweeper reviewWhat this changesAdds an optional absolute directory for OCM supervisor startup logs, carries the selection into the service environment, and documents and tests its transient behavior. Merge readiness⛔ Blocked before merge - 5 items remain The recovery capability remains absent from current main and v0.2.48, so this PR still has distinct value. The previous review’s configuration decision and patched native proof requirements remain unresolved. Priority: P2 Review scores
Verification
How this fits togetherOCM’s supervisor runs managed OpenClaw gateways through an operating-system background service. Service generation selects the supervisor’s startup log paths, while gateway logs continue to use the OCM store. flowchart TD
A[CLI environment and OCM store] --> B{Bootstrap log override supplied}
B -->|Yes| C[Validate absolute directory]
B -->|No| D[Default store log directory]
C --> E[Generate background service definition]
D --> E
E --> F[OS service manager starts supervisor]
F --> G[Supervisor startup logs]
F --> H[Managed OpenClaw gateways]
Decision needed
Why: Both lifetimes are coherent interfaces, but the transient one can undo the recovery during later regeneration; choosing the supported contract requires maintainer intent. Before merge
Agent review detailsSecurityNone. Review metrics
Root-cause clusterRelationship: Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Provide an explicit bootstrap-only destination retained across later CLI calls and regeneration, preserve existing defaults and ownership checks, and demonstrate recovery on the affected Mac. Do we have a high-confidence way to reproduce the issue? Unclear for the native failure: the report provides a controlled path-only Mac diagnostic, but no patched current setup was exercised. Source establishes the missing independent destination and the candidate’s path-selection behavior. Is this the best way to solve the issue? Unclear until the configuration lifetime is approved. Bootstrap-only relocation is narrowly scoped, but durable explicit selection would avoid losing recovery when later CLI calls omit the variable. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against eb48770b06f4. LabelsLabel changes: No label changes. Label justifications:
EvidenceWhat I checked:
Likely related people:
Rank-up movesOptional improvements that raise the rating; they are not merge blockers.
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
History |
|
🦞👀 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. |
Related: #148
What Problem This Solves
Resolves a problem where operators keeping an OCM store on an affected external filesystem have no supported way to relocate just the supervisor's bootstrap logs. The observed workaround requires hand-editing a generated service definition, which OCM can overwrite later. This draft proposes a recovery control; it does not establish that the affected-host startup failure is fixed.
Why This Change Was Made
Proposes an opt-in
OCM_DAEMON_LOG_DIRfor only the supervisor'sdaemon.stdout.loganddaemon.stderr.log. Without it, paths are unchanged. With it, the generated definition and daemon summary use the same absolute directory, created through the existing private-directory writer. Empty and relative paths fail explicitly.The value is included in the supervisor environment so daemon-initiated operations retain it. Operators must keep it set for later CLI lifecycle/inspection commands; this is deliberately not a new persistent per-store setting. The usage guide explains that boundary and the maintenance-window requirement for applying a changed location to an existing service.
User Impact
Operators can keep state, runtimes, workspaces, working directory and gateway logs in their existing locations while putting just bootstrap logs on an accessible filesystem. No new dependencies, automatic relocation, permission grants or service-ownership changes.
This is independent of #149's plist ownership parser fix. Maintainer decision: is a caller-supplied override sufficient, or must the selection persist per store and survive later CLI invocations without the variable? This candidate implements only the former. It preserves the selection during regeneration when the variable is supplied, including by the generated daemon environment; it does not meet an unconditional durable-setting requirement. An automatic platform-specific default would be a separate compatibility choice.
Evidence
Base:
c5e7126c9b6cf7fb9b6f7903f59393f95e4c6ef1(0.2.38). Candidate:36eda83882adfc7bfc1e7ec5c4c4678ad8ba62c0.cargo test --locked supervisor::tests --lib: 27 passed, 0 failed.cargo check --workspace --all-targets --locked, formatting and diff checks passed.All five CI jobs at this head passed: formatting, Rust 1.88 minimum, Windows compilation, Ubuntu tests and macOS tests. Run the focused regression with
cargo test --locked daemon_bootstrap_log_override_preserves_store_logs --lib.Before merge: settle the configuration/persistence contract, then exercise a patched native binary on the affected external-volume host. Tests prove path selection and regeneration with the option present, not launchd access policy, reboot recovery or boot-time volume availability. This PR uses
Related, not an automatic issue-closing keyword, until native acceptance and the interface decision are settled. No live OCM service or installed executable was changed during this patch work.