fix(shields): clean up permissive runtime temp dir on failed shields down (#7964)

[jason-ma-nv]

Summary

nemoclaw <sandbox> shields down leaked a 0700 temporary directory into the
system temp directory whenever the transition failed partway through.

shieldsDown builds the merged permissive policy into a mkdtemp directory, but
only the policy-apply try/finally removed it. Two early exits sit between the
build and that apply, and both skipped cleanup:

1. the auto-restore timer failure (Cannot start auto-restore timer: …), and
2. the saveShieldsState failure (rethrow).

Each failed transition therefore leaked one nemoclaw-permissive-runtime-*
directory holding the merged permissive policy YAML. The merge runs on
effectively every real shields down, because a live openshell policy get --base always carries filesystem_policy.read_only/read_write entries.

Fixes #7964.

Changes

• src/lib/shields/index.ts: route all three exits (timer failure,
  saveShieldsState failure, and the existing apply finally) through a single
  cleanupRuntimePolicyFile() closure, so a failed shields down leaves nothing
  behind.
• src/lib/shields/flow.test.ts: add a regression test that drives the real
  shieldsDown timer-failure path with a live policy carrying
  filesystem_policy paths (so the permissive merge writes a temp file, matching
  production) and asserts no nemoclaw-permissive-runtime-* directory remains.
  A new livePolicyYaml harness option supplies that live policy.

Verification

Run on the Ubuntu host (npm ci + plugin build, Node 22), against a clean clone
of this branch:

• With the fix: npx vitest run --project cli src/lib/shields/flow.test.ts -t "removes the permissive runtime temp directory" → 1 passed.
• Reverting only the source fix, keeping the new test: same command →
  1 failed with AssertionError: expected [ Array(1) ] to deeply equal []
  and a leftover nemoclaw-permissive-runtime-* directory.

The second run confirms the test exercises the real shieldsDown code path (not
hand-built internal state) and reproduces the reported leak; the first confirms
the fix removes it.

Documentation

No user-visible surface changes (no CLI, config, output, or documented behavior
change). Behavior is corrected to match the already-documented contract that a
failed shields down leaves nothing behind. No docs update required.

Signed-off-by: Jason Ma jama@nvidia.com

Summary by CodeRabbit

• Bug Fixes
  • Fixed cleanup of temporary runtime policy files when automatic restoration fails to start.
  • Ensured temporary permissive-policy directories are removed after state-save failures and policy-application completion.
  • Improved cleanup reliability across policy restoration outcomes, preventing temporary files from being left behind.
  • Added regression coverage for cleanup during failed timer startup.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 6b2b947f-422f-404a-8a9d-c40553269f4b

📥 Commits

Reviewing files that changed from the base of the PR and between f092199 and 172f40d.

📒 Files selected for processing (1)

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

🚧 Files skipped from review as they are similar to previous changes (1)

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

---

📝 Walkthrough

Walkthrough

The shields-down flow now shares cleanup for temporary permissive-policy directories across timer startup failure, state persistence failure, and policy application. Tests can provide live policy YAML and verify cleanup after timer startup failure.

Changes

Shields runtime policy cleanup

Layer / File(s)	Summary
Centralize shields-down policy cleanup src/lib/shields/index.ts	shieldsDownWithoutHostLock uses one cleanup callback for timer startup failure, state persistence failure, and policy application completion.
Test failed timer startup cleanup src/lib/shields/flow.test.ts	The harness accepts live policy YAML, and a regression test verifies that failed timer startup removes temporary permissive-runtime directories.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

• NVIDIA/NemoClaw#7886: Both PRs modify Shields timer behavior and policy cleanup tests.
• NVIDIA/NemoClaw#7965: Both PRs update shieldsDownWithoutHostLock and tests for temporary permissive-policy directory cleanup.

Suggested labels: area: sandbox, bug-fix

Suggested reviewers: cv

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly describes cleanup of the permissive runtime temporary directory after failed shields-down transitions.
Linked Issues check	✅ Passed	The changes address issue #7964 by cleaning up temporary policy directories on all failure paths and adding the required regression coverage.
Out of Scope Changes check	✅ Passed	The source and test changes directly support the linked issue and introduce no unrelated scope.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/7964-shields-permissive-temp-cleanup

---

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

[github-code-quality]

Code Coverage Overview

Languages: TypeScript

TypeScript / code-coverage/plugin

The overall coverage in commit 172f40d in the fix/7964-shields-per... branch remains at 96%, unchanged from commit 91fc63e in the main branch.

TypeScript / code-coverage/cli

The overall coverage in commit 172f40d in the fix/7964-shields-per... branch remains at 81%, unchanged from commit 91fc63e in the main branch.

Show a code coverage summary of the most impacted files.

File	main 91fc63e	fix/7964-shields-per... 172f40d	+/-
src/lib/onboard...flight-ports.ts	67%	33%	-34%
src/lib/onboard...box-prebuild.ts	92%	74%	-18%
src/lib/actions...ocker-health.ts	82%	65%	-17%
src/lib/actions...confirmation.ts	79%	69%	-10%
src/lib/onboard...box-gpu-mode.ts	97%	92%	-5%
src/lib/shields/index.ts	69%	69%	0%
src/lib/onboard/preflight.ts	80%	80%	0%
src/lib/onboard...host-anchors.ts	90%	94%	+4%
src/lib/domain/.../connect-env.ts	89%	97%	+8%
src/lib/onboard/docker-cdi.ts	70%	80%	+10%

---

_{Updated July 31, 2026 04:58 UTC}

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/lib/shields/flow.test.ts`:
- Around line 880-908: Update the test around shieldsDown and the mocked fork to
capture the newly created nemoclaw-permissive-runtime directory during policy
application. Assert that a runtime-policy directory was observed before the
expected auto-restore timer error, then assert that no such directory remains
afterward, ensuring the test exercises temporary-policy creation and cleanup
rather than only the failure path.

🪄 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: 6da5fdc2-8154-4858-ae30-7cc8ad8c2bb5

📥 Commits

Reviewing files that changed from the base of the PR and between 91fc63e and f092199.

📒 Files selected for processing (2)

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

[github-actions]

PR Review Advisor — No blocking findings reported

Advisor assessment: No blocking advisor findings reported
Next action: Review the warnings below.
Findings: 0 blockers · 1 warning · 0 suggestions

Model lanes

• GPT-5.6 Terra (primary): Completed · high confidence · 0 blockers · 1 warning · 0 suggestions
• Nemotron 3 Ultra (second opinion): Completed · high confidence · 0 blockers · 0 warnings · 0 suggestions
• Model comparison: normalized findings differ; normalized E2E selections match; Nemotron reported the same number of blockers, 1 fewer warning, the same number of suggestions.

Nemotron output stays in workflow artifacts and does not change the assessment above.

E2E guidance

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

Recommended E2E: cloud-inference, security-posture

1 optional E2E recommendation

• shields-config

1 warning · 0 suggestions

Warnings

Warnings do not block.

PRA-1 Warning — Clean up the runtime policy before pre-timer validation throws

• Location: src/lib/shields/index.ts:2699
• Category: correctness
• Problem: The runtime permissive policy can create its temporary directory before process-token validation and transition construction. An invalid supplied token or missing owner process-start identity throws before the timer try/catch, so either exit bypasses cleanupRuntimePolicyFile.
• Impact: A failed shields-down attempt can still leave a nemoclaw-permissive-runtime directory in the system temporary directory, despite the new every-exit cleanup guarantee.
• Recommendation: Place the post-merge transition in a cleanup boundary that also covers process-token validation and owner-identity failures, while retaining the existing guard that never cleans a caller-supplied policy path.
• Verification: Read shieldsDownWithoutHostLock from buildRuntimePermissivePolicy through the timer try block. Confirm that throws at process-token validation and ownerStartIdentity construction bypass cleanupRuntimePolicyFile.
• Test coverage: Add flow tests with a filesystem_policy live policy for an invalid processToken and an unavailable owner-start identity. Assert each shieldsDown call throws and the nemoclaw-permissive-runtime directory list returns to its pre-call value.
• Evidence: src/lib/shields/index.ts:2663-2685 creates a temporary policy and defines cleanupRuntimePolicyFile. src/lib/shields/index.ts:2697-2699 throws for an invalid process token before the timer try/catch. src/lib/shields/index.ts:2776,2797,2805 are the only new cleanup call sites. src/lib/shields/flow.test.ts: added regression covers timer startup failure, not this pre-try throw. src/lib/shields/index.ts:2708-2712 throws when readProcessStartIdentity(process.pid) returns null, also before the timer try/catch. src/lib/shields/index.ts:2776,2797,2805 are the cleanup call sites after those throws.

Workflow run details

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

[jason-ma-nv]

Addressed the CodeRabbit review in 172f40d: the regression test now captures the temp-directory set inside the fork mock (which shieldsDown invokes after building the permissive policy) and asserts exactly one nemoclaw-permissive-runtime-* directory exists during the transition, in addition to asserting none remains afterward. The test is now self-proving — it verifies temp-policy creation and cleanup rather than only absence.

Re-verified on the Ubuntu host (clean clone + npm ci + plugin build, Node 22):

• With the fix → 1 passed.
• Reverting only the source fix, keeping the strengthened test → 1 failed (expected [ Array(1) ] to deeply equal [], leftover nemoclaw-permissive-runtime-*), while the mid-transition assertion still confirmed the directory was created.

[apurvvkumaria]

Reviewed exact head 172f40d. The change routes every post-merge failure exit through the existing scoped temporary-policy cleanup while retaining the normal apply-path cleanup, and the regression test proves the directory exists at the failure point and is removed afterward. I found no blocking correctness, security, compatibility, or regression issue; current CI is green.