Skip to content

fix: allow a separate supervisor bootstrap log directory - #150

Open
TheAngryPit wants to merge 1 commit into
openclaw:mainfrom
TheAngryPit:fix/daemon-bootstrap-log-directory
Open

TheAngryPit wants to merge 1 commit into
openclaw:mainfrom
TheAngryPit:fix/daemon-bootstrap-log-directory

Conversation

@TheAngryPit

@TheAngryPit TheAngryPit commented Sep 5, 2026 •

Copy link
Copy Markdown
Contributor

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_DIR for only the supervisor's daemon.stdout.log and daemon.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.

  • The new test first failed on unchanged production code because the requested bootstrap destination was ignored. It passes with this patch.
  • It checks both stream paths, unchanged store-log resolution and working directory, supervisor-environment propagation, matching summary paths, directory creation, repeat definition generation, default compatibility, and empty/relative path rejection.
  • Windows cargo test --locked supervisor::tests --lib: 27 passed, 0 failed. cargo check --workspace --all-targets --locked, formatting and diff checks passed.
  • Historical native observation on macOS 26.6.2: changing only the two bootstrap plist paths from an affected external volume to internal storage changed supervisor startup from exit 78 to running, followed by gateway health/catalog success. No privacy permission was changed.

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.

@clawsweeper clawsweeper Bot added P2 Normal priority bug or improvement with limited blast radius. merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Sep 5, 2026
@clawsweeper

clawsweeper Bot commented Sep 5, 2026 •

Copy link
Copy Markdown

Codex review: needs real behavior proof before merge. Reviewed October 2, 2026, 3:25 PM ET / 19:25 UTC (Revision 3).

ClawSweeper review

What this changes

Adds 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
Reviewed head: 36eda83882adfc7bfc1e7ec5c4c4678ad8ba62c0
Owner decision: Required. See Decision needed.

Review scores

Measure Result What it means
Overall readiness 🦪 silver shellfish (2/6) The implementation is focused with useful regression coverage, but changed-path native proof remains absent.
Proof confidence 🦪 silver shellfish (2/6) Needs real behavior proof before merge: The changed supervisor definition and summary paths have unit and CI coverage, but the historical native success used manual plist edits. Supply redacted patched-binary output showing default compatibility, opt-in installation, and existing-service refresh on the affected Mac; terminal output, logs, screenshots, or recordings count. Redact private paths, addresses, endpoints, and credentials. Updating the PR body should trigger re-review; otherwise ask a maintainer to comment @clawsweeper re-review. No persisted store schema or data-model contract changes. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Needs proof Needs real behavior proof before merge: The changed supervisor definition and summary paths have unit and CI coverage, but the historical native success used manual plist edits. Supply redacted patched-binary output showing default compatibility, opt-in installation, and existing-service refresh on the affected Mac; terminal output, logs, screenshots, or recordings count. Redact private paths, addresses, endpoints, and credentials. Updating the PR body should trigger re-review; otherwise ask a maintainer to comment @clawsweeper re-review. No persisted store schema or data-model contract changes. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Evidence reviewed 9 items Pinned introduced patch: The exact merge-base-to-head patch changes only docs/USAGE.md and src/supervisor/mod.rs. It adds bootstrap-only path selection and environment propagation; it does not alter gateway log selection, service ownership validation, dependencies, or release automation.
Current main still lacks independent bootstrap placement: Both daemon summary and generated service definition on fetched main derive bootstrap streams from supervisor_logs_dir. The store layout resolves that directory beneath the OCM store; current source contains no OCM_DAEMON_LOG_DIR capability.
Latest release retains existing behavior: The supplied latest release v0.2.48 also derives the supervisor bootstrap log directory from the store. It does not implement this PR’s recovery control.
Findings None None.
Security None None.

How this fits together

OCM’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]
Loading

Decision needed

Question Recommendation
May bootstrap-log selection require OCM_DAEMON_LOG_DIR on every later CLI invocation, or must the selected directory persist per store? Persist the explicit choice: Retain the selected directory across CLI invocations while leaving stores without a selection on their existing default.

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

  • Add real behavior proof - Needs real behavior proof before merge: The changed supervisor definition and summary paths have unit and CI coverage, but the historical native success used manual plist edits. Supply redacted patched-binary output showing default compatibility, opt-in installation, and existing-service refresh on the affected Mac; terminal output, logs, screenshots, or recordings count. Redact private paths, addresses, endpoints, and credentials. Updating the PR body should trigger re-review; otherwise ask a maintainer to comment @clawsweeper re-review. No persisted store schema or data-model contract changes. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
  • Resolve merge risk (P1) - Omitting the override during later CLI regeneration returns bootstrap logs to the potentially inaccessible store location; maintainers have not accepted that transient recovery contract.
  • Resolve merge risk (P1) - The current head conflicts with main; its refreshed integration result has not been reviewed.
  • Complete next step (P2) - Settle the configuration lifetime, resolve current-main conflicts, and provide patched affected-host startup and existing-service refresh proof before merge.
  • Resolve maintainer decision - Resolve the maintainer decision shown above before merge.
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Rust line delta production +21/-2; tests +56/-0 Production growth is confined to the stated override, validation, and service-environment propagation.

Root-cause cluster

Relationship: fixed_by_candidate
Canonical: #148
Summary: This PR proposes the bootstrap-log recovery capability tracked by the canonical issue; the merged ownership parser repair is separate.

Members:

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

Merge-risk options

Maintainer options:

  1. Retain the recovery selection (recommended)
    Choose a durable per-store contract and verify regeneration from a later CLI invocation without the original variable.
  2. Approve transient selection
    Explicitly accept the operator requirement to preserve the variable and verify recovery and regeneration under that contract.

Technical review

Best 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.

Labels

Label changes:

No label changes.

Label justifications:

  • P2: This supplies a bounded recovery capability for affected external-volume service setups.
  • merge-risk: 🚨 compatibility: Later regeneration without the variable can discard the selected recovery destination, and that configuration lifetime remains unapproved.
  • rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🐚 platinum hermit.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The changed supervisor definition and summary paths have unit and CI coverage, but the historical native success used manual plist edits. Supply redacted patched-binary output showing default compatibility, opt-in installation, and existing-service refresh on the affected Mac; terminal output, logs, screenshots, or recordings count. Redact private paths, addresses, endpoints, and credentials. Updating the PR body should trigger re-review; otherwise ask a maintainer to comment @clawsweeper re-review. No persisted store schema or data-model contract changes. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Evidence

What I checked:

  • Pinned introduced patch: The exact merge-base-to-head patch changes only docs/USAGE.md and src/supervisor/mod.rs. It adds bootstrap-only path selection and environment propagation; it does not alter gateway log selection, service ownership validation, dependencies, or release automation. (src/supervisor/mod.rs:798, 36eda83882ad)
  • Current main still lacks independent bootstrap placement: Both daemon summary and generated service definition on fetched main derive bootstrap streams from supervisor_logs_dir. The store layout resolves that directory beneath the OCM store; current source contains no OCM_DAEMON_LOG_DIR capability. (src/supervisor/mod.rs:943, eb48770b06f4)
  • Latest release retains existing behavior: The supplied latest release v0.2.48 also derives the supervisor bootstrap log directory from the store. It does not implement this PR’s recovery control. (src/supervisor/mod.rs:943, 6c563651c7f0)
  • Transient selection is explicit and undecided: The captured PR body and added usage guidance require the variable on subsequent CLI invocations. Regeneration without it returns to the default location. The contributor explicitly asks whether that lifetime is sufficient or the choice must persist per store; no maintainer acceptance is recorded. (docs/USAGE.md:358, 36eda83882ad)
  • Useful diagnostic, but no patched native acceptance: The captured body reports macOS recovery after manually changing only the plist output paths, plus passing path-selection tests and CI. It expressly says no installed executable or live service was changed by this patch work and patched affected-host verification remains outstanding. The focused test covers generated definitions, not native launchd startup. (src/supervisor/mod.rs:2739, 36eda83882ad)
  • Related work has separate boundaries: macOS supervisor cannot start with bootstrap logs on an affected external volume #148 remains the canonical external-volume bootstrap-placement report. The merged fix: accept reformatted launchd service ownership #149 repairs plist ownership parsing and does not supply bootstrap-log relocation.

Likely related people:

  • shakkernerd: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)
  • vincentkoc: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)

Rank-up moves

Optional improvements that raise the rating; they are not merge blockers.

  • Obtain an explicit decision on transient versus per-store configuration.
  • Provide patched native evidence covering unchanged defaults, opt-in installation, and existing-service refresh on the affected Mac.
  • Resolve current-main conflicts and refresh review and validation for the resulting head.

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
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.

Workflow

  • 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.

History

Review history (2 earlier review cycles)
  • reviewed 2026-09-05T22:42:50.178Z sha 36eda83 :: needs real behavior proof before merge. :: none
  • reviewed 2026-09-05T23:24:48.940Z sha 36eda83 :: needs real behavior proof before merge. :: none

@TheAngryPit
TheAngryPit marked this pull request as ready for review September 5, 2026 23:22
@clawsweeper

clawsweeper Bot commented Sep 5, 2026 •

Copy link
Copy Markdown

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. P2 Normal priority bug or improvement with limited blast radius. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant