fix(security): harden trusted-private endpoint lifecycle - #8494
fix(security): harden trusted-private endpoint lifecycle#8494jyaunches wants to merge 10 commits into
Conversation
jyaunches
added
area: docs
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughChangesThis change adds durable custom-policy transition journals, scoped Shields mutation authority, broader lifecycle guards, and stricter endpoint validation. MCP and trusted-private flows now use host-bound capabilities and complete canonical address sets. Documentation and tests cover recovery, ownership, and blocked operations. Policy and endpoint safety
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (15)
src/lib/policy/custom-policy-transition.test.ts (1)
230-249: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMake the
runtest double fail loudly for unexpected commands.
args.indexOf("--policy")returns-1when a call is not apolicy setinvocation. The mock then readsargs[0]as a file path and throws an unrelatedENOENT. Assert the flag exists so an unexpected command produces a clear failure.The static analysis path-traversal hint on Line 232 is a false positive here. The path comes from the code under test, not from external input.
♻️ Proposed guard for the policy path lookup
mocks.run.mockImplementation((args: string[]) => { state.setCalls += 1; - const policyPath = args[args.indexOf("--policy") + 1]; + const policyFlagIndex = args.indexOf("--policy"); + if (policyFlagIndex < 0) throw new Error(`unexpected openshell call: ${args.join(" ")}`); + const policyPath = args[policyFlagIndex + 1]; const desiredPolicy = fs.readFileSync(policyPath, "utf8");🤖 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/policy/custom-policy-transition.test.ts` around lines 230 - 249, Update the mocks.run test double to assert that args contains "--policy" before deriving policyPath, so unexpected commands fail with a clear assertion instead of an unrelated file-read error. Keep the existing policy-path lookup and state behavior unchanged; treat the static path-traversal warning as a false positive for this test-controlled path.Source: Linters/SAST tools
test/package-contract/cli/policy-dispatch.test.ts (1)
185-188: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAttach a rejection handler to
mainPromise.The script now evaluates
require(CLI_PATH).mainPromisewithout a handler. If the promise rejects instead of callingprocess.exit, Node 22 terminates the process on the unhandled rejection and prints a stack trace to stderr. Theexitlistener still writes__CALLS__, so the test passes, but the diagnostic output becomes noisy and the real failure reason is harder to read. Add acatchthat keeps the observed exit behavior.♻️ Proposed handler
process.on("exit", () => { process.stdout.write("\n__CALLS__" + JSON.stringify(calls)); }); -require(${CLI_PATH}).mainPromise; +Promise.resolve(require(${CLI_PATH}).mainPromise).catch(() => { + process.exitCode = process.exitCode || 1; +});🤖 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/package-contract/cli/policy-dispatch.test.ts` around lines 185 - 188, Attach a rejection handler to the CLI module’s mainPromise evaluation so rejected promises are caught without producing an unhandled-rejection stack trace. Preserve the existing process.exit behavior and __CALLS__ output from the process.on("exit") listener.test/policies.test.ts (1)
745-763: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the fake
openshellpropagate policy-file failures.
"$4"is the correct policy file for the currentpolicy set --policy <file> --wait <sandbox>arguments. However,cpfailures still lead toexit 0. Parse the value after--policy, reject missing files, and return non-zero whencpfails.🤖 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/policies.test.ts` around lines 745 - 763, Update the fake openshell script in the policy test setup to parse the policy file argument following --policy, reject a missing argument or nonexistent file, and propagate cp failure with a non-zero exit status in the policy set branch; preserve successful policy get and policy set behavior.src/lib/policy/index.ts (2)
112-119: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument why
assertInternalShieldsPolicyMutationAlloweduses a runtimerequire.Line 30 already imports the type from
../shields/transition-lock, but this helper resolves the module throughrequireon every call. The likely reason is the import cycle withsrc/lib/shields/index.ts, which imports this module. Add a short comment that states the cycle. A future reader can otherwise "clean up" this call into a static import and reintroduce the cycle.♻️ Proposed comment
function assertInternalShieldsPolicyMutationAllowed( sandboxName: string, authority?: ShieldsPolicyMutationAuthority, ): void { + // Resolved lazily: `../shields/transition-lock` is reachable from + // `../shields/index.ts`, which imports this module. A static import would + // close that cycle at module-init time. const { assertShieldsPolicyMutationAllowed } = require("../shields/transition-lock") as typeof import("../shields/transition-lock"); assertShieldsPolicyMutationAllowed(sandboxName, authority); }🤖 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/policy/index.ts` around lines 112 - 119, Add a brief comment immediately above the runtime require in assertInternalShieldsPolicyMutationAllowed explaining that it avoids the import cycle between this policy module and src/lib/shields/index.ts. Keep the existing type-only import and runtime behavior unchanged.
777-781: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
removeCustomPresetnever readsoptions.The transactional path always pushes with
nonFatal: trueat Line 870, so a caller value ofnonFatal: falseis discarded. If that is intentional, prefix the parameter with_as the coding guidelines require, and state that the journal settlement owns fatality.♻️ Proposed fix
function removeCustomPreset( sandboxName: string, presetName: string, - options: { nonFatal?: boolean }, + // Fatality is owned by journal settlement: the push is always non-fatal so + // the durable transition can be committed or rolled back. + _options: { nonFatal?: boolean }, ): boolean {As per coding guidelines: "Prefix intentionally unused variables with
_".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/policy/index.ts` around lines 777 - 781, Update the removeCustomPreset function parameter to indicate it is intentionally unused by renaming options to _options, since journal settlement always owns fatality with nonFatal: true. Preserve the existing transactional behavior.Source: Coding guidelines
src/lib/actions/sandbox/snapshot-restore-lifecycle.test.ts (1)
781-781: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep a failure-reason assertion on the cross-sandbox clone test.
{ exitCode: 1 }alone no longer identifies the cause. This PR adds several new guards that also exit with code 1, so this test can now pass for an unrelated reason. Assert the error message or the emitted stderr in addition to the exit code.As per path instructions: "Flag copied production algorithms, broad mocks that bypass the behavior under test, and 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/actions/sandbox/snapshot-restore-lifecycle.test.ts` at line 781, Strengthen the rejection assertion in the cross-sandbox clone test by retaining the exitCode check and also matching the specific expected failure message or emitted stderr. Ensure the assertion uniquely verifies the clone failure path rather than any unrelated guard that exits with code 1.Source: Path instructions
src/lib/shields/policy-mutation-authority.test.ts (1)
100-114: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd a case for the corrupt-state rejection.
assertShieldsPolicyMutationAllowedhas a third fail-closed branch: corrupt persisted state throws "while its Shields state is corrupt". No test covers it, so a regression that treats unparsable state as{}would pass. Write invalid JSON to the state file and assert the corrupt message, including with an authority present.💚 Proposed test
+ it("blocks policy mutation while persisted Shields state is corrupt (`#8176`)", () => { + fs.writeFileSync( + path.join(resolveNemoclawStateDir(), `shields-${sandboxName}.json`), + "{ not json", + { mode: 0o600 }, + ); + + expect(() => assertShieldsPolicyMutationAllowed(sandboxName)).toThrow(/state is corrupt/); + expect(() => + assertShieldsPolicyMutationAllowed( + sandboxName, + issueRebuildPolicyMutationAuthority(sandboxName), + ), + ).toThrow(/state is corrupt/); + });As per path instructions: "Require negative-path tests that prove the boundary rejects bypasses and does not leak secrets in errors, logs, state, or process arguments."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/shields/policy-mutation-authority.test.ts` around lines 100 - 114, Add a test beside the existing timer-recovery case that writes invalid JSON to the Shields state file, then calls assertShieldsPolicyMutationAllowed and asserts it throws “while its Shields state is corrupt.” Include a valid authority from issueRebuildPolicyMutationAuthority to verify the corrupt-state rejection cannot be bypassed by presenting authority.Source: Path instructions
src/lib/shields/transition-lock.ts (1)
294-306: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winEnforce the recorded purpose, not only the sandbox name.
issuePolicyMutationAuthoritystorespurpose, but the validator accepts either value. A managed-MCP authority therefore authorizes a rebuild-scoped policy mutation, and a rebuild authority authorizes managed-MCP key mutation. Take the expected purpose as a parameter so each consumer accepts only its own scope.🔒️ Proposed change
export function isShieldsPolicyMutationAuthority( sandboxName: string, authority: unknown, + expectedPurpose?: PolicyMutationAuthorityPurpose, ): authority is ShieldsPolicyMutationAuthority { if ((typeof authority !== "object" && typeof authority !== "function") || authority === null) { return false; } const binding = policyMutationAuthorityBindings.get(authority); - return ( - binding?.sandboxName === sandboxName && - (binding.purpose === "managed-mcp" || binding.purpose === "rebuild") - ); + if (binding?.sandboxName !== sandboxName) return false; + if (expectedPurpose !== undefined) return binding.purpose === expectedPurpose; + return binding.purpose === "managed-mcp" || binding.purpose === "rebuild"; }
assertShieldsPolicyMutationAllowedcan then forward an expected purpose from its callers.As per path instructions: "Preserve deny-by-default behavior, least privilege, redaction, and fail-closed handling."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/shields/transition-lock.ts` around lines 294 - 306, Update isShieldsPolicyMutationAuthority to accept an expected purpose parameter and require the recorded binding.purpose to exactly match it alongside sandboxName. Update assertShieldsPolicyMutationAllowed and every caller to forward the appropriate scope ("managed-mcp" or "rebuild"), preserving deny-by-default and fail-closed behavior.Source: Path instructions
src/lib/actions/sandbox/rebuild-shields-finally.test.ts (1)
88-91: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the redundant
afterEachrestore hook.Vitest files under
src/run in thecliproject, which enablesrestoreMocks. Spies created withvi.spyOnare therefore restored between tests without an explicit hook. Delete theafterEachblock and theafterEachimport at line 4.Based on learnings: Vitest test files under src are executed by the
cliVitest project, which enablesclearMocks,restoreMocks,unstubEnvs, andunstubGlobals; suite-level teardown should only clean up resources Vitest does not manage.🤖 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/rebuild-shields-finally.test.ts` around lines 88 - 91, Remove the redundant afterEach restore hook and its import from the test file; rely on the cli Vitest project's restoreMocks configuration to restore vi.spyOn mocks between tests.Source: Learnings
src/lib/actions/maintenance.test.ts (1)
340-379: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove the
readybranch intoit.eachinstead of an inner loop.The inner
for (const ready of [true, false])loop runs two independent scenarios inside one test. If the second iteration fails, Vitest reports a single failure and the output does not say which readiness branch broke. The manualvi.clearAllMocks()between iterations also exists only to re-create per-test isolation that Vitest already provides for thecliproject.Add
readyas a thirdit.eachtuple element, then delete the loop and the in-loopvi.clearAllMocks()call.🤖 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 340 - 379, Update the parameterized test containing “blocks a pending %s transition…” to add ready as a third it.each tuple value, then remove the inner for loop and its vi.clearAllMocks call. Keep the existing scenario setup and assertions, using the parameterized ready value directly so each readiness branch runs as an isolated test case.src/lib/actions/sandbox/policy-channel.ts (1)
1243-1273: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract one formatter for the pending-transition repair message. Five sites rebuild the same operation-specific retry string inline. The
removebranch yields a runnable command and theapplybranch yields the phrasepolicy add with --from-file or --from-dir. Two sites then wrap that phrase in backticks, so the output presents a phrase as a copy-pasteable command. One shared helper that returns both the message and a flag for whether the retry is a command fixes the rendering defect and removes the duplication.
src/lib/actions/sandbox/policy-channel.ts#L1243-L1273: replace the inlineretryconstruction inassertChannelPolicyMutationAllowedwith a call to the shared formatter, and keep the existingCannot ${operation} ...prefix.src/lib/actions/sandbox/policy-channel.ts#L504-L517: use the shared formatter inlistSandboxPoliciesand stop wrapping theapplyretry text in backticks at line 515.src/lib/actions/maintenance.ts#L73-L92: use the shared formatter inassertPolicyTransitionsSettledBeforeBackupinstead of the localretryternary.src/lib/actions/sandbox/status-text.ts#L338-L350: use the shared formatter inprintCustomPolicyRepairand stop wrapping theapplyretry text in backticks at line 348.src/lib/onboard/sandbox-registration.ts#L113-L123: use the shared formatter inbaselineExclusionsForCreate, which also aligns the quoting with the baseline branch below it.🤖 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/policy-channel.ts` around lines 1243 - 1273, Extract a shared pending-transition repair formatter that returns the retry text plus whether it is a runnable command. In src/lib/actions/sandbox/policy-channel.ts lines 1243-1273, use it in assertChannelPolicyMutationAllowed; in src/lib/actions/sandbox/policy-channel.ts lines 504-517, use it in listSandboxPolicies and remove backticks around the apply phrase; in src/lib/actions/maintenance.ts lines 73-92, use it in assertPolicyTransitionsSettledBeforeBackup; in src/lib/actions/sandbox/status-text.ts lines 338-350, use it in printCustomPolicyRepair and remove backticks around the apply phrase; and in src/lib/onboard/sandbox-registration.ts lines 113-123, use it in baselineExclusionsForCreate. Preserve existing message prefixes and quote only retry text identified as a command.src/lib/state/registry-normalization.ts (1)
110-115: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConfirm the asymmetric name patterns are intentional.
applyusesCUSTOM_POLICY_NAME_PATTERN(max 63 chars) andremoveusesLEGACY_CUSTOM_POLICY_NAME_PATTERN(unbounded length). A journal for a legacy long-named policy therefore normalizes forremovebut never forapply. That looks deliberate so legacy records stay removable, but it is not stated in the code. Add a short comment that records the intent, so a later refactor does not unify the two patterns and strand legacy removals.🤖 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/registry-normalization.ts` around lines 110 - 115, Document the intentional asymmetry in the nameIsValid conditional: apply must continue using CUSTOM_POLICY_NAME_PATTERN, while remove uses LEGACY_CUSTOM_POLICY_NAME_PATTERN so legacy long-named records remain removable. Add a brief comment near this logic warning against unifying the patterns.src/lib/actions/sandbox/mcp-bridge-policy.ts (1)
614-619: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the unreachable empty-pin check after
assertMcpBridgePolicyTarget.
assertMcpBridgePolicyTargetalready throws whentarget.addresses.length === 0(line 721), and it selects the correcttrusted-privateorpublicwording. The follow-up check at lines 615-619 can never run, and its message always says "public address pins", which would be wrong for a trusted-private entry.♻️ Proposed simplification
const resolvedAddresses = assertMcpBridgePolicyTarget(entry, target); - if (resolvedAddresses.length === 0) { - throw new McpBridgeError( - `Refusing to apply generated MCP policy '${entry.policyName}' without exact public address pins.`, - ); - } const adapter = isAgentMcpAdapter(entry.adapter) ? entry.adapter : "mcporter";
resolvedAddressesis otherwise unused in this function, so the local can also be dropped in favor of a bare call.🤖 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 614 - 619, Remove the unreachable empty-result check and unused resolvedAddresses local in the function applying the MCP bridge policy. Replace the assignment with a bare call to assertMcpBridgePolicyTarget(entry, target), preserving that helper’s existing validation and trusted-private/public-specific error wording.src/lib/actions/sandbox/mcp-bridge-url-validation.ts (1)
417-422: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMake the unparseable-URL path explicit.
The
catchat line 420 skips the trusted-private host binding check and delegates tonormalizeMcpServerUrl. That call re-parsesrawUrland throwsInvalid MCP server URL, so the behavior is still fail-closed. The control flow reads as if the host binding can be bypassed. State the intent, or throw directly.♻️ Proposed clarification
let rawParsed: URL; try { rawParsed = new URL(rawUrl); } catch { + // An unparseable URL cannot be host-bound here. `normalizeMcpServerUrl` + // re-parses it and rejects it with the canonical syntax error. return new URL(normalizeMcpServerUrl(rawUrl, { trustedPrivateHosts: [trustedPrivateHost] })); }🤖 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-url-validation.ts` around lines 417 - 422, Update the catch path around rawParsed in the URL validation flow to make its fail-closed behavior explicit: either throw the intended invalid MCP server URL error directly or clearly document that normalizeMcpServerUrl re-parses and rejects the unparseable input. Preserve the trusted-private host binding validation for parseable URLs and do not allow this path to proceed.test/hermes-mcp-private-target-validation.test.ts (1)
54-63: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExercise the public
addentrypoint.Drive the table through
execute("add", ...)ormain, while isolating filesystem and gateway side effects. Direct_validate_payloadcalls can pass while the publicaddpath no longer performs validation.🤖 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/hermes-mcp-private-target-validation.test.ts` around lines 54 - 63, Update the validation test to invoke the public add entrypoint via execute("add", ...) or main instead of calling module._validate_payload directly. Isolate filesystem and gateway side effects with the existing test seams, while preserving the accepted and rejected URL assertions and expected ValueError handling.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lib/actions/maintenance.ts`:
- Line 141: Update backupRegisteredSandbox to catch unsettledPolicyTransition
alongside mutationLockError, print the existing repair guidance, increment
failed, and return the per-sandbox failure result so backupAll continues
processing later sandboxes. Update the parameterized test in maintenance.test.ts
to expect a failed outcome rather than a rejected promise.
In `@src/lib/actions/sandbox/mcp-bridge-destroy-preflight.ts`:
- Around line 92-96: Update the providerless-add cleanup flow around
removeCustomPolicyByName and setBridgeState so both mutations execute within a
single state-layer transaction and are persisted atomically, preserving the
existing McpBridgeError behavior on failure. Add a test covering a
transaction/save failure between these operations and verify neither mutation is
partially persisted.
In `@src/lib/actions/sandbox/policy-channel.ts`:
- Around line 198-211: Update the pending-repair handling around
repairPendingCustomPolicyApply so a completed repair only returns when it
corresponds to the current requested preset; otherwise continue processing the
requested --from-file or --from-dir operation (or block and report the
mismatch). Preserve the existing blocked behavior, including the
built-in-request path, and do not exit successfully before applying the current
request.
In `@src/lib/security/trusted-private-endpoint.ts`:
- Around line 213-243: Update MCP lifecycle validation in
replayTrustedPrivateEndpoint() consumers, especially the MCP bridge inspection
flow, to accept public recorded pins when the complete validated set includes at
least one operator-trustable private pin. Remove any per-address rejection based
solely on isOperatorTrustablePrivateIp(), rely on replayTrustedPrivateEndpoint()
for set validation, and add lifecycle coverage for recovery or restart with a
persisted mixed pin set.
In `@src/lib/state/registry-normalization.test.ts`:
- Around line 289-304: Update the “missing previous” and “missing desired” cases
in the normalizeCustomPolicyTransition test data to remove those properties
rather than overriding them with undefined, so Object.prototype.hasOwnProperty
checks fail. Keep the incomplete-journal assertion focused on the presence-gate
error message and ensure both cases exercise the missing-property path.
---
Nitpick comments:
In `@src/lib/actions/maintenance.test.ts`:
- Around line 340-379: Update the parameterized test containing “blocks a
pending %s transition…” to add ready as a third it.each tuple value, then remove
the inner for loop and its vi.clearAllMocks call. Keep the existing scenario
setup and assertions, using the parameterized ready value directly so each
readiness branch runs as an isolated test case.
In `@src/lib/actions/sandbox/mcp-bridge-policy.ts`:
- Around line 614-619: Remove the unreachable empty-result check and unused
resolvedAddresses local in the function applying the MCP bridge policy. Replace
the assignment with a bare call to assertMcpBridgePolicyTarget(entry, target),
preserving that helper’s existing validation and trusted-private/public-specific
error wording.
In `@src/lib/actions/sandbox/mcp-bridge-url-validation.ts`:
- Around line 417-422: Update the catch path around rawParsed in the URL
validation flow to make its fail-closed behavior explicit: either throw the
intended invalid MCP server URL error directly or clearly document that
normalizeMcpServerUrl re-parses and rejects the unparseable input. Preserve the
trusted-private host binding validation for parseable URLs and do not allow this
path to proceed.
In `@src/lib/actions/sandbox/policy-channel.ts`:
- Around line 1243-1273: Extract a shared pending-transition repair formatter
that returns the retry text plus whether it is a runnable command. In
src/lib/actions/sandbox/policy-channel.ts lines 1243-1273, use it in
assertChannelPolicyMutationAllowed; in src/lib/actions/sandbox/policy-channel.ts
lines 504-517, use it in listSandboxPolicies and remove backticks around the
apply phrase; in src/lib/actions/maintenance.ts lines 73-92, use it in
assertPolicyTransitionsSettledBeforeBackup; in
src/lib/actions/sandbox/status-text.ts lines 338-350, use it in
printCustomPolicyRepair and remove backticks around the apply phrase; and in
src/lib/onboard/sandbox-registration.ts lines 113-123, use it in
baselineExclusionsForCreate. Preserve existing message prefixes and quote only
retry text identified as a command.
In `@src/lib/actions/sandbox/rebuild-shields-finally.test.ts`:
- Around line 88-91: Remove the redundant afterEach restore hook and its import
from the test file; rely on the cli Vitest project's restoreMocks configuration
to restore vi.spyOn mocks between tests.
In `@src/lib/actions/sandbox/snapshot-restore-lifecycle.test.ts`:
- Line 781: Strengthen the rejection assertion in the cross-sandbox clone test
by retaining the exitCode check and also matching the specific expected failure
message or emitted stderr. Ensure the assertion uniquely verifies the clone
failure path rather than any unrelated guard that exits with code 1.
In `@src/lib/policy/custom-policy-transition.test.ts`:
- Around line 230-249: Update the mocks.run test double to assert that args
contains "--policy" before deriving policyPath, so unexpected commands fail with
a clear assertion instead of an unrelated file-read error. Keep the existing
policy-path lookup and state behavior unchanged; treat the static path-traversal
warning as a false positive for this test-controlled path.
In `@src/lib/policy/index.ts`:
- Around line 112-119: Add a brief comment immediately above the runtime require
in assertInternalShieldsPolicyMutationAllowed explaining that it avoids the
import cycle between this policy module and src/lib/shields/index.ts. Keep the
existing type-only import and runtime behavior unchanged.
- Around line 777-781: Update the removeCustomPreset function parameter to
indicate it is intentionally unused by renaming options to _options, since
journal settlement always owns fatality with nonFatal: true. Preserve the
existing transactional behavior.
In `@src/lib/shields/policy-mutation-authority.test.ts`:
- Around line 100-114: Add a test beside the existing timer-recovery case that
writes invalid JSON to the Shields state file, then calls
assertShieldsPolicyMutationAllowed and asserts it throws “while its Shields
state is corrupt.” Include a valid authority from
issueRebuildPolicyMutationAuthority to verify the corrupt-state rejection cannot
be bypassed by presenting authority.
In `@src/lib/shields/transition-lock.ts`:
- Around line 294-306: Update isShieldsPolicyMutationAuthority to accept an
expected purpose parameter and require the recorded binding.purpose to exactly
match it alongside sandboxName. Update assertShieldsPolicyMutationAllowed and
every caller to forward the appropriate scope ("managed-mcp" or "rebuild"),
preserving deny-by-default and fail-closed behavior.
In `@src/lib/state/registry-normalization.ts`:
- Around line 110-115: Document the intentional asymmetry in the nameIsValid
conditional: apply must continue using CUSTOM_POLICY_NAME_PATTERN, while remove
uses LEGACY_CUSTOM_POLICY_NAME_PATTERN so legacy long-named records remain
removable. Add a brief comment near this logic warning against unifying the
patterns.
In `@test/hermes-mcp-private-target-validation.test.ts`:
- Around line 54-63: Update the validation test to invoke the public add
entrypoint via execute("add", ...) or main instead of calling
module._validate_payload directly. Isolate filesystem and gateway side effects
with the existing test seams, while preserving the accepted and rejected URL
assertions and expected ValueError handling.
In `@test/package-contract/cli/policy-dispatch.test.ts`:
- Around line 185-188: Attach a rejection handler to the CLI module’s
mainPromise evaluation so rejected promises are caught without producing an
unhandled-rejection stack trace. Preserve the existing process.exit behavior and
__CALLS__ output from the process.on("exit") listener.
In `@test/policies.test.ts`:
- Around line 745-763: Update the fake openshell script in the policy test setup
to parse the policy file argument following --policy, reject a missing argument
or nonexistent file, and propagate cp failure with a non-zero exit status in the
policy set branch; preserve successful policy get and policy set behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: dca2875e-17ee-4a4c-8bc4-28b284e3d1af
📒 Files selected for processing (94)
agents/hermes/mcp-config-transaction.pyagents/langchain-deepagents-code/managed-dcode-runtime.pydocs/inference/custom-endpoint-security.mdxdocs/inference/set-up-openai-compatible-endpoint.mdxdocs/manage-sandboxes/add-mcp-server.mdxdocs/network-policy/create-custom-policy-presets.mdxdocs/reference/commands.mdxdocs/reference/network-policies.mdxsrc/lib/actions/maintenance.test.tssrc/lib/actions/maintenance.tssrc/lib/actions/sandbox/destroy-flow.test.tssrc/lib/actions/sandbox/destroy-preflight.tssrc/lib/actions/sandbox/doctor-flow.test.tssrc/lib/actions/sandbox/doctor.tssrc/lib/actions/sandbox/mcp-bridge-add-restart.tssrc/lib/actions/sandbox/mcp-bridge-contracts.tssrc/lib/actions/sandbox/mcp-bridge-destroy-preflight.tssrc/lib/actions/sandbox/mcp-bridge-destroy.tssrc/lib/actions/sandbox/mcp-bridge-input-targets.test.tssrc/lib/actions/sandbox/mcp-bridge-policy-render.tssrc/lib/actions/sandbox/mcp-bridge-policy.test.tssrc/lib/actions/sandbox/mcp-bridge-policy.tssrc/lib/actions/sandbox/mcp-bridge-private-lifecycle.test.tssrc/lib/actions/sandbox/mcp-bridge-provider-inspection.tssrc/lib/actions/sandbox/mcp-bridge-remove.tssrc/lib/actions/sandbox/mcp-bridge-restart.tssrc/lib/actions/sandbox/mcp-bridge-state.tssrc/lib/actions/sandbox/mcp-bridge-url-validation.tssrc/lib/actions/sandbox/policy-channel-journal-guard.test.tssrc/lib/actions/sandbox/policy-channel-list.test.tssrc/lib/actions/sandbox/policy-channel.tssrc/lib/actions/sandbox/rebuild-baseline-transition-preflight.test.tssrc/lib/actions/sandbox/rebuild-dcode-pre-delete-drift.test.tssrc/lib/actions/sandbox/rebuild-destroy-phase.test.tssrc/lib/actions/sandbox/rebuild-destroy-phase.tssrc/lib/actions/sandbox/rebuild-pipeline.tssrc/lib/actions/sandbox/rebuild-preflight-guards.tssrc/lib/actions/sandbox/rebuild-preflight-phase.tssrc/lib/actions/sandbox/rebuild-restore-phase.test.tssrc/lib/actions/sandbox/rebuild-restore-phase.tssrc/lib/actions/sandbox/rebuild-shields-finally.test.tssrc/lib/actions/sandbox/snapshot-restore-lifecycle.test.tssrc/lib/actions/sandbox/snapshot-restore-test-fixture.tssrc/lib/actions/sandbox/snapshot.tssrc/lib/actions/sandbox/status-flow.test.tssrc/lib/actions/sandbox/status-snapshot.tssrc/lib/actions/sandbox/status-text.tssrc/lib/adapters/http/curl-args.test.tssrc/lib/adapters/http/curl-args.tssrc/lib/inference/compatible-endpoint-context.test.tssrc/lib/inference/endpoint-ssrf-preflight.test.tssrc/lib/inference/endpoint-ssrf-preflight.tssrc/lib/onboard/inference-selection-validation.test.tssrc/lib/onboard/sandbox-registration.tssrc/lib/policy/custom-policy-transition.test.tssrc/lib/policy/custom-policy-validation.tssrc/lib/policy/index.tssrc/lib/policy/shields-policy-mutation-guard.test.tssrc/lib/policy/trusted-private-endpoints.test.tssrc/lib/policy/trusted-private-endpoints.tssrc/lib/security/trusted-private-endpoint.test.tssrc/lib/security/trusted-private-endpoint.tssrc/lib/shields/flow.test.tssrc/lib/shields/index.tssrc/lib/shields/mcp-policy-transition.test.tssrc/lib/shields/policy-mutation-authority.test.tssrc/lib/shields/timer.test.tssrc/lib/shields/transition-lock.tssrc/lib/state/registry-mcp.tssrc/lib/state/registry-normalization.test.tssrc/lib/state/registry-normalization.tssrc/lib/state/registry.tssrc/lib/state/registry/persistence.tssrc/lib/state/registry/types.tssrc/lib/state/sandbox.tstest/helpers/destroy-flow-test-harness.tstest/helpers/rebuild-flow-lifecycle-cases.tstest/helpers/rebuild-flow-recovery-cases.tstest/helpers/rebuild-flow-target-credentials-cases.tstest/helpers/shields-flow-fixtures.tstest/helpers/shields-flow-harness.tstest/hermes-mcp-config-transaction.test.tstest/hermes-mcp-private-target-validation.test.tstest/hermes-mcp-shields-order.test.tstest/langchain-deepagents-code-managed-mcp-hardening.test.tstest/mcp-destroy-lifecycle.test.tstest/mcp-policy-journal-guard.test.tstest/mcp-policy-key-ownership.test.tstest/mcp-policy-transition.test.tstest/mcp-restart-policy-order.test.tstest/package-contract/cli/policy-dispatch.test.tstest/policies.test.tstest/snapshot-custom-policy-transition.test.tstest/support/status-flow-test-harness.ts
💤 Files with no reviewable changes (1)
- test/hermes-mcp-config-transaction.test.ts
| try { | ||
| return await withSandboxMutationLock(sandboxName, async () => { | ||
| enteredTransactionLock = true; | ||
| assertPolicyTransitionsSettledBeforeBackup(sandboxName); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
One unresolved policy journal aborts the whole backup-all run.
assertPolicyTransitionsSettledBeforeBackup throws after enteredTransactionLock is set, so line 276 rethrows the error. backupRegisteredSandbox does not catch it, so the error escapes backupAll and no later sandbox in the loop is backed up. Every other failure mode in this file is per-sandbox: a mutation-lock failure and an unreadable sandbox both increment failed/skipped and continue.
backup-all runs as the installer pre-upgrade backup. With this change, a single sandbox that holds a pending custom-policy or baseline-exclusion journal blocks the backup of every healthy sandbox and, under the installer's strict mode, blocks the upgrade before the recovery phase can report the journal.
Report the blocked sandbox as a per-sandbox failure and continue the loop, so the strict gate still trips but healthy sandboxes are backed up. The parameterized test in src/lib/actions/maintenance.test.ts at lines 340-379 must then assert a failed outcome instead of a rejected promise.
♻️ Proposed direction
- assertPolicyTransitionsSettledBeforeBackup(sandboxName);
+ const unsettledPolicyTransition = describeUnsettledPolicyTransition(sandboxName);
+ if (unsettledPolicyTransition) {
+ return {
+ result: null,
+ orphanManifestMessage: null,
+ shieldsWindowOpened: false,
+ stoppedContainerUnavailable: false,
+ unsettledPolicyTransition,
+ };
+ }Then handle unsettledPolicyTransition in backupRegisteredSandbox next to the existing mutationLockError branch: print the repair guidance and increment failed.
🤖 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` at line 141, Update backupRegisteredSandbox
to catch unsettledPolicyTransition alongside mutationLockError, print the
existing repair guidance, increment failed, and return the per-sandbox failure
result so backupAll continues processing later sandboxes. Update the
parameterized test in maintenance.test.ts to expect a failed outcome rather than
a rejected promise.
| if (ownedRegistration && !registry.removeCustomPolicyByName(sandboxName, entry.policyName)) { | ||
| throw new McpBridgeError( | ||
| `Could not clear ownership for generated MCP policy '${entry.policyName}'.`, | ||
| ); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline src/lib/actions/sandbox/mcp-bridge-state.ts --items all
rg -n -C 10 '\bsetBridgeState\b' src/lib/actions/sandbox/mcp-bridge-state.ts
ast-grep outline src/lib/state/registry.ts --items all
rg -n -C 12 '\bremoveCustomPolicyByName\b|\bupdateSandbox\b' src/lib/state/registry.ts
rg -n -C 8 'discardSafeIncompleteMcpAdds|removeCustomPolicyByName|setBridgeState' \
src/lib/actions/sandbox --glob '*.test.ts'Repository: NVIDIA/NemoClaw
Length of output: 13200
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- preflight implementation ---'
cat -n src/lib/actions/sandbox/mcp-bridge-destroy-preflight.ts | sed -n '1,150p'
printf '%s\n' '--- bridge state implementation ---'
cat -n src/lib/actions/sandbox/mcp-bridge-state.ts | sed -n '70,115p'
printf '%s\n' '--- registry mutation implementations ---'
cat -n src/lib/state/registry.ts | sed -n '270,330p'
cat -n src/lib/state/registry.ts | sed -n '417,455p'
printf '%s\n' '--- persistence and locking ---'
cat -n src/lib/state/registry/persistence.ts | sed -n '1,240p'
cat -n src/lib/state/registry/lock.ts | sed -n '1,180p'
printf '%s\n' '--- relevant tests and callers ---'
rg -n -C 12 'providerless|destroy-preflight|discardSafeIncompleteMcpAdds|removeCustomPolicyByName|setBridgeState' \
src/lib/actions/sandbox src/lib/state --glob '*.{ts,tsx}'Repository: NVIDIA/NemoClaw
Length of output: 50371
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- config write semantics ---'
rg -n -C 18 'function writeConfigFile|export function writeConfigFile|renameSync|writeFileSync|fsync' \
src/lib/config-io.ts src/lib --glob 'config-io.ts'
printf '%s\n' '--- focused preflight tests ---'
rg -l 'discardSafeIncompleteMcpAdds|prepareMcpBridgesForAbsentSandboxDestroy|providerless|sandboxAbsent' \
src/lib/actions/sandbox --glob '*.test.ts' |
while read -r file; do
echo "--- $file"
rg -n -C 18 'discardSafeIncompleteMcpAdds|prepareMcpBridgesForAbsentSandboxDestroy|providerless|sandboxAbsent' "$file"
done
printf '%s\n' '--- policy cleanup implementations ---'
rg -n -C 24 'function removeGeneratedPolicy|export function removeGeneratedPolicy|removeGeneratedPolicy\(' \
src/lib/actions/sandbox --glob '*.ts'
printf '%s\n' '--- all registry transaction helpers ---'
rg -n -C 10 'completeMcpDestroy|withLock\(|save\(data\)' \
src/lib/state/registry.ts src/lib/state/registry --glob '*.ts' | head -n 260Repository: NVIDIA/NemoClaw
Length of output: 2721
🏁 Script executed:
#!/bin/bash
set -u
printf '%s\n' '--- focused preflight tests ---'
files=$(rg -l 'discardSafeIncompleteMcpAdds|prepareMcpBridgesForAbsentSandboxDestroy|providerless|sandboxAbsent' \
src/lib/actions/sandbox --glob '*.test.ts' || true)
for file in $files; do
echo "--- $file"
rg -n -C 18 'discardSafeIncompleteMcpAdds|prepareMcpBridgesForAbsentSandboxDestroy|providerless|sandboxAbsent' "$file"
done
printf '%s\n' '--- policy cleanup implementations ---'
rg -n -C 24 'function removeGeneratedPolicy|export function removeGeneratedPolicy|removeGeneratedPolicy\(' \
src/lib/actions/sandbox --glob '*.ts' || true
printf '%s\n' '--- registry transaction helpers ---'
rg -n -C 10 'completeMcpDestroy|withLock\(|save\(data\)' \
src/lib/state/registry.ts src/lib/state/registry --glob '*.ts' |
head -n 320Repository: NVIDIA/NemoClaw
Length of output: 50371
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- ownership validation ---'
rg -n -C 28 'assertGeneratedPolicyRegistrationMutationSafe' \
src/lib/actions/sandbox/mcp-bridge-policy.ts
printf '%s\n' '--- absent-sandbox preflight caller ---'
rg -n -C 24 'prepareMcpBridgesForAbsentSandboxDestroy|sandboxAbsent: true|discardSafeIncompleteMcpAdds' \
src/lib/actions/sandbox --glob '*.ts'
printf '%s\n' '--- complete cleanup transaction callers ---'
rg -n -C 18 'completeMcpDestroy\(' src/lib --glob '*.ts'
printf '%s\n' '--- focused test names only ---'
rg -n 'it\(|test\(' src/lib/actions/sandbox --glob '*.test.ts' |
rg -i 'incomplete|providerless|absent|preflight|policy|destroy' |
head -n 160Repository: NVIDIA/NemoClaw
Length of output: 50371
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
preflight = Path("src/lib/actions/sandbox/mcp-bridge-destroy-preflight.ts").read_text()
registry = Path("src/lib/state/registry.ts").read_text()
start = preflight.index("export async function discardSafeIncompleteMcpAdds")
end = preflight.index("\nexport function assertMcpDestroySnapshotCurrent", start)
body = preflight[start:end]
policy_pos = body.index("registry.removeCustomPolicyByName")
bridge_pos = body.index("setBridgeState")
assert policy_pos < bridge_pos
assert "removeCustomPolicyByName" in registry
assert "function completeMcpDestroy" in registry
assert "completeMcpDestroy(" not in body
print("providerless cleanup order: removeCustomPolicyByName before setBridgeState")
print("incomplete-add path uses separate registry APIs")
print("completeMcpDestroy exists but is not used by discardSafeIncompleteMcpAdds")
PYRepository: NVIDIA/NemoClaw
Length of output: 350
Make providerless-add cleanup atomic.
removeCustomPolicyByName and setBridgeState use separate registry saves. A crash or write failure between them can persist the bridge entry without its generated policy. Move both mutations into one state-layer transaction and add a failure-path 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/actions/sandbox/mcp-bridge-destroy-preflight.ts` around lines 92 -
96, Update the providerless-add cleanup flow around removeCustomPolicyByName and
setBridgeState so both mutations execute within a single state-layer transaction
and are persisted atomically, preserving the existing McpBridgeError behavior on
failure. Add a test covering a transaction/save failure between these operations
and verify neither mutation is partially persisted.
Source: Path instructions
| const pendingApplyRepair = policies.repairPendingCustomPolicyApply(sandboxName, { | ||
| dryRun, | ||
| externalSource: source.kind === "file" || source.kind === "dir", | ||
| }); | ||
| if (pendingApplyRepair.state === "completed") { | ||
| syncSessionPolicyPresetsWithRegistry(sandboxName, pendingApplyRepair.presetName, "add"); | ||
| refreshSandboxPolicyContextFile(sandboxName); | ||
| return; | ||
| } | ||
| if (pendingApplyRepair.state === "blocked") { | ||
| process.exit(1); | ||
| return; | ||
| } | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect repairPendingCustomPolicyApply and its returned states.
ast-grep run --pattern $'export function repairPendingCustomPolicyApply($$$) { $$$ }' --lang typescript src/lib/policy/index.ts
rg -nP --type=ts -C4 '"completed"|"blocked"|"none"' src/lib/policy/index.ts | rg -n -C4 'repairPendingCustomPolicyApply|state'Repository: NVIDIA/NemoClaw
Length of output: 6633
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- repair implementation ---'
sed -n '1478,1605p' src/lib/policy/index.ts
printf '%s\n' '--- policy-channel caller ---'
sed -n '150,225p' src/lib/actions/sandbox/policy-channel.ts
printf '%s\n' '--- call sites and tests ---'
rg -n -P --type=ts 'repairPendingCustomPolicyApply|pendingApplyRepair|externalSource' src test tests 2>/dev/null || trueRepository: NVIDIA/NemoClaw
Length of output: 11762
Do not return after repairing an unrelated pending custom policy apply
repairPendingCustomPolicyApply does not receive the requested preset name. A non-dry --from-file or --from-dir request can repair transition.name, return "completed", and exit successfully before applying the current request. Continue with the current request after repair, or block and report mismatched requests. Built-in requests already return "blocked".
🤖 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/policy-channel.ts` around lines 198 - 211, Update the
pending-repair handling around repairPendingCustomPolicyApply so a completed
repair only returns when it corresponds to the current requested preset;
otherwise continue processing the requested --from-file or --from-dir operation
(or block and report the mismatch). Preserve the existing blocked behavior,
including the built-in-request path, and do not exit successfully before
applying the current request.
| const { isPrivateIp } = require("../private-networks") as typeof import("../private-networks"); | ||
| const normalizedAddresses = addresses.map((address) => { | ||
| if (typeof address !== "string" || !isOperatorTrustablePrivateIp(address)) { | ||
| if (typeof address !== "string" || isIP(address) === 0) { | ||
| throw new Error( | ||
| `trusted private host "${normalizedHost}" has a disallowed recorded address pin`, | ||
| ); | ||
| } | ||
| return normalizeIpLiteral(address); | ||
| const normalizedAddress = normalizeIpLiteral(address); | ||
| if (isPrivateIp(normalizedAddress) && !isOperatorTrustablePrivateIp(normalizedAddress)) { | ||
| throw new Error( | ||
| `trusted private host "${normalizedHost}" has a disallowed recorded address pin`, | ||
| ); | ||
| } | ||
| return normalizedAddress; | ||
| }); | ||
| if (new Set(normalizedAddresses).size !== normalizedAddresses.length) { | ||
| throw new Error(`trusted private host "${normalizedHost}" has duplicate recorded address pins`); | ||
| } | ||
| if (!normalizedAddresses.some((address) => isOperatorTrustablePrivateIp(address))) { | ||
| throw new Error( | ||
| `trusted private host "${normalizedHost}" has no recorded address pin in a supported private range`, | ||
| ); | ||
| } | ||
| const pinnedAddresses = Object.freeze([...normalizedAddresses].sort()); | ||
| return Object.freeze({ | ||
| host: normalizedHost, | ||
| addresses: pinnedAddresses, | ||
| trustedPrivateCapability: issueTrustedPrivateEndpointCapability(pinnedAddresses), | ||
| trustedPrivateCapability: issueTrustedPrivateEndpointCapability( | ||
| normalizedHost, | ||
| pinnedAddresses, | ||
| ), |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Update MCP lifecycle validation for mixed trusted-private pins.
Line 213 now accepts public pins when the set also contains an operator-trustable private pin. However, src/lib/actions/sandbox/mcp-bridge-provider-inspection.ts still rejects any recorded pin for which isOperatorTrustablePrivateIp() is false. A newly valid mixed DNS result can therefore create a policy but fail MCP restart or recovery.
Accept public pins after replayTrustedPrivateEndpoint() validates the complete set. Add lifecycle coverage for a persisted mixed pin set.
🤖 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/security/trusted-private-endpoint.ts` around lines 213 - 243, Update
MCP lifecycle validation in replayTrustedPrivateEndpoint() consumers, especially
the MCP bridge inspection flow, to accept public recorded pins when the complete
validated set includes at least one operator-trustable private pin. Remove any
per-address rejection based solely on isOperatorTrustablePrivateIp(), rely on
replayTrustedPrivateEndpoint() for set validation, and add lifecycle coverage
for recovery or restart with a persisted mixed pin set.
| it.each([ | ||
| ["missing version", { version: undefined }], | ||
| ["unsupported version", { version: 2 }], | ||
| ["non-UUID id", { id: "tx-apply" }], | ||
| ["non-canonical UUID", { id: "123E4567-E89B-42D3-A456-426614174010" }], | ||
| ["unsupported operation", { operation: "replace" }], | ||
| ["unsafe name", { name: "private_api" }], | ||
| ["overlong name", { name: "a".repeat(64) }], | ||
| ["non-canonical timestamp", { startedAt: "today" }], | ||
| ["missing previous", { previous: undefined }], | ||
| ["missing desired", { desired: undefined }], | ||
| ])("rejects an incomplete journal with %s", (_label, override) => { | ||
| expect(() => normalizeCustomPolicyTransition({ ...applyTransition, ...override })).toThrow( | ||
| /custom policy transition.*before rebuilding/i, | ||
| ); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Two cases do not exercise the property-presence gate they claim.
normalizeCustomPolicyTransition gates on Object.prototype.hasOwnProperty.call(value, "previous") and the same check for "desired". The object spread { ...applyTransition, previous: undefined } keeps the own property with value undefined, so both presence checks still pass. The values then fall through to normalizeCustomPolicyTransitionEntry, which throws a different error. The assertion regex /custom policy transition.*before rebuilding/i matches that different message, so the cases named "missing previous" and "missing desired" pass without testing the presence gate.
Delete the keys instead, and assert the incomplete-journal message.
💚 Proposed fix for the presence cases
it.each([
["missing version", { version: undefined }],
["unsupported version", { version: 2 }],
["non-UUID id", { id: "tx-apply" }],
["non-canonical UUID", { id: "123E4567-E89B-42D3-A456-426614174010" }],
["unsupported operation", { operation: "replace" }],
["unsafe name", { name: "private_api" }],
["overlong name", { name: "a".repeat(64) }],
["non-canonical timestamp", { startedAt: "today" }],
- ["missing previous", { previous: undefined }],
- ["missing desired", { desired: undefined }],
])("rejects an incomplete journal with %s", (_label, override) => {
expect(() => normalizeCustomPolicyTransition({ ...applyTransition, ...override })).toThrow(
/custom policy transition.*before rebuilding/i,
);
});
+
+ it.each([
+ "previous",
+ "desired",
+ ] as const)("rejects a journal that omits the %s key", (field) => {
+ const { [field]: _omitted, ...withoutField } = applyTransition;
+ expect(() => normalizeCustomPolicyTransition(withoutField)).toThrow(
+ /incomplete custom policy transition.*before rebuilding/i,
+ );
+ });📝 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.each([ | |
| ["missing version", { version: undefined }], | |
| ["unsupported version", { version: 2 }], | |
| ["non-UUID id", { id: "tx-apply" }], | |
| ["non-canonical UUID", { id: "123E4567-E89B-42D3-A456-426614174010" }], | |
| ["unsupported operation", { operation: "replace" }], | |
| ["unsafe name", { name: "private_api" }], | |
| ["overlong name", { name: "a".repeat(64) }], | |
| ["non-canonical timestamp", { startedAt: "today" }], | |
| ["missing previous", { previous: undefined }], | |
| ["missing desired", { desired: undefined }], | |
| ])("rejects an incomplete journal with %s", (_label, override) => { | |
| expect(() => normalizeCustomPolicyTransition({ ...applyTransition, ...override })).toThrow( | |
| /custom policy transition.*before rebuilding/i, | |
| ); | |
| }); | |
| it.each([ | |
| ["missing version", { version: undefined }], | |
| ["unsupported version", { version: 2 }], | |
| ["non-UUID id", { id: "tx-apply" }], | |
| ["non-canonical UUID", { id: "123E4567-E89B-42D3-A456-426614174010" }], | |
| ["unsupported operation", { operation: "replace" }], | |
| ["unsafe name", { name: "private_api" }], | |
| ["overlong name", { name: "a".repeat(64) }], | |
| ["non-canonical timestamp", { startedAt: "today" }], | |
| ])("rejects an incomplete journal with %s", (_label, override) => { | |
| expect(() => normalizeCustomPolicyTransition({ ...applyTransition, ...override })).toThrow( | |
| /custom policy transition.*before rebuilding/i, | |
| ); | |
| }); | |
| it.each([ | |
| "previous", | |
| "desired", | |
| ] as const)("rejects a journal that omits the %s key", (field) => { | |
| const { [field]: _omitted, ...withoutField } = applyTransition; | |
| expect(() => normalizeCustomPolicyTransition(withoutField)).toThrow( | |
| /incomplete custom policy transition.*before rebuilding/i, | |
| ); | |
| }); |
🤖 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/registry-normalization.test.ts` around lines 289 - 304, Update
the “missing previous” and “missing desired” cases in the
normalizeCustomPolicyTransition test data to remove those properties rather than
overriding them with undefined, so Object.prototype.hasOwnProperty checks fail.
Keep the incomplete-journal assertion focused on the presence-gate error message
and ensure both cases exercise the missing-property path.
Source: Path instructions
|
🌿 Preview your docs: https://nvidia-preview-pr-8494.docs.buildwithfern.com/nemoclaw |
cv
left a comment
cv
left a comment
There was a problem hiding this comment.
Several correctness failures block this security hardening PR. A pending custom-policy repair can return success before applying the caller's different --from-file or --from-dir request. Mixed public/private validated pins are accepted at admission but rejected by MCP lifecycle inspection. One unsettled journal aborts backup-all instead of recording one sandbox failure and continuing healthy backups. Fix these state-transition inconsistencies and add the named lifecycle tests. Also correct the two property-presence tests so they delete previous or desired, resolve the remaining review threads, fix the failing CLI shards, refresh onto current main, and rerun all required checks.
cv
left a comment
cv
left a comment
There was a problem hiding this comment.
Additionally, please review this PR description and diff for comms and documentation guidelines in WRITING.md and linked artifacts.
2 participants
Summary
This is the security and architecture hardening follow-up to merged PR #8272. It binds trusted-private destination authority to exact content, preserves validated MCP pins across lifecycle operations, and makes custom policy mutations crash recoverable without widening egress. Recovery now rejects malformed or tampered durable targets before it reads, commits, or mutates live policy state.
Related Issue
Follow-up to #8272, which implemented #8176 and #8267. This PR does not reopen or close those completed issues.
Changes
custom-policy-transition.test.tsandregistry-normalization.test.tsprotect the boundary.Type of Change
Quality Gates
41644a979returned PASS with no actionable findings.Documentation Writer Review
docs-updateddocs/inference/custom-endpoint-security.mdx,docs/inference/set-up-openai-compatible-endpoint.mdx,docs/manage-sandboxes/add-mcp-server.mdx,docs/network-policy/create-custom-policy-presets.mdx,docs/reference/commands.mdx,docs/reference/network-policies.mdxDGX Station Hardware Evidence
Verification
Signed-off-by:line and every commit appears asVerifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run validate:prpassed after refreshingorigin/mainwhen hooks were skipped or unavailablenpm run docsbuilds without warnings (doc changes only)npm run docspassed with 0 errors and only the two documented repository-baseline Fern warnings; this PR adds no warning.Signed-off-by: Julie Yaunches jyaunches@nvidia.com
Summary by CodeRabbit