fix(security): harden post-merge state handling - #8074
Merged
Conversation
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@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 (5)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughThe change adds secure snapshot installation, Deep Agents fail-closed configuration locking, recovery-aware timer metadata, expanded security tests, and operator guidance for critical lock failures. ChangesDeep Agents security and recovery
Estimated code review effort: 5 (Critical) | ~90 minutes Sequence Diagram(s)sequenceDiagram
participant MigrationState
participant SnapshotBoundary
participant TrustedHelper
MigrationState->>SnapshotBoundary: Install sanitized snapshot configuration
SnapshotBoundary->>TrustedHelper: Validate and install pinned file
TrustedHelper-->>SnapshotBoundary: Return installation status
SnapshotBoundary-->>MigrationState: Continue or abort preparation
sequenceDiagram
participant Shields
participant LockTransaction
participant StateDirectory
Shields->>LockTransaction: Lock Deep Agents configuration
LockTransaction->>StateDirectory: Apply containment or rollback posture
StateDirectory-->>LockTransaction: Return verification status
LockTransaction-->>Shields: Return structured lock result
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 |
Contributor
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in commit 662d4ad in the TypeScript / code-coverage/cliThe overall coverage in commit 662d4ad in the Show a code coverage summary of the most impacted files.
Updated |
Contributor
|
🌿 Preview your docs: https://nvidia-preview-pr-8074.docs.buildwithfern.com/nemoclaw |
Contributor
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported Model lanes
5 terminology differences from the second opinionAdvisory only. These are normalized differences from the primary terminology receipt.
2 additional E2E selections from the second opinionAdvisory only. The primary lane did not select these E2E jobs or targets.
Second-opinion terminology and E2E selections are advisory. They do not change the primary assessment or E2E / PR Gate. 4 semantic terminology decisionsTerminology decisions are advisory. They affect the assessment only when a separate finding identifies concrete semantic impact.
E2E guidanceAdvisory only. E2E / PR Gate selects and runs jobs independently. Recommended E2E: 2 optional E2E recommendations
This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
![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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (1)
src/lib/shields/index.ts (1)
2379-2405: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPreserve the underlying error in the Deep Agents rollback diagnostics.
Both
catchblocks discard the caught error and substitute a fixed"command failed"string. The CRITICAL line then tells the operator that rollback failed without saying why. The OpenClaw branch at lines 2328-2352 keeps the cause throughopenClawRollbackIssue. Match that treatment so the two rollback paths give equivalent detail.♻️ Proposed change
try { rollbackIssues.push( ...restoreStateDirLockPosture(stateDirLockExec(sandboxName), target.configDir, true), ); - } catch { - rollbackIssues.push("state-directory rollback failed: command failed"); + } catch (rollbackError) { + const message = + rollbackError instanceof Error ? rollbackError.message : String(rollbackError); + rollbackIssues.push(`state-directory rollback failed: ${message}`); }Apply the same change to the
verifyShieldsLockStatecatchbelow 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 `@src/lib/shields/index.ts` around lines 2379 - 2405, Preserve the underlying exceptions in both Deep Agents rollback catch blocks within the rollback handling branch: capture each caught error and pass it through the same rollback-issue formatting or cause-preserving treatment used by openClawRollbackIssue. Apply this consistently to the state-directory restoration and verifyShieldsLockState catches so the final CRITICAL diagnostic includes the original failure details instead of only “command failed.”
🤖 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 `@nemoclaw/src/commands/migration-state-security.test.ts`:
- Around line 89-90: Remove the dead null-check following the non-null assertion
in the migration-state security test. Update the cleanupSnapshotBundle call to
use the narrowed snapshot value, following the as NonNullable<typeof ...>
pattern used in snapshot-sanitizer-failure.test.ts, while preserving the
existing assertion.
In `@nemoclaw/src/security/credential-filter.test.ts`:
- Around line 100-102: Replace the duplicated apiKey assertion in the
valueLooksLikeSecret tests with a distinct input that exercises another
CONTEXT_SECRET_PATTERNS shape, such as a camelCase Secret suffix or bare KEY
assignment, while keeping the expected result true.
In `@nemoclaw/src/security/snapshot-sanitizer-failure.test.ts`:
- Around line 128-157: Replace the unbounded shell spin loops in the race-test
wrappers with one shared bounded-wait helper. In
nemoclaw/src/security/snapshot-sanitizer-failure.test.ts lines 128-157, update
the wait around installDescriptorSnapshotFile to use waitUntil and ensure the
subshell exits when the cap is reached; in lines 159-191, use the same helper
for both target and alias size waits, passing the 20,000 ms timeout for the size
check and adding the helper’s short sleep. Preserve the existing race-test
behavior.
In `@src/lib/shields/index.ts`:
- Around line 804-833: Unify the persisted fallback-target construction and
registry-match behavior across src/lib/shields/index.ts lines 804-833 and
src/lib/shields/timer.ts lines 341-370: extract the shared helper from
resolvePersistedAutoRestoreTarget, explicitly choose consistent handling for
missing marker.agentName, and reuse the same configHashPath-derived
sensitive-file list. Update the timer recovery path to call this helper instead
of rebuilding persistedLockTarget inline; both sites must produce the same
target for identical markers, including markers without agentName.
In `@src/lib/shields/seal.test.ts`:
- Around line 845-862: Move the outcome.status and outcome.stderr assertions
into the existing try block in the runLock test, before the filesystem stat
checks, so any assertion failure still reaches the finally cleanup that restores
directory permissions.
In `@src/lib/shields/timer.ts`:
- Around line 352-364: Update the comment above the lockTarget conditional in
the timer logic to describe that the resolved target is used only when its
configPath, configDir, and optional agentName match the timer arguments;
otherwise persistedLockTarget is retained, while preserving the existing
explanation about sensitiveFiles and content-seal hash completeness.
In `@test/nemoclaw-plugin-secret-pattern-parity.test.ts`:
- Around line 12-16: Strengthen the test in “matches every canonical context
pattern source and flag” by asserting that CONTEXT_SECRET_PATTERNS is non-empty
before comparing fingerprints, while retaining the existing parity assertion
against CONTEXT_PATTERNS.
---
Nitpick comments:
In `@src/lib/shields/index.ts`:
- Around line 2379-2405: Preserve the underlying exceptions in both Deep Agents
rollback catch blocks within the rollback handling branch: capture each caught
error and pass it through the same rollback-issue formatting or cause-preserving
treatment used by openClawRollbackIssue. Apply this consistently to the
state-directory restoration and verifyShieldsLockState catches so the final
CRITICAL diagnostic includes the original failure details instead of only
“command failed.”
🪄 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: 7b434bec-0bbc-4513-8043-43671b1aaa66
📒 Files selected for processing (20)
docs/reference/commands.mdxdocs/reference/troubleshooting.mdxnemoclaw/src/commands/migration-state-security.test.tsnemoclaw/src/commands/migration-state.test.tsnemoclaw/src/commands/migration-state.tsnemoclaw/src/security/credential-filter.test.tsnemoclaw/src/security/credential-filter.tsnemoclaw/src/security/snapshot-sanitizer-failure.test.tsnemoclaw/src/shared/snapshot-sanitizer-boundary.ctsscripts/state-dir-guard.pysrc/lib/shields/flow.test.tssrc/lib/shields/index.tssrc/lib/shields/policy-transition.test.tssrc/lib/shields/seal.test.tssrc/lib/shields/seal.tssrc/lib/shields/timer-control.tssrc/lib/shields/timer.test.tssrc/lib/shields/timer.tstest/nemoclaw-plugin-secret-pattern-parity.test.tstest/state-dir-guard.test.ts
This was referenced Aug 3, 2026
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
23 tasks
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
src/lib/shields/policy-transition.test.ts (1)
192-210: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename
reportMissingTimerProcess.
reportMissingTimerProcessis the dispatcher. Its default branch reports a running process. The name states the opposite of the default behavior. Use a neutral name, for examplerespondToProcessKill, and keepreportTimerProcessMissingfor the ESRCH handler.🤖 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/policy-transition.test.ts` around lines 192 - 210, Rename the dispatcher function reportMissingTimerProcess to a neutral name such as respondToProcessKill, updating all references while preserving its lookup and default-running behavior. Keep reportTimerProcessMissing unchanged as the ESRCH handler.src/lib/shields/index.ts (1)
2191-2193: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse the computed
deepAgentsProtocolflag.Line 2148 already stores
isDeepAgentsTarget(target)indeepAgentsProtocol. Line 2191 recomputes the same predicate. Use the flag so both the preflight skip at Line 2165 and the lock branch cannot diverge.♻️ Proposed change
- if (isDeepAgentsTarget(target)) { + if (deepAgentsProtocol) { lockDeepAgentsTopConfig(sandboxName, target, !rollbackLocked); deepAgentsLockSucceeded = true;🤖 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 2191 - 2193, Update the lock branch around lockDeepAgentsTopConfig to use the existing deepAgentsProtocol flag instead of recomputing isDeepAgentsTarget(target), keeping the preflight skip and locking decision consistent.src/lib/shields/timer.ts (1)
177-181: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAlign the
agentNamecomparison with the path comparisons.Line 178 requires exact equality for
agentName. Lines 179-180 accept an absentmarker.configPathormarker.configDir. The marker is the authority in both cases, so the two rules should read the same way.The mismatch is not reachable today, because Line 170 also requires
marker.pid === process.pid, and marker and argv come from the same timer generation. Treat this as consistency only.♻️ Proposed change
- marker.agentName === args.agentName && + (marker.agentName === undefined || marker.agentName === args.agentName) &&🤖 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/timer.ts` around lines 177 - 181, Update the marker matching predicate to treat agentName like configPath and configDir: accept an undefined marker.agentName, or require it to equal args.agentName. Preserve the existing exact comparisons for leaseOwnerStartIdentity and the path fields.src/lib/shields/timer.test.ts (1)
55-73: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDo not copy the production derivation into the mock.
Lines 55-73 reproduce the
sensitiveFilesderivation fromsrc/lib/shields/index.tsLines 811-819, including the trailing-slash stripping and the Hermes.enventry. If the production rule changes, this mock keeps the old rule and the timer tests still pass, so the lock-target assertions stop proving anything.
src/lib/shields/auto-restore-target.test.tsalready covers the derivation against the real resolver. Return a fixed target per scenario here instead, and assert the target that reacheslockAgentConfigrather than the resolver call at Line 706.♻️ Proposed direction
- shieldsIndexMock.resolvePersistedAutoRestoreTarget = vi.fn( - ( - _sandboxName: string, - marker: { agentName?: string; configPath?: string; configDir?: string }, - ) => - marker.configPath && marker.configDir - ? { - ...(marker.agentName ? { agentName: marker.agentName } : {}), - configPath: marker.configPath, - configDir: marker.configDir, - sensitiveFiles: [ - `${marker.configDir.replace(/\/+$/, "")}/.config-hash`, - ...(marker.agentName === "hermes" - ? [`${marker.configDir.replace(/\/+$/, "")}/.env`] - : []), - ], - } - : undefined, - ); + // Each test sets the exact target it expects the timer to lock. + shieldsIndexMock.resolvePersistedAutoRestoreTarget = vi.fn(() => undefined);As per path instructions: "Prefer observable outcomes through the public boundary over source-text, private-shape, or mock-call assertions" and "Flag copied production algorithms".
🤖 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/timer.test.ts` around lines 55 - 73, Replace the production-like `sensitiveFiles` derivation inside `shieldsIndexMock.resolvePersistedAutoRestoreTarget` with fixed scenario-specific target values, leaving derivation coverage to the real resolver tests. Update the timer assertions around `lockAgentConfig` to verify the resolved target passed to that public boundary rather than asserting the resolver call.Source: Path instructions
src/lib/shields/auto-restore-target.test.ts (1)
85-105: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the deny-path test for an incomplete marker.
The guard at
src/lib/shields/index.tsLine 809 returnsundefinedwhenconfigPathorconfigDiris absent.src/lib/shields/timer.tsLines 337-346 depend on thatundefinedto fail closed and emit theshields_auto_restore_lock_warningaudit record. No test covers it.Add a case that proves the resolver returns
undefinedand never consults the registry for an incomplete marker.💚 Proposed test
it("returns no target and skips the registry when the marker has no configDir (`#8074`)", () => { const resolveConfig = vi.fn(); expect( resolvePersistedAutoRestoreTarget( "incomplete-marker", { agentName: "openclaw", configPath: "/sandbox/.openclaw/openclaw.json" }, resolveConfig, ), ).toBeUndefined(); expect(resolveConfig).not.toHaveBeenCalled(); });Import
vifromvitestat Line 4 for this case.As per path instructions: "Require negative-path tests that prove the boundary rejects bypasses and does not leak secrets in errors, logs, state, or process arguments."
🤖 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/auto-restore-target.test.ts` around lines 85 - 105, Add a deny-path test alongside the existing resolvePersistedAutoRestoreTarget tests for a marker missing configDir, asserting the resolver returns undefined and the injected registry resolver is not called. Import vi from vitest to create the mock resolver, preserving the fail-closed behavior without exposing marker details.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 `@src/lib/shields/policy-transition.test.ts`:
- Around line 184-190: Update rejectConfigLock to delegate non-lock commands to
runPythonFixtureCommand, while still throwing the supplied failure for
LOCK_COMMAND_KEY. In the rollback test, remove or narrow the
restoreStateDirLockPosture mock so the real state-directory restore executes,
then assert the resulting restored state rather than only verifying a mocked
call.
---
Nitpick comments:
In `@src/lib/shields/auto-restore-target.test.ts`:
- Around line 85-105: Add a deny-path test alongside the existing
resolvePersistedAutoRestoreTarget tests for a marker missing configDir,
asserting the resolver returns undefined and the injected registry resolver is
not called. Import vi from vitest to create the mock resolver, preserving the
fail-closed behavior without exposing marker details.
In `@src/lib/shields/index.ts`:
- Around line 2191-2193: Update the lock branch around lockDeepAgentsTopConfig
to use the existing deepAgentsProtocol flag instead of recomputing
isDeepAgentsTarget(target), keeping the preflight skip and locking decision
consistent.
In `@src/lib/shields/policy-transition.test.ts`:
- Around line 192-210: Rename the dispatcher function reportMissingTimerProcess
to a neutral name such as respondToProcessKill, updating all references while
preserving its lookup and default-running behavior. Keep
reportTimerProcessMissing unchanged as the ESRCH handler.
In `@src/lib/shields/timer.test.ts`:
- Around line 55-73: Replace the production-like `sensitiveFiles` derivation
inside `shieldsIndexMock.resolvePersistedAutoRestoreTarget` with fixed
scenario-specific target values, leaving derivation coverage to the real
resolver tests. Update the timer assertions around `lockAgentConfig` to verify
the resolved target passed to that public boundary rather than asserting the
resolver call.
In `@src/lib/shields/timer.ts`:
- Around line 177-181: Update the marker matching predicate to treat agentName
like configPath and configDir: accept an undefined marker.agentName, or require
it to equal args.agentName. Preserve the existing exact comparisons for
leaseOwnerStartIdentity and the path fields.
🪄 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: 907cd4d8-680b-4272-89cb-0939cec9108f
📒 Files selected for processing (13)
docs/reference/troubleshooting.mdxnemoclaw/src/commands/migration-state-security.test.tsnemoclaw/src/commands/migration-state.test.tsnemoclaw/src/security/credential-filter.test.tsnemoclaw/src/security/snapshot-sanitizer-failure.test.tsnemoclaw/src/shared/snapshot-sanitizer-boundary.ctssrc/lib/shields/auto-restore-target.test.tssrc/lib/shields/index.tssrc/lib/shields/policy-transition.test.tssrc/lib/shields/seal.test.tssrc/lib/shields/timer.test.tssrc/lib/shields/timer.tstest/nemoclaw-plugin-secret-pattern-parity.test.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- nemoclaw/src/security/credential-filter.test.ts
- nemoclaw/src/shared/snapshot-sanitizer-boundary.cts
- nemoclaw/src/security/snapshot-sanitizer-failure.test.ts
- nemoclaw/src/commands/migration-state.test.ts
- docs/reference/troubleshooting.mdx
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
This was referenced Aug 3, 2026
This was referenced Aug 4, 2026
wscurran
added
area: sandbox
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
4 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
Follow-up review of merged #7995 and #7847 found two independent state-handling gaps. Shields recovery now remains bound to its original configuration target. Migration preparation now installs only verified, credential-filtered configuration bytes through a pinned directory descriptor.
Changes
openclaw.jsonfrom the general recursive copy.0600.Type of Change
Quality Gates
662d4ad020369dfcad3a21da225090cce507b111against base SHAd6ac4027b75b15b8acd1456984acbbfb623cb231. No findings in secrets and credentials; input validation and data sanitization; authentication and authorization; dependencies and third-party libraries; error handling and logging; cryptography and data protection; configuration and security headers; security testing; or system security. The three addressed review threads are resolved.Documentation Writer Review
docs-updated662d4ad020369dfcad3a21da225090cce507b111against base SHAd6ac4027b75b15b8acd1456984acbbfb623cb231. Revieweddocs/reference/commands.mdx,docs/reference/troubleshooting.mdx, and changed explanatory text against the NemoClaw Writing Guide, Controlled Word List, documentation guidance, implementation, tests, and Deep Agents variant routing. Required checksdocs-review-receipt,cli-parity, andpreviewpass for this commit. No blocking finding remains.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 unavailablechecksconcludedSUCCESSfor commit SHA662d4ad020369dfcad3a21da225090cce507b111.E2E / PR Gateis running for commit SHA662d4ad020369dfcad3a21da225090cce507b111.npm run docsbuilds without warnings (documentation changes only) — Required documentation checks pass for commit SHA662d4ad020369dfcad3a21da225090cce507b111.Signed-off-by: Carlos Villela cvillela@nvidia.com
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests