fix(shields): keep shields status truthful when the permissive policy is rejected (#8198) - #8200
fix(shields): keep shields status truthful when the permissive policy is rejected (#8198)#8200jason-ma-nv wants to merge 8 commits into
Conversation
… is rejected (#8198) shieldsDown persists shieldsDown=true before it weakens the sandbox so a crash mid-transition is honestly recorded and the auto-restore timer can recover. But the permissive `policy set` ran with no rollback: run() exits the process on a non-zero status, so when OpenShell refuses a live landlock change (a Deep Agents sandbox whose policy is sealed at startup) the process died after the DOWN state was already persisted. Nothing was actually weakened, yet `shields status` then reported DOWN/permissive for an unlock that never happened — a status-integrity defect. Run the permissive `policy set` with ignoreError and, on a non-zero status, clear the persisted shields-down state and cancel the now-pointless auto-restore timer/transition before failing closed. `shields status` then truthfully reports the sandbox as still up, matching the config-unlock failure path that already rolls back. Add a flow test that drives the real shieldsDown timer path with a rejected `policy set` and asserts the persisted state and isShieldsDown report the sandbox as up. Signed-off-by: Jason Ma <jama@nvidia.com> Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe shields-down flow now captures rejected permissive policy updates, cleans temporary policy files, and restores shield state. It reports whether rollback or the existing auto-restore timer remains authoritative. ChangesShields failure recovery
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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 3045-3054: Update the rollback block around saveShieldsState so
clearShieldsDownTransition and killTimer always execute in a finally block,
including when the rollback state write throws. Preserve the existing transition
guard and report the persistence failure after cleanup using the surrounding
error-reporting mechanism.
🪄 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: 663caf93-9a10-4735-b7fc-2e5308dd7ba7
📒 Files selected for processing (2)
src/lib/shields/flow.test.tssrc/lib/shields/index.ts
| saveShieldsState(sandboxName, { | ||
| shieldsDown: false, | ||
| shieldsDownAt: null, | ||
| shieldsDownTimeout: null, | ||
| shieldsDownReason: null, | ||
| shieldsDownPolicy: null, | ||
| shieldsPolicySnapshotPath: null, | ||
| }); | ||
| if (transition) clearShieldsDownTransition(sandboxName, transition.processToken); | ||
| killTimer(sandboxName); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Cancel recovery authority if the rollback state write fails.
If saveShieldsState at Line 3045 throws, Lines 3053-3054 do not run. The earlier state write already recorded shieldsDown: true, so a filesystem failure can leave a DOWN transition and auto-restore timer after OpenShell rejected the policy.
Put transition and timer cleanup in a finally block. Then report that state rollback could not be persisted.
Proposed fix
- saveShieldsState(sandboxName, {
- shieldsDown: false,
- shieldsDownAt: null,
- shieldsDownTimeout: null,
- shieldsDownReason: null,
- shieldsDownPolicy: null,
- shieldsPolicySnapshotPath: null,
- });
- if (transition) clearShieldsDownTransition(sandboxName, transition.processToken);
- killTimer(sandboxName);
+ try {
+ saveShieldsState(sandboxName, {
+ shieldsDown: false,
+ shieldsDownAt: null,
+ shieldsDownTimeout: null,
+ shieldsDownReason: null,
+ shieldsDownPolicy: null,
+ shieldsPolicySnapshotPath: null,
+ });
+ } finally {
+ if (transition) clearShieldsDownTransition(sandboxName, transition.processToken);
+ killTimer(sandboxName);
+ }🤖 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 3045 - 3054, Update the rollback block
around saveShieldsState so clearShieldsDownTransition and killTimer always
execute in a finally block, including when the rollback state write throws.
Preserve the existing transition guard and report the persistence failure after
cleanup using the surrounding error-reporting mechanism.
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported Model lanes
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. 3 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: 1 optional E2E recommendation
This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
… fails (#8198) Address CodeRabbit review on #8200: on a rejected permissive `policy set`, the rollback clears the persisted shields-down state before clearing the transition/timer. If that state write throws, the sandbox still reads shields-down on disk, so the auto-restore timer must remain the recovery authority. Wrap the rollback state write and, on failure, keep the timer and transition marker and report — instead of clearing them, which would strand the sandbox reporting down with no recovery. The success path is unchanged. Signed-off-by: Jason Ma <jama@nvidia.com> Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Addressed the CodeRabbit review in 11def3c, with one deliberate deviation from the literal suggestion. The rollback clears the persisted shields-down state, then clears the transition marker + auto-restore timer. Unconditionally clearing the timer in a So instead I wrapped the rollback state write in try/catch and, on failure, keep the timer and transition marker authoritative (they reclaim the already-restrictive policy on owner death) and report the failure, rather than clearing them. This matches the config-unlock failure path directly below, which likewise leaves the scheduled auto-restore authoritative. The success path is unchanged. Re-verified on the Ubuntu host (clean clone + |
cv
left a comment
cv
left a comment
There was a problem hiding this comment.
Reviewed the exact head for correctness and the shields security boundary. The rollback preserves the restrictive policy, attempts to restore truthful persisted state, and intentionally retains the timer and transition as recovery authority if that state write fails. That makes the unresolved CodeRabbit suggestion to always cancel recovery invalid for the current second commit: cancellation would leave a persisted DOWN state with no automatic recovery. The focused flow test covers the primary rejected-policy path. I found no secret, input-validation, authorization, dependency, logging, cryptography, configuration, or privilege regression in this two-file diff.
I cannot approve this head because required Advisor jobs failed, the required E2E gate is cancelled, and the branch is behind current main. Refresh the branch and obtain passing current-SHA evidence. The final PR body must also include the documentation-writer review receipt, with no-docs-needed evidence if the independent review confirms that the corrected status behavior needs no docs change.
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/lib/shields/flow.test.ts (3)
907-911: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAssert the complete recovery state.
The fixture creates a policy snapshot and several shields-down fields, but the test checks only
shieldsDownandshieldsDownAt. A regression could leave stale recovery metadata or the temporary snapshot while this test still passes. Assert the canonical UP-state fields and confirm thatsnapshotPathis removed.As per path instructions, assert observable recovery outcomes. The PR objective also requires cleanup of persisted shields-down state and temporary policy files.
🤖 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/flow.test.ts` around lines 907 - 911, Update the recovery-state assertions in the affected test to validate the complete canonical UP state, including all shields-down metadata fields and the absence of persisted temporary policy data. Extend the parsed state expectation to cover the fixture’s policy snapshot and recovery fields, and assert that snapshotPath is removed alongside markerPath.Source: Path instructions
883-898: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFail fast on unmodeled Docker commands.
The
defaultbranch silently returns an empty string for every command that the harness does not model. An unexpected command can then bypass the intended behavior instead of failing at the mock boundary. Make the default branch throw, or record unexpected commands and assert the allowed command set.As per path instructions, tests must not use broad mocks that bypass the behavior under test.
🤖 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/flow.test.ts` around lines 883 - 898, The default branch in the dockerExecFileSync mock silently returns an empty string for any command not explicitly handled, allowing unmodeled Docker commands to bypass intended behavior. Replace the default case to throw an error when an unexpected command is encountered, providing details about the command that was not recognized. This ensures the test fails fast at the mock boundary rather than silently continuing with empty-string responses.Source: Path instructions
842-848: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAuthorization Bypass (CWE-863): Incorrect Authorization
Add a mismatched-token recovery test.
Use distinct valid tokens for the timer marker and destroy lock owner. Exercise expired-marker status recovery without blocking on the live owner. Assert that the owner remains alive and the transition lock remains present.
🤖 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/flow.test.ts` around lines 842 - 848, Add a recovery test around the JSON fixture in flow.test.ts using distinct valid tokens for the timer marker and destroy-lock owner. Exercise expired-marker status recovery while the owner is still live, then assert the owner remains alive and the transition lock is still present.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/lib/shields/flow.test.ts`:
- Around line 907-911: Update the recovery-state assertions in the affected test
to validate the complete canonical UP state, including all shields-down metadata
fields and the absence of persisted temporary policy data. Extend the parsed
state expectation to cover the fixture’s policy snapshot and recovery fields,
and assert that snapshotPath is removed alongside markerPath.
- Around line 883-898: The default branch in the dockerExecFileSync mock
silently returns an empty string for any command not explicitly handled,
allowing unmodeled Docker commands to bypass intended behavior. Replace the
default case to throw an error when an unexpected command is encountered,
providing details about the command that was not recognized. This ensures the
test fails fast at the mock boundary rather than silently continuing with
empty-string responses.
- Around line 842-848: Add a recovery test around the JSON fixture in
flow.test.ts using distinct valid tokens for the timer marker and destroy-lock
owner. Exercise expired-marker status recovery while the owner is still live,
then assert the owner remains alive and the transition lock is still present.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: a1c2105e-9f19-44ef-a3df-96ba503dfdb5
📒 Files selected for processing (1)
src/lib/shields/flow.test.ts
cv
left a comment
cv
left a comment
There was a problem hiding this comment.
Reviewed commit 3bd6802bc8. The only change after the prior security review is the main merge. The rollback still keeps the restrictive policy and preserves recovery authority when state persistence fails; the CodeRabbit cleanup request remains invalid because it would strand false DOWN state without recovery.
Approval is blocked by the shared main capability-union failure tracked by #8213, failed CI/E2E, and a documentation-writer receipt that must identify this commit.
wscurran
added
area: cli
|
Exact-head CI triage for The No branch change is requested for that shard failure. After #8233 lands, this PR will need a current-main refresh/re-run; its other failed checks and review findings remain independent and still need their own disposition. |
cjagwani
left a comment
cjagwani
left a comment
There was a problem hiding this comment.
Reviewed exact head 503f3450f8b93f02bf5986807df85b5db7798fa5. The primary policy-rejection path is correct: a nonzero permissive policy set no longer leaves a false DOWN record, and retaining the timer/transition when the state-clear write itself fails is safer than CodeRabbit's earlier suggestion to cancel them unconditionally.
One correctness/security branch still needs direct coverage, matching the primary advisor warning: make the initial shields-down state write succeed, make the rejected policy set return nonzero, then make only the rollback saveShieldsState({ shieldsDown: false, ... }) throw. Assert the command fails, the persisted DOWN record remains, and the exact timer marker plus transition remain authoritative (and are not killed/cleared). This branch is the reason for the second commit and must be protected against a future cleanup regression.
Please keep the test at the observable flow level and preserve the current fail-closed behavior. Exact-head CI/E2E still applies after the test commit.
…-status-integrity
Address review on #8200: add direct coverage for the case where the rejected permissive policy set is followed by a failing rollback state write. Assert the command fails, the persisted shields-down record remains, and the timer marker and transition stay authoritative (are not killed/cleared) — the recovery invariant behind not unconditionally cancelling the timer. Also strengthen the primary rejection test to assert the full cleared UP state. Signed-off-by: Jason Ma <jama@nvidia.com> Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
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 991-997: Update the inline fork fake in the auto-restore test to
capture its returned process mock, then assert after shieldsDown throws that the
mock’s kill method was not called. Preserve the existing fork behavior and
verify the timer process remains active when rollback persistence fails.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 4b3a55d5-6afa-4106-a757-b82362eed97b
📒 Files selected for processing (1)
src/lib/shields/flow.test.ts
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
5 participants
Summary
On a sandbox whose OpenShell policy is sealed at startup (e.g.
langchain-deepagents-codelandlock),nemoclaw <sandbox> shields downcannot apply the permissive policy — OpenShell rejects a live landlock change andopenshell policy setexits non-zero. The command exited, butshields statusthen falsely reported the sandbox asDOWN/permissiveeven though nothing was unlocked and the filesystem posture stayed identical to shielded-up. That is a status-integrity defect: status claimed a state that was never achieved.Root cause:
shieldsDowndeliberately persistsshieldsDown=truebefore it weakens the sandbox, so a crash mid-transition is honestly recorded and the auto-restore timer can recover. But the permissivepolicy setran with no rollback — only afinallythat cleaned up the temp policy dir. Sincerun()callsprocess.exit(1)on a non-zero status, a rejectedpolicy setkilled the process after the DOWN state was already persisted. Nothing was actually weakened, yet the persisted state (and thereforeshields status) reported DOWN.Fixes #8198.
Changes
src/lib/shields/index.ts: run the permissivepolicy setwithignoreErrorand, on a non-zero status, clear the persisted shields-down state and cancel the now-pointless auto-restore timer/transition before failing closed. This mirrors the config-unlock failure path directly below it, which already rolls back.shields statusthen truthfully reports the sandbox as still up.src/lib/shields/flow.test.ts: add a flow test that drives the realshieldsDowntimer path with a rejectedpolicy setand asserts bothisShieldsDownand the on-disk state report the sandbox as up.Scope note
This fixes the status-integrity / security-posture defect (part 2 of the report): after a rejected
policy set, the sandbox is correctly reported as still shielded, matching the issue's Expected Result ("fails non-zero AND status keeps reporting the sandbox as still UP/locked"). Whether/howshields downshould be supported on a landlock-at-startup Deep Agents sandbox (part 1 — the unlock fundamentally cannot complete because OpenShell seals policy at startup) is a separate Deep Agents shields-support design question and is out of scope here.Verification
Run on the Ubuntu host (
npm ci+ plugin build, Node 22), against a clean clone of this branch:npm run typecheck:cli→ exit 0.npx vitest run --project cli src/lib/shields/flow.test.ts -t "clears state and reports the sandbox as up"→ 1 passed.npx vitest run --project cli src/lib/shields/flow.test.ts→ 24 passed.expected [Function] to throw an error) — without the fix, the rejectedpolicy setis not handled, soshieldsDowndoes not fail-close and leaves the DOWN state, reproducing the reported status-integrity defect.The second run confirms the test exercises the real
shieldsDownpath (not hand-built state); the first confirms the fix restores truthful status.Documentation
No user-visible surface change (no CLI, config, or documented-behavior change). Behavior is corrected to match the documented shields contract — status must never claim a state that was not achieved. No docs update required.
Signed-off-by: Jason Ma jama@nvidia.com
Summary by CodeRabbit