fix(shields): preserve managed MCP policies - #7980
Conversation
|
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 change adds generation-aware lifecycle locks, managed MCP policy reconciliation, cooperative Shields auto-restore, sandbox mutation locking, isolated Vitest state, and expanded lifecycle and E2E coverage. ChangesShield lifecycle recovery
Estimated code review effort: 5 (Critical) | ~120 minutes 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 |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in commit 0f09878 in the TypeScript / code-coverage/cliThe overall coverage in commit 0f09878 in the Show a code coverage summary of the most impacted files.
Updated |
|
🌿 Preview your docs: https://nvidia-preview-pr-7980.docs.buildwithfern.com/nemoclaw |
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported Model lanes
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: This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/e2e/support/mcp-bridge-sandbox.test.ts (1)
305-334: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftReplace source-text assertions with behavioral coverage.
Both tests inspect string positions in
test/e2e/live/mcp-bridge.test.ts. They can pass when source text remains present but the E2E journey no longer performs the required public-boundary operation or validates its result.
test/e2e/support/mcp-bridge-sandbox.test.ts#L305-L334: Execute a focused scenario and assert the surviving policy outcome after route removal.test/e2e/support/mcp-bridge-sandbox.test.ts#L336-L380: Execute a focused Hermes lifecycle scenario and assert tool-call outcomes before and after Shields, removal, restart, and rediscovery.As per path instructions, “Prefer observable outcomes through the public boundary over source-text, private-shape, or mock-call assertions.”
🤖 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 `@test/e2e/support/mcp-bridge-sandbox.test.ts` around lines 305 - 334, Replace the source-text position and substring checks in test/e2e/support/mcp-bridge-sandbox.test.ts:305-334 with a focused public-boundary E2E scenario that performs route removal and asserts the surviving managed-policy outcome. Replace the source-text assertions in test/e2e/support/mcp-bridge-sandbox.test.ts:336-380 with a focused Hermes lifecycle scenario that observes tool-call results before and after Shields, removal, restart, and rediscovery. Use the existing public test helpers and assert observable outcomes rather than source structure, private state, or mock calls.Source: Path instructions
🧹 Nitpick comments (16)
src/lib/shields/timer-bound-lock.ts (2)
84-96: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAccept injectable dependencies for symmetry.
withTimerBoundShieldsMutationLockandwithTimerBoundShieldsMutationLockAsyncboth acceptdeps: TimerBoundLockDeps = defaultDeps.withTimerBoundAutoRestoreLockhardcodesdefaultDeps, so a focused unit test cannot drive its token-generation retry loop without real state files. Add the same optional parameter.♻️ Proposed change
export function withTimerBoundAutoRestoreLock<T>( sandboxName: string, command: string, fn: () => T, + deps: TimerBoundLockDeps = defaultDeps, ): T { return withTimerBoundShieldsMutationLockOptions( sandboxName, command, fn, { recoverStaleOwner: false, waitTimeoutMs: 0 }, - defaultDeps, + deps, ); }🤖 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-bound-lock.ts` around lines 84 - 96, Update withTimerBoundAutoRestoreLock to accept an optional deps: TimerBoundLockDeps parameter defaulting to defaultDeps, and pass that parameter to withTimerBoundShieldsMutationLockOptions instead of hardcoding defaultDeps. Preserve the existing call behavior for callers that omit deps.
11-18: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winImport the lifecycle lock APIs from their owning module.
This block re-exports six
src/lib/state/mcp-lifecycle-locksymbols from a shields helper. It creates a second import path for state-owned APIs, so a reader cannot tell from a call site which module owns lock ownership and containment.src/lib/shields/index.tscan import these names from../state/mcp-lifecycle-lockdirectly.Remove the forwarding block and update the shields callers to import from the state module.
Based on path instructions: "Review ownership against
src/lib/README.md: actions orchestrate, domain modules make pure decisions, adapters own host/process/network boundaries, and state modules own persisted files and state I/O. Flag cross-layer cycles, duplicate sources of truth, and forwarding wrappers that add a new layer without retiring the old owner and its callers."🤖 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-bound-lock.ts` around lines 11 - 18, Remove the six-symbol forwarding export from the shields helper and update all shields callers, including src/lib/shields/index.ts, to import these lifecycle lock APIs directly from ../state/mcp-lifecycle-lock. Preserve the existing API usage while ensuring the state module remains the sole ownership and import path.Source: Path instructions
src/lib/state/mcp-lifecycle-lock-acquisition.ts (1)
366-379: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the shared containment and timeout diagnostics.
The containment message at Line 371 duplicates the message at Line 234 verbatim. The timeout messages already diverge: the async path adds "Another lifecycle, policy, channel, shields, or snapshot operation is still running." and uses
owner pid, while this path usesowner PIDand omits that sentence. Operators then see different guidance for the same condition depending on which entry point they hit.Extract both strings into local helpers so the async and synchronous paths stay identical.
♻️ Proposed helpers
function containmentActiveMessage( sandboxName: string, lockPath: string, containmentPath: string, containment: LockObservation, ): string { return `Sandbox mutation containment is active for '${sandboxName}' at '${containmentPath}' (generation token '${containment.owner?.token ?? "invalid"}'). ...`; } function mutationLockTimeoutMessage(sandboxName: string, ownerPid: number | null): string { const ownerSuffix = ownerPid ? ` (owner pid ${ownerPid})` : ""; return `Timed out waiting for the sandbox mutation lock for '${sandboxName}'${ownerSuffix}. Another lifecycle, policy, channel, shields, or snapshot operation is still running.`; }🤖 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/state/mcp-lifecycle-lock-acquisition.ts` around lines 366 - 379, Extract shared local helpers for the containment-active and mutation-lock-timeout diagnostics, and use them in both the asynchronous and synchronous acquisition paths. Make the helpers produce identical wording, including the containment details and the timeout’s lowercase “owner pid” suffix plus the operation-in-progress guidance; update the visible synchronous branch around readMcpLifecycleLockObservationSync and the corresponding async branch without changing control flow.src/lib/state/mcp-lifecycle-lock-storage.ts (1)
201-234: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRecord the ordering invariants in the synchronous mirrors.
reclaimStaleMcpLifecycleLockGenerationSyncandwriteMcpLifecycleLockCandidateAndLinkSynccopy the exact security-relevant sequence from their asynchronous counterparts but drop the comments that state why the sequence is required: rename is the atomic claim, the moved inode must be verified before deletion, a raced replacement is restored with a hard link that cannot overwrite a newer generation, and publication is decided only by LINK plus owner-token reconciliation. A maintainer who edits only the synchronous path can break mutual exclusion without seeing that reasoning.Add a short comment in each synchronous function that points to the asynchronous function as the documented contract, and keep the two implementations in step.
♻️ Proposed comments
export function reclaimStaleMcpLifecycleLockGenerationSync( targetPath: string, expected: LockObservation, ): boolean { + // Synchronous mirror of reclaimStaleMcpLifecycleLockGeneration. The rename is + // the atomic claim, the moved inode is verified before any deletion, and a + // raced replacement is restored with a hard link. Keep both in step. const quarantinePath = `${targetPath}.reclaim-${process.pid}-${crypto.randomUUID()}`;export function writeMcpLifecycleLockCandidateAndLinkSync( lockPath: string, owner: McpLifecycleLockOwner, ): boolean { + // Synchronous mirror of writeMcpLifecycleLockCandidateAndLink. The hard link + // is the atomic publication point, and EEXIST is only a failed claim after + // link-count plus owner-token reconciliation. Keep both in step. const candidatePath = `${lockPath}.candidate-${process.pid}-${owner.token}`;Also applies to: 277-309
🤖 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/state/mcp-lifecycle-lock-storage.ts` around lines 201 - 234, Add concise contract comments to reclaimStaleMcpLifecycleLockGenerationSync and writeMcpLifecycleLockCandidateAndLinkSync, referencing their asynchronous counterparts. Document the required ordering: atomic rename claim, verify the moved inode before deletion, restore raced replacements via a non-overwriting hard link, and decide publication only through LINK plus owner-token reconciliation. Keep both synchronous implementations aligned with the asynchronous contract.src/lib/shields/permissive-runtime.ts (2)
246-251: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe deadline builder does not wrap read and compose failures.
buildRuntimeManagedMcpPolicywraps areadBasePolicyfailure asCannot read the Shields policy for managed MCP reconciliation.buildDeadlineRuntimeManagedMcpPolicycallsdeps.readBasePolicy()andcomposeDeadlineManagedMcpPoliciesoutside thetry, so those failures propagate with the raw cause. Both paths still fail closed, so this is message consistency only. Move both calls inside thetryif you want one diagnostic shape across the two builders.🤖 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/permissive-runtime.ts` around lines 246 - 251, Move the readBasePolicy and composeDeadlineManagedMcpPolicies calls inside the try block in buildDeadlineRuntimeManagedMcpPolicy, so both failures are wrapped with the same diagnostic message used by buildRuntimeManagedMcpPolicy while preserving fail-closed behavior.
195-198: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
basePolicyPathis unused in both new builders.
buildRuntimeManagedMcpPolicyandbuildDeadlineRuntimeManagedMcpPolicynever readbasePolicyPath. The base content arrives throughdeps.readBasePolicy(). The coding guidelines require a_prefix for intentionally unused variables. Either prefix the parameter or remove it and update the two call sites insrc/lib/shields/index.ts(lines 2646 and 2655) and the shields-down call site at line 3056.Keeping an ignored path parameter also invites a future reader to assume the function reads that file.
As per coding guidelines: "Prefix intentionally unused variables with
_and keep function complexity low."♻️ Minimal fix
export function buildRuntimeManagedMcpPolicy( - basePolicyPath: string, + _basePolicyPath: string, deps: ManagedMcpRuntimePolicyDeps, ): string {export function buildDeadlineRuntimeManagedMcpPolicy( - basePolicyPath: string, + _basePolicyPath: string, deps: ManagedMcpRuntimePolicyDeps, ): DeadlineManagedMcpRuntimePolicy {Also applies to: 242-245
🤖 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/permissive-runtime.ts` around lines 195 - 198, Remove the unused basePolicyPath parameter from buildRuntimeManagedMcpPolicy and buildDeadlineRuntimeManagedMcpPolicy, then update their call sites in the shields index flow, including the shields-down path, to match the new signatures. Continue obtaining base policy content through deps.readBasePolicy().Source: Coding guidelines
src/lib/shields/timer.ts (1)
514-533: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueName the deadline-fence timing values.
pollIntervalMs: 50andtimeoutMs: 5_000are inline literals in the lifecycle-critical restore path. This file already names its other timing budget asAUTO_RESTORE_RETRY_MS. Promote these two to named constants next to it so the fence acquisition budget is discoverable and tunable in one place.🤖 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 514 - 533, Define named constants for the lifecycle fence poll interval and timeout alongside AUTO_RESTORE_RETRY_MS, then replace the inline 50 and 5_000 values in the withMcpLifecycleDeadlineFence options within the restore path with those constants.src/lib/shields/transition-lock.ts (1)
493-511: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTwo inspect methods duplicate the owner projection.
inspectShieldsTransitionLockOwnerandinspectAnyShieldsTransitionLockOwnerrepeat the same snapshot read, projection, andcloseSnapshotsequence. Only the token predicate differs. Extract one private helper that takes an owner predicate so a future field added toInspectedShieldsTransitionOwnercannot diverge between the two paths.♻️ Proposed shared helper
+ private inspectOwnerMatching( + sandboxName: string, + matches: (owner: ShieldsTransitionLockOwner) => boolean, + ): InspectedShieldsTransitionOwner | null { + const validName = validateSandboxName(sandboxName); + const lockPath = shieldsTransitionLockPath(validName, this.stateDir); + const snapshot = readExistingLock(lockPath, validName); + if (!snapshot) return null; + try { + const owner = snapshot.owner; + if (!owner || !matches(owner)) return null; + return { + pid: owner.pid, + processStartIdentity: owner.processStartIdentity, + command: owner.command, + }; + } finally { + closeSnapshot(snapshot); + } + }Also applies to: 1175-1180
🤖 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/transition-lock.ts` around lines 493 - 511, Extract the shared snapshot-reading and owner-projection logic from inspectShieldsTransitionLockOwner and inspectAnyShieldsTransitionLockOwner into one private helper that accepts the differing owner/token predicate. Have both public inspect methods delegate to this helper, preserving null handling and closeSnapshot cleanup while centralizing construction of InspectedShieldsTransitionOwner.src/lib/shields/index.ts (1)
2662-2671: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe temp-file prefix is a repeated string literal.
cleanupTempDir(runtimePolicyPath, "nemoclaw-permissive-runtime")repeats theTEMP_FILE_PREFIXvalue defined insrc/lib/shields/permissive-runtime.tsline 31. The same literal appears again at line 3191.cleanupTempDironly removes a directory whose basename starts with${prefix}-, so a future rename ofTEMP_FILE_PREFIXwould silently stop cleaning up the staged policy directories instead of failing. Export the constant frompermissive-runtimeand import it here.🤖 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 2662 - 2671, Export the existing TEMP_FILE_PREFIX constant from permissive-runtime and import it into the code containing the cleanupTempDir calls. Replace both repeated "nemoclaw-permissive-runtime" literals, including the call near buildPolicySetCommand and the one near line 3191, with the shared constant.src/lib/shields/mcp-policy-transition.ts (1)
11-37: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe managed MCP policy grammar has two owners. The reserved key namespace and the policy-document parse helpers are defined independently in both modules. The inspector and the composer must agree on the same key namespace and the same document shape; two copies can drift so a key is classified in one path and unclassified in the other.
src/lib/shields/mcp-policy-transition.ts#L11-L37: keepRESERVED_MANAGED_MCP_POLICY_KEY_RE,CANONICAL_MANAGED_MCP_POLICY_KEY_RE,parsePolicyDocument, andreadNetworkPolicieshere as the single owner, and export the reserved-prefix predicate and the two document helpers.src/lib/actions/sandbox/mcp-bridge-policy.ts#L243-L243: replace the four inlinekey.startsWith("mcp_bridge_")checks (lines 243, 298-300, 379-381, 457-459) with the exported predicate, and delete the localparseManagedPolicyDocumentandreadManagedNetworkPoliciesduplicates at lines 60-83 in favor of the exported helpers.🤖 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/mcp-policy-transition.ts` around lines 11 - 37, Make src/lib/shields/mcp-policy-transition.ts the single owner of MCP policy grammar by exporting the reserved-prefix predicate derived from RESERVED_MANAGED_MCP_POLICY_KEY_RE, along with parsePolicyDocument and readNetworkPolicies. In src/lib/actions/sandbox/mcp-bridge-policy.ts at line 243 and the additional checks at lines 298-300, 379-381, and 457-459, replace inline mcp_bridge_ checks with the exported predicate; remove parseManagedPolicyDocument and readManagedNetworkPolicies at lines 60-83 and use the exported helpers instead.src/lib/shields/timer.test.ts (2)
11-16: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReset
completeAutoRestoreTransitionin setup too.Line 43 re-establishes the
applyShieldsPolicySnapshotimplementation for each test, butcompleteAutoRestoreTransitionkeeps only the implementation supplied atvi.hoistedtime. This project enablesrestoreMocks, which clears implementations onvi.fn()mocks between tests. If that implementation is lost, the mock returnsundefined, andsrc/lib/shields/timer.tslines 441-449 treat a falsy return asrevoked. The timer then skips theshields_auto_restoreaudit entry and skipscleanupOwnedTimerMarker, so the success test would fail for a reason unrelated to the behavior under test.Reset both mocks in the same setup block so the tests do not depend on ordering.
As per coding guidelines: "In deterministic tests, clear mock calls, restore spies, undo environment/global stubs, and explicitly reset mock implementations when needed."
♻️ Proposed setup addition
shieldsIndexMock.applyShieldsPolicySnapshot.mockImplementation(() => ({ status: 0 })); + shieldsIndexMock.completeAutoRestoreTransition.mockImplementation(() => true);Also applies to: 29-33, 43-43
🤖 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 11 - 16, Update the test setup around the existing applyShieldsPolicySnapshot reset to also restore completeAutoRestoreTransition’s successful implementation for every test. Ensure both mocks are explicitly reset in the same setup block so restoreMocks cannot leave completeAutoRestoreTransition returning undefined.Sources: Coding guidelines, Learnings
75-116: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe 200 ms file-existence poll can flake on a loaded runner.
invokeTimerAndExpectRetrypolls at most 200 times with a 1 ms sleep, so the effective budget is roughly 200 ms plus scheduling overhead. The loop then falls through whether or not the files appeared, and line 107 assertsfs.existsSync(deadlinePath)istrue. On a slow or contended CI runner the deadline file may not exist yet, and the test fails for timing reasons rather than behavior. Line 106 (expect(exitSpy).not.toHaveBeenCalled()) has the same dependency.Raise the budget substantially and fail with an explicit message when the wait expires, so a timeout is distinguishable from a real regression.
♻️ Proposed polling change
- for (let attempt = 0; attempt < 200; attempt += 1) { + const waitUntilMs = Date.now() + 10_000; + while (Date.now() < waitUntilMs) { if ( (!deadlinePath || fs.existsSync(deadlinePath)) && (!auditPath || fs.existsSync(auditPath)) ) { break; } - await new Promise((resolve) => setTimeout(resolve, 1)); + await new Promise((resolve) => setTimeout(resolve, 5)); }🤖 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 75 - 116, Update invokeTimerAndExpectRetry to use a substantially longer polling timeout than the current 200 attempts, and track whether the deadline and audit files become ready. If the wait expires, fail explicitly with a descriptive timeout message before the exit and deadline assertions; preserve the existing readiness conditions and retry behavior once the files appear.src/lib/shields/index.test.ts (2)
517-531: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRestore the
fs.readFileSyncandprocess.killspies in afinallyblock.This test installs two broad spies:
fs.readFileSync(which intercepts every read in the process) andprocess.kill. Neither is restored inside the test. If any assertion between lines 534 and 550 throws, both spies stay installed for the rest of this file. A leakedfs.readFileSyncspy can then corrupt unrelated tests in ways that are hard to attribute.Wrap the exercise and assertions in
try/finallyand callmockRestore()on both spies in thefinally.As per coding guidelines: "In deterministic tests, clear mock calls, restore spies, undo environment/global stubs, and explicitly reset mock implementations when needed." The retrieved learning states the same rule: restore locally created spies in
try/finallyso they are restored on failure paths, rather than relying only on the globalrestoreMocksbehavior.🤖 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.test.ts` around lines 517 - 531, Wrap the test exercise and assertions following the fs.readFileSync and process.kill spy setup in a try/finally block. Store both spy handles, then call mockRestore() for each in finally so they are restored even when assertions fail; keep the existing mock behavior and assertions unchanged.Sources: Coding guidelines, Learnings
486-494: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the applied policy instead of re-deriving the composition.
The test title claims that a deadline restore removes saved MCP keys. The only assertion about the restore itself is
result.managedMcpOmissionsat line 540. Lines 543-550 then callcomposeDeadlineManagedMcpPoliciesagain and assert on its output, which re-derives the expected result rather than checking whatapplyShieldsPolicySnapshotsent to the gateway. That duplicates the dedicated composition test at lines 486-494 and leaves the restore's observable outcome unverified.Capture the policy file that
applyShieldsPolicySnapshotpasses to the policy-set command and assert that it retainsrestrictive_baselineand dropsmcp_bridge_alpha.Both blocks also assert on raw YAML substrings. Parsing the YAML and asserting on the
network_policieskeys is the stronger check, andsrc/lib/shields/mcp-policy-transition.test.tsline 606 already uses that form.As per path instructions: "Prefer observable outcomes through the public boundary over source-text, private-shape, or mock-call assertions."
Also applies to: 543-551
🤖 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.test.ts` around lines 486 - 494, Update the deadline restore test around applyShieldsPolicySnapshot to capture the policy file submitted to the gateway policy-set command and assert its observable applied policy, rather than calling composeDeadlineManagedMcpPolicies again. Parse the captured YAML and verify network_policies retains restrictive_baseline while omitting mcp_bridge_alpha, replacing raw substring assertions and preserving the existing managedMcpOmissions check.Source: Path instructions
src/lib/shields/timer-control.ts (1)
219-255: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
terminatedis now a constantfalse.
killTimernever signals the timer process, soKillTimerResult.terminatedcan only befalse. The field is now dead information for callers. Consider removing it from the result contract, or documenting it as retained for compatibility with existing consumers.Also, the nested condition at lines 228-232 can collapse into one check.
♻️ Collapse the redundant nesting
if (marker) { wasAlive = isProcessAlive(marker.pid); if (wasAlive) { const verification = verifyTimerMarkerIdentity(marker); - if (!verification.verified) { - if (verification.warning) { - warnings.push(verification.warning); - } - } + if (!verification.verified && verification.warning) { + warnings.push(verification.warning); + } } }🤖 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-control.ts` around lines 219 - 255, Update killTimer and the KillTimerResult contract to address the constant terminated: false field: remove it if callers can be migrated, or explicitly retain and document it for compatibility. Also collapse the nested verification.warning condition in killTimer into a single guarded check while preserving warning collection behavior.src/lib/shields/mcp-policy-transition.test.ts (1)
304-412: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd negative-path coverage for ambiguous current keys in the composers.
composeManagedMcpPolicies(lines 77-79 ofsrc/lib/shields/mcp-policy-transition.ts) andcomposeDeadlineManagedMcpPolicies(lines 133-135) throwManaged MCP policy key '<key>' has ambiguous ownershipfor a duplicate or non-canonicalpolicy.keyincurrentPolicies. No test exercises either throw. That guard is the last barrier before an unvalidated key is written intonetwork_policies, so a regression would silently overlay an arbitrary key.Add two cases: one with two
currentPoliciesentries sharing a key, and one with a key that failsCANONICAL_MANAGED_MCP_POLICY_KEY_RE(for examplemcp_bridge_Alpha).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/mcp-policy-transition.test.ts` around lines 304 - 412, Add negative-path tests for both composeManagedMcpPolicies and composeDeadlineManagedMcpPolicies covering duplicate currentPolicies keys and a non-canonical key such as mcp_bridge_Alpha. Assert each composer throws the expected ambiguous-ownership error before writing the key, and verify error output does not expose sensitive policy contents or other secrets.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/actions/sandbox/snapshot-restore-lifecycle.test.ts`:
- Around line 26-43: Replace the source-text assertions in the lifecycle test
with a behavioral test of the public runSandboxSnapshot entrypoint for kind
"create". Mock withSandboxMutationLock and the backup operation using deferred
promises, start snapshot creation, then verify a competing mutation remains
blocked until the backup promise resolves; also assert the replacement path is
reached through the public entrypoint.
In `@src/lib/shields/flow.test.ts`:
- Around line 515-535: Strengthen the test around createHarness and
applyShieldsPolicySnapshot by inspecting the restored policy body captured in
harness.policySetBodies. Assert that the applied policy contains all 257 managed
MCP policy keys, including keys beyond the 256-item boundary, rather than
checking only the successful status.
In `@src/lib/shields/index.ts`:
- Around line 857-895: Bound retryInlineAutoRestore in src/lib/shields/index.ts
lines 857-895 with a wall-clock deadline or attempt budget, and fail closed
using the last recorded error when recovery cannot complete, including the
missing-snapshot case. Also bound prepareAutoRestoreTransitionTakeover in
src/lib/shields/index.ts lines 936-957 and deduplicate shields_up_failed audit
entries when the message is unchanged.
- Around line 3098-3106: Update the ownerMcpProcessIdentity initialization to
use the bounded fallback provided by the exported
prepareAutoRestoreTransitionTakeover transition-lock flow when
readMcpLockProcessIdentity returns null. Remove the unconditional throw so
shields down can continue on supported hosts where process identity lookup is
unavailable, while preserving the existing process identity path when available.
In `@src/lib/state/mcp-lifecycle-lock-acquisition.ts`:
- Around line 830-841: Update the exactLocalOwner check in the synchronous
reentrancy path to require owner.processIdentity to equal
readMcpLockProcessIdentity(process.pid), rather than merely being non-empty.
Preserve the existing sandbox, host, PID namespace, and PID checks so
recycled-PID records continue through stale-generation handling instead of
throwing NEMOCLAW_SYNC_REENTRANT_OWNER.
In `@src/lib/state/mcp-lifecycle-lock/shields-timer-authority.ts`:
- Around line 62-74: The readShieldsTimerMarker function must reject symlinked
marker files instead of following them. Open the marker with fs.openSync using
O_NOFOLLOW, read from that descriptor, and preserve null returns for missing,
invalid, corrupt, or unreadable markers; add a regression test confirming a
symlinked marker yields null.
In `@test/mcp-lifecycle-lock.test.ts`:
- Around line 866-880: The containment test must not assert inside swallowed
onContainment errors. In test/mcp-lifecycle-lock.test.ts lines 866-880, record
ownerPid in a variable, resolve containmentReported, then assert the recorded
value equals child.pid after awaiting the promise. In
test/mcp-lifecycle-lock.test.ts lines 910-912, record each
fs.existsSync(deadlinePath) result in an array within the vi.fn mock, then after
the rejection assertion verify the array is non-empty and every entry is true.
---
Outside diff comments:
In `@test/e2e/support/mcp-bridge-sandbox.test.ts`:
- Around line 305-334: Replace the source-text position and substring checks in
test/e2e/support/mcp-bridge-sandbox.test.ts:305-334 with a focused
public-boundary E2E scenario that performs route removal and asserts the
surviving managed-policy outcome. Replace the source-text assertions in
test/e2e/support/mcp-bridge-sandbox.test.ts:336-380 with a focused Hermes
lifecycle scenario that observes tool-call results before and after Shields,
removal, restart, and rediscovery. Use the existing public test helpers and
assert observable outcomes rather than source structure, private state, or mock
calls.
---
Nitpick comments:
In `@src/lib/shields/index.test.ts`:
- Around line 517-531: Wrap the test exercise and assertions following the
fs.readFileSync and process.kill spy setup in a try/finally block. Store both
spy handles, then call mockRestore() for each in finally so they are restored
even when assertions fail; keep the existing mock behavior and assertions
unchanged.
- Around line 486-494: Update the deadline restore test around
applyShieldsPolicySnapshot to capture the policy file submitted to the gateway
policy-set command and assert its observable applied policy, rather than calling
composeDeadlineManagedMcpPolicies again. Parse the captured YAML and verify
network_policies retains restrictive_baseline while omitting mcp_bridge_alpha,
replacing raw substring assertions and preserving the existing
managedMcpOmissions check.
In `@src/lib/shields/index.ts`:
- Around line 2662-2671: Export the existing TEMP_FILE_PREFIX constant from
permissive-runtime and import it into the code containing the cleanupTempDir
calls. Replace both repeated "nemoclaw-permissive-runtime" literals, including
the call near buildPolicySetCommand and the one near line 3191, with the shared
constant.
In `@src/lib/shields/mcp-policy-transition.test.ts`:
- Around line 304-412: Add negative-path tests for both
composeManagedMcpPolicies and composeDeadlineManagedMcpPolicies covering
duplicate currentPolicies keys and a non-canonical key such as mcp_bridge_Alpha.
Assert each composer throws the expected ambiguous-ownership error before
writing the key, and verify error output does not expose sensitive policy
contents or other secrets.
In `@src/lib/shields/mcp-policy-transition.ts`:
- Around line 11-37: Make src/lib/shields/mcp-policy-transition.ts the single
owner of MCP policy grammar by exporting the reserved-prefix predicate derived
from RESERVED_MANAGED_MCP_POLICY_KEY_RE, along with parsePolicyDocument and
readNetworkPolicies. In src/lib/actions/sandbox/mcp-bridge-policy.ts at line 243
and the additional checks at lines 298-300, 379-381, and 457-459, replace inline
mcp_bridge_ checks with the exported predicate; remove
parseManagedPolicyDocument and readManagedNetworkPolicies at lines 60-83 and use
the exported helpers instead.
In `@src/lib/shields/permissive-runtime.ts`:
- Around line 246-251: Move the readBasePolicy and
composeDeadlineManagedMcpPolicies calls inside the try block in
buildDeadlineRuntimeManagedMcpPolicy, so both failures are wrapped with the same
diagnostic message used by buildRuntimeManagedMcpPolicy while preserving
fail-closed behavior.
- Around line 195-198: Remove the unused basePolicyPath parameter from
buildRuntimeManagedMcpPolicy and buildDeadlineRuntimeManagedMcpPolicy, then
update their call sites in the shields index flow, including the shields-down
path, to match the new signatures. Continue obtaining base policy content
through deps.readBasePolicy().
In `@src/lib/shields/timer-bound-lock.ts`:
- Around line 84-96: Update withTimerBoundAutoRestoreLock to accept an optional
deps: TimerBoundLockDeps parameter defaulting to defaultDeps, and pass that
parameter to withTimerBoundShieldsMutationLockOptions instead of hardcoding
defaultDeps. Preserve the existing call behavior for callers that omit deps.
- Around line 11-18: Remove the six-symbol forwarding export from the shields
helper and update all shields callers, including src/lib/shields/index.ts, to
import these lifecycle lock APIs directly from ../state/mcp-lifecycle-lock.
Preserve the existing API usage while ensuring the state module remains the sole
ownership and import path.
In `@src/lib/shields/timer-control.ts`:
- Around line 219-255: Update killTimer and the KillTimerResult contract to
address the constant terminated: false field: remove it if callers can be
migrated, or explicitly retain and document it for compatibility. Also collapse
the nested verification.warning condition in killTimer into a single guarded
check while preserving warning collection behavior.
In `@src/lib/shields/timer.test.ts`:
- Around line 11-16: Update the test setup around the existing
applyShieldsPolicySnapshot reset to also restore completeAutoRestoreTransition’s
successful implementation for every test. Ensure both mocks are explicitly reset
in the same setup block so restoreMocks cannot leave
completeAutoRestoreTransition returning undefined.
- Around line 75-116: Update invokeTimerAndExpectRetry to use a substantially
longer polling timeout than the current 200 attempts, and track whether the
deadline and audit files become ready. If the wait expires, fail explicitly with
a descriptive timeout message before the exit and deadline assertions; preserve
the existing readiness conditions and retry behavior once the files appear.
In `@src/lib/shields/timer.ts`:
- Around line 514-533: Define named constants for the lifecycle fence poll
interval and timeout alongside AUTO_RESTORE_RETRY_MS, then replace the inline 50
and 5_000 values in the withMcpLifecycleDeadlineFence options within the restore
path with those constants.
In `@src/lib/shields/transition-lock.ts`:
- Around line 493-511: Extract the shared snapshot-reading and owner-projection
logic from inspectShieldsTransitionLockOwner and
inspectAnyShieldsTransitionLockOwner into one private helper that accepts the
differing owner/token predicate. Have both public inspect methods delegate to
this helper, preserving null handling and closeSnapshot cleanup while
centralizing construction of InspectedShieldsTransitionOwner.
In `@src/lib/state/mcp-lifecycle-lock-acquisition.ts`:
- Around line 366-379: Extract shared local helpers for the containment-active
and mutation-lock-timeout diagnostics, and use them in both the asynchronous and
synchronous acquisition paths. Make the helpers produce identical wording,
including the containment details and the timeout’s lowercase “owner pid” suffix
plus the operation-in-progress guidance; update the visible synchronous branch
around readMcpLifecycleLockObservationSync and the corresponding async branch
without changing control flow.
In `@src/lib/state/mcp-lifecycle-lock-storage.ts`:
- Around line 201-234: Add concise contract comments to
reclaimStaleMcpLifecycleLockGenerationSync and
writeMcpLifecycleLockCandidateAndLinkSync, referencing their asynchronous
counterparts. Document the required ordering: atomic rename claim, verify the
moved inode before deletion, restore raced replacements via a non-overwriting
hard link, and decide publication only through LINK plus owner-token
reconciliation. Keep both synchronous implementations aligned with the
asynchronous contract.
🪄 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: f1b6bb75-a7ae-49c2-98ac-5dc569d708e6
📒 Files selected for processing (42)
ci/env-var-doc-allowlist.jsonci/source-architecture-budget.jsonci/source-shape-test-budget.jsondocs/manage-sandboxes/backup-restore.mdxdocs/manage-sandboxes/runtime-controls.mdxdocs/reference/commands.mdxscripts/checks/openshell-policy-mutation-read.mtssrc/lib/actions/maintenance.test.tssrc/lib/actions/maintenance.tssrc/lib/actions/sandbox/mcp-bridge-policy.tssrc/lib/actions/sandbox/snapshot-baseline-exclusion-output.test.tssrc/lib/actions/sandbox/snapshot-help.test.tssrc/lib/actions/sandbox/snapshot-restore-lifecycle.test.tssrc/lib/actions/sandbox/snapshot.test.tssrc/lib/actions/sandbox/snapshot.tssrc/lib/shields/flow.test.tssrc/lib/shields/index.test.tssrc/lib/shields/index.tssrc/lib/shields/mcp-policy-transition.test.tssrc/lib/shields/mcp-policy-transition.tssrc/lib/shields/permissive-runtime.tssrc/lib/shields/timer-bound-lock.tssrc/lib/shields/timer-control.tssrc/lib/shields/timer.test.tssrc/lib/shields/timer.tssrc/lib/shields/transition-lock.test.tssrc/lib/shields/transition-lock.tssrc/lib/state/mcp-lifecycle-lock-acquisition.tssrc/lib/state/mcp-lifecycle-lock-identity.tssrc/lib/state/mcp-lifecycle-lock-storage.tssrc/lib/state/mcp-lifecycle-lock.tssrc/lib/state/mcp-lifecycle-lock/shields-timer-authority.tssrc/lib/state/paths.test.tssrc/lib/state/paths.tstest/e2e/live/mcp-bridge-sandbox.tstest/e2e/live/mcp-bridge.test.tstest/e2e/support/mcp-bridge-sandbox.test.tstest/helpers/isolate-test-state.tstest/mcp-lifecycle-lock.test.tstest/permissive-runtime.test.tstest/vitest-temp-root.test.tsvitest.config.ts
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/timer.test.ts`:
- Around line 103-110: Update the runRestoreTimer test around pending and
markerPath so it waits for the scheduled retry to execute while markerPath
remains removed, rather than relying on await pending alone. Add an assertion
that the retry does not apply the policy a second time, then allow the finally
block to restore markerContents.
🪄 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: 3917c4b5-ecdb-4847-9a0c-2f3dba12b773
📒 Files selected for processing (5)
src/lib/actions/maintenance.test.tssrc/lib/shields/flow.test.tssrc/lib/shields/index.test.tssrc/lib/shields/timer.test.tstest/mcp-lifecycle-lock.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/lib/shields/index.test.ts
- src/lib/actions/maintenance.test.ts
- test/mcp-lifecycle-lock.test.ts
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/lib/shields/index.ts (1)
3169-3254: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winNullish-throw pattern on
ownerMcpProcessIdentitystill abortsshields downwhen process identity lookup is unavailable.This exact segment was flagged in a previous review:
readMcpLockProcessIdentity(process.pid, true) ?? (() => { throw new Error("Cannot identify shields-down lifecycle owner process"); })()still throws unconditionally when the identity lookup returnsnull(for example when/procis unavailable orpsfails). No "Addressed" marker is attached to that past comment, and the code still matches the pattern it described. Confirm whether the second argument (true) toreadMcpLockProcessIdentityalready changes this behavior; if not, use the bounded fallback already used by the transition lock (prepareAutoRestoreTransitionTakeoveris exported) instead of the unconditional throw, soshields downcan proceed on supported hosts where process identity lookup is unavailable.🤖 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 3169 - 3254, Update the ownerMcpProcessIdentity initialization in the shields-down transition to avoid unconditionally throwing when readMcpLockProcessIdentity returns null. Verify whether its true argument provides the required fallback; otherwise reuse the bounded fallback behavior exposed by prepareAutoRestoreTransitionTakeover, allowing shields down to proceed when process identity lookup is unavailable while preserving transition ownership safety.
🧹 Nitpick comments (1)
src/lib/shields/index.ts (1)
921-971: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated bounded-retry-with-audit-dedup logic.
retryInlineAutoRestore(lines 925-963) and the transition-takeover loop insidewithExpiredAutoRestoreDeadlineFence(lines 1013-1040) both implement the same shape: a boundedforloop, try/catch,notifiedErrordeduplication beforeappendAuditEntryBestEffort,Atomics.waitbetween attempts, and escalation tofailInteractiveAutoRestoreClosed. Extract a shared helper (e.g.,runBoundedInteractiveRetry(attempt => ..., onExhausted)) to avoid the two copies drifting apart as retry semantics evolve.Also applies to: 973-1063
🤖 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 921 - 971, The bounded retry and audit-deduplication flow is duplicated between retryInlineAutoRestore and withExpiredAutoRestoreDeadlineFence. Extract the shared loop behavior into a helper such as runBoundedInteractiveRetry, including attempt limits, error deduplication, Atomics.wait delays, and exhaustion handling, then update both callers to provide only their operation-specific recovery and failInteractiveAutoRestoreClosed behavior.
🤖 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.
Duplicate comments:
In `@src/lib/shields/index.ts`:
- Around line 3169-3254: Update the ownerMcpProcessIdentity initialization in
the shields-down transition to avoid unconditionally throwing when
readMcpLockProcessIdentity returns null. Verify whether its true argument
provides the required fallback; otherwise reuse the bounded fallback behavior
exposed by prepareAutoRestoreTransitionTakeover, allowing shields down to
proceed when process identity lookup is unavailable while preserving transition
ownership safety.
---
Nitpick comments:
In `@src/lib/shields/index.ts`:
- Around line 921-971: The bounded retry and audit-deduplication flow is
duplicated between retryInlineAutoRestore and
withExpiredAutoRestoreDeadlineFence. Extract the shared loop behavior into a
helper such as runBoundedInteractiveRetry, including attempt limits, error
deduplication, Atomics.wait delays, and exhaustion handling, then update both
callers to provide only their operation-specific recovery and
failInteractiveAutoRestoreClosed behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 61e6a0fe-3d3e-4118-8fbb-81fb6ccd3e83
📒 Files selected for processing (14)
docs/manage-sandboxes/backup-restore.mdxdocs/manage-sandboxes/runtime-controls.mdxdocs/reference/commands.mdxsrc/lib/actions/sandbox/snapshot-restore-lifecycle.test.tssrc/lib/actions/sandbox/snapshot-restore-test-fixture.tssrc/lib/shields/flow.test.tssrc/lib/shields/index.test.tssrc/lib/shields/index.tssrc/lib/shields/timer.test.tssrc/lib/shields/timer.tssrc/lib/state/mcp-lifecycle-lock-acquisition.tssrc/lib/state/mcp-lifecycle-lock/shields-timer-authority.tstest/e2e/support/mcp-bridge-sandbox.test.tstest/mcp-lifecycle-lock.test.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- docs/manage-sandboxes/backup-restore.mdx
- src/lib/shields/timer.ts
- src/lib/state/mcp-lifecycle-lock-acquisition.ts
- src/lib/actions/sandbox/snapshot-restore-lifecycle.test.ts
- src/lib/shields/index.test.ts
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
src/lib/shields/flow.test.ts (1)
64-74: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive the policy key from the rendered policy instead of re-deriving it.
managedMcpPolicyhardcodesmcp_bridge_${server}, which duplicates the production key derivation inbuildMcpBridgePolicyKey. The current server names (alpha,beta,server0…server256) hide any normalization the production helper applies. If that helper changes,YAML.parse(content).network_policies[key]returnsundefinedand the assertions compareundefinedtoundefinedrather than failing clearly.Read the key from the rendered document, or import
buildMcpBridgePolicyKey.♻️ Proposed refactor
function managedMcpPolicy(server: string, address = "8.8.8.8") { - const key = `mcp_bridge_${server}`; const content = buildMcpBridgePolicyYaml( server, `https://${server}.example.com/mcp`, "hermes-config", [address], ); - const networkPolicy = YAML.parse(content).network_policies[key]; - return { content, networkPolicy, server }; + const networkPolicies = YAML.parse(content).network_policies as Record<string, unknown>; + const [key, networkPolicy] = Object.entries(networkPolicies)[0]; + return { content, key, networkPolicy, server }; }Callers that build key lists can then use
policies.map(({ key }) => key).🤖 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 64 - 74, Update managedMcpPolicy to obtain the policy key from the parsed rendered document instead of constructing mcp_bridge_${server} locally. Use the key exposed by YAML.parse(content).network_policies, and preserve returning the rendered content, selected networkPolicy, and server.src/lib/shields/index.test.ts (1)
562-569: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the applied policy instead of recomputing it with the production composer.
Lines 562-569 call
composeDeadlineManagedMcpPoliciesfrom the test and then assert the composer's own output. That assertion passes for any composer behavior, so it does not prove whatapplyShieldsPolicySnapshotapplied. Capture the policy file passed to the policy-set command (or the staged runtime YAML) and assert thatmcp_bridge_alphais absent there.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/index.test.ts` around lines 562 - 569, Replace the test’s use of composeDeadlineManagedMcpPolicies with an assertion against the policy actually applied by applyShieldsPolicySnapshot. Capture the policy file supplied to the policy-set command or the staged runtime YAML, then verify it contains restrictive_baseline and excludes mcp_bridge_alpha without reusing the production composer.Source: Path instructions
src/lib/shields/permissive-runtime.ts (1)
195-198: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBoth new managed-MCP builders accept an unused
basePolicyPath. Each builder reads the base policy only throughdeps.readBasePolicy(), so the first positional parameter is dead. Callers already track temp-versus-base themselves.
src/lib/shields/permissive-runtime.ts#L195-L198: prefixbasePolicyPathwith_inbuildRuntimeManagedMcpPolicy, or remove it and update the call sites insrc/lib/shields/index.ts.src/lib/shields/permissive-runtime.ts#L242-L245: apply the same change inbuildDeadlineRuntimeManagedMcpPolicy.As per coding guidelines: "Prefix intentionally unused variables with
_".🤖 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/permissive-runtime.ts` around lines 195 - 198, Both managed-MCP builders declare an unused base policy path parameter. In src/lib/shields/permissive-runtime.ts lines 195-198, rename buildRuntimeManagedMcpPolicy’s basePolicyPath parameter with an underscore prefix, and apply the same change to buildDeadlineRuntimeManagedMcpPolicy at lines 242-245; leave call sites unchanged.Source: Coding guidelines
🤖 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 889-915: Bound the retry loop in failInteractiveAutoRestoreClosed
instead of using an unbounded for(;;): track a finite attempt budget or
wall-clock deadline while preserving marker checks and containment-file success
handling. When the budget is exhausted, throw an error containing the last
containment failure and the operator-resolution instruction, so interactive
callers such as shieldsStatus and getShieldsPosture return rather than hanging.
- Around line 3128-3164: Ensure every early return or exception after policy
composition releases a staged temporary policy. Restructure the transition
surrounding policyFile, including saveShieldsState, auto-restore timer setup,
and policy application, under a try/finally that calls
cleanupTempDir(policyFile, "nemoclaw-permissive-runtime") when policyFileIsTemp
remains true and the apply flow has not consumed the file; preserve existing
cleanup behavior without double-cleaning consumed files.
In `@test/mcp-lifecycle-lock.test.ts`:
- Line 381: Clear each deferred marker-rotation timer after the corresponding
expectation completes. At test/mcp-lifecycle-lock.test.ts lines 381, 988, and
1022, retain the handle returned by setTimeout for the 40 ms writeTimerMarker
call and clear it in a finally block surrounding the awaited assertion.
---
Nitpick comments:
In `@src/lib/shields/flow.test.ts`:
- Around line 64-74: Update managedMcpPolicy to obtain the policy key from the
parsed rendered document instead of constructing mcp_bridge_${server} locally.
Use the key exposed by YAML.parse(content).network_policies, and preserve
returning the rendered content, selected networkPolicy, and server.
In `@src/lib/shields/index.test.ts`:
- Around line 562-569: Replace the test’s use of
composeDeadlineManagedMcpPolicies with an assertion against the policy actually
applied by applyShieldsPolicySnapshot. Capture the policy file supplied to the
policy-set command or the staged runtime YAML, then verify it contains
restrictive_baseline and excludes mcp_bridge_alpha without reusing the
production composer.
In `@src/lib/shields/permissive-runtime.ts`:
- Around line 195-198: Both managed-MCP builders declare an unused base policy
path parameter. In src/lib/shields/permissive-runtime.ts lines 195-198, rename
buildRuntimeManagedMcpPolicy’s basePolicyPath parameter with an underscore
prefix, and apply the same change to buildDeadlineRuntimeManagedMcpPolicy at
lines 242-245; leave call sites 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: 1b2f889e-4302-4af6-9a02-d05ac0239b8d
📒 Files selected for processing (43)
ci/env-var-doc-allowlist.jsonci/source-architecture-budget.jsonci/source-shape-test-budget.jsondocs/manage-sandboxes/backup-restore.mdxdocs/manage-sandboxes/runtime-controls.mdxdocs/reference/commands.mdxscripts/checks/openshell-policy-mutation-read.mtssrc/lib/actions/maintenance.test.tssrc/lib/actions/maintenance.tssrc/lib/actions/sandbox/mcp-bridge-policy.tssrc/lib/actions/sandbox/snapshot-baseline-exclusion-output.test.tssrc/lib/actions/sandbox/snapshot-help.test.tssrc/lib/actions/sandbox/snapshot-restore-lifecycle.test.tssrc/lib/actions/sandbox/snapshot-restore-test-fixture.tssrc/lib/actions/sandbox/snapshot.test.tssrc/lib/actions/sandbox/snapshot.tssrc/lib/shields/flow.test.tssrc/lib/shields/index.test.tssrc/lib/shields/index.tssrc/lib/shields/mcp-policy-transition.test.tssrc/lib/shields/mcp-policy-transition.tssrc/lib/shields/permissive-runtime.tssrc/lib/shields/timer-bound-lock.tssrc/lib/shields/timer-control.tssrc/lib/shields/timer.test.tssrc/lib/shields/timer.tssrc/lib/shields/transition-lock.test.tssrc/lib/shields/transition-lock.tssrc/lib/state/mcp-lifecycle-lock-acquisition.tssrc/lib/state/mcp-lifecycle-lock-identity.tssrc/lib/state/mcp-lifecycle-lock-storage.tssrc/lib/state/mcp-lifecycle-lock.tssrc/lib/state/mcp-lifecycle-lock/shields-timer-authority.tssrc/lib/state/paths.test.tssrc/lib/state/paths.tstest/e2e/live/mcp-bridge-sandbox.tstest/e2e/live/mcp-bridge.test.tstest/e2e/support/mcp-bridge-sandbox.test.tstest/helpers/isolate-test-state.tstest/mcp-lifecycle-lock.test.tstest/permissive-runtime.test.tstest/vitest-temp-root.test.tsvitest.config.ts
🚧 Files skipped from review as they are similar to previous changes (32)
- test/helpers/isolate-test-state.ts
- src/lib/actions/sandbox/snapshot-restore-test-fixture.ts
- ci/source-shape-test-budget.json
- src/lib/actions/maintenance.test.ts
- src/lib/actions/sandbox/snapshot-baseline-exclusion-output.test.ts
- src/lib/actions/sandbox/snapshot-help.test.ts
- src/lib/state/paths.test.ts
- ci/source-architecture-budget.json
- src/lib/state/paths.ts
- src/lib/actions/sandbox/snapshot.test.ts
- src/lib/state/mcp-lifecycle-lock.ts
- ci/env-var-doc-allowlist.json
- docs/reference/commands.mdx
- scripts/checks/openshell-policy-mutation-read.mts
- test/vitest-temp-root.test.ts
- src/lib/actions/sandbox/snapshot-restore-lifecycle.test.ts
- test/e2e/support/mcp-bridge-sandbox.test.ts
- vitest.config.ts
- src/lib/shields/transition-lock.ts
- src/lib/actions/maintenance.ts
- src/lib/shields/timer-bound-lock.ts
- src/lib/shields/timer-control.ts
- src/lib/state/mcp-lifecycle-lock-identity.ts
- test/e2e/live/mcp-bridge-sandbox.ts
- src/lib/state/mcp-lifecycle-lock-acquisition.ts
- src/lib/shields/mcp-policy-transition.test.ts
- src/lib/shields/timer.ts
- src/lib/shields/mcp-policy-transition.ts
- src/lib/actions/sandbox/mcp-bridge-policy.ts
- src/lib/actions/sandbox/snapshot.ts
- test/e2e/live/mcp-bridge.test.ts
- docs/manage-sandboxes/backup-restore.mdx
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
Signed-off-by: Julie Yaunches <jyaunches@nvidia.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 `@docs/manage-sandboxes/runtime-controls.mdx`:
- Around line 127-130: Rewrite the third sentence so NemoClaw is the active
actor that records durable containment when the deadline expires or reaping is
interrupted, while preserving the surviving-descendants condition. Use
professional present-tense second-person wording throughout the changed prose,
addressing the reader as “you.”
🪄 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: 40b59f74-68e6-414b-a1f5-02dc9d099632
📒 Files selected for processing (9)
docs/manage-sandboxes/runtime-controls.mdxdocs/reference/commands.mdxsrc/lib/shields/flow.test.tssrc/lib/shields/index.test.tssrc/lib/shields/index.tssrc/lib/shields/permissive-runtime.tssrc/lib/state/mcp-lifecycle-lock-acquisition.tstest/config-set-nested-ssrf.test.tstest/mcp-lifecycle-lock.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/lib/shields/index.test.ts
- docs/reference/commands.mdx
- src/lib/shields/permissive-runtime.ts
- src/lib/shields/index.ts
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
Security reviewVerdict: PASS I reviewed the complete 42-file diff for commit The product scope gate passes. Issue #7952 defines the accepted defect, supported lifecycle behavior, security expectations, and validation criteria. This PR repairs an existing supported surface rather than creating a new integration or product surface.
Key implementation evidence:
All 30 commits in the PR are GitHub-verified with reason The required I did not run local tests; GitHub checks are the validation authority for this review. Files reviewed
|
|
Superseded by the reviewable split requested here: #8130 contains the generic Shields lifecycle/deadline safety prerequisite, and #8141 contains the focused managed-MCP policy reconciliation plus the corrected Hermes regression order. #8141 is stacked on #8130. Closing this oversized combined PR in favor of those two scoped changes. |
<!-- markdownlint-disable MD041 --> ## Summary `nemoclaw shields down` replaced the complete live OpenShell policy and dropped generated policy entries for registered Model Context Protocol (MCP) servers. This change reconciles only exact NemoClaw-managed MCP entries during Shields transitions, so a surviving server remains reachable while removed servers stay removed. Stacked on prerequisite #8130, which makes Shields deadline recovery serialize with lifecycle mutations without signaling the lock owner, this focused fix supersedes the MCP portion of #7980. ## Related Issue Fixes #7952 ## Changes - Prove managed MCP policy ownership from exact agreement between the sandbox registry, committed generated-policy record, and live gateway policy. - Save the owned MCP key manifest with the Shields snapshot, remove snapshot-time managed entries during restoration, and overlay only current exact entries. - Fail closed on ambiguous, stale, incomplete, malformed, or legacy ownership during manual transitions. At an expired deadline, omit unproven managed MCP entries and audit the omission instead of extending the Shields-down window. - Preserve current managed MCP entries when building the permissive runtime policy, while rejecting an unreadable or ambiguous live policy. - Clean staged runtime policy files across early failure paths. - Restore the Hermes live regression assertions at the actual failure boundary and around the unrelated server lifecycle. - Document MCP policy reconciliation for manual and automatic restoration. ## Failure Timing and Hermes Upgrade Context The original journey had a hidden Shields lifecycle between the first successful call to server A and the later lifecycle for server B: 1. Run `shields up`. 2. Restart the Hermes gateway. 3. Run `shields down`. 4. Exercise the configuration rollback path. 5. Add and remove B. 6. Call A. Boundary instrumentation recorded in #7952 showed that A remained healthy through Shields up and the gateway restart. It became unusable immediately after Shields down, which dropped A's generated MCP policy. The later failure after B was removed was only where the test noticed the already-broken route; B removal was a misleading correlation. This surfaced during the Hermes upgrade work because new coverage and upgrade repairs landed nearly back-to-back: - #7761 added the Hermes MCP helper containing Shields up, gateway restart, Shields down, and rollback. Its verification collected and imported the live target but did not run the complete live E2E. - #7771 upgraded Hermes the next day, but its selected E2Es skipped the `mcp-bridge` target. - #7849 repaired Hermes 0.19 migrations and updated MCP tool naming, allowing the live test to progress far enough to expose the later failure. - #7866 moved the explicit `mcp restart A` before the first post-removal call. Restart reapplied A's generated policy and masked the missing-policy state. The corrected regression order is: 1. Run `shields up`. 2. Restart the Hermes gateway. 3. Run `shields down`. 4. Call A immediately. 5. Exercise the configuration rollback path. 6. Add B, prove the DNS-rebinding connection is denied, remove B, and verify that A's managed policy is unchanged while B's policy is gone. 7. Call A before the later explicit restart. 8. Capture the authenticated rediscovery offset. 9. Run `mcp restart A` without resupplying the secret. 10. Call A and verify authenticated rediscovery. Whole-policy Shields replacement and the filesystem-only runtime merge predate the Hermes upgrade. This is a latent NemoClaw Shields policy-composition defect detected by expanded Hermes regression coverage, not a Hermes upgrade regression. ## Type of Change - [ ] Code change (feature, bug fix, or refactor) - [x] Code change with doc updates - [ ] Doc only (prose changes, no code sample modifications) - [ ] Doc only (includes code sample changes) ## Quality Gates - [x] Tests added or updated for changed behavior - [ ] Existing tests cover changed behavior — justification: - [ ] Tests not applicable — justification: - [x] Docs updated for user-facing behavior changes - [ ] Docs not applicable — justification: - [x] Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging) - [x] Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: Independent exact-head Codex security review passed all nine categories at `18039569796d6ac7604de032edb7abf84f2c73c4`; no findings. - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## Documentation Writer Review - [x] Documentation writer subagent reviewed the completed changes - Result: `docs-updated` - Evidence: Reviewed `docs/manage-sandboxes/runtime-controls.mdx` and `docs/reference/commands.mdx`, all rendered guide variants, changed operator-facing text, comments, test titles, and the Hermes E2E chronology. Verified claims against source, issue #7952, and PRs #7761, #7771, #7849, and #7866. `npm run docs` completed with 0 errors and 2 existing Fern warnings. - Agent: Codex Desktop <!-- docs-review-head-sha: 1803956 --> <!-- docs-review-agents-blob-sha: 3dd7c24 --> ## DGX Station Hardware Evidence - [ ] Tested on DGX Station - Tested commit: Not applicable; `scripts/prepare-dgx-station-host.sh` is unchanged. - Station profile/scenario: Not applicable. - Result: Not applicable. - Supporting evidence: Not applicable. ## Verification - [x] PR description includes a `Signed-off-by:` line and every commit appears as `Verified` in GitHub - [x] 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 - [x] Targeted behavior tests pass for the current change set, or tests are marked not applicable above — command/result or justification: Focused CLI 123/123, integration 11/11, E2E support 13/13, `npm run typecheck:cli`, `npm run checks:repository`, test-size guardrail, E2E semantic phase plans, and serial `npm run test:changed` 674/674 passed. - [x] Applicable broad gate passed — `npm test` for broad runtime/test-harness changes; `npm run check` for repo-wide validation/coverage changes — command/result: [Standard PR CI run 30824992396](https://github.com/NVIDIA/NemoClaw/actions/runs/30824992396) passed. One inherited 50 ms lifecycle-lock assertion timing flake passed on the failed-job rerun without a code change. - [x] Quality Gates section completed with required justifications or waivers - [x] No secrets, API keys, or credentials committed - [ ] `npm run docs` builds without warnings (doc changes only) - [x] Doc pages follow the [style guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md) (doc changes only) - [ ] New doc pages include SPDX header and frontmatter (new pages only) `npm run docs` passed with 0 errors and 2 existing Fern warnings, so the warning-free checkbox remains unchecked. No new documentation pages were added. Trusted E2E [run 30826792180](https://github.com/NVIDIA/NemoClaw/actions/runs/30826792180) passed all 10 selected checks: cloud inference, cloud onboard, security posture, inference routing, MCP bridge, MCP bridge dev, network policy, onboard repair, onboard resume, and OpenShell credential-generation window. The primary review advisor reported no findings. Nemotron completed after retrying a protocol-only failure; its one test warning requested the exact transition/state ownership-mismatch deadline regression already present in `src/lib/shields/policy-transition.test.ts`, which passed. --- Signed-off-by: Julie Yaunches <jyaunches@nvidia.com> Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
4 participants
Summary
nemoclaw shields downreplaced the complete live OpenShell policy and dropped generated policy entries for registered Model Context Protocol (MCP) servers. This change reconciles only exact NemoClaw-managed MCP entries during Shields transitions, so a surviving server remains reachable while removed servers stay removed.The Hermes live E2E now checks server A at the first surviving-server checkpoint after Shields down, before server B is added, and again after B is removed without restarting A first.
Related Issue
Fixes #7952
Changes
If restoration cannot complete and commit, convert the deadline gate into durable containment before returning the failure.
Recover only an exact, valid stale ordinary lifecycle-lock generation under the exclusive reaper gate.
Keep durable containment for expired deadline owners, interrupted reapers, and unsafe or ambiguous generations where surviving descendants cannot be ruled out.
Do not signal an active mutation process.
O_NOFOLLOWfile descriptor so its kind check and content read have no check-then-use window.Failure Timing and Hermes Upgrade Context
The earlier journey summary omitted an intermediate Shields lifecycle between the first successful call to A and the later lifecycle for B:
shields up.shields down.A remained healthy through Shields up and the gateway restart. Investigation placed the first failure at Shields down, which removed A's generated MCP policy. B removal was only where the prior test detected the already-broken route.
The failure became visible as live coverage changed:
mcp-bridgejob.This PR restores the meaningful order:
remove B -> call A -> restart A -> call AThe test checks A before any restart. The explicit restart without resupplying the secret predates #7866; #7866 added the authenticated rediscovery assertion. This PR keeps both after the survival assertion.
The whole-policy Shields replacement and filesystem-only runtime merge predate the Hermes upgrade. This is a latent NemoClaw Shields policy-composition defect detected by expanded Hermes regression coverage, not a Hermes upgrade regression.
Type of Change
Quality Gates
0f0987864b97f404b8be94844d2c178bdaeaf68dagainst base SHA4cd4d64fe67143b57707f874afa0b9d269dfeff2passed all nine categories with no findings: review comment.Documentation Writer Review
docs-updated0f0987864b97f404b8be94844d2c178bdaeaf68d. The existing source pagesdocs/manage-sandboxes/backup-restore.mdx,docs/manage-sandboxes/runtime-controls.mdx, anddocs/reference/commands.mdxremain accurate and byte-identical. Restoringsrc/lib/actions/maintenance.tsand its tests to currentmainremoves the redundant nested lock while retaining the stronger whole-backup lifecycle lock; ratchetingci/source-architecture-budget.jsonis non-user-facing. No additional documentation edits are needed. Agent-variant sync remains represented by blob3dd7c2425.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 unavailablenpm run test:changedpassed its 4 selected tests. Merging the direct source map with the exact prior Linux eight-shard map puts acquisition at 83.78% functions and 67.48% branches and storage at 100% functions and 76.23% branches, above the 75%/60% security floors. The final CodeQL remediation passed 29 Shields flow tests and 33 combined focused tests. CLI typecheck, build, Biome, repository checks, test-project membership, source architecture, live-E2E structure, and the 1,945-file test-conditional scan passed.f31b034f8is pending.npm run docsbuilds without warnings (doc changes only) — build completed with 0 errors and 2 warnings.Signed-off-by: Julie Yaunches jyaunches@nvidia.com