Doctor: expose sandbox registry findings#84326
Conversation
|
Proof artifact for the sandbox structured repair stage: Run note and image source: https://gist.github.com/giodl73-repo/c6ff5e2d3d3d5ca04e5fd3c5fb0c660d Crabbox proof: direct AWS |
|
Codex review: needs maintainer review before merge. Reviewed June 21, 2026, 12:29 PM ET / 16:29 UTC. Summary PR surface: Source +73, Tests +88. Total +161 across 5 files. Reproducibility: not applicable. this is a feature PR for structured doctor output, not a bug report. The changed behavior is reviewable from the PR body, current source, and the doctor lint/repair code path. 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 detailsBest possible solution: Land the focused structured sandbox registry check after maintainer acceptance of warning-level lint behavior, or adjust severity/default selection if upgraded legacy residue should not fail lint. Do we have a high-confidence way to reproduce the issue? Not applicable: this is a feature PR for structured doctor output, not a bug report. The changed behavior is reviewable from the PR body, current source, and the doctor lint/repair code path. Is this the best way to solve the issue? Yes, with maintainer acceptance: keeping the structured check inside AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against c2ee9b0be8ae. Label changesLabel justifications:
Evidence reviewedPR surface: Source +73, Tests +88. Total +161 across 5 files. View PR surface stats
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
|
|
ClawSweeper PR egg 🎁 Pass real behavior proof to wake the egg and unlock a hatchable treat. Where did the egg go?
|
75cfed4 to
6d28631
Compare
34abca2 to
0559f8f
Compare
0559f8f to
3730823
Compare
4a18342 to
452e286
Compare
1ef5cd4 to
050c116
Compare
|
🦞🧹 I asked ClawSweeper to review this item again. |
|
@clawsweeper re-review Fresh exact-head re-review request for the sandbox registry proof. The PR body already includes the source CLI lint/fix proof for |
|
@clawsweeper re-review Updated the branch to signed head This addresses the latest review finding: structured sandbox registry output now says valid legacy entries migrate to SQLite, and the doctor health contribution description says SQLite registry storage. I also updated the PR body with copied dry-run repair-effect proof for all three outcomes: Validation passed locally: focused sandbox/contribution Vitest (60 tests), targeted |
|
🦞🧹 I asked ClawSweeper to review this item again. |
|
@clawsweeper re-review Pushed c3b4033 to address the sharded sandbox registry structured-output finding. Change made:
Validation:
|
|
🦞🧹 I asked ClawSweeper to review this item again. |
|
@clawsweeper re-review Updated the PR body for current head No code changes since the pushed fix commit. Existing validation for
|
|
@clawsweeper re-review Pushed Change made:
Validation on
|
|
🦞🧹 I asked ClawSweeper to review this item again. |
|
@clawsweeper re-review Follow-up body correction for #84326 at head |
|
@clawsweeper re-review Rebased #84326 onto current Current head:
Rebase resolution:
Validation on
|
|
🦞🧹 I asked ClawSweeper to review this item again. |
|
Updated #84326 at This push fixes the Validation at current head:
@clawsweeper please re-review current head. |
|
🦞👀 I queued a lightweight read-only assist pass. It will post a separate answer comment and will not edit the durable ClawSweeper review comment or trigger close, merge, repair, label, or branch changes. Request: please re-review current head. |
|
ClawSweeper assist: This request needs a full correctness review of current head Evidence:
Suggested next action: Comment Source: #84326 (comment) |
|
Rebased #84326 onto current Validation after rebase:
@clawsweeper please re-review current head. |
|
🦞👀 I queued a lightweight read-only assist pass. It will post a separate answer comment and will not edit the durable ClawSweeper review comment or trigger close, merge, repair, label, or branch changes. Request: please re-review current head. |
|
ClawSweeper assist: This needs a full correctness review of current head Evidence:
Suggested next action: Comment Source: #84326 (comment) |
|
Added a follow-up cleanup commit Validation for the follow-up cleanup:
@clawsweeper please re-review current head. |
|
🦞👀 I queued a lightweight read-only assist pass. It will post a separate answer comment and will not edit the durable ClawSweeper review comment or trigger close, merge, repair, label, or branch changes. Request: please re-review current head. |
|
ClawSweeper assist: This request needs a full correctness review of current head Evidence:
Suggested next action: Comment Source: #84326 (comment) |
galiniliev
left a comment
There was a problem hiding this comment.
This PR adds a structured core/doctor/sandbox/registry-files lint check for legacy sandbox registry residue while keeping actual mutation in the existing sandbox doctor repair path. That matters because upgraded installs with old monolithic or sharded registry state now get machine-readable doctor findings instead of only legacy human doctor output.
LOC: +175/-14 (7 files)
Findings: No blocking findings.
Bug/behavior: expose legacy sandbox registry residue through openclaw doctor --lint and structured repair previews while preserving openclaw doctor --fix as the real migration/quarantine/removal owner.
Affected surface: src/commands/doctor-sandbox.ts, src/flows/doctor-health-contributions.ts, and focused doctor/tooling tests.
Provenance: N/A; this is a structured doctor feature slice, not a regression fix.
Best-fix verdict: acceptable mitigation / likely best within the current split-doctor migration. Keeping the check owned by doctor:sandbox and leaving non-dry-run mutation in maybeRepairSandboxRegistryFiles matches the existing pattern used by legacy-owned structured checks.
Alternatives considered:
- Move real migration into the split
repair()implementation now. Rejected for this PR becauserunSandboxHealthalready owns the mutating doctor path anddoctor --fixcallsmaybeRepairSandboxRegistryFilesdirectly; moving it here would be a broader ownership change. - Make the finding
infoinstead ofwarning. Rejected if maintainers accept that stale, noncanonical registry residue should fail default lint untilopenclaw doctor --fixhandles it; this is the open product choice already called out by ClawSweeper. - Add a separate contribution for this check. Rejected because the ordered
doctor:sandboxcontribution is the right owner for sandbox registry residue, and an extra registry entry would split the same invariant.
Code read: src/commands/doctor-sandbox.ts, src/agents/sandbox/registry.ts, src/flows/doctor-health-contributions.ts, src/flows/doctor-health.ts, src/flows/doctor-repair-flow.ts, src/flows/health-check-adapter.ts, src/commands/doctor-lint.ts, scripts/bench-gateway-restart.ts, src/scripts/test-projects.test.ts, test/scripts/bench-gateway-restart.test.ts, and adjacent changed tests. Also checked scoped guides scripts/AGENTS.md and test/AGENTS.md.
Remaining uncertainty: I did not rerun the PR's Vitest/tsgo matrix locally because the checkout has unrelated local edits; I verified the diff against fetched origin/main and checked git diff --check origin/main...origin/pr/84326 locally. Maintainer acceptance is still needed for warning-level doctor --lint behavior on upgraded installs with legacy sandbox registry files.
|
Merged via squash.
Thanks @giodl73-repo! |
Summary
doctor:sandboxlegacy run path as the only real mutating sandbox registry migration path for normaldoctor/doctor --fix.This is intentionally the small sandbox registry slice from the doctor repair stack. It does not migrate sandbox image builds, namespace probes, Docker checks, or plugin-sensitive doctor rules.
Stack
Stacked on #86627, which has landed, and rebased onto current
upstream/main.Current branch head is signed commit
ba2742aa6a05f1b5626e7c44e05acebe77c7ba63(Fix unused session accessor destructure). The last commit is a narrow rebased-base cleanup for an unusedsessionStoredestructure inresolveSessionTranscriptRuntimeReadTarget(...); it was needed so prod type/lint CI passes on current main.Real behavior proof
Behavior or issue addressed:
Legacy sandbox registry files are now visible through structured doctor health checks, while the existing
doctor --fixpath still owns the actual migration from legacy JSON/sharded registry files into SQLite-backed sandbox registry storage. Structured dry-run effects expose the intended migrate, quarantine, and remove-empty outcomes without mutating state.Real environment tested:
WSL Ubuntu 24.04, OpenClaw checkout
/root/src/openclaw-84326-proof, prior PR headdc2a3c13eb2b6dd41c7ad63926540d4f89391a7c, source CLI vianode scripts/run-node.mjs, source harness vianode --import tsx --input-type=module, isolated tempOPENCLAW_STATE_DIR, tempOPENCLAW_CONFIG_PATH, tempHOME, and synthetic legacy sandbox registry fixtures. The branch has since been rebased and updated toba2742aa6a05f1b5626e7c44e05acebe77c7ba63; the latest functional sandbox behavior is unchanged.Exact steps or command run after this patch:
sandbox/containers.jsonmigration into SQLite-backedsandbox_registry_entries.legacySandboxRegistryInspectionToHealthFindingandlegacySandboxRegistryInspectionToRepairEffectfromsrc/commands/doctor-sandbox.ts.checks-node-core-toolingbenchmark test locally, updated it to assert the SQLite restart intent row, and reran that tooling test successfully.sessionStoredestructure exposed by currentupstream/main, then reranpnpm tsgo:coresuccessfully.Evidence after fix:
The structured finding points at SQLite, and dry-run repair effects cover all three registry-file outcomes with source-specific targets. The earlier real CLI fix proof still applies to the mutating path:
doctor --fix --yes --non-interactiveremoved the legacycontainers.json, printed the migration/doctor-complete markers, exited 0, and persisted the syntheticopenclaw-sbx-proofrow in SQLitesandbox_registry_entries.Observed result after fix:
Structured sandbox registry findings and contribution metadata describe SQLite-backed registry storage. The dry-run repair-effect path reports migrate, quarantine, and remove-empty effects without mutating state, and sharded inspections report the sharded registry directory instead of the monolithic JSON file. The real mutating path remains owned by
doctor --fix.What was not tested:
The proof used synthetic legacy registry fixtures and did not run Docker, build sandbox images, probe namespaces, or exercise live sandbox container lifecycle. The prior
doctor --fixsource CLI run emitted an unrelated Gateway credentials warning while checking local Gateway status, but the command still exited 0 and the sandbox registry migration completed.ClawSweeper and maintainer status
Latest visible ClawSweeper status:
Maintainer severity finding:
core/doctor/sandbox/registry-filesas awarning, notinfo.openclaw doctor --fix.warningis for state the user should correct even if OpenClaw can still run;infois for advisory state that can be left indefinitely without affecting canonical behavior, repair clarity, upgrade safety, or reliability.rfcs/needs_refactoring/0002-doctor-health-upgrades.md, now records the warning/info bar in the structured Doctor health contract.Validation
Current head
ba2742aa6a05f1b5626e7c44e05acebe77c7ba63:node scripts\run-vitest.mjs run --config test\vitest\vitest.tooling.config.ts test\scripts\bench-gateway-restart.test.ts --reporter=verbose- 31 passed, 2 skipped$env:OPENCLAW_VITEST_MAX_WORKERS='1'; node scripts\run-vitest.mjs run src\scripts\test-projects.test.ts src\commands\doctor-sandbox.warns-sandbox-enabled-without-docker.test.ts src\flows\doctor-health-contributions.test.ts --reporter=dot- 143 tests passed across 3 shardspnpm exec oxlint scripts\bench-gateway-restart.ts test\scripts\bench-gateway-restart.test.ts src\scripts\test-projects.test.ts src\commands\doctor-sandbox.ts src\commands\doctor-sandbox.warns-sandbox-enabled-without-docker.test.ts src\flows\doctor-health-contributions.ts src\flows\doctor-health-contributions.test.ts src\config\sessions\session-accessor.tspnpm exec oxfmt --check scripts\bench-gateway-restart.ts test\scripts\bench-gateway-restart.test.ts src\scripts\test-projects.test.ts src\commands\doctor-sandbox.ts src\commands\doctor-sandbox.warns-sandbox-enabled-without-docker.test.ts src\flows\doctor-health-contributions.ts src\flows\doctor-health-contributions.test.ts src\config\sessions\session-accessor.tspnpm tsgo:corepnpm tsgo:test:srcgit diff --check