fix(shields): stop leaking the permissive runtime temp dir on failed shields down

[harjothkhara]

Summary

shields down builds the merged permissive policy into a 0700 permissive runtime temp directory, but only the policy-apply try/finally removed it. Two early exits sit between the build and that apply, so a failed transition left the directory behind. This builds the merged policy inside the apply try/finally instead, so the directory is never created before the only owner that can release it is in scope.

Related Issue

Fixes #7964

Changes

• src/lib/shields/index.ts: shieldsDownWithoutHostLock now stores a function that resolves the policy source and calls it inside the policy-apply try. Policy-source validation still happens before mutation, so an unknown policy name fails before any mutation. The two early exits between the old build site and policy application, the auto-restore timer failure and the saveShieldsState failure, can no longer leak the directory. A crash in that window also leaves no directory because policy materialization has not started.
• src/lib/shields/flow.test.ts: adds a livePolicyYaml harness option and two tests. The harness previously returned a live policy with no filesystem_policy, so buildRuntimePermissivePolicy always returned the static base path and the merge path was never exercised here.

This adds no abstraction, configuration, fallback, migration, or compatibility path. The local resolver function replaces two local variables that held the same result eagerly.

I chose this over wrapping the whole creation-through-apply region in one try/finally. That shape works too, but it re-indents about 120 lines of the timer and state-commit block, and #7749 is already touching this file. Deferring the build also closes the window rather than only adding a release point.

buildRuntimePermissivePolicy guards all of its I/O internally and degrades to the base path, so moving the call introduces no new failure mode at the new site.

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)

Quality Gates

• Tests added or updated for changed behavior
• Existing tests cover changed behavior — justification:
• Tests not applicable — justification:
• Docs updated for user-facing behavior changes
• Docs not applicable — justification: the permissive runtime temp directory lifecycle only. Command output, error strings, exit codes, flags, configuration, and the applied policy are unchanged.
• Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging)
• Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: Nine-category security review passed for commit SHA 7f940aad8f9bd62890c075062d38dc9c6e1fc994 against base SHA 4cd4d64fe67143b57707f874afa0b9d269dfeff2; no findings.
• Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue:

Documentation Writer Review

• Documentation writer subagent reviewed the completed changes
• Result: no-docs-needed
• Evidence: No documentation path changed. src/lib/shields/index.ts changes only when an internal permissive-policy temporary directory is materialized and removed, and src/lib/shields/flow.test.ts adds regression coverage. Command output, errors, flags, configuration, applied policy bytes, and documented workflows remain unchanged at commit 7f940aad8f9bd62890c075062d38dc9c6e1fc994. Changed comments and test titles were reviewed against docs/AGENTS.md, WRITING.md, and the controlled word list. No Fern source changed, so a docs build is not applicable.
• Agent: Codex Desktop

Verification

• Review evidence: commit SHA 7f940aad8f9bd62890c075062d38dc9c6e1fc994; base SHA 4cd4d64fe67143b57707f874afa0b9d269dfeff2; E2E / PR Gate is pending for this commit SHA.
• PR description includes a Signed-off-by: line and every commit appears as Verified in GitHub
• Normal pre-commit, commit-msg, and pre-push hooks passed, or npm run validate:pr passed after refreshing origin/main when hooks were skipped or unavailable
• Targeted behavior tests pass for the current change set, or tests are marked not applicable above — command/result: focused cleanup tests passed 25/25; repository checks for titles, source shape, and test size passed 14/14; the conditional-growth check reported no increase (0 to 0); npm run validate:pr passed.
• Applicable broad gate passed — npm test for broad runtime/test-harness changes; npm run check for repo-wide validation/coverage changes — command/result: not applicable. Two files change, one function and one test file, with no runtime or test-harness surface shared beyond them.
• Quality Gates section completed with required justifications or waivers
• No secrets, API keys, or credentials committed
• npm run docs builds without warnings (doc changes only)
• Doc pages follow the style guide (doc changes only)
• New doc pages include SPDX header and frontmatter (new pages only)

Red-green evidence

Before the fix, the timer-failure test reports the leftover directory:

AssertionError: expected [ Array(1) ] to deeply equal []

- []
+ [
+   "nemoclaw-permissive-runtime-76WB4a",
+ ]

After the fix both tests pass. The success-path test passes before and after; it is there to prove the merged policy is still what gets applied, which was the main risk of moving the build.

---

Signed-off-by: harjoth harjoth.khara@gmail.com

Summary by CodeRabbit

• Bug Fixes

  • Improved runtime handling when applying permissive security policies.
  • Ensured filesystem paths from live policies are included correctly.
  • Improved cleanup of temporary policy data after successful application and when automatic restoration cannot start.
  • Improved policy resolution timing to ensure the latest applicable settings are used.

• Tests

  • Added coverage for live policy merging, runtime application, temporary directory cleanup, and restoration startup failure scenarios.

[copy-pr-bot]

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

[coderabbitai]

📝 Walkthrough

Walkthrough

The shields-down flow defers policy resolution until application. Runtime permissive policies are created when needed, and temporary directories are cleaned up after application or auto-restore timer startup failure. Tests cover filesystem-path merging and both cleanup paths.

Changes

Runtime policy cleanup

Layer / File(s)	Summary
Deferred policy resolution and cleanup src/lib/shields/index.ts	Policy selection is deferred until application. Runtime permissive policies and explicit policy paths resolve lazily. Temporary policy directories are tracked and cleaned up after application.
Runtime policy cleanup coverage src/lib/shields/flow.test.ts	The harness accepts live policy YAML. Tests verify filesystem-path merging and cleanup after successful application and auto-restore timer startup failure.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related PRs

• NVIDIA/NemoClaw#6718: Extends policy behavior tests and policy flow around declarative configuration.
• NVIDIA/NemoClaw#7963: Modifies runtime permissive-policy handling, including network-route merging.
• NVIDIA/NemoClaw#7966: Extends runtime permissive-policy handling and cleanup paths.

Suggested labels: area: sandbox, bug-fix

Suggested reviewers: prekshivyas

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	The implementation and tests address temporary-directory cleanup for failed shields-down transitions and cover the filesystem-policy merge path [#7964].
Out of Scope Changes check	✅ Passed	All changes support policy cleanup behavior and its test coverage; no unrelated changes are identified.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly describes the main fix: preventing permissive runtime temporary-directory leaks when shields down fails.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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

[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 894-914: Update the test case “applies the merged permissive
policy and removes its temp directory once applied” to assert that
appliedPolicyBody contains the read_only filesystem path “/etc” in addition to
the existing “/opt/hermes” read_write assertion. Keep the existing merged-policy
and temporary-directory cleanup checks unchanged.

🪄 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: 989a570c-6caf-4e2d-9b06-a8cb5166af2a

📥 Commits

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

📒 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: No advisor follow-up needed.
Findings: 0 blockers · 0 warnings · 0 suggestions

Model lanes

• GPT-5.6 Terra (primary): Completed · high confidence · 0 blockers · 0 warnings · 0 suggestions
• Nemotron 3 Ultra (second opinion): Completed · high confidence · 0 blockers · 0 warnings · 0 suggestions
• Model comparison: normalized findings match; normalized terminology decisions differ; normalized E2E selections differ; severity counts match.

4 terminology differences from the second opinion

Advisory only. These are normalized differences from the primary terminology receipt.

• permissive runtime at src/lib/shields/flow.test.ts:910: selected only by the second-opinion lane as established.
• permissive runtime temp directory at src/lib/shields/flow.test.ts:916: selected only by the second-opinion lane as justified.
• merged permissive policy at src/lib/shields/flow.test.ts:893: selected only by the second-opinion lane as justified.
• runtime temp directory at src/lib/shields/flow.test.ts:22: selected only by the second-opinion lane as justified.

4 additional E2E selections from the second opinion

Advisory only. The primary lane did not select these E2E jobs or targets.

• network-policy: The completed second-opinion lane identified E2E coverage that the primary lane omitted.
• sandbox-operations: The completed second-opinion lane identified E2E coverage that the primary lane omitted.
• sandbox-survival: The completed second-opinion lane identified E2E coverage that the primary lane omitted.
• hermes-shields-config: The completed second-opinion lane identified E2E coverage that the primary lane omitted.

Second-opinion terminology and E2E selections are advisory. They do not change the primary assessment or E2E / PR Gate.

1 semantic terminology decision

Terminology decisions are advisory. They affect the assessment only when a separate finding identifies concrete semantic impact.

• justified — runtime temp directory at src/lib/shields/index.ts:2684: Retain “runtime temp directory” consistently for the transition-scoped directory.

E2E guidance

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

Recommended E2E: cloud-inference, security-posture

1 optional E2E recommendation

• shields-config

Workflow run details

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

[wscurran]

✨ Thanks for the PR. This fixes a temp directory leak in shields down by moving the merged policy build inside the apply try/finally block so the directory is only created when its cleanup owner is in scope. Maintainers will review the policy resolution thunk, the early-exit coverage, and the new test helper.

---

Related open PRs:

• #7749 refactor(architecture): collapse runtime cycle to policy seam

---

Related open issues:

• #7964 shields down leaks the permissive runtime temp directory when the transition fails

[cv]

Security review

Verdict: PASS

Reviewed commit SHA 7f940aad8f9bd62890c075062d38dc9c6e1fc994 against base SHA 4cd4d64fe67143b57707f874afa0b9d269dfeff2.

This change defers creation of the merged permissive-policy temporary file until the existing apply-and-cleanup scope owns it. The policy bytes, transition locks, recovery authority, and sandbox configuration behavior remain unchanged. The nine-category review found no security issue. Merge still requires all repository gates and human approval.

Findings

No findings.

Detailed analysis

1. Secrets and Credentials - PASS. The diff adds no secret, credential, token, key, or connection string.
2. Input Validation and Data Sanitization - PASS. The change adds no input surface. Sandbox-name validation and policy-path resolution remain in their existing order, and command arguments remain an array.
3. Authentication and Authorization - PASS. The diff adds no endpoint or authorization path. Existing Shields transition locks and timer authority remain unchanged.
4. Dependencies and Third-Party Libraries - PASS. The diff adds or changes no dependency.
5. Error Handling and Logging - PASS. Early timer and state-write failures now occur before temporary policy materialization. The existing finally still removes a materialized temporary directory after policy application, including when application throws.
6. Cryptography and Data Protection - PASS. The diff adds no cryptography. The existing temporary-file helper retains its operating-system-generated directory name, 0700 directory mode, and 0600 file mode.
7. Configuration and Security Headers - PASS. The diff changes no service, container, port, HTTP header, or debug configuration. It preserves the applied policy contents.
8. Security Testing - PASS. Regression tests cover merged-policy application and cleanup, timer-start failure, and state-write failure. Existing security coverage is not reduced.
9. System Security - PASS. The change removes a temporary-file lifetime gap without weakening Shields, changing mutation authority, or adding a TOCTOU path. Policy materialization remains inside the host-side transition lock and cleanup owner.

Files reviewed

Changed files:

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

Supporting contracts inspected:

• src/lib/shields/permissive-runtime.ts
• src/lib/onboard/temp-files.ts
• src/lib/policy/commands.ts
• src/lib/shields/legacy-hermes-compat.test.ts
• test/permissive-runtime.test.ts

[apurvvkumaria]

Closing as superseded by #7966, which merged into main and resolves the same #7964 leak in the same Shields flow.

The merged implementation routes timer-start failure, state-write failure, and policy-application completion through one temporary-policy cleanup owner. Its regression coverage also proves a runtime policy directory is created during the failed transition and absent afterward.

Because the reported behavior and all affected exits are now covered on main, carrying this conflicting duplicate forward would not add an independent fix. Thank you @harjothkhara and @cv for the implementation and test work here.