fix(shields): recover a locked sandbox whose startup never completed - #8460
fix(shields): recover a locked sandbox whose startup never completed#8460rluo8 wants to merge 8 commits into
Conversation
Signed-off-by: Rui Luo <ruluo@nvidia.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughOpenClaw failed-startup recovery now validates terminal startup failure, shares the mutation lock with the state-directory guard, unlocks configuration and state atomically, and restores both layers on failure. Shields integration, tests, E2E coverage, and recovery documentation expose the new flow. ChangesOpenClaw recovery flow
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant ShieldsDown
participant OpenClawConfigGuard
participant StateDirGuard
participant Sandbox
ShieldsDown->>OpenClawConfigGuard: handle startup-not-ready
OpenClawConfigGuard->>OpenClawConfigGuard: verify failed startup and reconfirm under mutex
OpenClawConfigGuard->>StateDirGuard: run unlock-failed-startup with plan and lock descriptor
StateDirGuard-->>OpenClawConfigGuard: return guard result
OpenClawConfigGuard-->>ShieldsDown: return recovery or rollback error
ShieldsDown->>Sandbox: restart after successful unlock
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
|
🌿 Preview your docs: https://nvidia-preview-pr-8460.docs.buildwithfern.com/nemoclaw |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
src/lib/shields/openclaw-config-lock.test.ts (1)
421-426: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the forwarded plan value, not only the flag.
toContain("--plan-json")passes even if the flag is pushed without its value or with the wrong value. The test title claims the recovery "forwards the plan", so assert the adjacent argument.💚 Proposed assertion
- expect(recovery).toContain("--plan-json"); + expect(recovery?.slice(-2)).toEqual(["--plan-json", '{"version":1}']);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/shields/openclaw-config-lock.test.ts` around lines 421 - 426, Update the recovery assertion around the recorded unlock-failed-startup command to verify that --plan-json is immediately followed by the expected plan value, rather than only checking for the flag. Preserve the existing timeout assertions and use the plan value established by the test setup.src/lib/shields/index.ts (1)
2138-2155: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBoth branch decisions depend on free-text guard details.
NOT_A_FAILED_STARTUPmatches the detail string built inscripts/openclaw-config-guard.pyat line 4342, andisOpenClawStartupNotReadymatches the rendered[startup-not-ready]code. Neither string is asserted by a shared contract. If the Python detail text is reworded, this branch stops recognizing the refusal and reports a hard failure instead of falling back.Both mismatch directions fail safely, so this is a maintainability concern rather than a defect. The guard already emits a structured
codeper issue;runOpenClawConfigGuardflattens it into a message before returning. Consider exposing the parsedGuardIssuecodes onOpenClawConfigGuardResultand matching oncode === "startup-not-ready".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/shields/index.ts` around lines 2138 - 2155, Update runOpenClawConfigGuard and OpenClawConfigGuardResult to preserve and expose each parsed GuardIssue code alongside its message, then change recoverOpenClawFailedStartupShields to identify the not-applicable case using code === "startup-not-ready" rather than matching NOT_A_FAILED_STARTUP or rendered free text. Keep non-startup issues surfaced as errors and remove the obsolete text-based constant and checks.src/lib/shields/openclaw-config-lock.ts (1)
355-360: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winRequire
planJsonlocally forunlock-failed-startup.
write-configrejects a missingexpectedConfigSha256before it runs anything privileged.unlock-failed-startuphas no equivalent check. IfplanJsonis absent or empty, line 431 silently drops the flag, the guard runs in the sandbox, acquires the mutation mutex, and only then fails withinvalid-state-lock-plan.Add the precondition next to the
write-configcheck. The truthiness test on line 431 also drops an empty string without a diagnostic.♻️ Proposed precondition
if (action === "write-config" && !options.expectedConfigSha256) { return { issues: ["OpenClaw config guard write-config requires expectedConfigSha256"], chattrApplied: false, }; } + if (action === "unlock-failed-startup" && !options.planJson) { + return { + issues: ["OpenClaw config guard unlock-failed-startup requires planJson"], + chattrApplied: false, + }; + }Also applies to: 431-431
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/shields/openclaw-config-lock.ts` around lines 355 - 360, Add a local precondition alongside the existing write-config validation for unlock-failed-startup that rejects a missing or empty options.planJson with the appropriate invalid-state-lock-plan issue before any privileged or mutex-protected work runs. Update the planJson handling near the guard invocation so empty values are not silently omitted and instead produce the same diagnostic.test/openclaw-config-guard-startup-failure-gate.test.ts (1)
85-113: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider covering the
main()gate, not only the helper gate.
cli_acceptsproves thatunlock-failed-startupappears in the parser choices and matches theActionunion. The tests then exercise_validate_action_readinessand_reconfirm_startup_failure_recoverydirectly.Two
main()refusals stay unproven:
main()raisesstartup-not-readywhen_validate_action_readinessreturnsFalseforunlock-failed-startup.main()raisesinvalid-state-lock-planwhen--plan-jsonis absent.Both are the last gates before the unseal runs. Add harness cases that call
guard.main([...])with a stubbed identity and assert the emitted issue codes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/openclaw-config-guard-startup-failure-gate.test.ts` around lines 85 - 113, Extend the startup-failure recovery harness to exercise guard.main() rather than only the helper gates. Add cases with a stubbed identity that verify main() emits startup-not-ready when _validate_action_readiness rejects unlock-failed-startup, and invalid-state-lock-plan when --plan-json is omitted; assert the emitted issue codes while preserving the existing helper coverage.docs/manage-sandboxes/recover-rebuild-sandboxes.mdx (1)
26-33: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPlace the shields guidance before the rebuild guidance.
Line 26 directs the reader to the printed
rebuild --yesguidance. The new block at line 29 then tells the reader to lower shields "before reaching for a rebuild". A reader who follows the page in order rebuilds first, which is the outcome this PR removes.Move the
<AgentOnly variant="openclaw">block above line 26.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/manage-sandboxes/recover-rebuild-sandboxes.mdx` around lines 26 - 33, Move the AgentOnly block for OpenClaw shields guidance before the paragraph directing readers to rebuild missing or failed containers, preserving its content and placement relative to the surrounding instructions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/openclaw-config-guard.py`:
- Around line 1481-1534: The recovery flow must use one shared deadline instead
of granting each state-dir guard invocation a separate full timeout. In
scripts/openclaw-config-guard.py lines 1481-1534, update _run_state_dir_guard
and its _run_failed_startup_unlock callers to compute the whole-action deadline
and pass only the remaining time to each unlock or relock invocation, ensuring
rollback cannot exceed the host budget. In
src/lib/shields/openclaw-config-lock.ts lines 22-24, set
RECOVERY_CONTAINER_TIMEOUT to cover the guard’s combined unlock-and-rollback
worst case and document both constants as a single coordinated budget.
In `@src/lib/shields/index.ts`:
- Around line 2124-2135: Move the existing shield-lowering doc comment from
above isOpenClawStartupNotReady to immediately above
recoverOpenClawFailedStartupShields, leaving the error-classification helper
with documentation that matches its actual behavior.
---
Nitpick comments:
In `@docs/manage-sandboxes/recover-rebuild-sandboxes.mdx`:
- Around line 26-33: Move the AgentOnly block for OpenClaw shields guidance
before the paragraph directing readers to rebuild missing or failed containers,
preserving its content and placement relative to the surrounding instructions.
In `@src/lib/shields/index.ts`:
- Around line 2138-2155: Update runOpenClawConfigGuard and
OpenClawConfigGuardResult to preserve and expose each parsed GuardIssue code
alongside its message, then change recoverOpenClawFailedStartupShields to
identify the not-applicable case using code === "startup-not-ready" rather than
matching NOT_A_FAILED_STARTUP or rendered free text. Keep non-startup issues
surfaced as errors and remove the obsolete text-based constant and checks.
In `@src/lib/shields/openclaw-config-lock.test.ts`:
- Around line 421-426: Update the recovery assertion around the recorded
unlock-failed-startup command to verify that --plan-json is immediately followed
by the expected plan value, rather than only checking for the flag. Preserve the
existing timeout assertions and use the plan value established by the test
setup.
In `@src/lib/shields/openclaw-config-lock.ts`:
- Around line 355-360: Add a local precondition alongside the existing
write-config validation for unlock-failed-startup that rejects a missing or
empty options.planJson with the appropriate invalid-state-lock-plan issue before
any privileged or mutex-protected work runs. Update the planJson handling near
the guard invocation so empty values are not silently omitted and instead
produce the same diagnostic.
In `@test/openclaw-config-guard-startup-failure-gate.test.ts`:
- Around line 85-113: Extend the startup-failure recovery harness to exercise
guard.main() rather than only the helper gates. Add cases with a stubbed
identity that verify main() emits startup-not-ready when
_validate_action_readiness rejects unlock-failed-startup, and
invalid-state-lock-plan when --plan-json is omitted; assert the emitted issue
codes while preserving the existing helper coverage.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 03025dce-2084-4d37-a703-5cdd5b3a08c8
📒 Files selected for processing (7)
docs/manage-sandboxes/recover-rebuild-sandboxes.mdxscripts/openclaw-config-guard.pyscripts/state-dir-guard.pysrc/lib/shields/index.tssrc/lib/shields/openclaw-config-lock.test.tssrc/lib/shields/openclaw-config-lock.tstest/openclaw-config-guard-startup-failure-gate.test.ts
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported Model lanes
Second-opinion terminology and E2E selections are advisory. Live E2E does not run automatically for pull requests. 3 semantic terminology decisionsTerminology decisions are advisory. They affect the assessment only when a separate finding identifies concrete semantic impact.
E2E guidanceAdvisory only. A maintainer can dispatch the default E2E suite against this exact revision. Recommended E2E: 1 warning · 0 suggestionsWarningsWarnings do not block.
|
Signed-off-by: Rui Luo <ruluo@nvidia.com>
There was a problem hiding this comment.
🧹 Nitpick comments (2)
test/openclaw-config-guard-startup-failure-gate.test.ts (2)
140-149: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBind the outcome variables before the
tryblocks.
timeout_code,transaction_error, andtimeout_transaction_codeare assigned only in theexcept guard.GuardErrorbranch. If the guard stops raising, the harness fails at line 199 withNameErrorinstead of reporting the missing error. That hides the regression the test is meant to catch. Initialize each name to a sentinel so the JSON payload shows the real outcome and the Vitest diff names the failure.♻️ Proposed change
+timeout_code = "no-error-raised" try: guard._run_state_dir_guard(+transaction_error = {"code": "no-error-raised", "detail": ""} try: guard._run_failed_startup_unlock( object(), object(), guard.PRODUCTION_CONFIG_DIR, "{}", 91, quarantine_untrusted=False )+timeout_transaction_code = "no-error-raised" try: guard._run_failed_startup_unlock( object(), object(), guard.PRODUCTION_CONFIG_DIR, "{}", 91, quarantine_untrusted=False ) except guard.GuardError as error: timeout_transaction_code = error.codeAlso applies to: 167-196
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/openclaw-config-guard-startup-failure-gate.test.ts` around lines 140 - 149, Initialize timeout_code, transaction_error, and timeout_transaction_code to distinct sentinel values before their respective try blocks in the test harness. Keep the existing GuardError assignments unchanged so successful exception paths overwrite the sentinels, while missing exceptions produce a JSON payload and Vitest diff that clearly report the unexpected outcome instead of raising NameError.
132-132: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNarrow the
os.path.isfilepatch.
guard.osis the realosmodule, so this assignment replacesos.path.isfilefor the whole interpreter, includingsubprocess,tempfile, andimportlib. The patch is broader than the behavior under test. Scope it to the guard's own lookup, or restore it after the calls that need it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/openclaw-config-guard-startup-failure-gate.test.ts` at line 132, Replace the global guard.os.path.isfile assignment with a scoped patch that affects only the guard’s own lookup, or reliably restores the original function immediately after the required calls. Keep the test’s forced-file-exists behavior while preventing subprocess, tempfile, and importlib from observing the patched os.path.isfile.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@test/openclaw-config-guard-startup-failure-gate.test.ts`:
- Around line 140-149: Initialize timeout_code, transaction_error, and
timeout_transaction_code to distinct sentinel values before their respective try
blocks in the test harness. Keep the existing GuardError assignments unchanged
so successful exception paths overwrite the sentinels, while missing exceptions
produce a JSON payload and Vitest diff that clearly report the unexpected
outcome instead of raising NameError.
- Line 132: Replace the global guard.os.path.isfile assignment with a scoped
patch that affects only the guard’s own lookup, or reliably restores the
original function immediately after the required calls. Keep the test’s
forced-file-exists behavior while preventing subprocess, tempfile, and importlib
from observing the patched os.path.isfile.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 8ae24f70-3173-4cd2-945b-a020f107bd20
📒 Files selected for processing (3)
scripts/openclaw-config-guard.pysrc/lib/shields/index.tstest/openclaw-config-guard-startup-failure-gate.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/lib/shields/index.ts
- scripts/openclaw-config-guard.py
Signed-off-by: Rui Luo <ruluo@nvidia.com>
Signed-off-by: Rui Luo <ruluo@nvidia.com>
|
CI classification for the current branch revision:
No PR code change or rerun is indicated by this failure. The remaining repository gate is independent approval. |
|
Security review receipt for the current branch revision (
Reviewed files: The existing documentation receipt check passes, and the documentation accurately describes the OpenClaw-only recovery flow. No additional documentation change is indicated by this review. |
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
Maintainer follow-up on the current branch revision ( Review-warning dispositionThe installed-sandbox evidence gap is addressed in
Successful recursive unlock is also evidence that the state guard accepted the mutation-lock descriptor inherited from the config guard; a separately opened descriptor remains covered by the negative unit test. Validation
The live Security review
No security finding requires a code change. Independent documentation review approved Remaining blockers
I am leaving the PR open and unmerged until those gates are resolved. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
test/e2e/live/shields-config.test.ts (1)
273-288: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReduce coupling to guard internals in the census probe.
_production_identityand_openshell_supervised_nonroot_start_censusreturn the expected values, butrunpy.run_pathstill binds this E2E test to private implementation details. Use a stable container liveness boundary, such as/proc, or expose a supported diagnostic boundary.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/e2e/live/shields-config.test.ts` around lines 273 - 288, Update the census probe around the script invoking runpy.run_path to stop importing and calling the private guard symbols _production_identity and _openshell_supervised_nonroot_start_census. Replace this with a stable supported diagnostic boundary, preferably a /proc-based container liveness/process census, while preserving the StartupCensus JSON shape and existing Docker execution assertions.Source: Path instructions
test/openclaw-config-guard-startup-failure-gate.test.ts (3)
244-247: 🔒 Security & Privacy | 🔵 Trivial | 🏗️ Heavy liftThe privilege boundary is not exercised:
root_uidequalssandbox_uidhere.The child sets
root_uidandsandbox_uidto the sameos.getuid(). Every check that distinguishes the root layer from the sandbox layer therefore passes trivially. The PR review record lists this as the outstanding blocker: the installed-sandbox boundary has no coverage for childless recovery success and for live-child refusal.The synthetic-
/procHARNESScoverslive_recoveryandreconfirm_child_appearedat the census level, but no test drives the installed helper with distinct root and sandbox uids.Do you want me to draft the installed-sandbox test that separates the two uids and covers both outcomes?
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/openclaw-config-guard-startup-failure-gate.test.ts` around lines 244 - 247, Update the installed-sandbox test setup around guard.Identity to use distinct root and sandbox UIDs, then add coverage for both childless recovery success and refusal when a live child is present. Preserve the existing synthetic-/proc census tests while exercising the installed helper across the actual privilege boundary.
85-88: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert CLI exposure through the parser boundary, not
parser._actions.
cli_acceptsreads the argparse private attribute_actions. An argparse internal rename would break this test without any behavior change. Useparse_argsinstead, and keep thetyping.get_args(guard.Action)cross-check.♻️ Proposed refactor to use the public parser boundary
def cli_accepts(action): parser = guard._parser() - choices = next(a.choices for a in parser._actions if a.dest == "action") - return action in set(choices) and set(choices) == set(typing.get_args(guard.Action)) + try: + parsed = parser.parse_args([action, "--config-dir", guard.PRODUCTION_CONFIG_DIR]) + except SystemExit: + return False + return parsed.action == action and parsed.plan_json is NoneThen assert the
Actionliteral coverage separately, for example by parsing every value oftyping.get_args(guard.Action).As per path instructions: "Prefer observable outcomes through the public boundary over source-text, private-shape, or mock-call assertions."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/openclaw-config-guard-startup-failure-gate.test.ts` around lines 85 - 88, Update cli_accepts to stop inspecting the private parser._actions structure and validate actions through guard._parser().parse_args instead. Preserve the typing.get_args(guard.Action) cross-check by separately parsing every Action literal value and confirming each is accepted by the CLI boundary.Source: Path instructions
159-196: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd an assertion that the rollback receives the remaining budget, not a fresh one.
Both fakes ignore
_deadline. No test proves that_run_failed_startup_unlockshares one deadline between the unseal and the relock. A regression that gives the rollback a freshSTATE_DIR_GUARD_TIMEOUT_SECONDSwould still pass this suite, and that regression is what makes the guard outlive the host budget.Record the
_deadlinevalue in each fake and assert that thelockrollback deadline equals theunlockdeadline.♻️ Proposed addition to capture the shared deadline
+deadlines = [] + def state_dir(action, _config_dir, _plan_json, lock_fd, _deadline): assert lock_fd == 91 + deadlines.append(_deadline) events.append(f"state-{action}") if action == "lock": raise guard.GuardError("state-lock-failed", guard.PRODUCTION_CONFIG_DIR, "lock failed")Then export
"shared_deadline": len(set(deadlines)) == 1and assert it istruein the Vitest expectation.As per path instructions: "Migration tests must prove the superseded path is unreachable or removed, not merely prove that the new path also works."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/openclaw-config-guard-startup-failure-gate.test.ts` around lines 159 - 196, Update the test fakes used by _run_failed_startup_unlock to record each _deadline received during the unseal and rollback transitions, then expose whether all recorded deadlines are identical (for example, via the existing result returned to the Vitest expectation). Assert that shared-deadline value is true, proving the lock rollback reuses the remaining budget rather than creating a fresh timeout.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/openclaw-config-guard.py`:
- Around line 1576-1584: Update _transition to pass its quarantine_untrusted
argument through to _freeze. Ensure both the forward unlock path and
failed-startup rollback invoke _transition with the correct quarantine setting
so untrusted reserved entries are quarantined.
In `@test/e2e/live/shields-config.test.ts`:
- Around line 312-317: Update the termination step around the docker kill
invocation and the terminate.exitCode assertion to tolerate the startup process
exiting between the census and signal. Validate the subsequent census result as
the source of truth, or conditionally accept the known “no such process” exit
while preserving failures for unexpected kill errors.
---
Nitpick comments:
In `@test/e2e/live/shields-config.test.ts`:
- Around line 273-288: Update the census probe around the script invoking
runpy.run_path to stop importing and calling the private guard symbols
_production_identity and _openshell_supervised_nonroot_start_census. Replace
this with a stable supported diagnostic boundary, preferably a /proc-based
container liveness/process census, while preserving the StartupCensus JSON shape
and existing Docker execution assertions.
In `@test/openclaw-config-guard-startup-failure-gate.test.ts`:
- Around line 244-247: Update the installed-sandbox test setup around
guard.Identity to use distinct root and sandbox UIDs, then add coverage for both
childless recovery success and refusal when a live child is present. Preserve
the existing synthetic-/proc census tests while exercising the installed helper
across the actual privilege boundary.
- Around line 85-88: Update cli_accepts to stop inspecting the private
parser._actions structure and validate actions through
guard._parser().parse_args instead. Preserve the typing.get_args(guard.Action)
cross-check by separately parsing every Action literal value and confirming each
is accepted by the CLI boundary.
- Around line 159-196: Update the test fakes used by _run_failed_startup_unlock
to record each _deadline received during the unseal and rollback transitions,
then expose whether all recorded deadlines are identical (for example, via the
existing result returned to the Vitest expectation). Assert that shared-deadline
value is true, proving the lock rollback reuses the remaining budget rather than
creating a fresh timeout.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: e32d1f6c-f0d5-4328-a60c-2e1e8c209a0f
📒 Files selected for processing (8)
docs/manage-sandboxes/recover-rebuild-sandboxes.mdxscripts/openclaw-config-guard.pyscripts/state-dir-guard.pysrc/lib/shields/index.tssrc/lib/shields/openclaw-config-lock.test.tssrc/lib/shields/openclaw-config-lock.tstest/e2e/live/shields-config.test.tstest/openclaw-config-guard-startup-failure-gate.test.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- docs/manage-sandboxes/recover-rebuild-sandboxes.mdx
- scripts/state-dir-guard.py
- src/lib/shields/openclaw-config-lock.test.ts
- src/lib/shields/index.ts
- src/lib/shields/openclaw-config-lock.ts
|
Superseded by #8488. The replacement preserves Rui Luo as the implementation author, retains Rui and Apurv DCO declarations, and all three replacement commits appear as GitHub Verified. It first reproduced this branch result on current main, then incorporated the two final actionable review findings: quarantine intent is preserved through forward recovery and rollback, and the live signal race accepts only the known already-exited result while the child census remains authoritative. Both original review threads link to the corresponding replacement evidence, and #8488 carries the full validation, documentation, and nine-category security-review receipt. Closing this PR only because #8488 is now the documented replacement. |
…8488) <!-- markdownlint-disable MD041 --> ## Summary This PR supersedes [#8460](#8460) with a clean Conventional Commits history while preserving Rui Luo's authorship and DCO declaration. It keeps the reviewed recovery behavior and incorporates all actionable review feedback: a shields-up OpenClaw sandbox whose startup terminally failed can lower shields without a destructive rebuild, but only after the installed guard proves there is no live startup child or readiness lease. ## Related Issue Fixes #8304 Supersedes [#8460](#8460). ## Changes - Preserve Rui Luo's recovery implementation and documentation as one contributor-authored, dual-DCO commit based on current `main`. The replacement first reproduced #8460's branch result; later commits preserve quarantine intent and address the completed review feedback with regression coverage. - Extract the supervised-start procfs scan into a stable census that fails closed for foreign, duplicate, changing, or undeterminable process topology. - Add the installed-only `unlock-failed-startup` action. It reconfirms childless failed startup under the config mutation mutex, passes the held descriptor to the installed state guard, atomically unseals both layers, and attempts to re-seal both layers after partial failure. - Route `shields down` through the ordinary read-only preflight first, use failed-startup recovery only for the readiness refusal, and surface every other transition, contract, parse, timeout, or rollback error. - Keep a ten-minute forward budget and a separate twelve-minute rollback reserve inside a twenty-five-minute container limit, while the host remains attached for twenty-six minutes; refuse recovery when the installed helper is unavailable. - Document the OpenClaw recovery sequence before rebuild guidance. - Extend the live Shields sandbox target to prove the installed guard refuses a live supervised startup child, accepts stable childless recovery with the installed state plan and inherited mutation lock, restarts successfully, and restores lockdown. - Preserve the untrusted-journal quarantine setting through forward recovery, transition freeze, and rollback; assert each handoff in the transaction regression harness. - Treat only the known already-exited process result as acceptable when the live target signals the startup child, while retaining the subsequent child census as the success authority and rejecting unexpected signal failures. ## Type of Change - [ ] Code change (feature, bug fix, or refactor) - [x] Code change with doc updates - [ ] Doc only (prose changes, no code sample modifications) - [ ] Doc only (includes code sample changes) ## Quality Gates <!-- Check one tests line and one docs line. Check other lines when applicable. Add every requested justification or approval reference. --> - [x] Tests added or updated for changed behavior - [ ] Existing tests cover changed behavior — justification: - [ ] Tests not applicable — justification: - [x] Docs updated for user-facing behavior changes - [ ] Docs not applicable — justification: - [x] Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging) - [x] Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: nine-category maintainer security review found no code issue; receipt: #8488 (comment) - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## Documentation Writer Review <!-- Required for code and documentation changes after the changes and applicable validation are complete. Keep one review checkbox and one instance of each visible or hidden field. For Evidence, list changed documentation paths. For documentation-only changes, also state that the writing rules and documentation style were reviewed. For other results, explain why no documentation change is needed or why the review is blocked. For Agent, use a consistent product and surface name, such as Codex Desktop, Codex CLI, Claude Code, or Cursor. After committing all review changes, put `git rev-parse --short HEAD` and `git rev-parse --short HEAD:AGENTS.md` in the hidden metadata below. Rerun the review and refresh that metadata after any new commit. This receipt is advisory during the data-collection pilot. --> - [x] Documentation writer subagent reviewed the completed changes - Result: `docs-updated` - Evidence: `docs/manage-sandboxes/recover-rebuild-sandboxes.mdx`; independent review confirmed that the OpenClaw-only recovery guidance matches the guarded failed-startup unlock, states the process and readiness checks precisely, names the required config and state guards, and separates the restart and Shields restoration steps. `npm run docs` passed with 0 errors and the 2 existing non-fatal Fern warnings. - Agent: Codex Desktop <!-- docs-review-head-sha: e7aecc8 --> <!-- docs-review-agents-blob-sha: c69aad4 --> ## DGX Station Hardware Evidence <!-- Required only when scripts/prepare-dgx-station-host.sh changes. Maintainers must review the linked evidence before approving or merging. This is human-reviewed evidence, not authenticated hardware provenance. Exceptional bypasses use existing repository governance and must be documented on the PR. --> - [ ] Tested on DGX Station - Tested commit: - Station profile/scenario: - Result: - Supporting evidence: ## Verification <!-- Check each applicable item only when supported by the requested evidence. Run targeted tests once per relevant change set and rerun after later edits or hook autofixes that can affect the tested behavior. Do not rerun hook-covered checks. --> - [x] PR description includes a `Signed-off-by:` line and every commit appears as `Verified` in GitHub - [x] Normal `pre-commit`, `commit-msg`, and `pre-push` hooks passed, or `npm run validate:pr` passed after refreshing `origin/main` when hooks were skipped or unavailable - [x] Targeted behavior tests pass for the current change set, or tests are marked not applicable above — command/result or justification: 68 focused CLI Shields/guard tests and 4 dedicated failed-startup transaction/classification tests passed on the current branch revision. CLI build, CLI type checking, repository integrity checks, Python compilation, full PR validation, and documentation validation also passed. The installed live target loaded and skipped before sandbox creation because the protected inference secret is unavailable locally. - [ ] Applicable broad gate passed — `npm test` for broad runtime/test-harness changes; `npm run check` for repo-wide validation/coverage changes — command/result: - [x] Quality Gates section completed with required justifications or waivers - [x] No secrets, API keys, or credentials committed - [ ] `npm run docs` builds without warnings (doc changes only) - [ ] Doc pages follow the [style guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md) (doc changes only) - [ ] New doc pages include SPDX header and frontmatter (new pages only) --- <!-- DCO sign-off is required in this PR description, and every commit must appear as Verified in GitHub. Run: git config user.name && git config user.email --> Signed-off-by: Rui Luo <ruluo@nvidia.com> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Added automatic recovery for sandboxes that remain unavailable after a failed startup. - Safely unlocks configuration and workspace state only after startup processes stop, then restores protections. - Preserves existing safeguards and reports specific recovery issues when conditions are not met. - **Documentation** - Added guidance for using `shields down` when restarting does not restore gateway availability. - Clarified readiness checks, required sandbox support, upgrade/rebuild steps, and when to restart before restoring shields. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Apurv Kumaria <akumaria@nvidia.com> Co-authored-by: Rui Luo <ruluo@nvidia.com> Co-authored-by: Carlos Villela <cvillela@nvidia.com>
3 participants
Summary
A shields-up sandbox whose agent gateway fails to start could not lower its shields. The container publishes its startup readiness lease only after the gateway is healthy, so a gateway that never starts leaves no lease, and the config guard then refused the actions that make up
shields down. The only exit wasrebuild, which destroys and recreates the sandbox. The guard now exposes one dedicated action that unseals both state layers atomically once it proves the startup terminally failed, soshields downfollowed bystartreturns the sandbox to Ready.Related Issue
Fixes #8304
Changes
scripts/openclaw-config-guard.pyinto_openshell_supervised_nonroot_start_census, returning the match count and PID, orNonewhen the topology cannot be established._openshell_supervised_nonroot_start_is_livekeeps its exact prior contract on top of it._openshell_supervised_nonroot_start_is_absent, a positive failed-startup discriminator. An undeterminable topology and a duplicated start both answerFalse, so it only widens authority when the supervisor is proven stable and proven childless.unlock-failed-startupaction. It is the only action the failed-startup escape authorizes, and it is refused whenever the ordinary readiness lease applies. It reconfirms the census under the mutation mutex, unseals the recursive state tree, commits the top-level handoff, and re-seals the tree if the handoff fails.scripts/state-dir-guard.pyto accept an inherited mutation-mutex descriptor. Both guards take the same mutex, so the recovery action passes its held descriptor throughpass_fds; the child inherits the lock instead of deadlocking on it. A separately opened descriptor blocks onflockand is refused, so this cannot be used to bypass serialization.installed_currentrequirement can be satisfied.startup-not-readypreflight — which is read-only and mutates nothing — falls back to the recovery action. A transition, rollback, contract, parse, or timeout failure surfaces instead of being masked.Type of Change
Quality Gates
Documentation Writer Review
docs-updateddocs/manage-sandboxes/recover-rebuild-sandboxes.mdx; independent review approved the scoped recovery conditions and complete operator sequence with no further edits.DGX Station Hardware Evidence
Verification
Signed-off-by:line and every commit appears asVerifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run validate:prpassed after refreshingorigin/mainwhen hooks were skipped or unavailablenpm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result:npm run docsbuilds without warnings (doc changes only)Signed-off-by: Rui Luo ruluo@nvidia.com
Summary by CodeRabbit
New Features
Bug Fixes
Tests