Skip to content

fix(shields): re-confirm auto-restore lock after settle to prevent config-hash drift (#4663) - #5195

Merged
cv merged 11 commits into
NVIDIA:mainfrom
TonyLuo-NV:fix/4663-shields-auto-restore-config-hash
Jun 13, 2026
Merged

fix(shields): re-confirm auto-restore lock after settle to prevent config-hash drift (#4663)#5195
cv merged 11 commits into
NVIDIA:mainfrom
TonyLuo-NV:fix/4663-shields-auto-restore-config-hash

Conversation

@TonyLuo-NV

@TonyLuo-NV TonyLuo-NV commented Jun 11, 2026

Copy link
Copy Markdown
Collaborator

Summary

Shields auto-restore (and the manual shields up drift-remediation path) locked the sandbox config to 444 root:root and verified it once. On DGX Station / DGX Spark an in-sandbox privileged reconciler (OpenClaw gateway / doctor-style perm normalization) re-permissioned /sandbox/.openclaw/.config-hash after the verified lock returned, reverting it to 660 sandbox:sandbox. The content was untouched (the SHA-256 seal still matched), so only mode/owner drifted and shields status reported UP (DRIFTED), exiting 2 after the timeout.

This adds a bounded lock → settle → re-confirm → re-lock cycle so shields are only declared UP once the on-disk perms are durably 444 root:root after the reconciler has had a chance to settle, and fail closed otherwise.

Related Issue

Fixes #4663

Changes

  • New src/lib/shields/durable-lock.tslockUntilDurable(): applies the lock, waits a settle window, then re-confirms via the existing verified lockAgentConfig path, retrying the whole cycle if a reconciler reverted perms during the wait. Bounded to 3 attempts; settle window defaults to 750 ms (clamped 010000, env-tunable via NEMOCLAW_SHIELDS_SETTLE_MS); synchronous (sleepMs) so callers stay synchronous. Returns ok:false (fail closed) when the lock will not hold.
  • src/lib/shields/timer.ts — auto-restore re-lock routed through lockUntilDurable; on failure leaves shields DOWN and audits shields_auto_restore_lock_warning / shields_up_failed (never marks UP while drifted).
  • src/lib/shields/index.tsshieldsUp drift-remediation re-lock routed through lockUntilDurable (the manual recovery was also being reverted), failing closed via failShieldsCommand.
  • Testsdurable-lock.test.ts (10 unit tests) + two acceptance tests in timer.test.ts (re-verify-after-settle; leave DOWN + audit on persistent revert).
  • Docs — documented NEMOCLAW_SHIELDS_SETTLE_MS in docs/reference/commands.mdx.

Fail-closed invariant: UP is written only after a post-settle verifyShieldsLockState passes; every exhaustion/error branch routes to shieldsDown:true + audit. Bounded total wait (≤ ~30 s worst case) — a constantly-reverting reconciler cannot wedge the timer.

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)

Verification

  • npx prek run --all-files passes
  • npm test passes
  • Tests added or updated for new or changed behavior
  • No secrets, API keys, or credentials committed
  • Docs updated for user-facing behavior changes

Verified locally: full shields suite 102/102 pass (incl. both #4663 acceptance tests + 10 durable-lock unit tests); npm run typecheck:cli clean; Biome clean on touched files; NEMOCLAW_* env-var documentation gate passes. prek --all-files / full npm test not green here only due to pre-existing, unrelated live-e2e (openshell-version-pin) and missing-dependency flakes outside src/lib/shields/.

Note

Reproduced as present on the latest tag (v0.0.62). The bug is platform-tied (aarch64 DGX Station / DGX Spark) — recommend confirming the fix on real DGX hardware in NV QA, since the unit tests model the reconciler revert but cannot exercise the host-specific timing. Also worth confirming the issue's flagged version mismatch (artifact reported nemoclaw v0.0.55 while the pipeline targeted v0.0.56).


Signed-off-by: Tony Luo xialuo@nvidia.com

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features

    • Shields auto-restore and drift remediation now re-confirm config locks after a configurable settle window; if re-confirmation fails within retries, shields remain DOWN.
  • Configuration

    • Added NEMOCLAW_SHIELDS_SETTLE_MS (default 750ms, clamped 0–10000ms) to tune the post-lock settle delay.
  • Documentation

    • Documented the new settle variable and recommended tuning for slow-host gateways.
  • Tests

    • Added tests covering re-lock/re-confirm behavior and settle-window resolution.

@coderabbitai

coderabbitai Bot commented Jun 11, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

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
  • ✅ Review completed - (🔄 Check again to review again)
📝 Walkthrough

Walkthrough

Adds a durable lock re-verification cycle to shields auto-restore and drift remediation, preventing .config-hash permission drift after initial lockdown. Introduces environment-configurable settle/retry logic, integrates into shieldsUp and timer flows, and includes comprehensive tests and documentation.

Changes

Config shields durable lock verification

Layer / File(s) Summary
Core relock-and-reconfirm module
src/lib/shields/relock-reconfirm.ts
New module exports LockResult, LockFn, RelockReconfirmOptions, and RelockReconfirmResult contracts. Implements resolveSettleMs() for environment-driven settle timing (default 750ms, clamped 0–10000, forced to 0 under VITEST=true). Implements relockAndReconfirm(lock, opts) with bounded retry cycle: apply+verify → sleep → re-confirm; fails immediately on initial apply error, retries entire cycle on re-confirm failure, returns ok/attempts/lastResult or ok false with error.
Relock-and-reconfirm unit tests
src/lib/shields/relock-reconfirm.test.ts
Vitest coverage for relockAndReconfirm success (lock called twice, single sleep), bounded failure when re-confirm repeatedly throws, cycle retry when re-confirm fails then succeeds, and immediate failure on initial apply error. Tests resolveSettleMs behavior under VITEST, NODE_ENV, clamping, and defaults.
ShieldsUp drift remediation with relock-and-reconfirm
src/lib/shields/index.ts
Imports relockAndReconfirm and replaces single lockAgentConfig try/catch with relockAndReconfirm(() => lockAgentConfig(...)). On non-ok result or missing lastResult, command fails with error and manual intervention instruction. On success, persists chattrApplied and fileHashes from reconfirmed lock result into shields state and continues audit/state flow.
Timer auto-restore lock verification with relock-and-reconfirm
src/lib/shields/timer.ts
Imports relockAndReconfirm and replaces instantaneous lock verification with relockAndReconfirm(...) call. On success captures chattrApplied and fileHashes from reconfirmed result. On failure sets lockVerified false, appends shields_auto_restore_lock_warning audit entry using relock.error as fallback, and continues fail-closed behavior (shields DOWN, shields_up_failed audit entry).
Timer auto-restore durability tests
src/lib/shields/timer.test.ts
Updates existing auto-restore persistence test to expect lockAgentConfig called twice (apply + re-confirm). Adds two #4663 tests: first asserts post-settle re-verify after permission drift restores shields UP with two lock invocations; second simulates post-settle re-lock drift by throwing on re-confirm, asserting timer exits with failure (code 1), keeps shields DOWN, and records shields_auto_restore_lock_warning (lock_verified false) and shields_up_failed audit entries.
Environment variable documentation
docs/reference/commands.mdx, docs/reference/commands-nemohermes.mdx
Documents NEMOCLAW_SHIELDS_SETTLE_MS in lifecycle behavior flags table (default 750ms, clamped 0–10000ms) describing settle window and lock re-confirmation behavior used by shields auto-restore and shieldsUp drift remediation, including failure behavior and note about chattr +i durability.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related issues

Suggested labels

area: sandbox, bug-fix, v0.0.63, NV QA

Suggested reviewers

  • cv
  • prekshivyas

Poem

🔐 A lock once applied, now sleeps and re-checks,
Catches the drift when permissions flex,
Settle, then verify—no gaps left to wreck,
The shield holds true, from startup to specs. ✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 62.50% 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
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The PR title 'fix(shields): re-confirm auto-restore lock after settle to prevent config-hash drift' accurately and concisely describes the main change: adding a re-confirmation step after a settle window to prevent config-hash drift in shields auto-restore.
Linked Issues check ✅ Passed The PR comprehensively addresses all coding requirements from issue #4663: implements re-lock-and-reconfirm logic with configurable settle window, integrates into auto-restore and shields up remediation, adds comprehensive tests, and documents NEMOCLAW_SHIELDS_SETTLE_MS environment variable.
Out of Scope Changes check ✅ Passed All changes are directly related to fixing the config-hash drift issue: new relock-reconfirm module, integration into shields timer and index, test coverage, and documentation of the settle window behavior.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
docs/reference/commands.mdx (1)

2103-2103: ⚡ Quick win

Prefer active voice in the description.

The phrase "Lets an in-sandbox reconciler that re-permissions config files after lockdown be detected" uses passive construction. The coding guidelines require active voice throughout documentation.

Suggested active-voice revision

Consider revising to active voice:

-| `NEMOCLAW_SHIELDS_SETTLE_MS` | milliseconds (default `750`, clamped to `0`–`10000`) | Settle window NemoClaw waits after re-applying a config lockdown (during shields auto-restore and `$$nemoclaw <name> shields up` drift remediation) before re-confirming the lock still holds. Lets an in-sandbox reconciler that re-permissions config files after lockdown be detected so the lock is re-applied; if the lock cannot be held durably, shields stay down. Raise it on hosts where the gateway settles slowly. |
+| `NEMOCLAW_SHIELDS_SETTLE_MS` | milliseconds (default `750`, clamped to `0`–`10000`) | Settle window NemoClaw waits after re-applying a config lockdown (during shields auto-restore and `$$nemoclaw <name> shields up` drift remediation) before re-confirming the lock still holds. Detects when an in-sandbox reconciler changes config file permissions after lockdown and re-applies the lock; if the lock cannot be held durably, shields stay down. Raise it on hosts where the gateway settles slowly. |

As per coding guidelines, "Active voice required. Flag passive constructions."

🤖 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/reference/commands.mdx` at line 2103, The description for
NEMOCLAW_SHIELDS_SETTLE_MS uses passive voice; rewrite the row so it uses active
voice and clearly names the agent (the in-sandbox reconciler) performing
detection and re-permissioning, preserving meaning (default 750 ms, clamp
0–10000, purpose during shields auto-restore and `$$nemoclaw <name> shields up`
drift remediation). Locate the table row for NEMOCLAW_SHIELDS_SETTLE_MS in
docs/reference/commands.mdx and replace the passive clause with an active
construction that states the reconciler detects re-permissioning after lockdown
so the lock can be re-applied, and keep the guidance about raising the value on
slow-settling gateways and that shields remain down if lock cannot be held
durably.

Source: Coding guidelines

🤖 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/timer.test.ts`:
- Around line 436-441: Replace the lenient assertions with exact expectations:
change the call-count assertion on lockMock (currently using
toBeGreaterThanOrEqual(2)) to expect exactly 2 calls (use toBe(2)) to enforce
the two-step lock+reconfirm contract, and replace the later OR-style audit
assertion that checks for either `shields_auto_restore_lock_warning` or
`shields_up_failed` with assertions that both audit events
(`shields_auto_restore_lock_warning` and `shields_up_failed`) are present in the
audit outcomes (assert both independently) so the test requires both required
audit outcomes; update assertions referencing lockMock, exitCode, stateFile,
shields_auto_restore_lock_warning, and shields_up_failed accordingly.

---

Nitpick comments:
In `@docs/reference/commands.mdx`:
- Line 2103: The description for NEMOCLAW_SHIELDS_SETTLE_MS uses passive voice;
rewrite the row so it uses active voice and clearly names the agent (the
in-sandbox reconciler) performing detection and re-permissioning, preserving
meaning (default 750 ms, clamp 0–10000, purpose during shields auto-restore and
`$$nemoclaw <name> shields up` drift remediation). Locate the table row for
NEMOCLAW_SHIELDS_SETTLE_MS in docs/reference/commands.mdx and replace the
passive clause with an active construction that states the reconciler detects
re-permissioning after lockdown so the lock can be re-applied, and keep the
guidance about raising the value on slow-settling gateways and that shields
remain down if lock cannot be held durably.
🪄 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: 1392e084-522c-43b7-9567-7c7cf718fc5d

📥 Commits

Reviewing files that changed from the base of the PR and between e67ff23 and d79bd83.

📒 Files selected for processing (6)
  • docs/reference/commands.mdx
  • src/lib/shields/durable-lock.test.ts
  • src/lib/shields/durable-lock.ts
  • src/lib/shields/index.ts
  • src/lib/shields/timer.test.ts
  • src/lib/shields/timer.ts

Comment thread src/lib/shields/timer.test.ts
…nfig-hash drift (NVIDIA#4663)

Auto-restore (and manual `shields up` drift remediation) locked
/sandbox/.openclaw/.config-hash to 444 root:root and verified it once. On DGX
Station / DGX Spark an in-sandbox privileged reconciler (OpenClaw gateway /
doctor-style perm normalization) re-permissioned the file in place after the
verified lock returned, reverting it to 660 sandbox:sandbox. Content was
untouched (the SHA-256 seal still matched), so only mode/owner drifted and
`shields status` reported UP (DRIFTED), exiting 2 after the timeout.

Add lockUntilDurable: a bounded lock -> settle -> re-confirm -> re-lock cycle
(3 attempts, 750ms settle, env-tunable via NEMOCLAW_SHIELDS_SETTLE_MS,
synchronous via sleepMs). Only declare the lock UP when the on-disk perms are
durably 444 root:root after the reconciler has settled; fail closed (leave
shields DOWN and audit shields_auto_restore_lock_warning / shields_up_failed)
when the lock will not hold. Wired into both the auto-restore timer and the
shieldsUp drift-remediation path.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: Tony Luo <xialuo@nvidia.com>
@TonyLuo-NV
TonyLuo-NV force-pushed the fix/4663-shields-auto-restore-config-hash branch from d79bd83 to 6e0309a Compare June 11, 2026 03:11
… settle-env doc

Address review feedback on PR NVIDIA#5195:

- timer.test.ts: assert the durable auto-restore lock calls lockAgentConfig
  exactly twice (apply + post-settle re-confirm) on the clean path, and assert
  both fail-closed audit outcomes (shields_auto_restore_lock_warning AND
  shields_up_failed) instead of an OR match.
- docs/reference/commands.mdx: rewrite the NEMOCLAW_SHIELDS_SETTLE_MS
  description in active voice per the docs style guide.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: Tony Luo <xialuo@nvidia.com>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
docs/reference/commands.mdx (1)

2120-2120: ⚡ Quick win

Prefer active voice over passive construction.

The phrase "if the lock cannot be held durably" uses passive voice. As per coding guidelines, active voice is required throughout NemoClaw documentation.

✏️ Suggested active-voice alternatives

Replace the passive phrase with one of these active constructions:

-| `NEMOCLAW_SHIELDS_SETTLE_MS` | milliseconds (default `750`, clamped to `0`–`10000`) | Settle window NemoClaw waits after re-applying a config lockdown (during shields auto-restore and `$$nemoclaw <name> shields up` drift remediation) before re-confirming the lock still holds. Detects when an in-sandbox reconciler changes config file permissions after lockdown and re-applies the lock; if the lock cannot be held durably, shields stay down. Raise it on hosts where the gateway settles slowly. |
+| `NEMOCLAW_SHIELDS_SETTLE_MS` | milliseconds (default `750`, clamped to `0`–`10000`) | Settle window NemoClaw waits after re-applying a config lockdown (during shields auto-restore and `$$nemoclaw <name> shields up` drift remediation) before re-confirming the lock still holds. Detects when an in-sandbox reconciler changes config file permissions after lockdown and re-applies the lock; if NemoClaw cannot hold the lock durably, shields stay down. Raise it on hosts where the gateway settles slowly. |

or

-| `NEMOCLAW_SHIELDS_SETTLE_MS` | milliseconds (default `750`, clamped to `0`–`10000`) | Settle window NemoClaw waits after re-applying a config lockdown (during shields auto-restore and `$$nemoclaw <name> shields up` drift remediation) before re-confirming the lock still holds. Detects when an in-sandbox reconciler changes config file permissions after lockdown and re-applies the lock; if the lock cannot be held durably, shields stay down. Raise it on hosts where the gateway settles slowly. |
+| `NEMOCLAW_SHIELDS_SETTLE_MS` | milliseconds (default `750`, clamped to `0`–`10000`) | Settle window NemoClaw waits after re-applying a config lockdown (during shields auto-restore and `$$nemoclaw <name> shields up` drift remediation) before re-confirming the lock still holds. Detects when an in-sandbox reconciler changes config file permissions after lockdown and re-applies the lock; if the lock does not hold durably, shields stay down. Raise it on hosts where the gateway settles slowly. |
🤖 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/reference/commands.mdx` at line 2120, Update the description for
NEMOCLAW_SHIELDS_SETTLE_MS to use active voice: find the table entry for
NEMOCLAW_SHIELDS_SETTLE_MS and replace the passive clause "if the lock cannot be
held durably, shields stay down" with an active construction such as "if a
process (for example, an in-sandbox reconciler) cannot hold the lock durably,
shields stay down" or "if the gateway cannot hold the lock durably, shields stay
down", keeping the rest of the sentence intact and preserving the examples about
in-sandbox reconcilers and slow gateways.

Source: Coding guidelines

🤖 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 `@docs/reference/commands.mdx`:
- Line 2120: Update the description for NEMOCLAW_SHIELDS_SETTLE_MS to use active
voice: find the table entry for NEMOCLAW_SHIELDS_SETTLE_MS and replace the
passive clause "if the lock cannot be held durably, shields stay down" with an
active construction such as "if a process (for example, an in-sandbox
reconciler) cannot hold the lock durably, shields stay down" or "if the gateway
cannot hold the lock durably, shields stay down", keeping the rest of the
sentence intact and preserving the examples about in-sandbox reconcilers and
slow gateways.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: c6142d07-fcbe-47c8-96b6-21da4d54c36c

📥 Commits

Reviewing files that changed from the base of the PR and between 6e0309a and ec42079.

📒 Files selected for processing (2)
  • docs/reference/commands.mdx
  • src/lib/shields/timer.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/lib/shields/timer.test.ts

…TLE_MS

The agent-variant doc check requires commands-nemohermes.mdx to stay in sync with commands.mdx. Regenerate it via docs:sync-agent-variants so the new NEMOCLAW_SHIELDS_SETTLE_MS row is mirrored, fixing the failing Validate docs CI step.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: Tony Luo <xialuo@nvidia.com>
@prekshivyas prekshivyas self-assigned this Jun 11, 2026

@prekshivyas prekshivyas left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed (code + 9-cat security, TOCTOU focus) — verified against source at head.

💬 Comment (mergeable, but please address the framing). The mechanism is correct and fails closed: lockUntilDurable does apply→verify, settle, re-apply→re-verify (bounded 3× ≤10s); verifyShieldsLockState re-stats + sha256sums on disk every call, so the re-confirm is a genuine fresh read, and every non-ok branch routes to shieldsDown:true + audit and never marks UP. The VITEST-only settle-zeroing (explicitly not NODE_ENV=test) is a good secure default. This is strictly better than the status quo for #4663.

My concern is accuracy, not correctness: the naming (lockUntilDurable, docs "durably 444 root:root") oversells the guarantee. It narrows the window — after the final re-confirm returns, an in-sandbox reconciler can revert perms one settle-window later (the same race, shifted). The only true durable defense is chattr +i, which is applied best-effort and may be absent (e.g. no CAP_LINUX_IMMUTABLE). Requests:

  • Soften the wording to "re-confirm after settle" and document the residual window so a future maintainer doesn't assume the TOCTOU is closed.
  • The deeper issue is authz (LOW): a reconciler that can chown a root-owned locked file back to sandbox:sandbox is itself a privilege concern — worth a tracking issue (this PR correctly treats it as fail-closed drift, which is the right posture).

Security: all 9 pass; the authz/holistic items above are design observations, not defects in this diff. Tests are good (always-holds, bounded-failure, retry-then-hold, settle clamp matrix; timer acceptance tests for caught-revert and persistent-revert→exit 1 + audit). Gap: no index.ts shields up drift-path acceptance test (only the timer path) — the unit tests on lockUntilDurable make this acceptable.

… revert window (NVIDIA#4663)

Address reviewer feedback (PR NVIDIA#5195): the previous lockUntilDurable naming and 'durably 444 root:root' wording oversold the guarantee. The helper narrows the reconciler revert window via a bounded lock -> settle -> re-confirm cycle, but does not close the TOCTOU — a reconciler can revert perms one settle window after the final re-confirm. The chattr +i immutable bit (best-effort, inside lockAgentConfig) is the only fully durable defense.

Rename relock-reconfirm.ts module and relockAndReconfirm function/types; reword module header, docstrings, call-site comments, the NEMOCLAW_SHIELDS_SETTLE_MS doc (active voice + residual-window note), and regenerate the hermes command variant. No behavior change.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: Tony Luo <xialuo@nvidia.com>
@TonyLuo-NV

Copy link
Copy Markdown
Collaborator Author

@prekshivyas thanks for the careful security pass — you're right that the original framing oversold the guarantee. Addressed in b47b8debc:

  • Renamed lockUntilDurablerelockAndReconfirm (and the module durable-lock.tsrelock-reconfirm.ts, plus the result/options types). The "durable" name implied a guarantee the mechanism doesn't provide.
  • Reworded the module header, docstrings, both call-site comments, and the NEMOCLAW_SHIELDS_SETTLE_MS doc to say it re-confirms after a settle window and explicitly that this narrows, not closes, the revert window — a reconciler can revert perms one settle window after the final re-confirm (TOCTOU shifted, not eliminated). The header now calls out chattr +i (best-effort, may lack CAP_LINUX_IMMUTABLE) as the only fully durable defense, and frames this helper as a fail-closed mitigation.
  • The CodeRabbit active-voice nit on the doc row is folded into the same edit.

No behavior change — 102/102 shields tests, typecheck, env-var gate, and npm run docs all green.

On the authz point (a reconciler that can chown a root-owned locked file back to sandbox:sandbox): agreed it's a real privilege concern. The PR keeps the correct fail-closed-drift posture and I've documented it inline as a known residual rather than opening a separate tracking issue for now — happy to file one if you'd prefer it tracked.

Re the shields up drift-path acceptance gap: noted; the unit tests on relockAndReconfirm cover the retry/fail-closed logic, and the timer path has the two acceptance tests, so the shared helper's behavior is exercised even though the index.ts CJS boundary isn't directly driven.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/index.ts`:
- Around line 1265-1267: The snapshot restore and expired-timer recovery paths
call lockAgentConfig() directly (in activateLockdownFromSnapshot and the
shieldsUp/expired-timer flows) which leaves them unprotected from immediate
post-settle drift; change those call sites to route through the durable helper
relockAndReconfirm by replacing direct lockAgentConfig(sandboxName, target)
invocations with relockAndReconfirm(() => lockAgentConfig(sandboxName, target)),
and then check relock.ok and relock.lastResult the same way you do where relock
is already used; ensure you update activateLockdownFromSnapshot and the inline
expired-timer recovery and shieldsUp snapshots to bail or log the same error
message when relock fails so all relock paths use the same durable verification
flow.
🪄 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: aeda6866-44a7-43e8-80ce-6f1a678b84a3

📥 Commits

Reviewing files that changed from the base of the PR and between 76a060d and b47b8de.

📒 Files selected for processing (7)
  • docs/reference/commands-nemohermes.mdx
  • docs/reference/commands.mdx
  • src/lib/shields/index.ts
  • src/lib/shields/relock-reconfirm.test.ts
  • src/lib/shields/relock-reconfirm.ts
  • src/lib/shields/timer.test.ts
  • src/lib/shields/timer.ts
🚧 Files skipped from review as they are similar to previous changes (2)
  • docs/reference/commands-nemohermes.mdx
  • src/lib/shields/timer.test.ts

Comment thread src/lib/shields/index.ts
TonyLuo-NV and others added 2 commits June 12, 2026 11:25
Fixes the static-checks CI failure: the relockAndReconfirm rename left the warning assignment wrapped and the import list unsorted. No behavior change.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: Tony Luo <xialuo@nvidia.com>
@cv cv added v0.0.65 and removed v0.0.64 labels Jun 12, 2026
@TonyLuo-NV
TonyLuo-NV requested a review from prekshivyas June 12, 2026 05:52
@wscurran wscurran added area: sandbox OpenShell sandbox lifecycle, runtime, config, or recovery area: security Security controls, permissions, secrets, or hardening bug-fix PR fixes a bug or regression v0.0.64 labels Jun 12, 2026
@wscurran

Copy link
Copy Markdown
Contributor

@prekshivyas prekshivyas left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review — approve ✅

My earlier concern is fully resolved: b47b8debc renamed lockUntilDurable → relockAndReconfirm and the new module header states plainly that this narrows the race window and does not close the TOCTOU (the reconciler can revert one settle window after the final re-confirm; chattr +i is the only fully durable defense). Mechanism unchanged, still fails closed, CI green. Nice rewording.

Approving. One non-blocking nit below.

nit (completeness) — other declare-UP lock paths aren't re-confirmed

Verified against source: the durable re-confirm guards the auto-restore timer path (timer.ts) and the shields up drift path (index.ts:1265), but two other sites declare shields UP right after a bare lockAgentConfig (no re-confirm), so they keep the original post-settle drift exposure:

  • activateLockdownFromSnapshot (index.ts:855) → used by the expired-auto-restore inline recovery; saveShieldsState({ shieldsDown: false, … }) follows the bare lock, so a reconciler revert leaves the same DRIFTED state #4663 is about (via the expired-marker route).
  • the rollback re-lock (index.ts:799) — same saveShieldsState({ shieldsDown:false }) pattern; lower priority (error path).

Severity is lower than the primary timer path (rare/recovery flows), and the settle cost with the default settleMs is sub-second-to-a-few-seconds, so routing these through relockAndReconfirm(() => lockAgentConfig(...)) is cheap and closes the gap. Not holding the PR on it — I'll push that follow-up to this branch.

prekshivyas and others added 2 commits June 12, 2026 11:16
…NVIDIA#4663)

The auto-restore timer and shields-up drift paths already route their re-lock through relockAndReconfirm, but activateLockdownFromSnapshot (used by the expired-auto-restore inline recovery and the shields-up snapshot restore) and the shields-down rollback re-lock still called lockAgentConfig directly, then marked shields UP. A reconciler revert after those locks would leave the same DRIFTED state NVIDIA#4663 is about. Route both through relockAndReconfirm so they re-confirm past the settle window and fail closed (no false UP) when the lock will not hold.

Co-authored-by: Tony Luo <xialuo@nvidia.com>
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@cv cv assigned cv and unassigned prekshivyas Jun 12, 2026
@cv
cv enabled auto-merge (squash) June 13, 2026 05:38
@cv
cv merged commit 739f4eb into NVIDIA:main Jun 13, 2026
30 checks passed
@TonyLuo-NV
TonyLuo-NV deleted the fix/4663-shields-auto-restore-config-hash branch June 15, 2026 06:42
@miyoungc miyoungc mentioned this pull request Jun 16, 2026
13 tasks
cv pushed a commit that referenced this pull request Jun 17, 2026
## Summary
Refreshes release-prep documentation for NemoClaw v0.0.65.
Adds the v0.0.65 release-notes section and refreshes generated
`nemoclaw-user-*` skills from the Fern MDX source docs.

## Changes
- Added the v0.0.65 release notes to `docs/about/release-notes.mdx` with
links to the deeper docs pages for lifecycle, troubleshooting,
inference, CLI commands, messaging, credentials, network policy, Hermes,
and sub-agents.
- Regenerated the `nemoclaw-user-*` skills with
`scripts/docs-to-skills.py` so release-prep skill output matches the
merged source docs.
- Used the v0.0.65 announcement discussion as release context:
#5472.

## Source Summary
- #2492 -> `docs/about/release-notes.mdx`: Documents deadline-based
gateway wait reliability in the v0.0.65 recovery summary.
- #4958 -> `docs/about/release-notes.mdx`: Documents re-execed OpenClaw
gateway health check recovery in the sandbox recovery summary.
- #5163 -> `docs/about/release-notes.mdx`: Documents safer uninstall TTY
confirmation behavior in the day-two CLI summary.
- #5178 -> `docs/about/release-notes.mdx`: Documents fail-closed config
restore merge behavior in the rebuild and restore summary.
- #5179 -> `docs/about/release-notes.mdx`: Documents WeChat QR token
redaction in the messaging summary.
- #5182 -> `docs/about/release-notes.mdx`: Documents sustained gateway
serving checks in the recovery summary.
- #5194 -> `docs/about/release-notes.mdx`: Documents model-router
teardown during uninstall in the day-two CLI summary.
- #5195 -> `docs/about/release-notes.mdx`: Documents Shields
auto-restore lock reconfirmation in the rebuild and restore summary.
- #5198 -> `docs/about/release-notes.mdx`: Documents Docker Desktop WSL
CDI injection failure handling in the onboarding diagnostics summary.
- #5201 -> `docs/about/release-notes.mdx`: Documents sandbox
download/upload wrappers and sessions export in the day-two CLI summary.
- #5205 -> `docs/about/release-notes.mdx`: Documents reporter-owned
model metadata preservation in the rebuild and restore summary.
- #5214 -> `docs/about/release-notes.mdx`: Documents managed vLLM model
preflight before side effects in the inference setup summary.
- #5215 -> `docs/about/release-notes.mdx`: Documents managed vLLM extra
serve arguments in the inference setup summary.
- #5216 -> `docs/about/release-notes.mdx`: Documents silent OpenClaw
runtime fallback surfacing in the onboarding diagnostics summary.
- #5225 -> `docs/about/release-notes.mdx`: Documents persisted sandbox
gateway lookup in the gateway recovery summary.
- #5238 -> `docs/about/release-notes.mdx`: Documents sub-agent gateway
dial-back through the sandbox interface in the Hermes and sub-agent
summary.
- #5248 -> `docs/about/release-notes.mdx`: Documents Discord per-account
proxy resolution in the messaging summary.
- #5264 -> `docs/about/release-notes.mdx`: Documents reserved Hermes
port `8642` handling in the Hermes compatibility summary.
- #5267 -> `docs/about/release-notes.mdx`: Documents the narrower Hermes
baseline policy in the Hermes compatibility summary.
- #5321 -> `docs/about/release-notes.mdx`: Documents restored gateway
guard chains in the gateway recovery summary.
- #5328 -> `docs/about/release-notes.mdx`: Documents compact persisted
messaging plans in the messaging summary.
- #5338 -> `docs/about/release-notes.mdx`: Documents manifest channel
migration in the messaging summary.
- #5352 -> `docs/about/release-notes.mdx`: Documents persisted agent
preservation through registry recovery in the rebuild and restore
summary.
- #5371 ->
`.agents/skills/nemoclaw-user-reference/references/commands.md`:
Refreshes generated skill output for custom build cache and
layer-ordering source docs.
- #5379 -> `docs/about/release-notes.mdx`: Documents dashboard port
allocation across multiple NemoClaw gateways in the recovery summary.
- #5382 -> `docs/about/release-notes.mdx`: Documents recovery when an
active gateway has no sandbox spec in the recovery summary.
- #5389 ->
`.agents/skills/nemoclaw-user-reference/references/troubleshooting.md`:
Refreshes generated skill output for declared agent `forward_ports`
recovery source docs.
- #5400 -> `docs/about/release-notes.mdx`: Documents bounded compatible
endpoint probes in the inference setup summary.
- #5410 -> `docs/about/release-notes.mdx`: Documents provider credential
hash removal from sandbox registry entries in the messaging summary.
- #5418 -> `docs/about/release-notes.mdx`: Documents summarized
inference validation failures in the onboarding diagnostics summary.
- #5457 -> `docs/about/release-notes.mdx`: Documents context-window
recomputation after runtime model switches in the inference setup
summary.
- #5463 -> `docs/about/release-notes.mdx`: Documents cleanup of
hard-coded messaging channel stragglers in the messaging summary.

## Skipped
- #5366 matched `docs/.docs-skip` entries through skipped experimental
paths, so this PR does not add new release-note text for that commit.

## Type of Change
- [ ] Code change (feature, bug fix, or refactor)
- [ ] Code change with doc updates
- [ ] Doc only (prose changes, no code sample modifications)
- [x] Doc only (includes code sample changes)

## Verification
- [x] Git hooks passed during commit and push, or `npx prek run
--from-ref main --to-ref HEAD` passes
- [ ] Targeted tests pass for changed behavior
- [ ] Full `npm test` passes (broad runtime changes only)
- [ ] Tests added or updated for new or changed behavior
- [x] No secrets, API keys, or credentials committed
- [x] Docs updated for user-facing behavior changes
- [ ] `npm run docs` builds without warnings (doc changes only)
- [x] 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)

Verification notes:
- `npm run docs` passed after rerunning outside the sandbox. Fern
reported 0 errors and 1 hidden warning.
- The first sandboxed `npm run docs` attempt failed before validation
because `tsx` could not create its local IPC pipe under sandbox
restrictions.
- `npm run build:cli` passed before push to refresh the local `dist/`
artifacts used by the CLI typecheck hook.
- `npm test` was not run because this is a docs-only release refresh.

---
Signed-off-by: Miyoung Choi <miyoungc@nvidia.com>

<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit

* **New Features**
* Released NemoClaw v0.0.65 with improved gateway/sandbox recovery,
safer day-two workflows, and enhanced Hermes compatibility.
* Added managed vLLM extra-arguments configuration via
`NEMOCLAW_VLLM_EXTRA_ARGS_JSON`.
* Added Hermes troubleshooting guidance for port forwarding and health
checks.

* **Documentation**
* Updated NVIDIA Endpoints/NIM setup and examples to use
`NVIDIA_INFERENCE_API_KEY`.
* Refined NVIDIA network policy and Model Router API base configuration.
* Expanded CLI/environment variable documentation (including sub-agent
gateway connectivity) and plugin build performance tips.

* **Tests**
  * Expanded Vitest-backed E2E release validation coverage.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
@wscurran wscurran added the NV QA Bugs found by the NVIDIA QA Team label Jun 26, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: sandbox OpenShell sandbox lifecycle, runtime, config, or recovery area: security Security controls, permissions, secrets, or hardening bug-fix PR fixes a bug or regression NV QA Bugs found by the NVIDIA QA Team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[DGX Station + DGX Spark][Security] shields auto-restore leaves sandbox in DRIFTED state after timeout

4 participants