fix(shields): preserve mutable-default rollback

[HOYALIM]

Summary

Preserve a fresh OpenClaw sandbox's mutable-default posture when a Shields-down transaction rolls back. Reject corrupt persisted Shields state before any policy, timer, host-state, or sandbox-config mutation, and retain fail-closed lockdown when rollback cannot revoke timer authority or verify mutable restoration.

Related Issue

Closes #7538

Changes

• Accept an exact mutable OpenClaw posture as an idempotent unlock after restart preflight.
• Restore the valid pre-transaction Shields state when policy or timer handoff fails.
• Report the verified rollback posture without claiming stale lockdown or timer authority.
• Reject corrupt persisted Shields state before mutation, including corrupt non-UTF-8 bytes over an initially locked physical posture.
• Require rollback to revoke the authoritative timer marker before reporting mutable-default restoration; otherwise apply fail-closed lockdown.
• Exercise fresh mutable-default shields down through the canonical protected shields-config E2E.
• Refuse a new Shields-down window before any mutation when stale auto-restore timer authority cannot be revoked.

Type of Change

• Code change (feature, bug fix, or refactor)
• Code change with doc updates
• Doc only (prose changes, no code sample modifications)
• Doc only (includes code sample changes)

Quality Gates

• Tests added or updated for changed behavior
• Existing tests cover changed behavior — justification:
• Tests not applicable — justification:
• Docs updated for user-facing behavior changes
• Docs not applicable — justification:
• Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging)
• Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: Codex Desktop independently reviewed revision ba9cf0395577cdd13e76c34104f24d4cd44ead34 with base 4cd4d64fe67143b57707f874afa0b9d269dfeff2. All nine security categories passed: secrets and credentials; input validation and data sanitization; authentication and authorization; dependencies; error handling and logging; cryptography and data protection; configuration and security defaults; security testing; and system security. The no-op config path checks the mutable directory and file posture, parses the configuration as JSON5, verifies that the open directory still refers to the same inode, and preserves file bytes and inode identity. Corrupt state or unsuccessful timer-marker removal stops before policy, configuration, or state mutation. Rollback restores mutable posture only after policy restoration and timer-authority revocation; on failure, it locks the configuration again. Current diff SHA-256: a099ea70451abdd9b91604508e2df43891599b5714e7714a5ed0f037a859952f. No findings remain.
• Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue:

Documentation Writer Review

• Documentation writer subagent reviewed the completed changes
• Result: docs-updated
• Evidence: Codex Desktop independently reviewed revision ba9cf0395577cdd13e76c34104f24d4cd44ead34 with base 4cd4d64fe67143b57707f874afa0b9d269dfeff2. docs/reference/commands.mdx accurately describes stale timer-authority cleanup and its failure result; terminology, structure, voice, and code-sample presentation passed review. The page is present in the OpenClaw, Deep Agents, and Hermes guide variants. GitHub Docs / Fern Preview preparation and validation passed: https://github.com/NVIDIA/NemoClaw/actions/runs/30811171284/job/91680561828.
• Agent: Codex Desktop

DGX Station Hardware Evidence

• Tested on DGX Station
• Tested commit:
• Station profile/scenario:
• Result:
• Supporting evidence:

Verification

• PR description includes a Signed-off-by: line and every commit appears as Verified in GitHub
• Normal pre-commit, commit-msg, and pre-push hooks passed, or npm run check:diff passed when hooks were skipped or unavailable
• Targeted behavior tests pass for the current change set — Shields interaction suites passed 108/108 on the source-equivalent production patch; the validated malformed-JSON5 guard regression passed 1/1, and that regression plus the legacy guard suite passed 46/46 before size-budget extraction.
• Applicable broad gate passed — npm test for broad runtime/test-harness changes; npm run check for repo-wide validation/coverage changes — command/result: Not applicable to this scoped Shields rollback and guard fix; targeted tests and npm run check:diff cover every changed source and test path.
• Quality Gates section completed with required justifications or waivers
• No secrets, API keys, or credentials committed
• npm run docs builds without errors (doc changes only; 2 existing suppressed warnings)
• Doc pages follow the style guide (doc changes only)
• New doc pages include SPDX header and frontmatter (new pages only)

Review Resolution

Revision ba9cf03955 and all preceding PR commits are GitHub-verified. The change combines cleanly with current main at 03fbf5ebf; the combined code passes 58 Shields tests and the malformed-JSON5 guard regression. The required static, DCO, commit-lint, and installer-hash gates pass. The protected E2E gate timed out while waiting for approval of the fork run, and the contributor fork still needs an authorized branch update. After synchronization, GitHub CI, protected shields-config E2E, automated review, documentation review, security review, and maintainer review must all settle successfully before merge.

---

Signed-off-by: Ho Lim subhoya@gmail.com

[copy-pr-bot]

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[coderabbitai]

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

📝 Walkthrough

Walkthrough

The OpenClaw guard now accepts verified mutable posture without rewriting files. Shields-down rollback preserves the initial state, restores mutable-default posture when possible, and reports fail-closed lockdown when restoration cannot be verified.

Changes

Mutable-default shields-down recovery

Layer / File(s)	Summary
Idempotent mutable config transition scripts/openclaw-config-guard.py, test/openclaw-config-guard.test.ts	The guard preflights and exits for already-mutable posture; tests verify unlock preserves config and hash inodes and contents.
Initial posture rollback handling src/lib/shields/index.ts	Rollback restores the initial mutable-default posture and shields-state snapshot when possible, while corrupt state fails early and unverifiable restoration falls back to fail-closed lockdown.
Failure recovery coverage src/lib/shields/flow.test.ts, test/e2e/live/shields-config.test.ts	Tests cover timer ownership failures, corrupt state preservation, unsuccessful mutable-default restoration, and fresh mutable-default shields-down behavior.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Sequence Diagram(s)

sequenceDiagram
  participant ShieldsDown
  participant OpenClawGuard
  participant Rollback
  participant StateFile
  ShieldsDown->>OpenClawGuard: unlock OpenClaw config
  OpenClawGuard-->>ShieldsDown: complete mutable transition or fail
  ShieldsDown->>Rollback: pass initial mode and state
  Rollback->>OpenClawGuard: restore mutable posture
  Rollback->>StateFile: restore shields snapshot
  Rollback-->>ShieldsDown: report restored or fail-closed lockdown

Loading

Possibly related PRs

• NVIDIA/NemoClaw#7467: Updates the same OpenClaw guard transition flow with .config-hash repair and related fail-closed behavior.
• NVIDIA/NemoClaw#7557: Adds related fail-fast handling for corrupt persisted shields state in the shields-down flow.

Suggested labels: integration: openclaw, area: sandbox, area: security, bug-fix

Suggested reviewers: copilot, cv

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Changes implement #7538 by making exact mutable-default unlock idempotent, preserving rollback state, and reporting fail-closed lockdown when needed.
Out of Scope Changes check	✅ Passed	The changes stay focused on shields-down/OpenClaw posture handling and its tests, with no obvious unrelated features added.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly identifies the shields fix and the preservation of mutable-default rollback, which matches the primary change.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 1

🤖 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 `@src/lib/shields/flow.test.ts`:
- Around line 320-360: The test for fail-closed rollback currently validates
only state and logging, while the stubbed command results do not prove
containment executed. Update createHarness or the test to expose the observed
OpenClaw posture, or perform a post-failure status check, then assert that the
resulting configuration is actually locked after shieldsDown throws. Keep the
existing shieldsDown, shields-open state, and error-log assertions.

🪄 Autofix (Beta)

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: 182b9e3b-17d3-419d-badc-0cecebaea6ca

📥 Commits

Reviewing files that changed from the base of the PR and between 29bd189 and 2be209c.

📒 Files selected for processing (4)

• scripts/openclaw-config-guard.py
• src/lib/shields/flow.test.ts
• src/lib/shields/index.ts
• test/openclaw-config-guard.test.ts

[github-actions]

PR Review Advisor — Informational

Advisor assessment: Informational / low confidence
Next action: No advisor follow-up needed.
Findings: 0 blockers · 0 warnings · 0 suggestions
Status: Partial review preserved 0 canonical finding(s) and 2 terminology decision(s) before the advisor stopped.

Model lanes

• GPT-5.6 Terra (primary): Failed after a partial review · low confidence · 0 blockers · 0 warnings · 0 suggestions
• Nemotron 3 Ultra (second opinion): Completed · high confidence · 0 blockers · 0 warnings · 0 suggestions

Second-opinion terminology and E2E selections are advisory. They do not change the primary assessment or E2E / PR Gate.

2 semantic terminology decisions

Terminology decisions are advisory. They affect the assessment only when a separate finding identifies concrete semantic impact.

• established — timer authority at docs/reference/commands.mdx:1218: Keep `timer authority` for the marker-backed auto-restore authority.
• justified — configuration and policy posture at docs/reference/commands.mdx:1220: Keep the combined phrase where the documented guarantee covers both configuration and network policy.

E2E guidance

Advisory only. E2E / PR Gate selects and runs jobs independently.

Recommended E2E: cloud-inference, managed-image-multiarch-startup, security-posture, shields-config

Workflow run details

This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review is ineligible. To be eligible to request a review, you need a paid Copilot license, or your organization must enable Copilot code review.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review is ineligible. To be eligible to request a review, you need a paid Copilot license, or your organization must enable Copilot code review.

[coderabbitai]

Actionable comments posted: 1

🤖 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 `@src/lib/shields/flow.test.ts`:
- Around line 322-358: Add an assertion in the test case around shieldsDown to
verify the public outcome with harness.getOpenClawPosture(), expecting "mutable"
after rollback. Keep the existing byte-preservation and logging assertions
unchanged.

🪄 Autofix (Beta)

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: 9e139d55-4d9c-42ab-b44e-50af70c0bf40

📥 Commits

Reviewing files that changed from the base of the PR and between 4f05209 and fba7def.

📒 Files selected for processing (2)

• src/lib/shields/flow.test.ts
• src/lib/shields/index.ts

[apurvvkumaria]

Addressed the Advisor rollback-integrity warning in signed, Verified commit fba7def0b, then synced current main in exact head 5317f8817.

Corrupt shields-state snapshots now retain their exact original bytes in memory and restore those bytes verbatim if the mutable-default transition rolls back; persisted runtime markers are still stripped from normal state writes. A regression seeds invalid bytes, forces timer-handoff failure, and verifies byte-for-byte restoration.

Local validation: CLI build/typecheck; shields flow and state tests 54/54; all repository hooks and pre-push checks. Documentation-writer review found no docs change required. Fresh exact-head CI/Advisor/E2E remains required before merge readiness.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review is ineligible. To be eligible to request a review, you need a paid Copilot license, or your organization must enable Copilot code review.

[cv]

Security review: FAIL on exact head 3e721d71a01ade1e1c7c4b0888916c2607df4e87.

Three blockers remain:

1. Corrupt persisted state is still classified as mutable_default. If the physical OpenClaw config is actually locked, the transaction can unlock it and the mutable rollback path can leave it mutable. The new commit adds only expect(...).toBe("mutable") to a harness that already starts mutable; it does not exercise corrupt state plus an initially locked posture. Fail before mutation for _isCorrupt, or first establish authoritative physical posture and add a corrupt+locked regression.
2. _rawFileContents remains a UTF-8 string, so invalid bytes are replaced on read/write and restoration is not byte-exact. Failing early for corrupt state removes this path; otherwise preserve a Buffer and test non-UTF-8 bytes.
3. Issue #7538 requires protected live proof of a fresh mutable-default shields down transition. The current live test first runs shields up, and the PR risk plan does not select shields-config, so current E2E cannot prove the reported path.

The focused 69 tests and build/check evidence pass, but they do not cover these security boundaries. Please also add the exact-head documentation-writer receipt after the final change.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/lib/shields/index.ts (1)

2848-2855: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

Report the actual rollback outcome. rollbackShieldsDown can restore mutable-default, but these callers unconditionally describe lockdown/timer authority. That can tell operators the sandbox is locked when it was restored to mutable-default.

• src/lib/shields/index.ts#L2848-L2855: return and consume a rollback outcome; do not say the auto-restore timer remains authoritative after mutable-default restoration removes it.
• src/lib/shields/index.ts#L2874-L2881: select the handoff-failure status from that same outcome rather than always saying lockdown was restored.
• src/lib/shields/flow.test.ts#L318-L320: assert the stale “lockdown was restored” diagnostic is absent after successful mutable-default restoration.

🤖 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 2848 - 2855, Update rollback handling
around rollbackShieldsDown in src/lib/shields/index.ts:2848-2855 to return and
consume its rollback outcome, reporting timer authority only when lockdown
remains restored and not after mutable-default restoration removes it. At
src/lib/shields/index.ts:2874-2881, derive the handoff-failure status from that
same outcome instead of always reporting lockdown restored. In
src/lib/shields/flow.test.ts:318-320, assert successful mutable-default
restoration does not emit the stale “lockdown was restored” diagnostic.

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

Outside diff comments:
In `@src/lib/shields/index.ts`:
- Around line 2848-2855: Update rollback handling around rollbackShieldsDown in
src/lib/shields/index.ts:2848-2855 to return and consume its rollback outcome,
reporting timer authority only when lockdown remains restored and not after
mutable-default restoration removes it. At src/lib/shields/index.ts:2874-2881,
derive the handoff-failure status from that same outcome instead of always
reporting lockdown restored. In src/lib/shields/flow.test.ts:318-320, assert
successful mutable-default restoration does not emit the stale “lockdown was
restored” diagnostic.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: ce9c8e10-221b-4b78-85ba-e1801c328e21

📥 Commits

Reviewing files that changed from the base of the PR and between 3e721d7 and fb6e689.

📒 Files selected for processing (3)

• src/lib/shields/flow.test.ts
• src/lib/shields/index.ts
• test/e2e/live/shields-config.test.ts

[HOYALIM]

@cv Current head ba9cf03955 is now frozen.

• Ordinary code checks are green.
• There are no active non-outdated review threads or current-head human change requests.
• The primary advisor reported 0 blockers. The failed Nemotron lane was an incomplete second opinion.
• E2E / PR Gate timed out while waiting for maintainer approval to run fork E2E, so that remaining gate is external to the contributor.

I will not sync main or push another CI-refresh commit unless a concrete current-head blocker or merge conflict appears. To avoid resetting checks and approvals again, could you please choose one disposition for this head: merge when repository gates permit, identify the remaining code or evidence blocker, or mark the PR superseded / otherwise close its review path?

[apurvvkumaria]

Protected E2E completed: https://github.com/NVIDIA/NemoClaw/actions/runs/30860798177

The cloud-inference, OpenClaw and Hermes security-posture, and shields-config lanes all stopped during sandbox-image construction, before reaching this PR's changed Shields behavior. Each failure is the shared MCP tool-discovery runtime audit rejecting the current fast-uri, hono, and ip-address advisories.

The branch still merges cleanly with current main, and all 59 focused Shields tests pass on that combined tree. This is therefore a repository dependency blocker, not evidence of a PR-specific regression. Do not rerun until the applicable dependency remediation lands; no failed-gate exception or bypass is requested.

[cjagwani]

Exact-head follow-up for ba9cf0395577cdd13e76c34104f24d4cd44ead34: the selected E2E run finished with failures in cloud-inference, both OpenClaw and Hermes security-posture targets, and shields-config. The coordination result has no retry marker, and the required native E2E check remains red, so I am not rerunning it as transient infrastructure. A new commit addressing those failures is needed before exact-head validation can continue. This is a coordination comment, not a Changes Requested review.

[cjagwani]

Exact-head CI follow-up for 3344632: the refreshed branch now fails the required codebase-growth-guardrails check because src/lib/shields/flow.test.ts is 1,617 lines against the 1,500-line budget. This is a branch-owned contract failure, so the test file needs to be split or reduced before the PR is merge-ready. The trusted E2E gate is still in progress; I am not rerunning it.

[cjagwani]

Verified new head b0fb1f2: the follow-up mechanically extracts the existing Shields test harness into test/helpers/shields-flow-test-harness.ts, keeps the test call sites and assertions intact, and the required codebase-growth-guardrails check is now green. The refreshed trusted E2E gate is in progress; no rerun or additional branch edit is needed.