feat(shields): seal locked files with SHA-256 and detect content drift#4460
Merged
Conversation
Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (8)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (6)
📝 WalkthroughWalkthroughAdds per-file SHA-256 content seals captured at lock time, persists them in shields state, verifies them in shields status and shields up to detect host-induced content drift, refuses to re-seal tampered baselines by default (opt-in for legacy baselines), and adds tests and docs for these flows. ChangesContent sealing and drift detection
Sequence Diagram(s)sequenceDiagram
participant CLI as "nemoclaw (shields status/up)"
participant Verifier as "verifyShieldsLockState"
participant Sandbox as "sandbox (sha256sum)"
CLI->>Verifier: call with expectedHashes (state.fileHashes)
Verifier->>Sandbox: run sha256sum <file>
Sandbox-->>Verifier: sha256sum output / error
Verifier-->>CLI: issues or ok (drift / clean)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Comment |
Contributor
|
🌿 Preview your docs: https://nvidia-preview-pr-4460.docs.buildwithfern.com/nemoclaw |
Contributor
E2E Advisor RecommendationRequired E2E: Dispatch hint: Auto-dispatched E2E: Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
Dispatch hint
|
Contributor
E2E Scenario Advisor RecommendationRequired scenario E2E: None Full scenario advisor summaryE2E Scenario AdvisorBase: Required scenario E2E
Optional scenario E2E
Relevant changed files
|
Contributor
PR Review AdvisorFindings: 3 needs attention, 5 worth checking, 0 nice ideas Review findings🛠️ Needs attention
🔎 Worth checking
🌱 Nice ideas
Since last review detailsCurrent findings:
This is an automated advisory review. A human maintainer must make the final merge decision. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
docs/security/best-practices.mdx (1)
207-207: ⚡ Quick winSplit this into one sentence per source line.
This line currently contains multiple sentences; the docs style requires one sentence per line for readable diffs.
As per coding guidelines: "One sentence per line in source (makes diffs readable). Flag paragraphs where multiple sentences appear on the same line."
🤖 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/security/best-practices.mdx` at line 207, The sentence block describing the shields workflow packs multiple sentences on one source line; split it so each sentence becomes its own source line (one sentence per line) for readability and diff hygiene. Specifically break the paragraph so lines each contain a single sentence referencing the commands and artifacts: "nemoclaw <name> shields up", the SHA-256 seal of "openclaw.json" and other locked files, the host-side shields state file, how "shields status" recomputes the hash and surfaces drift, and that "shields up" refuses to re-seal a tampered baseline—ensuring each of those sentences is on its own line.
🤖 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 `@docs/security/best-practices.mdx`:
- Line 207: Update the wording to limit the claim about hash re-verification to
sealed sandboxes: change the sentence that currently reads "Every `shields
status` call recomputes the hash..." to something like "For sealed sandboxes
(when the shields state contains `expectedHashes`/`fileHashes`), `shields
status` recomputes the hashes inside the sandbox and surfaces drift on any
mismatch; legacy state without `expectedHashes`/`fileHashes` falls back to
permission-only verification." Keep the rest about `shields up` refusing to
re-seal a tampered baseline and referencing `openclaw.json` and the host-side
shields state file intact.
In `@src/lib/shields/index.ts`:
- Around line 292-301: The isOptionalHashMap type guard currently accepts any
string for fileHashes; change it to validate each value is a SHA-256 hex string
(64 hex characters) instead of any string: inside isOptionalHashMap (and any
similar guard used for fileHashes/shieldsUp), if value === undefined return
true, ensure value is an object record, then check every Object.values(value)
matches a SHA-256 hex regex (e.g. /^[a-fA-F0-9]{64}$/) and return false if any
do not; update the type guard so downstream code (e.g., shieldsUp) only receives
validated hash strings.
- Around line 1165-1178: The current filter only detects entries that include
"content drifted"; broaden the refusal so hash verification failures also block
re-sealing by changing the predicate on issues (used to build contentDrift) to
match entries that include "content drifted" OR contain verification failure
indicators such as "sha256sum failed" or "sha256sum output unparsable" (or
simply any entry that includes "sha256sum" / "output unparsable"); keep the same
logging and failure flow (console.error block and the call to
failShieldsCommand) but ensure the error message and contentDrift join still
reflect the broader set of matched entries (referencing the issues array,
contentDrift variable, failShieldsCommand, and opts.throwOnError).
In `@src/lib/shields/verify-lock.ts`:
- Around line 111-137: The verification branches in verify-lock where issues are
pushed (inside the options.expectedHashes loop over filesToVerify) must prefix
all hash-verification failures with "content drifted" so shieldsUp can detect
drift; update the messages produced in the branches that push `"${f} no seal
recorded (expected SHA-256)"`, `"${f} sha256sum failed: ${msg}"`, and `"${f}
sha256sum output unparsable: ${raw.trim()}"` (the code around exec(),
parseSha256Output(), and the final content-drift comparison) to start with
"content drifted" (e.g. "content drifted: <existing message>") while leaving the
existing details intact.
---
Nitpick comments:
In `@docs/security/best-practices.mdx`:
- Line 207: The sentence block describing the shields workflow packs multiple
sentences on one source line; split it so each sentence becomes its own source
line (one sentence per line) for readability and diff hygiene. Specifically
break the paragraph so lines each contain a single sentence referencing the
commands and artifacts: "nemoclaw <name> shields up", the SHA-256 seal of
"openclaw.json" and other locked files, the host-side shields state file, how
"shields status" recomputes the hash and surfaces drift, and that "shields up"
refuses to re-seal a tampered baseline—ensuring each of those sentences is on
its own line.
🪄 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: a0ae2ca5-8264-4c0b-9d09-a60d51b4a319
📒 Files selected for processing (6)
docs/security/best-practices.mdxsrc/lib/shields/index.test.tssrc/lib/shields/index.tssrc/lib/shields/verify-lock.test.tssrc/lib/shields/verify-lock.tstest/repro-2681-group-writable.test.ts
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 26597763562
|
Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Contributor
Selective E2E Results — ❌ Some jobs failedRun: 26613650342
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/lib/shields/verify-lock.ts (1)
105-123:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPrefix all hash-verification failure paths with
content drifted.
shields updrift gating relies on drift-classified issues; these branches still emit non-drift strings (no seal recorded,sha256sum failed,sha256sum output unparsable), which can allow reseal on failed verification paths.Suggested minimal fix
if (!want) { // Seal was missing for this file — flag explicitly rather than // silently passing. Callers that genuinely lack a seal pass // `expectedHashes: undefined` instead of an empty record. - issues.push(`${f} no seal recorded (expected SHA-256)`); + issues.push(`${f} content drifted (no seal recorded; expected SHA-256)`); continue; } let raw: string; try { raw = exec(["sha256sum", f]); } catch (err) { const msg = err instanceof Error ? err.message : String(err); - issues.push(`${f} sha256sum failed: ${msg}`); + issues.push(`${f} content drifted (sha256sum failed: ${msg})`); continue; } const got = parseSha256Output(raw); if (!got) { - issues.push(`${f} sha256sum output unparsable: ${raw.trim()}`); + issues.push(`${f} content drifted (sha256sum output unparsable: ${raw.trim()})`); continue; }🤖 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/verify-lock.ts` around lines 105 - 123, The error messages in verify-lock.ts that are pushed into the issues array for hash verification failures (when want is falsy, when exec(["sha256sum", f]) throws, and when parseSha256Output(raw) returns falsy) must be prefixed so they are classified as drift; update the three pushes that produce `${f} no seal recorded (expected SHA-256)`, `${f} sha256sum failed: ${msg}`, and `${f} sha256sum output unparsable: ${raw.trim()}` to start with "content drifted" (e.g., "content drifted: ...") while keeping the original details, referencing the surrounding variables and helpers (want, f, exec, parseSha256Output, got, issues) so callers relying on drift-classified issues see these as drift.
🤖 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 `@test/e2e/test-shields-config.sh`:
- Around line 346-350: Replace the set +e / set -e toggling with a non-mutating
command group that captures both output and exit code without changing global
errexit; e.g. for the shields status probe, use a grouped construct like {
STATUS_TAMPER_OUTPUT=$(nemoclaw "${SANDBOX_NAME}" shields status 2>&1);
STATUS_TAMPER_EXIT=$?; } || true and then echo "$STATUS_TAMPER_OUTPUT", and
apply the same pattern to the other probe (the block that sets
STATUS_TAMPER_OUTPUT/STATUS_TAMPER_EXIT around the shields status call) so you
no longer flip errexit for the rest of the test.
---
Duplicate comments:
In `@src/lib/shields/verify-lock.ts`:
- Around line 105-123: The error messages in verify-lock.ts that are pushed into
the issues array for hash verification failures (when want is falsy, when
exec(["sha256sum", f]) throws, and when parseSha256Output(raw) returns falsy)
must be prefixed so they are classified as drift; update the three pushes that
produce `${f} no seal recorded (expected SHA-256)`, `${f} sha256sum failed:
${msg}`, and `${f} sha256sum output unparsable: ${raw.trim()}` to start with
"content drifted" (e.g., "content drifted: ...") while keeping the original
details, referencing the surrounding variables and helpers (want, f, exec,
parseSha256Output, got, issues) so callers relying on drift-classified issues
see these as drift.
🪄 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: ad68e9ee-2347-42b4-97d7-9c8eb366ca8a
📒 Files selected for processing (9)
docs/security/best-practices.mdxsrc/lib/shields/index.test.tssrc/lib/shields/index.tssrc/lib/shields/seal.test.tssrc/lib/shields/seal.tssrc/lib/shields/timer.tssrc/lib/shields/verify-lock.test.tssrc/lib/shields/verify-lock.tstest/e2e/test-shields-config.sh
✅ Files skipped from review due to trivial changes (2)
- src/lib/shields/seal.ts
- src/lib/shields/seal.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- docs/security/best-practices.mdx
- src/lib/shields/verify-lock.test.ts
- src/lib/shields/index.test.ts
- src/lib/shields/index.ts
Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Contributor
Selective E2E Results — ❌ Some jobs failedRun: 26614673882
|
Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
docs/reference/commands.mdx (1)
1399-1399: ⚡ Quick winUse second person ("you") instead of third person ("the operator").
The phrase "asks the operator to rebuild" should use "you" to address the reader directly. As per coding guidelines, documentation should use second person when addressing the reader.
📝 Suggested rewrite
-| `NEMOCLAW_SHIELDS_ACCEPT_LEGACY_BASELINE` | `1` to opt in | When `nemoclaw <name> shields up` runs against a sandbox that was locked before the SHA-256 content seal landed, the existing on-disk bytes have no independently verified baseline. By default, `shields up` refuses to capture a seal in that state and asks the operator to rebuild the sandbox for a known-good baseline. Set this to `1` to accept the current bytes as the trusted baseline and let the seal be captured anyway. Once captured, subsequent `shields status` runs detect any future drift. | +| `NEMOCLAW_SHIELDS_ACCEPT_LEGACY_BASELINE` | `1` to opt in | When `nemoclaw <name> shields up` runs against a sandbox that was locked before the SHA-256 content seal landed, the existing on-disk bytes have no independently verified baseline. By default, `shields up` refuses to capture a seal in that state and asks you to rebuild the sandbox for a known-good baseline. Set this to `1` to accept the current bytes as the trusted baseline and let the seal be captured anyway. Once captured, subsequent `shields status` runs detect any future drift. |🤖 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 1399, Change the phrasing in the description for NEMOCLAW_SHIELDS_ACCEPT_LEGACY_BASELINE to use second person ("you") instead of third person ("the operator"); specifically rewrite the sentence "By default, `shields up` refuses to capture a seal in that state and asks the operator to rebuild the sandbox for a known-good baseline." to address the reader (e.g., "By default, `nemoclaw <name> shields up` refuses to capture a seal in that state and asks you to rebuild the sandbox for a known-good baseline.") while preserving references to `NEMOCLAW_SHIELDS_ACCEPT_LEGACY_BASELINE`, `nemoclaw <name> shields up`, and `shields status`.
🤖 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 1399: Change the phrasing in the description for
NEMOCLAW_SHIELDS_ACCEPT_LEGACY_BASELINE to use second person ("you") instead of
third person ("the operator"); specifically rewrite the sentence "By default,
`shields up` refuses to capture a seal in that state and asks the operator to
rebuild the sandbox for a known-good baseline." to address the reader (e.g., "By
default, `nemoclaw <name> shields up` refuses to capture a seal in that state
and asks you to rebuild the sandbox for a known-good baseline.") while
preserving references to `NEMOCLAW_SHIELDS_ACCEPT_LEGACY_BASELINE`, `nemoclaw
<name> shields up`, and `shields status`.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 4900a610-3754-475f-aec4-9e0f4b8f945f
📒 Files selected for processing (3)
docs/reference/commands.mdxsrc/lib/shields/verify-lock.test.tssrc/lib/shields/verify-lock.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/lib/shields/verify-lock.test.ts
- src/lib/shields/verify-lock.ts
Contributor
Selective E2E Results — ❌ Some jobs failedRun: 26615118718
|
Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 26615584566
|
Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 26617046247
|
Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 26617726235
|
Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
laitingsheng
changed the title
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 26618302904
|
cv
approved these changes
May 30, 2026
This was referenced May 30, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
shields upnow captures a SHA-256 seal of each locked file into the host-side shields state.shields statusrecomputes the hash inside the sandbox and flags content drift, so a host-root tamper that flips perms back to444 root:rootafter rewriting the file is detected.shields uprefuses to launder hash drift, missing seals, or other hash-trust failures; legacy lockdowns require an explicit operator opt-in to seal the current bytes.Related Issue
Fixes #4243.
Changes
src/lib/shields/seal.ts(new) +seal.test.ts: shared SHA-256 helpers (SHA256_HEX_RE,isSha256Hex,parseSha256Output) and theisHashVerificationIssueclassifier used byshieldsUpandshieldsStatus.src/lib/shields/verify-lock.ts+ tests: newexpectedHashesoption; every hash-trust failure is prefixed withcontent drifted(missing seal, sha256sum failure, unparsable output, hash mismatch) so callers can refuse to launder.src/lib/shields/index.ts+ tests:ShieldsState.fileHashesschema withisOptionalHashMapvalidating SHA-256 hex;lockAgentConfigreturns{ chattrApplied, fileHashes };shieldsUp,activateLockdownFromSnapshot,rollbackShieldsDownpersist the seal;shieldsStatusthreadsstate.fileHashesinto the verifier, splits the recovery hint by drift type, and surfaces the legacy-state notice when no seal is recorded; legacy and partial-seal lockdowns refuse to seal withoutNEMOCLAW_SHIELDS_ACCEPT_LEGACY_BASELINE=1.src/lib/shields/timer.ts+ tests: auto-restore timer passes the resolved agent target (incl. sensitive files) tolockAgentConfigand persistschattrApplied/fileHashesin the state file.test/repro-2681-group-writable.test.ts: lock-probe stub answerssha256sumso the post-lock seal capture completes.test/e2e/test-shields-config.sh: new Phase 5b drives adocker exec -u 0chmod-write-chmod tamper, verifiesshields statusexits 2 with the DRIFTED line, verifiesshields uprefuses to re-seal, and uses a byte-preserving temp-file backup so the content restore matches the original SHA-256 exactly.docs/security/best-practices.mdx,docs/reference/commands.mdx: document the content seal, the legacy-baseline refusal, and theNEMOCLAW_SHIELDS_ACCEPT_LEGACY_BASELINEopt-in.Type of Change
Verification
npx prek run --all-filespassesnpm testpassesnpm run docsbuilds without warnings (doc changes only)Signed-off-by: Tinson Lai tinsonl@nvidia.com
Summary by CodeRabbit
Release Notes
New Features
shields statusreports drift with exit code 2.Documentation
Tests