fix(shields): serialize deadline recovery - #8130
Conversation
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
jyaunches
added
area: docs
|
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 deadline-fenced lifecycle locking, durable containment, cooperative Shields recovery, managed MCP policy validation, per-sandbox backup locking, and isolated Vitest state directories. Tests and documentation cover these lifecycle changes. ChangesLifecycle containment and recovery
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Comment |
|
🌿 Preview your docs: https://nvidia-preview-pr-8130.docs.buildwithfern.com/nemoclaw |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in commit eb0184e in the TypeScript / code-coverage/cliThe overall coverage in commit eb0184e in the Show a code coverage summary of the most impacted files.
Updated |
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: 3
🧹 Nitpick comments (18)
src/lib/actions/maintenance.ts (2)
378-420: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSimplify the redundant local state.
resultandorphanManifestMessageare initialized tonulland then overwritten fromattempt.mutationLockFailedduplicates the"mutationLockError" in attemptcheck. Read the values directly fromattempt.♻️ Proposed simplification
- let result: sandboxState.BackupResult | null = null; - let orphanManifestMessage: string | null = null; - let mutationLockError: unknown; - let mutationLockFailed = false; const attempt = await backupSandboxWithinShieldsWindow( @@ - result = attempt.result; - orphanManifestMessage = attempt.orphanManifestMessage; - if ("mutationLockError" in attempt) { - mutationLockError = attempt.mutationLockError; - mutationLockFailed = true; - } - if (mutationLockFailed) { + const result = attempt.result; + const orphanManifestMessage = attempt.orphanManifestMessage; + if ("mutationLockError" in attempt) { + const mutationLockError = attempt.mutationLockError; const detail = mutationLockError instanceof Error ? mutationLockError.message : String(mutationLockError); console.error(` ${RD}✗${R} ${sb.name}: backup failed (mutation lock: ${detail})`); failed++; return; }🤖 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/actions/maintenance.ts` around lines 378 - 420, Remove the redundant result, orphanManifestMessage, mutationLockError, and mutationLockFailed locals around backupSandboxWithinShieldsWindow. Read result and orphanManifestMessage directly from attempt, and replace the mutationLockFailed branch with an inline "mutationLockError" in attempt check while deriving the error detail from attempt.mutationLockError.
257-299: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider collapsing the failure-combination cascade.
The block enumerates five explicit combinations of
backupError,orphanManifestMessage,relockError, andstoppedContainerCleanupError. Each new failure source doubles the branches. Collect the errors in one array and build a single message from the present causes. That keeps behavior identical and reduces the branch count.♻️ Sketch of a combined aggregation
const causes: unknown[] = []; if (hasBackupError) causes.push(backupError); else if (orphanManifestMessage) causes.push(new Error(orphanManifestMessage)); if (relockError) causes.push(relockError); if (stoppedContainerCleanupError) causes.push(stoppedContainerCleanupError); if (causes.length > 1) { throw new AggregateError(causes, backupAllFailureMessage(sandboxName, { hasBackupError, orphanManifest: Boolean(orphanManifestMessage), relockFailed: Boolean(relockError), cleanupFailed: Boolean(stoppedContainerCleanupError), })); } if (causes.length === 1) throw causes[0];Note that the current messages are asserted by tests, so keep the exact strings if you apply this.
🤖 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/actions/maintenance.ts` around lines 257 - 299, Replace the failure-combination cascade in the maintenance failure-handling block with one causes collection using backupError or orphanManifestMessage, relockError, and stoppedContainerCleanupError. Aggregate when multiple causes exist and throw the sole cause unchanged when only one exists. Preserve the exact currently asserted error messages by deriving the message from the same combinations of present failure flags.src/lib/actions/maintenance.test.ts (3)
277-302: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the reported failure reason.
The test proves that no mutation happens. It does not prove that the sandbox is reported as a mutation-lock failure.
backupAllproducesbackup failed (mutation lock: ...)for this branch, and an accidental reclassification to "could not safely unlock shields" would still pass this test. Capture theconsole.errorspy and assert the reason.💚 Proposed addition
vi.spyOn(console, "log").mockImplementation(() => undefined); - vi.spyOn(console, "error").mockImplementation(() => undefined); + const errorSpy = vi.spyOn(console, "error").mockImplementation(() => undefined); @@ expect(mocks.returnSandboxContainerToStopped).not.toHaveBeenCalled(); + expect(errorSpy.mock.calls.flat().join("\n")).toContain( + "backup failed (mutation lock: Timed out waiting for the sandbox mutation lock)", + );🤖 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/actions/maintenance.test.ts` around lines 277 - 302, Update the test for backupAll so it captures the console.error spy and asserts the logged failure includes the expected “backup failed (mutation lock: ...)” reason from the withSandboxMutationLock rejection. Keep the existing assertions verifying no container start or backup mutations occur.
833-877: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMake the loop-abort claim explicit.
The test registers
betain addition tosb-stopped, andtoHaveBeenCalledTimes(3)passes only because the AggregateError aborts the loop beforebetais attempted. That dependency is implicit. Assert the abort directly, or dropbetaif the abort is not part of this test's claim.💚 Proposed addition
expect(mocks.withSandboxMutationLock).toHaveBeenCalledTimes(3); + expect(mocks.withSandboxMutationLock).not.toHaveBeenCalledWith( + "beta", + expect.anything(), + expect.anything(), + );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 `@src/lib/actions/maintenance.test.ts` around lines 833 - 877, Make the test’s loop-abort expectation explicit around backupAll: verify the second sandbox, “beta,” is not processed after the cleanup lock failure, using an observable public outcome rather than relying solely on withSandboxMutationLock call counts. Keep the existing assertions for the AggregateError and stopped-container cleanup behavior.Source: Path instructions
277-302: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for a setup failure inside the acquired lock.
The changed tests cover a rejected setup lock. They do not cover the case where the setup interval is entered and then fails. That path in
src/lib/actions/maintenance.tsat lines 160-170 starts the stopped container, throws while opening the Shields window, and produces the AggregateError message "Backup setup for '' failed and its started container could not be returned to the stopped state." Nothing in the changed ranges exercises it, so a regression in that branch would ship silently. Add a test that makesopenBackupShieldsWindowthrow withreturnSandboxContainerToStoppedreturningfalse.I can draft that test if you want it.
As per path instructions: "Destructive lifecycle operations must validate before mutation, preserve state/backup invariants, and cover failure, recovery, rebuild, and resume behavior without bypassing the public action boundary."
🤖 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/actions/maintenance.test.ts` around lines 277 - 302, Add a backupAll() test covering failure after setup enters the acquired mutation lock: configure startStoppedSandboxContainerForBackup to start a container, make openBackupShieldsWindow throw, and make returnSandboxContainerToStopped return false. Assert the operation rejects or exits with the AggregateError message “Backup setup for '<name>' failed and its started container could not be returned to the stopped state,” while verifying the started container is not backed up and the failed restoration path is invoked.Source: Path instructions
docs/reference/commands.mdx (1)
2913-2919: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFix inconsistent capitalization of "Shields."
Lines 2915 and 2918 use "Shields posture" with a capital S. Elsewhere in this file the term is lowercase, for example "shields transitions" in line 1163 and "shields to be down" in the surrounding sections. Use lowercase "shields" here to match the established term for the same concept.
As per coding guidelines, "Use the same term for the same concept. Do not vary terminology for style."
✏️ Proposed fix
-After the copy, it uses a third lock interval with a 30-second acquisition budget to restore the previous Shields posture. +After the copy, it uses a third lock interval with a 30-second acquisition budget to restore the previous shields posture. If the timer expires during the copy, the deadline gate blocks new mutations and waits for the exact backup owner to finish without signaling it. An initial lock or unlock failure marks that sandbox as failed, and `backup-all` continues with the next sandbox. -A failure to restore the previous Shields posture stops `backup-all` before it processes another sandbox. +A failure to restore the previous shields posture stops `backup-all` before it processes another sandbox.🤖 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 `@docs/reference/commands.mdx` around lines 2913 - 2919, In the backup-all documentation, update both occurrences of “Shields posture” in the affected lock and restore descriptions to “shields posture,” preserving the existing wording and capitalization elsewhere.docs/manage-sandboxes/backup-restore.mdx (1)
213-219: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFix inconsistent capitalization of "Shields."
Lines 213, 214, 216, and 219 use "Shields up," "Shields down," and "Shields posture" with a capital S. The rest of this file uses lowercase "shields" as a common noun, for example "requires shields to be down" in line 91 and "the shields auto-restore timer" in line 92. Use lowercase "shields" in these new lines to match the established term for the same concept.
As per coding guidelines, "Use the same term for the same concept. Do not vary terminology for style."
✏️ Proposed fix
-When an eligible sandbox starts with Shields up, `backup-all` acquires the lifecycle lock and opens a 30-minute shields-down window. -A sandbox that starts with Shields down remains down. +When an eligible sandbox starts with shields up, `backup-all` acquires the lifecycle lock and opens a 30-minute shields-down window. +A sandbox that starts with shields down remains down. `backup-all` reacquires the lock under that exact timer generation while it copies sandbox state. -After the copy, it uses a third lock interval with a 30-second acquisition budget to restore the previous Shields posture. +After the copy, it uses a third lock interval with a 30-second acquisition budget to restore the previous shields posture. If the timer expires during the copy, the deadline gate blocks new mutations and waits for the exact backup owner to finish without signaling it. An initial lock or unlock failure marks that sandbox as failed, and `backup-all` continues with the next sandbox. -NemoClaw attempts to restore the previous Shields posture before it processes the next sandbox, including when the backup fails. +NemoClaw attempts to restore the previous shields posture before it processes the next sandbox, including when the backup fails.🤖 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 `@docs/manage-sandboxes/backup-restore.mdx` around lines 213 - 219, In the backup-all behavior description, update the newly added references in the sentences around “backup-all” and “NemoClaw” to use lowercase “shields” consistently, including “shields up,” “shields down,” and “shields posture,” while preserving the existing meaning and wording.src/lib/state/mcp-lifecycle-lock-acquisition.ts (3)
506-508: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReset
lastOwnerPidwhen the main lock is absent.
acquireMcpLifecycleLockSyncclearslastOwnerPidin the equivalent branch at line 655. The async loop keeps the previous value. The timeout message at line 383 can therefore name an owner PID whose generation already disappeared, which misleads an operator during recovery triage.♻️ Proposed fix
} else { + lastOwnerPid = null; resetCorruptGenerationTracker(corruptMainTracker); }🤖 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 506 - 508, Update the main-lock-absent branch in the async lifecycle-lock acquisition flow to clear corruptMainTracker.lastOwnerPid before or alongside resetCorruptGenerationTracker. Match the equivalent behavior in acquireMcpLifecycleLockSync while preserving the existing tracker reset logic.
785-870: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRequire
stateDirin the async fence signature.
acquireDeadlineFenceSyncdeclaresoptions: McpLifecycleDeadlineFenceSyncOptions & { stateDir: string }, so every state-directory read inside it is unambiguous.acquireDeadlineFencedoes not, and line 846 compensates withoptions.stateDir ?? resolveNemoclawStateDir()while lines 804, 826, and 796 readoptions.stateDirdirectly. The only caller,withMcpLifecycleDeadlineFence, always injectsstateDir. Encode that in the type so both fences resolve the state directory in exactly one place.♻️ Proposed fix
async function acquireDeadlineFence( sandboxName: string, takeoverToken: string, - options: McpLifecycleDeadlineFenceOptions, + options: McpLifecycleDeadlineFenceOptions & { stateDir: string }, ): Promise<AcquiredMcpLifecycleLock> {ensureDurableContainmentForStaleGenerationSync( lockPath, sandboxName, - options.stateDir ?? resolveNemoclawStateDir(), + options.stateDir, observation, "An auto-restore deadline owner exited before its recovery operation completed", );🤖 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 785 - 870, Update acquireDeadlineFence to require options: McpLifecycleDeadlineFenceOptions & { stateDir: string }, matching acquireDeadlineFenceSync and the guaranteed stateDir supplied by withMcpLifecycleDeadlineFence. Remove the fallback resolveNemoclawStateDir() from ensureDurableContainmentForStaleGenerationSync and use options.stateDir directly so every state-directory read uses the required injected value.
696-712: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument that this constructor mutates the lease.
durableMcpLifecycleContainmentFailurelooks like a pure error factory, but line 701 setsretainForDurableContainmenton the active lease. That side effect is what keeps the owned generation after the callback returns normally, as the tests atsrc/lib/state/mcp-lifecycle-lock-acquisition.test.tslines 394-420 require. A reader who assumes a pure factory could reorder or memoize the call and silently lose gate retention.Add a doc comment that states the side effect.
♻️ Proposed doc comment
+/** + * Build a coded durable-containment failure and mark the currently held + * lifecycle lease for retention. The retention flag keeps the exact owned + * generation closed even when the caller catches this error, so calling this + * function is not side-effect free. + */ export function durableMcpLifecycleContainmentFailure( error: unknown, lockPath: string, ): Error & { code: string } {🤖 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 696 - 712, Document durableMcpLifecycleContainmentFailure with a doc comment stating that it mutates the active lease by setting retainForDurableContainment, preserving the lease across normal callback completion; make clear that callers must not treat this error factory as pure.src/lib/state/mcp-lifecycle-lock-storage.ts (1)
152-156: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCarry the generation-safety rationale into the synchronous reclaim.
The async versions explain two non-obvious invariants: the rename is the atomic claim, and a raced replacement owner must be restored by hard link rather than unlinked. The synchronous mirrors implement the same invariants without that explanation. A future edit to the sync path alone could delete an owner generation it did not claim.
Add a short pointer comment so both paths document the same contract.
♻️ Proposed comment additions
export function safelyReleaseMcpLifecycleLockSync(lockPath: string, token: string): void { const observation = readMcpLifecycleLockObservationSync(lockPath); if (!observation || observation.owner?.token !== token) return; + // Same contract as safelyReleaseMcpLifecycleLock: claim and verify the + // generation before deletion. reclaimStaleMcpLifecycleLockGenerationSync(lockPath, observation); }export function reclaimStaleMcpLifecycleLockGenerationSync( targetPath: string, expected: LockObservation, ): boolean { + // Synchronous mirror of reclaimStaleMcpLifecycleLockGeneration. Rename is the + // atomic claim; a raced replacement owner is restored by hard link and never + // unlinked. const quarantinePath = `${targetPath}.reclaim-${process.pid}-${crypto.randomUUID()}`;Also applies to: 201-234
🤖 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 152 - 156, Add a concise pointer comment to the synchronous lifecycle-lock release/reclaim path, covering both safelyReleaseMcpLifecycleLockSync and reclaimStaleMcpLifecycleLockGenerationSync, that references the async implementation’s atomic rename claim and hard-link restoration for raced replacement owners. Do not change the synchronization logic.src/lib/state/mcp-lifecycle-lock/shields-timer-authority.ts (1)
91-121: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the shared marker-authority predicate.
readShieldsTimerTakeoverTokenandisShieldsTimerDeadlineExpiredrepeat the same three-part validation: sandbox-name match,processTokentype, and the 32-hex pattern. A single helper keeps both authority checks identical if the token format changes later.♻️ Proposed refactor
+function validatedMarkerToken( + marker: ShieldsTimerMarker | null, + sandboxName: string, +): string | undefined { + if ( + marker?.sandboxName !== sandboxName || + typeof marker.processToken !== "string" || + !/^[0-9a-f]{32}$/.test(marker.processToken) + ) { + return undefined; + } + return marker.processToken; +} + export function readShieldsTimerTakeoverToken( sandboxName: string, stateDir = resolveNemoclawStateDir(), ): string | undefined { - const marker = readShieldsTimerMarker(sandboxName, stateDir); - if ( - marker?.sandboxName !== sandboxName || - typeof marker.processToken !== "string" || - !/^[0-9a-f]{32}$/.test(marker.processToken) - ) { - return undefined; - } - return marker.processToken; + return validatedMarkerToken(readShieldsTimerMarker(sandboxName, stateDir), sandboxName); } export function isShieldsTimerDeadlineExpired( sandboxName: string, stateDir = resolveNemoclawStateDir(), now = Date.now(), ): boolean { const marker = readShieldsTimerMarker(sandboxName, stateDir); - if ( - marker?.sandboxName !== sandboxName || - typeof marker.processToken !== "string" || - !/^[0-9a-f]{32}$/.test(marker.processToken) - ) { - return false; - } + if (!validatedMarkerToken(marker, sandboxName) || !marker) return false; const restoreAtMs = new Date(marker.restoreAt).getTime(); return Number.isFinite(restoreAtMs) && restoreAtMs <= now; }🤖 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/shields-timer-authority.ts` around lines 91 - 121, Extract the repeated marker validation from readShieldsTimerTakeoverToken and isShieldsTimerDeadlineExpired into a shared helper that checks sandbox-name equality, string processToken type, and the 32-hex token format. Update both functions to reuse this predicate while preserving their existing return behavior and deadline validation.src/lib/shields/timer-bound-lock.ts (1)
85-97: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAccept an injectable
depsparameter for symmetry and unit testing.
withTimerBoundShieldsMutationLockandwithTimerBoundShieldsMutationLockAsyncboth acceptdeps: TimerBoundLockDeps = defaultDeps.withTimerBoundAutoRestoreLockhardcodesdefaultDeps, so a unit test cannot inject a fakereadTokenorwithLockto exercise the generation-retry path in isolation. This function guards the auto-restore recovery path, so direct unit coverage is valuable.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 85 - 97, Update withTimerBoundAutoRestoreLock to accept an optional deps: TimerBoundLockDeps parameter defaulting to defaultDeps, and pass that parameter to withTimerBoundShieldsMutationLockOptions instead of hardcoding defaultDeps, matching the injectable dependency pattern of the related lock functions.src/lib/shields/timer.test.ts (1)
92-100: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRaise the
vi.waitFortimeout for CI headroom.The 200 ms timeout must cover deadline-fence acquisition, the first failed restore attempt, and the audit write, all of which touch the filesystem. On a loaded CI runner this budget is tight and can produce intermittent failures. The 1 ms interval already makes the wait exit as soon as both files appear, so a larger timeout does not slow the passing case.
Raise
timeoutto about 2000 ms.♻️ Proposed change
- { interval: 1, timeout: 200 }, + { interval: 1, timeout: 2_000 },🤖 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 92 - 100, Increase the timeout in waitForRetryBoundary’s vi.waitFor options from 200 ms to approximately 2000 ms, while keeping the 1 ms polling interval and file-existence assertions unchanged.src/lib/shields/index.ts (3)
937-987: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReport progress while the inline retry loop blocks.
This loop can block the command for about 30 seconds across 7 attempts, because
Atomics.waitblocks the main thread forINTERACTIVE_AUTO_RESTORE_RETRY_MSbetween attempts.shields statusis an interactive command. During the wait it writes nothing to the terminal, so the command appears hung.Write one
console.errorline before each wait to state that recovery is retrying behind the lifecycle gate. The failure paths already audit to the log, but the operator at the terminal receives no signal until the loop ends.♻️ Proposed change
if (attempt + 1 < INTERACTIVE_AUTO_RESTORE_MAX_ATTEMPTS) { + console.error( + ` Recovery: auto-restore did not complete; retrying behind the lifecycle gate (attempt ${String( + attempt + 2, + )} of ${String(INTERACTIVE_AUTO_RESTORE_MAX_ATTEMPTS)}).`, + ); Atomics.wait(transitionPollBuffer, 0, 0, INTERACTIVE_AUTO_RESTORE_RETRY_MS); }🤖 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 937 - 987, Update retryInlineAutoRestore so that, immediately before each Atomics.wait call, it writes one console.error line indicating inline auto-restore is retrying behind the lifecycle gate. Keep the message in the existing retry condition and preserve the current wait behavior and audit logging.
2495-2506: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the duplicated transition read and snapshot check.
inspectAutoRestoreTransitionTakeoverOwnerat Line 2495 already reads the transition and throws whentransition.snapshotPath !== snapshotPath. Lines 2500-2503 read the same file again and repeat the same check. The second read can observe a newer record, but both checks raise the identical error, so the extra read adds no guarantee.Return the transition from the helper, or reuse the first read, so this function has one source of truth for the transition record.
🤖 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 2495 - 2506, Remove the duplicate readShieldsDownTransition call and snapshot-path validation in the surrounding auto-restore flow. Update inspectAutoRestoreTransitionTakeoverOwner to return the transition it already reads, then reuse that returned record for the subsequent waitForShieldsDownForwardCommit condition so there is one source of truth.
285-298: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCentralize the
.containmentpath suffix in one exported helper.This file builds the containment path from the string literal
.containmentat three places: Line 290, Line 881, and Line 1042. Line 1042 uses that path to decide whether to rethrow an error instead of retrying. If the suffix ever changes inmcp-lifecycle-lock, these sites keep compiling and silently stop detecting containment.Export a
getMcpLifecycleContainmentPath(sandboxName, stateDir)accessor from../state/mcp-lifecycle-lockand call it here and at the other two sites.♻️ Proposed change at this site
- const containmentPath = `${getMcpLifecycleLockPath(sandboxName, STATE_DIR)}.containment`; + const containmentPath = getMcpLifecycleContainmentPath(sandboxName, STATE_DIR);🤖 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 285 - 298, Export and use a shared getMcpLifecycleContainmentPath(sandboxName, stateDir) helper from ../state/mcp-lifecycle-lock instead of constructing the ".containment" suffix locally. Update persistUnresolvedShieldsContainment and the other two containment-path call sites in this file, including the error-retry check, to use the helper consistently.src/lib/shields/timer-control.ts (1)
236-253: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the obsolete
terminatedfield.
killTimeralways returnsterminated: false, and no production caller reads it. Remove it fromKillTimerResultand update the assertions insrc/lib/shields/index.test.ts.🤖 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 236 - 253, Remove the obsolete terminated property from the KillTimerResult type and the return object in killTimer. Update the related assertions in shields/index.test.ts to stop expecting terminated while preserving the remaining result fields and 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.
Inline comments:
In `@src/lib/shields/index.ts`:
- Around line 2979-2983: Update the ownerMcpProcessIdentity initialization in
the shields-down flow to use ownerStartIdentityFallback when
readMcpLockProcessIdentity(process.pid, true) returns null, rather than throwing
and blocking shutdown. Preserve the fresh lookup while allowing shields down to
proceed when /proc or ps cannot provide an identity.
In `@src/lib/shields/timer.ts`:
- Around line 477-505: Bound the retry loop in restoreWhileDeadlineOwned using
the same INTERACTIVE_AUTO_RESTORE_MAX_ATTEMPTS policy as the interactive
recovery path. Track attempts, stop retrying when the cap is reached, and
transition to the existing durable-containment terminal behavior while
preserving marker validation and normal complete/revoked outcomes.
In `@src/lib/state/mcp-lifecycle-lock-acquisition.test.ts`:
- Around line 75-88: Remove the if statement from
publishTimerWhenStaleOwnerIsObserved and use the file’s existing optional-call
idiom to conditionally publish the marker and throw ESRCH only when the PID is
2_147_483_647 and the signal is 0. Preserve delegation to realProcessKill for
all other process.kill calls.
---
Nitpick comments:
In `@docs/manage-sandboxes/backup-restore.mdx`:
- Around line 213-219: In the backup-all behavior description, update the newly
added references in the sentences around “backup-all” and “NemoClaw” to use
lowercase “shields” consistently, including “shields up,” “shields down,” and
“shields posture,” while preserving the existing meaning and wording.
In `@docs/reference/commands.mdx`:
- Around line 2913-2919: In the backup-all documentation, update both
occurrences of “Shields posture” in the affected lock and restore descriptions
to “shields posture,” preserving the existing wording and capitalization
elsewhere.
In `@src/lib/actions/maintenance.test.ts`:
- Around line 277-302: Update the test for backupAll so it captures the
console.error spy and asserts the logged failure includes the expected “backup
failed (mutation lock: ...)” reason from the withSandboxMutationLock rejection.
Keep the existing assertions verifying no container start or backup mutations
occur.
- Around line 833-877: Make the test’s loop-abort expectation explicit around
backupAll: verify the second sandbox, “beta,” is not processed after the cleanup
lock failure, using an observable public outcome rather than relying solely on
withSandboxMutationLock call counts. Keep the existing assertions for the
AggregateError and stopped-container cleanup behavior.
- Around line 277-302: Add a backupAll() test covering failure after setup
enters the acquired mutation lock: configure
startStoppedSandboxContainerForBackup to start a container, make
openBackupShieldsWindow throw, and make returnSandboxContainerToStopped return
false. Assert the operation rejects or exits with the AggregateError message
“Backup setup for '<name>' failed and its started container could not be
returned to the stopped state,” while verifying the started container is not
backed up and the failed restoration path is invoked.
In `@src/lib/actions/maintenance.ts`:
- Around line 378-420: Remove the redundant result, orphanManifestMessage,
mutationLockError, and mutationLockFailed locals around
backupSandboxWithinShieldsWindow. Read result and orphanManifestMessage directly
from attempt, and replace the mutationLockFailed branch with an inline
"mutationLockError" in attempt check while deriving the error detail from
attempt.mutationLockError.
- Around line 257-299: Replace the failure-combination cascade in the
maintenance failure-handling block with one causes collection using backupError
or orphanManifestMessage, relockError, and stoppedContainerCleanupError.
Aggregate when multiple causes exist and throw the sole cause unchanged when
only one exists. Preserve the exact currently asserted error messages by
deriving the message from the same combinations of present failure flags.
In `@src/lib/shields/index.ts`:
- Around line 937-987: Update retryInlineAutoRestore so that, immediately before
each Atomics.wait call, it writes one console.error line indicating inline
auto-restore is retrying behind the lifecycle gate. Keep the message in the
existing retry condition and preserve the current wait behavior and audit
logging.
- Around line 2495-2506: Remove the duplicate readShieldsDownTransition call and
snapshot-path validation in the surrounding auto-restore flow. Update
inspectAutoRestoreTransitionTakeoverOwner to return the transition it already
reads, then reuse that returned record for the subsequent
waitForShieldsDownForwardCommit condition so there is one source of truth.
- Around line 285-298: Export and use a shared
getMcpLifecycleContainmentPath(sandboxName, stateDir) helper from
../state/mcp-lifecycle-lock instead of constructing the ".containment" suffix
locally. Update persistUnresolvedShieldsContainment and the other two
containment-path call sites in this file, including the error-retry check, to
use the helper consistently.
In `@src/lib/shields/timer-bound-lock.ts`:
- Around line 85-97: Update withTimerBoundAutoRestoreLock to accept an optional
deps: TimerBoundLockDeps parameter defaulting to defaultDeps, and pass that
parameter to withTimerBoundShieldsMutationLockOptions instead of hardcoding
defaultDeps, matching the injectable dependency pattern of the related lock
functions.
In `@src/lib/shields/timer-control.ts`:
- Around line 236-253: Remove the obsolete terminated property from the
KillTimerResult type and the return object in killTimer. Update the related
assertions in shields/index.test.ts to stop expecting terminated while
preserving the remaining result fields and behavior.
In `@src/lib/shields/timer.test.ts`:
- Around line 92-100: Increase the timeout in waitForRetryBoundary’s vi.waitFor
options from 200 ms to approximately 2000 ms, while keeping the 1 ms polling
interval and file-existence assertions unchanged.
In `@src/lib/state/mcp-lifecycle-lock-acquisition.ts`:
- Around line 506-508: Update the main-lock-absent branch in the async
lifecycle-lock acquisition flow to clear corruptMainTracker.lastOwnerPid before
or alongside resetCorruptGenerationTracker. Match the equivalent behavior in
acquireMcpLifecycleLockSync while preserving the existing tracker reset logic.
- Around line 785-870: Update acquireDeadlineFence to require options:
McpLifecycleDeadlineFenceOptions & { stateDir: string }, matching
acquireDeadlineFenceSync and the guaranteed stateDir supplied by
withMcpLifecycleDeadlineFence. Remove the fallback resolveNemoclawStateDir()
from ensureDurableContainmentForStaleGenerationSync and use options.stateDir
directly so every state-directory read uses the required injected value.
- Around line 696-712: Document durableMcpLifecycleContainmentFailure with a doc
comment stating that it mutates the active lease by setting
retainForDurableContainment, preserving the lease across normal callback
completion; make clear that callers must not treat this error factory as pure.
In `@src/lib/state/mcp-lifecycle-lock-storage.ts`:
- Around line 152-156: Add a concise pointer comment to the synchronous
lifecycle-lock release/reclaim path, covering both
safelyReleaseMcpLifecycleLockSync and
reclaimStaleMcpLifecycleLockGenerationSync, that references the async
implementation’s atomic rename claim and hard-link restoration for raced
replacement owners. Do not change the synchronization logic.
In `@src/lib/state/mcp-lifecycle-lock/shields-timer-authority.ts`:
- Around line 91-121: Extract the repeated marker validation from
readShieldsTimerTakeoverToken and isShieldsTimerDeadlineExpired into a shared
helper that checks sandbox-name equality, string processToken type, and the
32-hex token format. Update both functions to reuse this predicate while
preserving their existing return behavior and deadline validation.
🪄 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: 2177445b-5b2e-4db0-bb78-53ba2f3ac67f
📒 Files selected for processing (32)
ci/env-var-doc-allowlist.jsonci/source-shape-test-budget.jsondocs/manage-sandboxes/backup-restore.mdxdocs/manage-sandboxes/runtime-controls.mdxdocs/reference/commands.mdxsrc/commands/sandbox/shields/status.tssrc/commands/sandbox/shields/up.tssrc/lib/actions/maintenance.test.tssrc/lib/actions/maintenance.tssrc/lib/actions/sandbox/snapshot.tssrc/lib/shields/flow.test.tssrc/lib/shields/index.test.tssrc/lib/shields/index.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.test.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/config-set-nested-ssrf.test.tstest/helpers/isolate-test-state.tstest/mcp-lifecycle-lock.test.tstest/vitest-temp-root.test.tsvitest.config.ts
| ownerMcpProcessIdentity: | ||
| readMcpLockProcessIdentity(process.pid, true) ?? | ||
| (() => { | ||
| throw new Error("Cannot identify shields-down lifecycle owner process"); | ||
| })(), |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect readMcpLockProcessIdentity to determine when it returns null.
set -euo pipefail
fd -t f 'mcp-lifecycle-lock-identity.ts' src
fd -t f 'mcp-lifecycle-lock-identity.ts' src --exec ast-grep outline {} --items all
# Show the full implementation and its null-return paths.
ast-grep run --lang typescript \
--pattern 'export function readMcpLockProcessIdentity($$$) { $$$ }' \
src
# Find every caller and how each handles a null result.
rg -nP -C4 '\breadMcpLockProcessIdentity\s*\(' srcRepository: NVIDIA/NemoClaw
Length of output: 1839
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file=$(fd -t f 'mcp-lifecycle-lock-identity.ts' src | head -n1)
cat -n "$file" | sed -n '1,225p'
printf '\n--- call sites ---\n'
rg -n -C5 '\breadMcpLockProcessIdentity\s*\(' src
printf '\n--- transition-lock identity fallback ---\n'
rg -n -C8 'ownerStartIdentityFallback|readMcpLockProcessIdentity|processIdentity' src/lib/state/transition-lock.ts src/lib/stateRepository: NVIDIA/NemoClaw
Length of output: 50371
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- fallback references ---'
rg -n -C6 'ownerStartIdentityFallback|transition-lock|start identity fallback' src
printf '%s\n' '--- supported platform declarations ---'
rg -n -i -C3 'windows|macos|darwin|linux|supported platform|platform support' README.md package.json src/lib/README.md docs 2>/dev/null || true
printf '%s\n' '--- shields-down identity context ---'
cat -n src/lib/shields/index.ts | sed -n '2968,2990p'Repository: NVIDIA/NemoClaw
Length of output: 50374
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- exact fallback locations ---'
rg -l 'ownerStartIdentityFallback' src | while read -r file; do
printf '\nFILE %s\n' "$file"
rg -n -C12 'ownerStartIdentityFallback' "$file"
done
printf '%s\n' '--- exact platform support references ---'
rg -n -i 'Windows|macOS|Linux' docs/reference/platform-support.mdx docs/get-started/prerequisites.mdx docs/get-started/additional-setup/windows-preparation.mdx 2>/dev/null | head -n 120 || trueRepository: NVIDIA/NemoClaw
Length of output: 14116
Use the identity fallback for shields down. fresh = true only bypasses the cache. It does not prevent readMcpLockProcessIdentity from returning null when /proc or ps cannot provide an identity. Reuse ownerStartIdentityFallback or handle null without blocking shields down.
🤖 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 2979 - 2983, Update the
ownerMcpProcessIdentity initialization in the shields-down flow to use
ownerStartIdentityFallback when readMcpLockProcessIdentity(process.pid, true)
returns null, rather than throwing and blocking shutdown. Preserve the fresh
lookup while allowing shields down to proceed when /proc or ps cannot provide an
identity.
PR Review Advisor — InformationalAdvisor assessment: Informational / low confidence Model lanes
Second-opinion terminology and E2E selections are advisory. They do not change the primary assessment or E2E / PR Gate. 3 semantic terminology decisionsTerminology decisions are advisory. They affect the assessment only when a separate finding identifies concrete semantic impact.
E2E guidanceAdvisory only. E2E / PR Gate selects and runs jobs independently. Recommended E2E: This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
|
Stacked focused follow-up: #8141 contains the managed MCP policy reconciliation and corrected Hermes regression order. Its standard CI, automated reviews, and all 10 trusted E2E selections are green. |
apurvvkumaria
left a comment
apurvvkumaria
left a comment
There was a problem hiding this comment.
Blocking finding confirmed at exact head c023a4b: restoreWhileDeadlineOwned in src/lib/shields/timer.ts retries forever when restore or re-lock keeps failing and the timer marker remains current. The callback runs inside withMcpLifecycleDeadlineFence, which releases its main lock and deadline fence only after the callback returns. A deterministic persistent failure therefore leaves ordinary lifecycle, policy, channel, shields, and snapshot operations blocked indefinitely while audit entries continue accumulating. This makes supported recovery workflows unusable. Please bound the timer retry loop by attempts or elapsed time, then enter the existing durable-containment terminal state with actionable operator recovery while retaining the fail-closed lifecycle gate. The existing CodeRabbit inline identifies the same affected loop, so I am not duplicating an inline comment.
<!-- 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>
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (5)
src/lib/shields/index.ts (1)
3014-3014: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the recovery-token validation into one helper.
The literal
/^[0-9a-f]{32}$/test onmarker.processTokenappears three times in this function, and it also appears at Line 2728 and Line 3728. One helper keeps the token contract in a single place and prevents drift if the token format changes.♻️ Suggested helper
function isRecoveryProcessToken(value: string | undefined): value is string { return typeof value === "string" && /^[0-9a-f]{32}$/.test(value); }- if (marker.processToken && /^[0-9a-f]{32}$/.test(marker.processToken)) { + if (isRecoveryProcessToken(marker.processToken)) {Also applies to: 3042-3048, 3078-3078
🤖 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` at line 3014, Extract the repeated recovery-token regex into a shared isRecoveryProcessToken helper near the relevant utilities, accepting string | undefined and narrowing valid values to string. Replace the direct /^[0-9a-f]{32}$/ checks in the current function and the other occurrences around the referenced token-validation logic with this helper, preserving the existing validation behavior.src/lib/shields/flow.test.ts (1)
80-106: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive the fixture ownership values from the shared constants.
managedMcpSandboxhardcodessourcePath: "generated:nemoclaw-mcp-bridge"andpolicyName: \mcp-bridge-${server}`. Production ownership checks compare againstMCP_BRIDGE_POLICY_SOURCEandbuildMcpBridgePolicyName(server)`. If either value changes, this fixture stops representing an owned bridge and the managed MCP tests lose their meaning.Import both symbols instead.
♻️ Proposed fixture change
-import { buildMcpBridgePolicyYaml } from "../actions/sandbox/mcp-bridge-policy-render"; +import { MCP_BRIDGE_POLICY_SOURCE } from "../actions/sandbox/mcp-bridge-policy"; +import { + buildMcpBridgePolicyName, + buildMcpBridgePolicyYaml, +} from "../actions/sandbox/mcp-bridge-policy-render";customPolicies: policies.map(({ content, server }) => ({ - name: `mcp-bridge-${server}`, + name: buildMcpBridgePolicyName(server), content, - sourcePath: "generated:nemoclaw-mcp-bridge", + sourcePath: MCP_BRIDGE_POLICY_SOURCE, })),- policyName: `mcp-bridge-${server}`, + policyName: buildMcpBridgePolicyName(server),🤖 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 80 - 106, Update the managedMcpSandbox fixture to import and use the shared MCP_BRIDGE_POLICY_SOURCE and buildMcpBridgePolicyName symbols for sourcePath and policyName, respectively, instead of hardcoded ownership values. Keep the generated fixture behavior unchanged while ensuring it follows production ownership checks.src/lib/shields/timer.test.ts (1)
11-21: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winType the mock result with the exported omission type.
The inline type makes
serverrequired.ManagedMcpPolicyOmissioninsrc/lib/actions/sandbox/mcp-bridge-policy.ts(lines 40-45) declareskey,policyName, andserveras optional and onlyreasonas required. Key-only omissions, such as the reserved-key case, cannot be expressed by this mock. The mock then no longer tracks the real contract iftimer.tsstarts readingkeyorpolicyName.♻️ Proposed mock typing
+import type { ManagedMcpPolicyOmission } from "../actions/sandbox/mcp-bridge-policy"; + const shieldsIndexMock = vi.hoisted(() => ({ applyShieldsPolicySnapshot: vi.fn( (): { status: number; - managedMcpOmissions?: Array<{ server: string; reason: string }>; + managedMcpOmissions?: ManagedMcpPolicyOmission[]; } => ({ status: 0 }), ),Note that
vi.hoistedruns before imports, so keep the reference type-only.🤖 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 - 21, Update the applyShieldsPolicySnapshot mock result in shieldsIndexMock to use the exported ManagedMcpPolicyOmission type from mcp-bridge-policy.ts via a type-only reference, preserving vi.hoisted’s runtime import constraints. Remove the inline omission shape so optional key, policyName, and server fields and required reason match the production contract.src/lib/shields/permissive-runtime.ts (1)
242-269: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the shared temp-policy staging logic and wrap the base read.
buildDeadlineRuntimeManagedMcpPolicyrepeats the staging and cleanup logic ofbuildRuntimeManagedMcpPolicy(lines 214-234) with a different branch shape. Both paths must keep identical cleanup semantics for the 0700 mkdtemp directory, so one helper is safer than two copies.Line 246 also calls
deps.readBasePolicy()without the contextual wrapper used at lines 202-208. A snapshot read failure at the deadline then surfaces a raw filesystem error to the operator instead of the staging message.♻️ Proposed shared helper
+function stageRuntimePolicy( + yaml: string, + deps: ManagedMcpRuntimePolicyDeps, + failureMessage: string, +): string { + if (deps.writeTempPolicy) { + try { + return deps.writeTempPolicy(yaml); + } catch (error) { + throw new Error(failureMessage, { cause: error }); + } + } + let tmpPath: string | null = null; + try { + tmpPath = secureTempFile(TEMP_FILE_PREFIX, ".yaml"); + fs.writeFileSync(tmpPath, yaml, { mode: 0o600 }); + return tmpPath; + } catch (error) { + if (tmpPath) cleanupTempDir(tmpPath, TEMP_FILE_PREFIX); + throw new Error(failureMessage, { cause: error }); + } +}): DeadlineManagedMcpRuntimePolicy { - const baseYaml = deps.readBasePolicy(); + let baseYaml: string; + try { + baseYaml = deps.readBasePolicy(); + } catch (error) { + throw new Error("Cannot read the deadline Shields policy for managed MCP reconciliation", { + cause: error, + }); + }🤖 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 242 - 269, Extract the duplicated temporary-policy writing and cleanup flow from buildRuntimeManagedMcpPolicy and buildDeadlineRuntimeManagedMcpPolicy into one shared helper, preserving identical 0700 temp-directory cleanup semantics and error propagation. Update both builders to use that helper, and wrap deps.readBasePolicy() in buildDeadlineRuntimeManagedMcpPolicy with the same contextual error message used by the existing base-policy read path.src/lib/actions/sandbox/mcp-bridge-policy.ts (1)
60-83: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider sharing the policy-document helpers with
src/lib/shields/mcp-policy-transition.ts.
parseManagedPolicyDocumentandreadManagedNetworkPoliciesduplicateparsePolicyDocumentandreadNetworkPoliciesinsrc/lib/shields/mcp-policy-transition.ts(lines 14-37). The two copies can drift on parse strictness, and both feed the same fail-closed decisions. Extract one shared helper module.🤖 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/actions/sandbox/mcp-bridge-policy.ts` around lines 60 - 83, Extract the shared YAML policy parsing and network-policy mapping logic from parseManagedPolicyDocument and readManagedNetworkPolicies into a common helper module, then update both mcp-bridge-policy.ts and mcp-policy-transition.ts to reuse it. Preserve the existing validation, error behavior, and fail-closed handling in both callers while removing the duplicated implementations.
🤖 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`:
- Line 119: Update the sentence in the runtime controls documentation to
hyphenate the compound modifier, changing “snapshot-time managed MCP entries” to
“snapshot-time-managed MCP entries.”
In `@scripts/checks/openshell-policy-mutation-read.mts`:
- Line 61: Replace the count-only allowance around expectedReadCalls and
buildPolicyGetCommand with a canonical, site-specific invariant that identifies
and validates the three approved policy-read call sites. Update the audit to
reject any unapproved extra read even when the count is adjusted, preserve the
intended three-read behavior, and add focused tests covering both rejection of
an extra read and acceptance of the approved sites.
In `@test/e2e/live/mcp-bridge-sandbox.ts`:
- Around line 80-86: Update expectExitNonZero to explicitly reject null exitCode
values before the output-pattern assertion, while still requiring a completed
nonzero exit status. Preserve the existing diagnostic message and resultText
matching for valid nonzero exits.
In `@test/e2e/support/mcp-bridge-sandbox.test.ts`:
- Around line 306-333: Update the test around
assertManagedMcpPolicySurvivedRemoval to use independent, structurally
equivalent policy snapshots rather than reusing the same survivingPolicy object.
Add a preservation assertion with the removed policy key absent and a separately
constructed surviving-policy object, while retaining the failing case where that
key remains present, so the test verifies structural preservation rather than
object identity.
---
Nitpick comments:
In `@src/lib/actions/sandbox/mcp-bridge-policy.ts`:
- Around line 60-83: Extract the shared YAML policy parsing and network-policy
mapping logic from parseManagedPolicyDocument and readManagedNetworkPolicies
into a common helper module, then update both mcp-bridge-policy.ts and
mcp-policy-transition.ts to reuse it. Preserve the existing validation, error
behavior, and fail-closed handling in both callers while removing the duplicated
implementations.
In `@src/lib/shields/flow.test.ts`:
- Around line 80-106: Update the managedMcpSandbox fixture to import and use the
shared MCP_BRIDGE_POLICY_SOURCE and buildMcpBridgePolicyName symbols for
sourcePath and policyName, respectively, instead of hardcoded ownership values.
Keep the generated fixture behavior unchanged while ensuring it follows
production ownership checks.
In `@src/lib/shields/index.ts`:
- Line 3014: Extract the repeated recovery-token regex into a shared
isRecoveryProcessToken helper near the relevant utilities, accepting string |
undefined and narrowing valid values to string. Replace the direct
/^[0-9a-f]{32}$/ checks in the current function and the other occurrences around
the referenced token-validation logic with this helper, preserving the existing
validation behavior.
In `@src/lib/shields/permissive-runtime.ts`:
- Around line 242-269: Extract the duplicated temporary-policy writing and
cleanup flow from buildRuntimeManagedMcpPolicy and
buildDeadlineRuntimeManagedMcpPolicy into one shared helper, preserving
identical 0700 temp-directory cleanup semantics and error propagation. Update
both builders to use that helper, and wrap deps.readBasePolicy() in
buildDeadlineRuntimeManagedMcpPolicy with the same contextual error message used
by the existing base-policy read path.
In `@src/lib/shields/timer.test.ts`:
- Around line 11-21: Update the applyShieldsPolicySnapshot mock result in
shieldsIndexMock to use the exported ManagedMcpPolicyOmission type from
mcp-bridge-policy.ts via a type-only reference, preserving vi.hoisted’s runtime
import constraints. Remove the inline omission shape so optional key,
policyName, and server fields and required reason match the production 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: 0421881b-d059-4033-b09a-2d0dc5d6f47f
📒 Files selected for processing (19)
ci/source-architecture-budget.jsondocs/manage-sandboxes/runtime-controls.mdxdocs/reference/commands.mdxscripts/checks/openshell-policy-mutation-read.mtssrc/lib/actions/sandbox/mcp-bridge-policy.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/policy-transition.test.tssrc/lib/shields/timer.test.tssrc/lib/shields/timer.tstest/e2e/live/mcp-bridge-hermes-lifecycle.tstest/e2e/live/mcp-bridge-sandbox.tstest/e2e/live/mcp-bridge.test.tstest/e2e/support/mcp-bridge-sandbox.test.tstest/permissive-runtime.test.ts
💤 Files with no reviewable changes (1)
- test/e2e/live/mcp-bridge-hermes-lifecycle.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- docs/reference/commands.mdx
- src/lib/shields/timer.ts
| Retry a command that the auto-restore deadline interrupts after you open a new shields-down window. | ||
| Before a manual Shields transition replaces a policy, NemoClaw requires exact Model Context Protocol (MCP) agreement among the sandbox registry, generated-policy record, and live gateway policy. | ||
| `shields down` carries the proven managed MCP policy entries into the relaxed policy. | ||
| Restoration removes snapshot-time managed MCP entries before it overlays current exact entries. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Hyphenate the compound modifier.
Change snapshot-time managed MCP entries to snapshot-time-managed MCP entries.
🧰 Tools
🪛 LanguageTool
[grammar] ~119-~119: Use a hyphen to join words.
Context: ...olicy. Restoration removes snapshot-time managed MCP entries before it overlays c...
(QB_NEW_EN_HYPHEN)
🤖 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 `@docs/manage-sandboxes/runtime-controls.mdx` at line 119, Update the sentence
in the runtime controls documentation to hyphenate the compound modifier,
changing “snapshot-time managed MCP entries” to “snapshot-time-managed MCP
entries.”
Source: Linters/SAST tools
| export function expectExitNonZero(result: ShellProbeResult, label: string, pattern: RegExp): void { | ||
| assert.notEqual( | ||
| result.exitCode, | ||
| 0, | ||
| `${label}\nstdout:\n${result.stdout}\nstderr:\n${result.stderr}`, | ||
| ); | ||
| assert.match(resultText(result), pattern); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Require a completed nonzero exit.
ShellProbeResult.exitCode can be null. Line 81 accepts that value as a valid failure. A timed-out or signaled probe can then satisfy the output assertion without completing the expected rejection path. Reject timed-out probes and null exit codes before matching output.
Proposed fix
export function expectExitNonZero(result: ShellProbeResult, label: string, pattern: RegExp): void {
- assert.notEqual(
- result.exitCode,
- 0,
+ assert.ok(
+ !result.timedOut && result.exitCode !== null && result.exitCode !== 0,
`${label}\nstdout:\n${result.stdout}\nstderr:\n${result.stderr}`,
);
assert.match(resultText(result), pattern);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export function expectExitNonZero(result: ShellProbeResult, label: string, pattern: RegExp): void { | |
| assert.notEqual( | |
| result.exitCode, | |
| 0, | |
| `${label}\nstdout:\n${result.stdout}\nstderr:\n${result.stderr}`, | |
| ); | |
| assert.match(resultText(result), pattern); | |
| export function expectExitNonZero(result: ShellProbeResult, label: string, pattern: RegExp): void { | |
| assert.ok( | |
| !result.timedOut && result.exitCode !== null && result.exitCode !== 0, | |
| `${label}\nstdout:\n${result.stdout}\nstderr:\n${result.stderr}`, | |
| ); | |
| assert.match(resultText(result), pattern); | |
| } |
🤖 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/live/mcp-bridge-sandbox.ts` around lines 80 - 86, Update
expectExitNonZero to explicitly reject null exitCode values before the
output-pattern assertion, while still requiring a completed nonzero exit status.
Preserve the existing diagnostic message and resultText matching for valid
nonzero exits.
| it("accepts an unchanged surviving policy only after the unrelated policy is absent", () => { | ||
| const survivingPolicy = { | ||
| endpoints: [{ host: "surviving.example.test", allowed_ips: ["203.0.113.10"] }], | ||
| }; | ||
|
|
||
| it("captures the Hermes rediscovery offset after route removal and before restart", () => { | ||
| const source = fs.readFileSync("test/e2e/live/mcp-bridge.test.ts", "utf8"); | ||
| const denialProof = source.indexOf("rebound request must not reach the upstream MCP server"); | ||
| const restore = source.indexOf("await restoreDnsRebindingHostsFixture", denialProof); | ||
| const remove = source.indexOf("const remove = await host.nemoclaw", denialProof); | ||
| const hermesTest = source.indexOf('mcpBridgeShardTest("hermes")'); | ||
| const rebinding = source.indexOf("await assertAdapterDnsRebindingDenied", hermesTest); | ||
| const offset = source.indexOf( | ||
| "const survivingDiscoveryOffset = fakeMcp.requests.length", | ||
| rebinding, | ||
| ); | ||
| const restart = source.indexOf("await restartBridgeWithoutHostSecret", offset); | ||
| const toolCall = source.indexOf("await assertRealAdapterToolCall", restart); | ||
| const rediscovery = source.indexOf("await assertAuthenticatedMcpRediscovery", toolCall); | ||
|
|
||
| expect(denialProof).toBeGreaterThanOrEqual(0); | ||
| expect(restore).toBeGreaterThan(denialProof); | ||
| expect(remove).toBeGreaterThan(restore); | ||
| expect(rebinding).toBeGreaterThan(hermesTest); | ||
| expect(offset).toBeGreaterThan(rebinding); | ||
| expect(restart).toBeGreaterThan(offset); | ||
| expect(toolCall).toBeGreaterThan(restart); | ||
| expect(rediscovery).toBeGreaterThan(toolCall); | ||
| expect(source).toContain("Hermes MCP rediscovery after explicit restart"); | ||
| expect(() => | ||
| assertManagedMcpPolicySurvivedRemoval( | ||
| survivingPolicy, | ||
| { | ||
| networkPolicies: { mcp_bridge_surviving: survivingPolicy }, | ||
| policy: survivingPolicy, | ||
| }, | ||
| "mcp_bridge_rebinding", | ||
| ), | ||
| ).not.toThrow(); | ||
| expect(() => | ||
| assertManagedMcpPolicySurvivedRemoval( | ||
| survivingPolicy, | ||
| { | ||
| networkPolicies: { | ||
| mcp_bridge_rebinding: { endpoints: [] }, | ||
| mcp_bridge_surviving: survivingPolicy, | ||
| }, | ||
| policy: survivingPolicy, | ||
| }, | ||
| "mcp_bridge_rebinding", | ||
| ), | ||
| ).toThrow(); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use independent policy snapshots in the preservation case.
Lines 313-317 use the same survivingPolicy object for both snapshots. This test would pass if the helper used identity comparison instead of structural comparison. The failing case only proves that the removed policy key must be absent. Add a separate changed surviving-policy case with the removed key absent.
Proposed fix
- const survivingPolicy = {
+ const survivingPolicyBefore = {
endpoints: [{ host: "surviving.example.test", allowed_ips: ["203.0.113.10"] }],
};
+ const survivingPolicyAfter = {
+ endpoints: [{ host: "surviving.example.test", allowed_ips: ["203.0.113.10"] }],
+ };
+ const changedSurvivingPolicy = {
+ endpoints: [{ host: "changed.example.test", allowed_ips: ["203.0.113.10"] }],
+ };
expect(() =>
assertManagedMcpPolicySurvivedRemoval(
- survivingPolicy,
+ survivingPolicyBefore,
{
- networkPolicies: { mcp_bridge_surviving: survivingPolicy },
- policy: survivingPolicy,
+ networkPolicies: { mcp_bridge_surviving: survivingPolicyAfter },
+ policy: survivingPolicyAfter,
},
"mcp_bridge_rebinding",
),
).not.toThrow();
+
+ expect(() =>
+ assertManagedMcpPolicySurvivedRemoval(
+ survivingPolicyBefore,
+ {
+ networkPolicies: { mcp_bridge_surviving: changedSurvivingPolicy },
+ policy: changedSurvivingPolicy,
+ },
+ "mcp_bridge_rebinding",
+ ),
+ ).toThrow();As per path instructions, review tests for behavioral confidence rather than implementation lock-in.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it("accepts an unchanged surviving policy only after the unrelated policy is absent", () => { | |
| const survivingPolicy = { | |
| endpoints: [{ host: "surviving.example.test", allowed_ips: ["203.0.113.10"] }], | |
| }; | |
| it("captures the Hermes rediscovery offset after route removal and before restart", () => { | |
| const source = fs.readFileSync("test/e2e/live/mcp-bridge.test.ts", "utf8"); | |
| const denialProof = source.indexOf("rebound request must not reach the upstream MCP server"); | |
| const restore = source.indexOf("await restoreDnsRebindingHostsFixture", denialProof); | |
| const remove = source.indexOf("const remove = await host.nemoclaw", denialProof); | |
| const hermesTest = source.indexOf('mcpBridgeShardTest("hermes")'); | |
| const rebinding = source.indexOf("await assertAdapterDnsRebindingDenied", hermesTest); | |
| const offset = source.indexOf( | |
| "const survivingDiscoveryOffset = fakeMcp.requests.length", | |
| rebinding, | |
| ); | |
| const restart = source.indexOf("await restartBridgeWithoutHostSecret", offset); | |
| const toolCall = source.indexOf("await assertRealAdapterToolCall", restart); | |
| const rediscovery = source.indexOf("await assertAuthenticatedMcpRediscovery", toolCall); | |
| expect(denialProof).toBeGreaterThanOrEqual(0); | |
| expect(restore).toBeGreaterThan(denialProof); | |
| expect(remove).toBeGreaterThan(restore); | |
| expect(rebinding).toBeGreaterThan(hermesTest); | |
| expect(offset).toBeGreaterThan(rebinding); | |
| expect(restart).toBeGreaterThan(offset); | |
| expect(toolCall).toBeGreaterThan(restart); | |
| expect(rediscovery).toBeGreaterThan(toolCall); | |
| expect(source).toContain("Hermes MCP rediscovery after explicit restart"); | |
| expect(() => | |
| assertManagedMcpPolicySurvivedRemoval( | |
| survivingPolicy, | |
| { | |
| networkPolicies: { mcp_bridge_surviving: survivingPolicy }, | |
| policy: survivingPolicy, | |
| }, | |
| "mcp_bridge_rebinding", | |
| ), | |
| ).not.toThrow(); | |
| expect(() => | |
| assertManagedMcpPolicySurvivedRemoval( | |
| survivingPolicy, | |
| { | |
| networkPolicies: { | |
| mcp_bridge_rebinding: { endpoints: [] }, | |
| mcp_bridge_surviving: survivingPolicy, | |
| }, | |
| policy: survivingPolicy, | |
| }, | |
| "mcp_bridge_rebinding", | |
| ), | |
| ).toThrow(); | |
| it("accepts an unchanged surviving policy only after the unrelated policy is absent", () => { | |
| const survivingPolicyBefore = { | |
| endpoints: [{ host: "surviving.example.test", allowed_ips: ["203.0.113.10"] }], | |
| }; | |
| const survivingPolicyAfter = { | |
| endpoints: [{ host: "surviving.example.test", allowed_ips: ["203.0.113.10"] }], | |
| }; | |
| const changedSurvivingPolicy = { | |
| endpoints: [{ host: "changed.example.test", allowed_ips: ["203.0.113.10"] }], | |
| }; | |
| expect(() => | |
| assertManagedMcpPolicySurvivedRemoval( | |
| survivingPolicyBefore, | |
| { | |
| networkPolicies: { mcp_bridge_surviving: survivingPolicyAfter }, | |
| policy: survivingPolicyAfter, | |
| }, | |
| "mcp_bridge_rebinding", | |
| ), | |
| ).not.toThrow(); | |
| expect(() => | |
| assertManagedMcpPolicySurvivedRemoval( | |
| survivingPolicyBefore, | |
| { | |
| networkPolicies: { mcp_bridge_surviving: changedSurvivingPolicy }, | |
| policy: changedSurvivingPolicy, | |
| }, | |
| "mcp_bridge_rebinding", | |
| ), | |
| ).toThrow(); |
🤖 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 306 - 333, Update
the test around assertManagedMcpPolicySurvivedRemoval to use independent,
structurally equivalent policy snapshots rather than reusing the same
survivingPolicy object. Add a preservation assertion with the removed policy key
absent and a separately constructed surviving-policy object, while retaining the
failing case where that key remains present, so the test verifies structural
preservation rather than object identity.
Source: Path instructions
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/shields/index.ts (1)
3213-3226: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winFix the argument order in
activateLockdownFromSnapshot.Pass
undefinedforallowLegacyHermesProtocol, thencachedTarget,undefinedforcachedProtocol, and the recovery options as the sixth argument. The current call fails type-checking and does not pass the recovery options toapplyShieldsPolicySnapshot.🤖 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 3213 - 3226, Correct the argument order in the activateLockdownFromSnapshot call: pass undefined for allowLegacyHermesProtocol, cachedTarget as the next argument, undefined for cachedProtocol, and the existing recovery options as the sixth argument so they reach applyShieldsPolicySnapshot.Source: Linters/SAST tools
🧹 Nitpick comments (1)
src/lib/shields/flow.test.ts (1)
1014-1014: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace the positional
forkargument assertion.
timerArgs.at(9)couples this test to the internal timer argv layout. A harmless argument reorder will fail the test.Assert the observable timer behavior instead. Use the persisted marker or the recovery outcome. 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 `@src/lib/shields/flow.test.ts` at line 1014, Replace the positional timerArgs assertion in the relevant flow test with an assertion on observable behavior through the public boundary, using the persisted marker or recovery outcome. Remove dependence on the internal timer argv layout while preserving coverage that the timer flow handles the “openclaw” fork behavior correctly.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lib/shields/timer.test.ts`:
- Around line 471-473: Update the audit-ordering assertion in
invokeTimerAndCaptureExit’s test to avoid requiring distinct millisecond
timestamps; verify the failed audit precedes the success audit using the audit
collection’s write/order information, or otherwise use a non-strict comparison
that preserves the intended ordering.
In `@src/lib/state/mcp-lifecycle-lock/shields-timer-authority.ts`:
- Around line 70-79: Update readShieldsTimerMarker to validate that the parsed
marker’s sandboxName matches the requested sandboxName before returning it;
return null for mismatches so recovery cannot use another sandbox’s snapshot.
Add a test covering a marker file for one sandbox being rejected when read for a
different sandbox.
---
Outside diff comments:
In `@src/lib/shields/index.ts`:
- Around line 3213-3226: Correct the argument order in the
activateLockdownFromSnapshot call: pass undefined for allowLegacyHermesProtocol,
cachedTarget as the next argument, undefined for cachedProtocol, and the
existing recovery options as the sixth argument so they reach
applyShieldsPolicySnapshot.
---
Nitpick comments:
In `@src/lib/shields/flow.test.ts`:
- Line 1014: Replace the positional timerArgs assertion in the relevant flow
test with an assertion on observable behavior through the public boundary, using
the persisted marker or recovery outcome. Remove dependence on the internal
timer argv layout while preserving coverage that the timer flow handles the
“openclaw” fork behavior correctly.
🪄 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: d10eeb70-49b2-4c1b-ac14-6de3c81e7619
📒 Files selected for processing (11)
ci/source-architecture-budget.jsonci/source-shape-test-budget.jsondocs/reference/commands.mdxsrc/lib/shields/flow.test.tssrc/lib/shields/index.tssrc/lib/shields/policy-transition.test.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/shields-timer-authority.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- ci/source-shape-test-budget.json
- src/lib/shields/transition-lock.ts
- ci/source-architecture-budget.json
- docs/reference/commands.mdx
- src/lib/shields/timer.ts
| expect(Date.parse(successAudits[0].timestamp)).toBeGreaterThan( | ||
| Date.parse(failedAudit.timestamp), | ||
| ); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Make the audit-ordering assertion resistant to same-millisecond timestamps.
invokeTimerAndCaptureExit passes retryDelayMs: 1. The failed audit and the success audit can therefore both land in the same millisecond. ISO-8601 timestamps have millisecond resolution, so Date.parse returns equal values and toBeGreaterThan fails intermittently.
Assert the relative order instead, or relax the comparison.
🩹 Proposed fix using write order
- expect(Date.parse(successAudits[0].timestamp)).toBeGreaterThan(
- Date.parse(failedAudit.timestamp),
- );
+ expect(audits.indexOf(successAudits[0])).toBeGreaterThan(audits.indexOf(failedAudit));
+ expect(Date.parse(successAudits[0].timestamp)).toBeGreaterThanOrEqual(
+ Date.parse(failedAudit.timestamp),
+ );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| expect(Date.parse(successAudits[0].timestamp)).toBeGreaterThan( | |
| Date.parse(failedAudit.timestamp), | |
| ); | |
| expect(audits.indexOf(successAudits[0])).toBeGreaterThan( | |
| audits.indexOf(failedAudit), | |
| ); | |
| expect(Date.parse(successAudits[0].timestamp)).toBeGreaterThanOrEqual( | |
| Date.parse(failedAudit.timestamp), | |
| ); |
🤖 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 471 - 473, Update the
audit-ordering assertion in invokeTimerAndCaptureExit’s test to avoid requiring
distinct millisecond timestamps; verify the failed audit precedes the success
audit using the audit collection’s write/order information, or otherwise use a
non-strict comparison that preserves the intended ordering.
| export function readShieldsTimerMarker( | ||
| sandboxName: string, | ||
| stateDir = resolveNemoclawStateDir(), | ||
| ): ShieldsTimerMarker | null { | ||
| try { | ||
| return readShieldsTimerMarkerFile(shieldsTimerMarkerPath(sandboxName, stateDir)); | ||
| } catch { | ||
| return null; | ||
| } | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Reject a timer marker for another sandbox.
readShieldsTimerMarker returns a valid marker even when marker.sandboxName !== sandboxName. The expired inline recovery path then consumes marker.snapshotPath without an equivalent check.
If shields-timer-alpha.json contains a marker for beta, recovery for alpha can apply beta's snapshot. Return null unless the parsed marker sandbox name matches the requested sandbox. Add a mismatched-marker test.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/state/mcp-lifecycle-lock/shields-timer-authority.ts` around lines 70
- 79, Update readShieldsTimerMarker to validate that the parsed marker’s
sandboxName matches the requested sandboxName before returning it; return null
for mismatches so recovery cannot use another sandbox’s snapshot. Add a test
covering a marker file for one sandbox being rejected when read for a different
sandbox.
apurvvkumaria
left a comment
apurvvkumaria
left a comment
There was a problem hiding this comment.
Re-reviewed exact head ca34650. The blocking availability defect remains in src/lib/shields/timer.ts: restoreWhileDeadlineOwned still uses an unbounded for loop, and every persistent policy-restore or config re-lock failure returns retry while the marker remains valid. Because this loop runs inside withMcpLifecycleDeadlineFence, it can retain the sandbox lifecycle deadline indefinitely, make ordinary supported mutations time out, and append audit records without bound. Please cap attempts or wall-clock duration using the existing interactive recovery policy, then enter the established durable-containment terminal path and release the fence. Add a regression with a persistently failing restore that proves the timer terminates, the deadline file is released, and a waiting lifecycle mutation proceeds. No other current automated suggestions are blocking this review.
Stop detached restore retries after seven attempts. Preserve exact lifecycle authority through durable containment or retained gates. Sanitize MCP policy diagnostics and add race-focused regression coverage. Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
Count deadline setup, publication, and restoration against one bounded retry budget. On exhaustion, commit durable containment. If that commit fails, retain exact owned gates and return actionable recovery guidance. Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/lib/shields/flow.test.ts (1)
1392-1400: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBuild the injected failure with the production helper.
This block hand-writes the durable-containment error shape, including the
NEMOCLAW_DURABLE_CONTAINMENTcode string and theretainOwnedLifecycleGatesflag. The test then asserts the same contract at Line 1415. If production renames the code or changes the flag, this test keeps passing while the real containment path breaks.Call
durableMcpLifecycleContainmentFailurefromsrc/lib/state/mcp-lifecycle-lock-acquisition.tswith{ retainOwnedLifecycleGates: true }instead.As per path instructions: "Flag copied production algorithms, broad mocks that bypass the behavior under test".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/shields/flow.test.ts` around lines 1392 - 1400, Replace the hand-built error in the beginContainment mock with a call to the production helper durableMcpLifecycleContainmentFailure, passing { retainOwnedLifecycleGates: true }. Keep the injected failure behavior and existing assertions unchanged while ensuring the test uses the production error contract.Source: Path instructions
src/lib/shields/timer.test.ts (1)
138-151: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAssert the call count unconditionally.
policyApplicationsBeforeRevocationstaysundefinedwhenwaitForRetryBoundarythrows, so theifat Line 146 skips the assertion. The outer failure still propagates, so no defect hides here, but the guard adds a conditional that the test does not need. Capture the count before thetry, or assert inside thetryright after the capture. The test body then stays linear, which also matches the growth guardrail for test conditionals.♻️ Proposed restructure
const pending = runRestoreTimer(args, { retryDelayMs: 50 }); - let policyApplicationsBeforeRevocation: number | undefined; try { await waitForRetryBoundary(deadlinePath, auditPath); expect(exitSpy).not.toHaveBeenCalled(); expect(fs.existsSync(deadlinePath)).toBe(true); - policyApplicationsBeforeRevocation = - shieldsIndexMock.applyShieldsPolicySnapshot.mock.calls.length; + const policyApplicationsBeforeRevocation = + shieldsIndexMock.applyShieldsPolicySnapshot.mock.calls.length; + fs.rmSync(markerPath, { force: true }); + await pending; + expect(shieldsIndexMock.applyShieldsPolicySnapshot).toHaveBeenCalledTimes( + policyApplicationsBeforeRevocation, + ); } finally { - fs.rmSync(markerPath, { force: true }); - await pending; fs.writeFileSync(markerPath, markerContents); exitSpy.mockRestore(); } - if (policyApplicationsBeforeRevocation !== undefined) { - expect(shieldsIndexMock.applyShieldsPolicySnapshot).toHaveBeenCalledTimes( - policyApplicationsBeforeRevocation, - ); - } }As per path instructions: "conditionals that make a test pass without exercising its claim".
🤖 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 138 - 151, Make the call-count assertion unconditional in the test around the policyApplicationsBeforeRevocation capture. Move the capture before the try/finally or assert immediately after capturing it inside the try, then remove the if guard while preserving the cleanup and pending-retry handling.Source: Path instructions
src/lib/shields/index.ts (1)
312-313: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
committedContainmentPathfor all containment-path checks.Export the state helper and use it at lines 312, 973, and 1136. This keeps the security-relevant
.containmentpath definition 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/index.ts` around lines 312 - 313, Update the containment checks in the relevant lifecycle logic to use the exported state helper’s committedContainmentPath value instead of constructing paths directly with getMcpLifecycleLockPath and the .containment suffix. Apply this consistently at the checks near the current location and the corresponding locations around lines 973 and 1136, preserving their existing control flow.
🤖 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-recovery-budget.test.ts`:
- Around line 119-127: Remove the path-based conditional fault-injection logic
from the mockImplementation blocks in the timer-recovery budget tests. Add or
reuse a shared helper such as failFsCallForPath in test/helpers to encapsulate
matching the target path, tracking failure attempts, creating the configured
filesystem error, and delegating other calls; update the affected mkdir and
filesystem mock call sites to use that helper as single expressions while
preserving existing failure behavior.
---
Nitpick comments:
In `@src/lib/shields/flow.test.ts`:
- Around line 1392-1400: Replace the hand-built error in the beginContainment
mock with a call to the production helper durableMcpLifecycleContainmentFailure,
passing { retainOwnedLifecycleGates: true }. Keep the injected failure behavior
and existing assertions unchanged while ensuring the test uses the production
error contract.
In `@src/lib/shields/index.ts`:
- Around line 312-313: Update the containment checks in the relevant lifecycle
logic to use the exported state helper’s committedContainmentPath value instead
of constructing paths directly with getMcpLifecycleLockPath and the .containment
suffix. Apply this consistently at the checks near the current location and the
corresponding locations around lines 973 and 1136, preserving their existing
control flow.
In `@src/lib/shields/timer.test.ts`:
- Around line 138-151: Make the call-count assertion unconditional in the test
around the policyApplicationsBeforeRevocation capture. Move the capture before
the try/finally or assert immediately after capturing it inside the try, then
remove the if guard while preserving the cleanup and pending-retry handling.
🪄 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: 55d392ce-dd59-4a84-8c4b-bffdc23ad2b9
📒 Files selected for processing (13)
docs/manage-sandboxes/backup-restore.mdxdocs/manage-sandboxes/runtime-controls.mdxdocs/reference/commands.mdxsrc/lib/actions/sandbox/mcp-bridge-policy.tssrc/lib/actions/sandbox/snapshot-baseline-exclusion-output.test.tssrc/lib/shields/flow.test.tssrc/lib/shields/index.tssrc/lib/shields/mcp-policy-transition.test.tssrc/lib/shields/timer-recovery-budget.test.tssrc/lib/shields/timer.test.tssrc/lib/shields/timer.tssrc/lib/state/mcp-lifecycle-lock-acquisition.test.tssrc/lib/state/mcp-lifecycle-lock-acquisition.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- docs/manage-sandboxes/backup-restore.mdx
- docs/manage-sandboxes/runtime-controls.mdx
- src/lib/shields/mcp-policy-transition.test.ts
- src/lib/state/mcp-lifecycle-lock-acquisition.test.ts
- src/lib/actions/sandbox/mcp-bridge-policy.ts
- docs/reference/commands.mdx
- src/lib/shields/timer.ts
| vi.spyOn(fs.promises, "mkdir").mockImplementation(async (targetPath, options) => { | ||
| if (String(targetPath) === lifecycleDirectory && setupFailuresRemaining > 0) { | ||
| setupFailuresRemaining -= 1; | ||
| const error = new Error("simulated pre-fence setup failure") as NodeJS.ErrnoException; | ||
| error.code = "EIO"; | ||
| throw error; | ||
| } | ||
| return await originalMkdir(targetPath, options); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Remove the conditional fault injection from this test file to unblock CI.
The Codebase Growth Guardrails job fails on this file: "test file contains 4 if statements, up from 0." The four if statements are the path checks at Line 120, Line 156, Line 193, and Line 204. The guardrail counts them even though they sit inside mockImplementation bodies.
Move the path-matched fault injection into a shared test helper outside this file, so the conditional lives in helper code and the test bodies stay linear. A helper such as failFsCallForPath(target, method, errorCode) in test/helpers/ keeps every call site a single expression.
Also applies to: 155-165, 192-201, 203-212
🤖 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-recovery-budget.test.ts` around lines 119 - 127, Remove
the path-based conditional fault-injection logic from the mockImplementation
blocks in the timer-recovery budget tests. Add or reuse a shared helper such as
failFsCallForPath in test/helpers to encapsulate matching the target path,
tracking failure attempts, creating the configured filesystem error, and
delegating other calls; update the affected mkdir and filesystem mock call sites
to use that helper as single expressions while preserving existing failure
behavior.
Source: Pipeline failures
apurvvkumaria
left a comment
apurvvkumaria
left a comment
There was a problem hiding this comment.
Re-reviewed exact head f0f888b. The prior blocking unbounded-recovery defect is resolved: one seven-attempt budget now covers deadline-fence setup, main-generation publication, and restore/re-lock attempts. On exhaustion, durable containment becomes the terminal state and owned deadline/main gates are released when that containment is proven; if containment publication itself fails, the exact owned gates are retained with actionable recovery guidance. Exact-head focused evidence passed the new recovery-budget suite 5/5, lifecycle-lock acquisition 28/28, CLI build/typecheck, and 21/22 timer tests; the sole local timer failure was the sandboxed Darwin process-identity fixture, while the revision macOS check is green. The failed CLI shard is an unrelated managed-image-registry fetch timeout. The remaining growth-guardrail failure is an automated test-shape issue, not a product-blocking defect. No blocking correctness, security, regression, or compatibility issue found.
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>
<!-- markdownlint-disable MD041 --> ## Summary Prepares the canonical v0.0.102 release documentation from the current release-labeled scope. The change adds a dated changelog for all 38 user-facing shipping PRs and corrects the OpenClaw agent command reference for the behavior delivered by #8191. ## Changes - Add `docs/changelog/2026-08-04.mdx` with the v0.0.102 release summary, detailed behavior changes, support boundaries, security evidence links, and links to durable documentation. - Update `docs/reference/commands.mdx` to describe non-JSON OpenClaw output capture, its combined limit, marker handling, stream suppression, recovery guidance, and exit behavior. - [#8167](#8167) -> `docs/changelog/2026-08-04.mdx`: Records authenticated attachment of operator-managed llama.cpp servers. - [#8129](#8129) -> `docs/changelog/2026-08-04.mdx`: Records the Experimental managed vLLM profile for two DGX Spark systems. - [#7983](#7983) -> `docs/changelog/2026-08-04.mdx`: Records qualification of the May 2026 GB300WS factory image. - [#8207](#8207) -> `docs/changelog/2026-08-04.mdx`: Records the qualified DGX Station driver transaction. - [#8208](#8208) -> `docs/changelog/2026-08-04.mdx`: Records mode-bound Express resume state. - [#8158](#8158) -> `docs/changelog/2026-08-04.mdx`: Records recovery of host-global dual-Station runtime ownership. - [#8145](#8145) -> `docs/changelog/2026-08-04.mdx`: Records Windows-host Ollama validation from Docker Desktop's network context. - [#8190](#8190) -> `docs/changelog/2026-08-04.mdx`: Records HTTP model pulls when WSL has no local Ollama executable. - [#8195](#8195) -> `docs/changelog/2026-08-04.mdx`: Records reuse of a healthy installer-managed CLI. - [#8053](#8053) -> `docs/changelog/2026-08-04.mdx`: Records early rejection of incompatible OpenShell gateway versions. - [#8098](#8098) -> `docs/changelog/2026-08-04.mdx`: Records the bounded package-service-to-standalone gateway recovery transition. - [#8216](#8216) -> `docs/changelog/2026-08-04.mdx`: Records the final dashboard port selected during multi-sandbox onboarding. - [#8146](#8146) -> `docs/changelog/2026-08-04.mdx`: Records managed startup-state restoration for stopped sandboxes. - [#8092](#8092) -> `docs/changelog/2026-08-04.mdx`: Records gateway watchdog recovery for classified not-serving states. - [#8182](#8182) -> `docs/changelog/2026-08-04.mdx`: Records consistent managed-recovery wait configuration. - [#8040](#8040) -> `docs/changelog/2026-08-04.mdx`: Records Docker sandbox rollback authority through late validation. - [#8130](#8130) -> `docs/changelog/2026-08-04.mdx`: Records bounded Shields deadline recovery and durable containment. - [#8086](#8086) -> `docs/changelog/2026-08-04.mdx`: Records repair of narrowly validated permission-only configuration drift. - [#8122](#8122) -> `docs/changelog/2026-08-04.mdx`: Records prompt failure and guidance for corrupt transition locks. - [#8124](#8124) -> `docs/changelog/2026-08-04.mdx`: Records policy restoration flags, previews, and target revalidation. - [#7886](#7886) -> `docs/changelog/2026-08-04.mdx`: Records explicit destruction after pre-delete Shields hardening failures while preserving recovery authority. - [#7901](#7901) -> `docs/changelog/2026-08-04.mdx`: Records multi-port uninstall behavior and shared-resource preservation. - [#7984](#7984) -> `docs/changelog/2026-08-04.mdx`: Records one classified transient remote MCP startup retry. - [#7954](#7954) -> `docs/changelog/2026-08-04.mdx`: Records bounded hosted-inference probe replies. - [#7574](#7574) -> `docs/changelog/2026-08-04.mdx`: Records preservation of validated reasoning capabilities through onboarding. - [#8089](#8089) -> `docs/changelog/2026-08-04.mdx`: Records proxy routing for Hermes WhatsApp pairing and media traffic. - [#7682](#7682) -> `docs/changelog/2026-08-04.mdx`: Records native Hermes session deletion and identifier validation. - [#8150](#8150) -> `docs/changelog/2026-08-04.mdx`: Records corporate CA trust for LangChain Deep Agents Code image builds. - [#8156](#8156) -> `docs/changelog/2026-08-04.mdx`: Records reviewed managed runtime dependency remediation. - [#8180](#8180) -> `docs/changelog/2026-08-04.mdx`: Records reviewed MCP discovery runtime dependency updates. - [#8196](#8196) -> `docs/changelog/2026-08-04.mdx`: Records private npm dependency remediation across managed images. - [#8203](#8203) -> `docs/changelog/2026-08-04.mdx`: Records reviewed Hermes and LangChain Deep Agents Code Python dependency updates. - [#8125](#8125) -> `docs/changelog/2026-08-04.mdx`: Records bounded diagnostics for invalid enumerated CLI values. - [#8193](#8193) -> `docs/changelog/2026-08-04.mdx`: Records bounded diagnostics for unresolved sandbox base images. - [#8118](#8118) -> `docs/changelog/2026-08-04.mdx`: Records bounded diagnostics for changed gateway authority. - [#8191](#8191) -> `docs/changelog/2026-08-04.mdx`, `docs/reference/commands.mdx`: Records output capture, marker handling, recovery guidance, and exit behavior for non-JSON OpenClaw agent commands. - [#8187](#8187) -> `docs/changelog/2026-08-04.mdx`: Records the aligned interactive-installation start across supported agents. - [#8153](#8153) -> `docs/changelog/2026-08-04.mdx`: Records current product capabilities and support boundaries. ## Type of Change - [ ] Code change (feature, bug fix, or refactor) - [ ] Code change with doc updates - [x] 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: - [x] Tests not applicable — justification: This documentation-only release preparation does not change executable behavior. Existing changelog and published-route tests pass. - [x] Docs updated for user-facing behavior changes - [ ] Docs not applicable — justification: - [ ] 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: - [ ] 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: Independently reviewed `docs/changelog/2026-08-04.mdx` and `docs/reference/commands.mdx` at commit `b89913780`. All 38 user-facing v0.0.102 PRs are represented, #8191 behavior matches the implementation, and the writing rules, documentation style, controlled terminology, route structure, and skip policy pass review. Targeted tests pass 36/36 and the documentation build completes with 0 errors. - Agent: Codex Desktop independent documentation writer <!-- docs-review-head-sha: b899137 --> <!-- docs-review-agents-blob-sha: 3dd7c24 --> ## DGX Station Hardware Evidence - [ ] Tested on DGX Station - Tested commit: Not applicable - 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 — `npx vitest run --project integration test/changelog-docs.test.ts test/check-docs-published-routes.test.ts` passed 36/36. - [x] Applicable broad gate passed — not applicable to documentation-only changes; `npm run docs` completed successfully with 0 errors. - [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) — completed with 0 errors and 2 existing Fern warnings. - [x] Doc pages follow the [style guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md) (doc changes only) - [x] New doc pages include SPDX header and frontmatter (new pages only) — the native dated changelog uses the required parser-safe MDX SPDX comment and intentionally has no frontmatter. --- Signed-off-by: Apurv Kumaria <akumaria@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Documentation** - Added release notes for v0.0.102, covering authentication, hardware setup, WSL, installer recovery, sandbox resilience, policy management, inference reliability, CLI improvements, and unified quickstarts. - Updated command documentation to explain how non-JSON agent output is collected, replayed, and reported. - **Bug Fixes** - Improved command-output recovery guidance when output exceeds limits or contains unsupported fallback markers. - Preserved accurate command exit-status reporting after output processing. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
<!-- markdownlint-disable MD041 --> ## Summary Shields down previously replaced the complete live OpenShell policy, which removed NemoClaw-generated MCP entries and made a surviving Hermes MCP server unreachable. This change reconciles only exact, independently proven NemoClaw-managed MCP entries so server A stays reachable while a removed server B stays removed. This is the focused MCP-policy change built on the lifecycle and deadline prerequisite merged in #8130. It replaces the focused behavior from the closed historical work in #7980 and #8141. ## Related Issue Fixes #7952 ## Changes - Prove managed MCP ownership from the registry, generated-policy record, and live gateway policy before preserving an entry. - Save the managed-key manifest with the Shields snapshot, remove snapshot-time managed entries during restoration, and overlay only the current exact entries. - Fail closed for ambiguous, malformed, or manually edited ownership; deadline restoration omits unproven entries and records the omission count. - Preserve exact current managed entries in the permissive Shields policy without copying unrelated live egress. - Sanitize untrusted registry and policy identifiers before including them in operator diagnostics. - Clean staged policy files across success and failure paths. - Reuse the validated unchanged snapshot when both saved and current managed MCP sets are empty, so deadline restoration does not depend on temporary staging. - Update the Hermes MCP regression so it calls A immediately after Shields down and again after B removal, before the later explicit `mcp restart A` coverage. - Document managed MCP policy behavior during Shields transitions. ### Why this appeared during the Hermes upgrade The original live journey contained a hidden lifecycle between the first successful call to A and the later B lifecycle: 1. Raise Shields. 2. Restart the Hermes gateway. 3. Lower Shields. 4. Exercise config rollback. 5. Add and remove B. 6. Call A. 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 removal was only where the test detected the damage; B removal was a misleading correlation. This surfaced alongside the Hermes upgrade because coverage and upgrade fixes landed close together: - #7761 added the helper containing Shields up, gateway restart, Shields down, and rollback, but did not run the complete live E2E. - #7771 upgraded Hermes, while its selected E2E did not include the MCP bridge target. - #7849 fixed Hermes 0.19 migrations and the `mcp__fake__*` tool naming, allowing the journey to progress far enough to expose the later failure. - #7866 moved an explicit restart of A before the post-removal call, which reapplied A's policy and masked the defect. The whole-policy Shields replacement predates those changes. This is a latent NemoClaw Shields policy-composition bug exposed by expanded Hermes upgrade regression coverage, not evidence of a Hermes regression. ### Corrected live regression order 1. Raise Shields and restart the Hermes gateway. 2. Lower Shields and call A immediately. 3. Exercise config rollback. 4. Add B, prove DNS-rebinding access is denied, remove B, and verify A's policy is unchanged while B is gone. 5. Call A before the later explicit restart. 6. Capture authenticated rediscovery state, restart A without resupplying its secret, and call A again. ## 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 maintainer security review of commit `5d46d4a2ee924edf743ac36d04807948eec01c96` passed all nine categories. The review covered managed-policy ownership, diagnostic sanitization, deadline restoration, empty-MCP staging failure, and current-main integration. - [ ] 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 the complete PR change at commit `5d46d4a2ee924edf743ac36d04807948eec01c96` against current main, including generated OpenClaw and Hermes variants, operator-facing assertions, test titles, and the empty-MCP deadline-restore regression. The current-main merge was mechanical, contributor attribution remains intact, and the existing documentation remains accurate. Normal hooks passed; GitHub CI is the current validation authority. - Agent: Codex Desktop <!-- docs-review-head-sha: 5d46d4a --> <!-- 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 - [ ] Targeted behavior tests pass for the current change set, or tests are marked not applicable above — command/result or justification: GitHub CI is running for commit `5d46d4a2ee924edf743ac36d04807948eec01c96`. - [ ] Applicable broad gate passed — `npm test` for broad runtime/test-harness changes; `npm run check` for repo-wide validation/coverage changes — command/result: GitHub CI is running for commit `5d46d4a2ee924edf743ac36d04807948eec01c96`. - [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) — GitHub CI is running for commit `5d46d4a2ee924edf743ac36d04807948eec01c96`. - [x] Doc pages follow the [style guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md) (doc changes only) - [x] New doc pages include SPDX header and frontmatter (new pages only) — no new pages --- Signed-off-by: Julie Yaunches <jyaunches@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Shields transitions now securely track and reconcile managed MCP policies. * Existing MCP servers retain verified endpoints and address pins during unlock and restoration. * Removed MCP servers remain removed instead of being unintentionally restored. * Automatic restoration omits policies that cannot be independently verified and records clear warnings. * **Bug Fixes** * Improved fail-closed behavior for malformed, mismatched, incomplete, or unavailable policy data. * **Documentation** * Expanded guidance on MCP policy handling during manual and automatic Shields transitions. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Julie Yaunches <jyaunches@nvidia.com> Signed-off-by: Carlos Villela <cvillela@nvidia.com> Co-authored-by: Carlos Villela <cvillela@nvidia.com>
3 participants
Summary
Before this change, an expired Shields auto-restore timer could signal a process that still held the sandbox lifecycle lock. Deadline recovery now blocks new mutations, waits for the recorded owner to release its exact lock generation without signaling it, and enters durable containment when safe recovery cannot be proven.
This is the lifecycle/deadline prerequisite for #7952. It does not add NemoClaw-managed MCP network policy composition; that focused fix remains a separate stacked PR.
Related Issue
Part of #7952.
Changes
restoreAt, wait for verified live owners without consuming the recovery budget, bound recovery to seven attempts, and retain the owning gates after a durable-containment failure.SIGSTOPorSIGKILL.Full E2E run
30775326819on main candidate98a0e0c5d6bb982ad4d0311ce023670acd2a4dfdexposed the deadline race inshields-config:shields down --timeout 10swas still changing policy and lock state when auto-restore reachedrestoreAt, and the process receivedSIGKILL. The same candidate passed scheduled run30775345807, so the failure was timing-dependent. This PR fixes ownership and recovery instead of increasing the E2E timeout.Type of Change
Quality Gates
Documentation Writer Review
docs-updateddocs/manage-sandboxes/backup-restore.mdx;docs/manage-sandboxes/runtime-controls.mdx;docs/reference/commands.mdx. Exact-headnpm run docspassed with 0 errors and 2 pre-existing hidden-page warnings. Reviewed lifecycle deadline ownership, bounded interactive and detached recovery, durable containment, exact-generation operator guidance, the complete backup transaction, cooperative timer cancellation, startup-access recovery, operator-facing diagnostics, comments, test titles, and rendered OpenClaw, Hermes, and Deep Agents variants. Confirmed this prerequisite makes no managed MCP policy-preservation or composition claim. The exact-head refresh changed only test linearization, one test timeout, and Atomics mock typing; it changed no explanatory text or runtime behavior.DGX Station Hardware Evidence
scripts/prepare-dgx-station-host.shis unchanged.Verification
Signed-off-by:line and every commit appears asVerifiedin GitHub — all 16 current PR commits reportverified: truewith reasonvalid.pre-commit,commit-msg, andpre-pushhooks passed, ornpm run validate:prpassed after refreshingorigin/mainwhen hooks were skipped or unavailable — normal hooks passed for the current change set; the final pre-push CLI TypeScript and tag-sync checks passed.npm run docsbuilds without warnings (doc changes only) — exact-head documentation review found 0 errors and 2 pre-existing hidden-page warnings.Signed-off-by: Julie Yaunches jyaunches@nvidia.com