Repository navigation
fix(rebuild): validate DCode recreation before deletion - #6214
Merged
Merged
Conversation
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Contributor
|
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:
📝 WalkthroughWalkthroughThis PR adds DCode-specific rebuild preflight, prepared-image and prepared-build-context handoff support, and wires that flow through rebuild, onboarding, registry wiring, and e2e coverage. It also updates docs and allowlists for the new internal GPU patch network handoff variable. ChangesDCode Atomic Rebuild Preflight
Estimated code review effort: 5 (Critical) | ~120 minutes Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
|
🌿 Preview your docs: https://nvidia-preview-pr-6214.docs.buildwithfern.com/nemoclaw |
Contributor
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in the Show a code coverage summary of the most covered files.
TypeScript / code-coverage/cliThe overall coverage in the Show a code coverage summary of the most covered files.
Updated |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
Contributor
PR Review Advisor — No blocking findingsMerge posture: No blocking advisor findings Action checklist
Test follow-ups to resolve or justifyIf these cover changed behavior, prefer adding them in this PR; otherwise state why existing coverage is enough or link the follow-up.
This is an automated, non-binding review; it still expects maintainers and agents to respond to each required or warning item. Treat suggestions as current-PR improvements when they touch changed code; defer only with maintainer rationale or a linked follow-up. A human maintainer must make the final merge decision. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Contributor
PR Review Advisor (Nemotron Ultra) — BlockedMerge posture: Do not merge until addressed Action checklist
Findings index
🚨 Required before mergeAddress these before merging unless a maintainer explicitly overrides the advisor with rationale.
|
Contributor
E2E Advisor RecommendationRequired E2E: Dispatch hint: Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
Dispatch hint
|
Contributor
E2E Target RecommendationRequired E2E targets: Dispatch required E2E targets:
Full E2E target advisor summaryE2E Target AdvisorBase: Required E2E targets
Optional E2E targets
Relevant changed files
|
wscurran
added
area: sandbox
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/actions/sandbox/rebuild-flow-helpers.ts (1)
203-225: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
loadAgentfailure bypasses the graceful preflight-bail path.
loadAgent(rebuildAgent)(line 208) runs outside thetry/catchthat wrapsensureAgentBaseImage. If the persisted rebuild agent name doesn't resolve to an installed manifest (a realistic drift scenario this preflight exists to catch),loadAgentthrows uncaught instead of hitting the samebail(message)+ "Sandbox is untouched — no data was lost." messaging used for every other failure in this function.🐛 Proposed fix
if (!rebuildAgent) return { ok: true, imageRef: null, overrideEnvVar: null }; - const agentDef = loadAgent(rebuildAgent); - const overrideEnvVar = `NEMOCLAW_${agentDef.name - .toUpperCase() - .replace(/[^A-Z0-9]+/g, "_")}_SANDBOX_BASE_IMAGE_REF`; try { + const agentDef = loadAgent(rebuildAgent); + const overrideEnvVar = `NEMOCLAW_${agentDef.name + .toUpperCase() + .replace(/[^A-Z0-9]+/g, "_")}_SANDBOX_BASE_IMAGE_REF`; const result = ensureAgentBaseImage(agentDef, { forceBaseImageRebuild: true }); return { ok: true, imageRef: result.imageTag, overrideEnvVar }; } catch (err) {🤖 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-flow-helpers.ts` around lines 203 - 225, The preflight path in ensureRebuildAgentBaseImage lets loadAgent(rebuildAgent) throw before the existing graceful bail handling. Move the loadAgent call into the same try/catch used around ensureAgentBaseImage so any manifest-resolution failure is converted into the same message flow, including console.error output and bail(message), with the final untouched-sandbox notice. Keep the overrideEnvVar derivation and return shape in ensureRebuildAgentBaseImage unchanged.
🧹 Nitpick comments (12)
docs/manage-sandboxes/lifecycle.mdx (1)
231-231: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSplit semicolon-joined sentences and code-format the literal port.
Lines 231, 234, and 235 each join two independent clauses with a semicolon on a single line, and line 231 has the literal port number
8080outside of code formatting.✏️ Proposed fix
-LangChain Deep Agents Code has no dashboard; rebuild does not treat its OpenShell gateway listener, including port 8080, as a dashboard conflict. +LangChain Deep Agents Code has no dashboard. +Rebuild does not treat its OpenShell gateway listener, including port `8080`, as a dashboard conflict.-When matching onboarding state records Brave Search as enabled, rebuild also probes that route; a legacy Brave-enabled entry without matching state fails closed. +When matching onboarding state records Brave Search as enabled, rebuild also probes that route. +A legacy Brave-enabled entry without matching state fails closed.-If both CPU and memory values are recorded, rebuild validates and replays both; a partial record fails before deletion, while legacy entries with neither value use OpenShell defaults. +If both CPU and memory values are recorded, rebuild validates and replays both. +A partial record fails before deletion, while legacy entries with neither value use OpenShell defaults.As per path instructions,
docs/**/*.{md,mdx}should "Keep one sentence per line in Markdown and MDX source files" and "Usecodeformatting for commands, paths, flags, environment variables, file names, and literal values."Also applies to: 234-234, 235-235
🤖 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/lifecycle.mdx` at line 231, Split the semicolon-joined sentences in the affected MDX text so each independent clause is on its own line, following the one-sentence-per-line rule. In the relevant paragraph about LangChain Deep Agents Code and its OpenShell gateway listener, ensure the literal port 8080 is wrapped in code formatting. Keep the wording consistent while adjusting the prose in the surrounding markdown.Source: Path instructions
docs/reference/commands.mdx (1)
1683-1683: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSplit semicolon-joined sentences and code-format the literal port.
Lines 1683, 1698, and 1699 each join two independent clauses with a semicolon on one line, and line 1683 has the literal port number
8080outside of code formatting.✏️ Proposed fix
-LangChain Deep Agents Code has no dashboard; rebuild does not treat its OpenShell gateway listener, including port 8080, as a dashboard conflict. +LangChain Deep Agents Code has no dashboard. +Rebuild does not treat its OpenShell gateway listener, including port `8080`, as a dashboard conflict.-When matching onboarding state records Brave Search as enabled, rebuild also probes that route; a legacy Brave-enabled entry without matching state fails closed. +When matching onboarding state records Brave Search as enabled, rebuild also probes that route. +A legacy Brave-enabled entry without matching state fails closed.-If both CPU and memory values are recorded, rebuild validates and replays both; a partial record fails before deletion, while legacy entries with neither value use OpenShell defaults. +If both CPU and memory values are recorded, rebuild validates and replays both. +A partial record fails before deletion, while legacy entries with neither value use OpenShell defaults.As per path instructions,
docs/**/*.{md,mdx}should "Keep one sentence per line in Markdown and MDX source files" and "Usecodeformatting for commands, paths, flags, environment variables, file names, and literal values."Also applies to: 1698-1698, 1699-1699
🤖 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` at line 1683, In commands.mdx, split the semicolon-joined sentences so each independent clause is on its own line, following the one-sentence-per-line convention for Markdown/MDX, and update the LangChain Deep Agents Code/OpenShell gateway mention to use code formatting for the literal port value 8080. Make the same sentence split cleanup anywhere the affected text appears in the nearby content so the documentation style stays consistent.Source: Path instructions
src/lib/actions/sandbox/rebuild-custom-image-preflight.ts (1)
203-218: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winRedundant re-registration of the already-armed exit listener.
cleanupBuildContextis the closure returned byinstallBuildContextCleanupFallback, which already callsprocess.on("exit", cleanup)internally (line 105). Re-registering the same function reference here (line 212) creates a duplicate listener entry. In the common (non-interrupt) failure path this duplicate happens to get cleaned up by the interplay of the internalremove()and the outerremoveListenerincleanupBuildContextWithExitFallback, but it's fragile and hard to reason about — e.g. if the inline cleanup returnsfalse(cleanup failed), the duplicate listener is never removed and will invokecleanupBuildCtx()twice on actual process exit.♻️ Proposed simplification
if (!retainBuildContext && cleanupBuildContext) { - // Keep an exit safety net armed if inline cleanup fails; staged contexts - // may contain copied source or secret-bearing build inputs. - process.on("exit", cleanupBuildContext); try { cleanupBuildContextWithExitFallback(cleanupBuildContext); } catch { // Best effort: preserve the original preflight result. } }The exit listener installed by
installBuildContextCleanupFallbackalready serves as the safety net; no need to re-arm it here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/actions/sandbox/rebuild-custom-image-preflight.ts` around lines 203 - 218, The finally block in rebuild-custom-image-preflight is redundantly re-adding the exit listener for cleanupBuildContext, even though installBuildContextCleanupFallback already armed it. Remove the extra process.on("exit", cleanupBuildContext) call and rely on the existing fallback managed by cleanupBuildContextWithExitFallback and cleanupBuildContext, keeping the inline cleanup attempt and best-effort error handling intact.src/lib/actions/sandbox/rebuild-web-search-preflight.ts (1)
15-26: 🚀 Performance & Scalability | 🔵 TrivialLive billed API call on every rebuild preflight.
buildRebuildBraveSearchProbeCommandissues a real query againstapi.search.brave.com(q=ping) each time this preflight runs. Brave Search API is now a metered, credit-based service (roughly $0.003–$0.005 per query), so every rebuild attempt (and any retries) incurs a small real cost. Worth confirming this is an acceptable, intentional tradeoff for the atomicity guarantee, and that retries/backoff on the caller side don't multiply this unexpectedly.🤖 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-web-search-preflight.ts` around lines 15 - 26, The probe built by buildRebuildBraveSearchProbeCommand is making a real billed Brave Search query on every preflight, so adjust the command or its caller to avoid repeated paid requests unless this is explicitly intended. Review the q=ping curl flow in buildRebuildBraveSearchProbeCommand and either replace it with a non-billed validation path or add throttling/caching/retry limits around this preflight so rebuild retries do not multiply API costs unexpectedly.src/lib/onboard/sandbox-messaging-preflight.ts (1)
101-101: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNewly exported
checkMessagingPlanConflictswidens this module's public surface.Minor note: worth double-checking no other callers outside this cohort need the exported symbol (vs. only tests), to avoid an incidental new public dependency.
🤖 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/onboard/sandbox-messaging-preflight.ts` at line 101, The newly exported checkMessagingPlanConflicts function is expanding the module’s public API unnecessarily; verify whether it is only needed by local callers and tests, and if so remove the export or keep the symbol internal to avoid creating an incidental public dependency.src/lib/onboard/sandbox-provider-cleanup.ts (1)
54-59: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTwo parallel regex families for similar OpenShell output classification.
STRICT_NOT_ATTACHED_OUTPUT_RE/TOLERATED_ATTACH_OUTPUT_REare new and distinct from the existingTOLERATED_DETACH_OUTPUT_REused bydetachSandboxProviders. If the two are meant to recognize the same OpenShell CLI output shapes (just naming it "strict" for the exact-set path), consider deriving one from the other to avoid the two drifting apart as the CLI's error text evolves.🤖 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/onboard/sandbox-provider-cleanup.ts` around lines 54 - 59, The new OpenShell output regexes for attach cleanup are duplicating patterns already handled by the existing detach classification, so they may drift apart over time. Update the sandbox-provider-cleanup logic around STRICT_NOT_ATTACHED_OUTPUT_RE and TOLERATED_ATTACH_OUTPUT_RE to reuse the same underlying matcher structure as TOLERATED_DETACH_OUTPUT_RE, or derive the new expressions from it, so attach/detach output recognition stays consistent as CLI text changes.src/lib/onboard/initial-policy.test.ts (1)
12-24: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd negative-path coverage for the new custom-preset validation.
networkPoliciesHasAllowedIpsis stubbed to always returnfalse(Line 24), and no test in this suite exercises the invalid-YAML, missing-network_policies-mapping, or disallowed-allowed_ipsthrow paths thatprepareInitialSandboxCreatePolicynow implements foradditionalPresetContents(seeinitial-policy.tslines 274-299). These are the new security-boundary checks this PR introduces; without a test that makes the mock returntrue(or supplies genuinely invalid content), the rejection path is unverified.✅ Suggested test additions
mergePresetNamesIntoPolicy: (policy: string, presetNames: string[]) => { const missingPresets: string[] = presetNames.filter((name) => name === "missing-preset"); return { policy: `${policy.trimEnd()}\n slack: {}\n`, appliedPresets: presetNames.filter((name) => !missingPresets.includes(name)), missingPresets, }; }, - networkPoliciesHasAllowedIps: () => false, + networkPoliciesHasAllowedIps: (np: Record<string, unknown>) => + JSON.stringify(np).includes("allowed_ips"),+ it("throws when a custom preset contains disallowed allowed_ips", () => { + const basePolicyPath = tmpPolicy("version: 1\nnetwork_policies:\n base: {}\n"); + + expect(() => + prepareInitialSandboxCreatePolicy(basePolicyPath, [], { + additionalPresetContents: [ + { + name: "unsafe", + content: "network_policies:\n unsafe:\n allowed_ips: [1.2.3.4]\n", + }, + ], + }), + ).toThrow("disallowed allowed_ips"); + }); + + it("throws when a custom preset has no network_policies mapping", () => { + const basePolicyPath = tmpPolicy("version: 1\nnetwork_policies:\n base: {}\n"); + + expect(() => + prepareInitialSandboxCreatePolicy(basePolicyPath, [], { + additionalPresetContents: [{ name: "bad", content: "version: 1\n" }], + }), + ).toThrow("no valid network_policies mapping"); + });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."
Also applies to: 276-311
🤖 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/onboard/initial-policy.test.ts` around lines 12 - 24, Add negative-path coverage for prepareInitialSandboxCreatePolicy’s custom-preset validation by exercising the invalid YAML, missing network_policies mapping, and disallowed allowed_ips rejection paths instead of relying on the broad networkPoliciesHasAllowedIps stub. Update the tests in initial-policy.test.ts so at least one case supplies content that makes networkPoliciesHasAllowedIps return true (or otherwise violates the new checks) and asserts the thrown error from the additionalPresetContents handling in prepareInitialSandboxCreatePolicy. Use the existing helpers extractPresetEntries, mergePresetIntoPolicy, and mergePresetNamesIntoPolicy to keep the test focused on the validation behavior.Source: Path instructions
src/lib/actions/sandbox/rebuild-gateway-drift.test.ts (1)
30-53: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract duplicated fixture helpers into shared test-support module.
healthyGatewayRecovery,sandboxGpuConfig, andruntimePreflightResultare re-declared verbatim in this file,rebuild-resume-snapshot.test.ts, andrebuild-shields-finally.test.ts.rebuild-flow.test-support.tsalready exists as the shared home for cross-test helpers (e.g.requireDist,originalSandboxName); moving these fixtures there would remove the triplication and keep them in sync going forward.♻️ Suggested consolidation
+// in rebuild-flow.test-support.ts +export function healthyGatewayRecovery(gatewayName: string) { ... } +export const sandboxGpuConfig = { ... }; +export const runtimePreflightResult = { ... };Then import from the three test files instead of redefining locally.
🤖 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-gateway-drift.test.ts` around lines 30 - 53, The test fixtures are duplicated across multiple rebuild sandbox tests, so move healthyGatewayRecovery, sandboxGpuConfig, and runtimePreflightResult into the shared rebuild-flow.test-support.ts module alongside existing helpers like requireDist and originalSandboxName. Then update rebuild-gateway-drift.test.ts, rebuild-resume-snapshot.test.ts, and rebuild-shields-finally.test.ts to import those shared fixtures instead of redefining them locally, keeping the recovery and preflight setup consistent in one place.src/lib/actions/sandbox/rebuild-gpu-opt-out.ts (1)
141-141: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRename the shadowed
dashboardPortinside the Hermes block.The outer
dashboardPort(line 141) is the general control/dashboard port; the innerconst dashboardPort = args.sb.hermesDashboardPort(line 182) shadows it with a different meaning inside the same function. The code works today (the outer var is correctly referenced later forcontrolUiPort, andargs.sb.dashboardPortis explicitly used for the collision check), but the reused name is a latent trap for future edits.♻️ Proposed rename
if (args.rebuildAgent === "hermes" && args.sb?.hermesDashboardEnabled === true) { - const dashboardPort = args.sb.hermesDashboardPort; - const internalPort = args.sb.hermesDashboardInternalPort; - if (dashboardPort == null || internalPort == null) { + const hermesDashboardPort = args.sb.hermesDashboardPort; + const internalPort = args.sb.hermesDashboardInternalPort; + if (hermesDashboardPort == null || internalPort == null) { throw new Error("Cannot recreate an enabled Hermes dashboard without its persisted ports."); } authoritativeHermesDashboardConfig = readHermesDashboardConfig({ [HERMES_DASHBOARD_ENABLE_ENV]: "1", - [HERMES_DASHBOARD_PORT_ENV]: String(dashboardPort), + [HERMES_DASHBOARD_PORT_ENV]: String(hermesDashboardPort), [HERMES_DASHBOARD_INTERNAL_PORT_ENV]: String(internalPort), ...(args.sb.hermesDashboardTui ? { [HERMES_DASHBOARD_TUI_ENV]: "1" } : {}), }); if ( - dashboardPort === internalPort || - dashboardPort === HERMES_OPENAI_API_PORT || + hermesDashboardPort === internalPort || + hermesDashboardPort === HERMES_OPENAI_API_PORT || internalPort === HERMES_OPENAI_API_PORT || (args.sb.dashboardPort != null && - (dashboardPort === args.sb.dashboardPort || internalPort === args.sb.dashboardPort)) + (hermesDashboardPort === args.sb.dashboardPort || internalPort === args.sb.dashboardPort)) ) { throw new Error("Invalid persisted Hermes dashboard port configuration."); } }Also applies to: 180-202
🤖 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-gpu-opt-out.ts` at line 141, Rename the inner dashboardPort declared in the Hermes block of rebuild-gpu-opt-out.ts so it does not shadow the outer dashboardPort from the same function. Update the Hermes-specific variable near args.sb.hermesDashboardPort to a distinct name and adjust its uses within that block, while leaving the outer control/dashboard port and the later controlUiPort logic unchanged.src/lib/onboard/machine/handlers/provider-inference.test.ts (1)
169-184: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider also asserting the "inference" step skip.
The production path (per
handleProviderInferenceState) also callsdeps.skippedStepMessage("inference", ...)andrecordStateSkipped("inference", ...). Asserting on that too would close a small coverage gap where a regression in the second skip could go undetected.🤖 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/onboard/machine/handlers/provider-inference.test.ts` around lines 169 - 184, The provider-inference test currently asserts the provider_selection skip but misses the inference skip. Update provider-inference.test.ts to also verify the production flow in handleProviderInferenceState triggers deps.skippedStepMessage("inference", ...) and recordStateSkipped("inference", ...) alongside the existing skipped assertion, using the existing calls.skipped and result expectations to locate the test case.Source: Path instructions
src/lib/onboard/fatal-runtime-preflight.ts (1)
51-51: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRepeated default-exitProcess boilerplate across files.
(code: number) => never = (code) => process.exit(code)(or the equivalent namedexitProcessByDefault) is duplicated verbatim in this file,sandbox-gpu-preflight.ts, andbridge-dns-preflight.ts(per this cohort alone). Extracting a single shareddefaultExitProcessinto a small utility module would remove the repetition and give one place to change behavior (e.g. flushing logs before exit) across all injectable-exit call sites.♻️ Suggested shared helper
// src/lib/onboard/exit-process.ts export const defaultExitProcess = (code: number): never => process.exit(code);Also applies to: 86-95, 132-135, 292-295
🤖 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/onboard/fatal-runtime-preflight.ts` at line 51, The default exit-process implementation is duplicated in fatal-runtime-preflight and the other preflight files, so replace the local exitProcessByDefault / inline `(code: number) => process.exit(code)` boilerplate with a shared utility such as defaultExitProcess from a new exit-process module. Update the injectable-exit call sites in fatal-runtime-preflight to import and use that shared helper so all preflights share one behavior source and can be changed centrally later.src/lib/onboard/machine/handlers/preflight.test.ts (1)
269-305: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winTest doesn't cover the persisted step-status outcome.
This test checks that GPU-detection/validation calls are skipped and that the returned values match the authoritative inputs, but it never asserts what
recordStateSkipped/recordStepCompleteare called with, nor the resulting session step status. Given the implementation calls both functions back-to-back for the same step (seepreflight.tscomment), a test asserting the final persistedsession.steps.preflightshape would catch regressions in that sequencing.As per path instructions for test files: "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/onboard/machine/handlers/preflight.test.ts` around lines 269 - 305, Add coverage for the persisted preflight step outcome in this test: in handlePreflightState, the resume path with authoritativePreflight should assert the final session step status produced by recordStateSkipped and recordStepComplete, not just the skipped runtime behavior. Update the test around skippedStepMessage to verify the resulting session.steps.preflight shape and the exact skipped/completed transition for the preflight step, using the handlePreflightState and session persistence helpers as the observable boundary.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.
Outside diff comments:
In `@src/lib/actions/sandbox/rebuild-flow-helpers.ts`:
- Around line 203-225: The preflight path in ensureRebuildAgentBaseImage lets
loadAgent(rebuildAgent) throw before the existing graceful bail handling. Move
the loadAgent call into the same try/catch used around ensureAgentBaseImage so
any manifest-resolution failure is converted into the same message flow,
including console.error output and bail(message), with the final
untouched-sandbox notice. Keep the overrideEnvVar derivation and return shape in
ensureRebuildAgentBaseImage unchanged.
---
Nitpick comments:
In `@docs/manage-sandboxes/lifecycle.mdx`:
- Line 231: Split the semicolon-joined sentences in the affected MDX text so
each independent clause is on its own line, following the one-sentence-per-line
rule. In the relevant paragraph about LangChain Deep Agents Code and its
OpenShell gateway listener, ensure the literal port 8080 is wrapped in code
formatting. Keep the wording consistent while adjusting the prose in the
surrounding markdown.
In `@docs/reference/commands.mdx`:
- Line 1683: In commands.mdx, split the semicolon-joined sentences so each
independent clause is on its own line, following the one-sentence-per-line
convention for Markdown/MDX, and update the LangChain Deep Agents Code/OpenShell
gateway mention to use code formatting for the literal port value 8080. Make the
same sentence split cleanup anywhere the affected text appears in the nearby
content so the documentation style stays consistent.
In `@src/lib/actions/sandbox/rebuild-custom-image-preflight.ts`:
- Around line 203-218: The finally block in rebuild-custom-image-preflight is
redundantly re-adding the exit listener for cleanupBuildContext, even though
installBuildContextCleanupFallback already armed it. Remove the extra
process.on("exit", cleanupBuildContext) call and rely on the existing fallback
managed by cleanupBuildContextWithExitFallback and cleanupBuildContext, keeping
the inline cleanup attempt and best-effort error handling intact.
In `@src/lib/actions/sandbox/rebuild-gateway-drift.test.ts`:
- Around line 30-53: The test fixtures are duplicated across multiple rebuild
sandbox tests, so move healthyGatewayRecovery, sandboxGpuConfig, and
runtimePreflightResult into the shared rebuild-flow.test-support.ts module
alongside existing helpers like requireDist and originalSandboxName. Then update
rebuild-gateway-drift.test.ts, rebuild-resume-snapshot.test.ts, and
rebuild-shields-finally.test.ts to import those shared fixtures instead of
redefining them locally, keeping the recovery and preflight setup consistent in
one place.
In `@src/lib/actions/sandbox/rebuild-gpu-opt-out.ts`:
- Line 141: Rename the inner dashboardPort declared in the Hermes block of
rebuild-gpu-opt-out.ts so it does not shadow the outer dashboardPort from the
same function. Update the Hermes-specific variable near
args.sb.hermesDashboardPort to a distinct name and adjust its uses within that
block, while leaving the outer control/dashboard port and the later
controlUiPort logic unchanged.
In `@src/lib/actions/sandbox/rebuild-web-search-preflight.ts`:
- Around line 15-26: The probe built by buildRebuildBraveSearchProbeCommand is
making a real billed Brave Search query on every preflight, so adjust the
command or its caller to avoid repeated paid requests unless this is explicitly
intended. Review the q=ping curl flow in buildRebuildBraveSearchProbeCommand and
either replace it with a non-billed validation path or add
throttling/caching/retry limits around this preflight so rebuild retries do not
multiply API costs unexpectedly.
In `@src/lib/onboard/fatal-runtime-preflight.ts`:
- Line 51: The default exit-process implementation is duplicated in
fatal-runtime-preflight and the other preflight files, so replace the local
exitProcessByDefault / inline `(code: number) => process.exit(code)` boilerplate
with a shared utility such as defaultExitProcess from a new exit-process module.
Update the injectable-exit call sites in fatal-runtime-preflight to import and
use that shared helper so all preflights share one behavior source and can be
changed centrally later.
In `@src/lib/onboard/initial-policy.test.ts`:
- Around line 12-24: Add negative-path coverage for
prepareInitialSandboxCreatePolicy’s custom-preset validation by exercising the
invalid YAML, missing network_policies mapping, and disallowed allowed_ips
rejection paths instead of relying on the broad networkPoliciesHasAllowedIps
stub. Update the tests in initial-policy.test.ts so at least one case supplies
content that makes networkPoliciesHasAllowedIps return true (or otherwise
violates the new checks) and asserts the thrown error from the
additionalPresetContents handling in prepareInitialSandboxCreatePolicy. Use the
existing helpers extractPresetEntries, mergePresetIntoPolicy, and
mergePresetNamesIntoPolicy to keep the test focused on the validation behavior.
In `@src/lib/onboard/machine/handlers/preflight.test.ts`:
- Around line 269-305: Add coverage for the persisted preflight step outcome in
this test: in handlePreflightState, the resume path with authoritativePreflight
should assert the final session step status produced by recordStateSkipped and
recordStepComplete, not just the skipped runtime behavior. Update the test
around skippedStepMessage to verify the resulting session.steps.preflight shape
and the exact skipped/completed transition for the preflight step, using the
handlePreflightState and session persistence helpers as the observable boundary.
In `@src/lib/onboard/machine/handlers/provider-inference.test.ts`:
- Around line 169-184: The provider-inference test currently asserts the
provider_selection skip but misses the inference skip. Update
provider-inference.test.ts to also verify the production flow in
handleProviderInferenceState triggers deps.skippedStepMessage("inference", ...)
and recordStateSkipped("inference", ...) alongside the existing skipped
assertion, using the existing calls.skipped and result expectations to locate
the test case.
In `@src/lib/onboard/sandbox-messaging-preflight.ts`:
- Line 101: The newly exported checkMessagingPlanConflicts function is expanding
the module’s public API unnecessarily; verify whether it is only needed by local
callers and tests, and if so remove the export or keep the symbol internal to
avoid creating an incidental public dependency.
In `@src/lib/onboard/sandbox-provider-cleanup.ts`:
- Around line 54-59: The new OpenShell output regexes for attach cleanup are
duplicating patterns already handled by the existing detach classification, so
they may drift apart over time. Update the sandbox-provider-cleanup logic around
STRICT_NOT_ATTACHED_OUTPUT_RE and TOLERATED_ATTACH_OUTPUT_RE to reuse the same
underlying matcher structure as TOLERATED_DETACH_OUTPUT_RE, or derive the new
expressions from it, so attach/detach output recognition stays consistent as CLI
text changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 75b55c65-e50f-4817-92c4-165510174c2b
📒 Files selected for processing (84)
ci/env-var-doc-allowlist.jsondocs/manage-sandboxes/lifecycle.mdxdocs/reference/commands-nemohermes.mdxdocs/reference/commands.mdxsrc/lib/actions/sandbox/rebuild-atomic-preflight.test.tssrc/lib/actions/sandbox/rebuild-custom-image-preflight.test.tssrc/lib/actions/sandbox/rebuild-custom-image-preflight.tssrc/lib/actions/sandbox/rebuild-env-isolation.test.tssrc/lib/actions/sandbox/rebuild-env-isolation.tssrc/lib/actions/sandbox/rebuild-flow-helpers.tssrc/lib/actions/sandbox/rebuild-flow.test-support.tssrc/lib/actions/sandbox/rebuild-flow.test.tssrc/lib/actions/sandbox/rebuild-gateway-drift.test.tssrc/lib/actions/sandbox/rebuild-gpu-opt-out.test.tssrc/lib/actions/sandbox/rebuild-gpu-opt-out.tssrc/lib/actions/sandbox/rebuild-inference-preflight.test.tssrc/lib/actions/sandbox/rebuild-inference-preflight.tssrc/lib/actions/sandbox/rebuild-resume-config.test.tssrc/lib/actions/sandbox/rebuild-resume-config.tssrc/lib/actions/sandbox/rebuild-resume-snapshot.test.tssrc/lib/actions/sandbox/rebuild-shields-finally.test.tssrc/lib/actions/sandbox/rebuild-web-search-preflight.test.tssrc/lib/actions/sandbox/rebuild-web-search-preflight.tssrc/lib/actions/sandbox/rebuild.tssrc/lib/gateway-runtime-action.tssrc/lib/inference/selection.tssrc/lib/onboard.tssrc/lib/onboard/authoritative-rebuild-target.test.tssrc/lib/onboard/authoritative-rebuild-target.tssrc/lib/onboard/branding.tssrc/lib/onboard/bridge-dns-preflight-exit.test.tssrc/lib/onboard/bridge-dns-preflight.tssrc/lib/onboard/build-context-stage.tssrc/lib/onboard/docker-driver-gateway-env.test.tssrc/lib/onboard/docker-driver-gateway-env.tssrc/lib/onboard/docker-driver-gateway-runtime.test.tssrc/lib/onboard/docker-driver-gateway-runtime.tssrc/lib/onboard/docker-gpu-local-inference.tssrc/lib/onboard/exit-step-failure.tssrc/lib/onboard/fatal-runtime-preflight.test.tssrc/lib/onboard/fatal-runtime-preflight.tssrc/lib/onboard/gateway-binding.test.tssrc/lib/onboard/gateway-binding.tssrc/lib/onboard/gateway-reuse.test.tssrc/lib/onboard/gateway-reuse.tssrc/lib/onboard/gateway-sandbox-reachability.test.tssrc/lib/onboard/gateway-sandbox-reachability.tssrc/lib/onboard/initial-policy.test.tssrc/lib/onboard/initial-policy.tssrc/lib/onboard/machine/core-flow-phases.tssrc/lib/onboard/machine/handlers/gateway.test.tssrc/lib/onboard/machine/handlers/gateway.tssrc/lib/onboard/machine/handlers/preflight.test.tssrc/lib/onboard/machine/handlers/preflight.tssrc/lib/onboard/machine/handlers/provider-inference.test.tssrc/lib/onboard/machine/handlers/provider-inference.tssrc/lib/onboard/machine/initial-flow-phases.tssrc/lib/onboard/messaging-prep.test.tssrc/lib/onboard/messaging-prep.tssrc/lib/onboard/onboard-exit-handler.tssrc/lib/onboard/preflight-cdi.test.tssrc/lib/onboard/preflight.tssrc/lib/onboard/resource-profile-selection.test.tssrc/lib/onboard/resource-profile-selection.tssrc/lib/onboard/sandbox-dockerfile-patch-flow.test.tssrc/lib/onboard/sandbox-dockerfile-patch-flow.tssrc/lib/onboard/sandbox-gpu-preflight.test.tssrc/lib/onboard/sandbox-gpu-preflight.tssrc/lib/onboard/sandbox-messaging-preflight.test.tssrc/lib/onboard/sandbox-messaging-preflight.tssrc/lib/onboard/sandbox-provider-cleanup.tssrc/lib/onboard/sandbox-registration.test.tssrc/lib/onboard/sandbox-registration.tssrc/lib/resources-cmd.test.tssrc/lib/resources-cmd.tssrc/lib/state/registry.tstest/gateway-state-reconcile-2276.test.tstest/onboard-exit-handler.test.tstest/rebuild-credential-preflight.test.tstest/rebuild-shields-auto-unlock.test.tstest/rebuild-stale-recovery.test.tstest/registry.test.tstest/repro-2201.test.tstest/sandbox-provider-cleanup.test.ts
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Contributor
Vitest E2E Target Results — ❌ Some jobs failedRun: 28629810289
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
test/rebuild-credential-preflight.test.ts (1)
261-279: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueFake
sandbox exechardcodes the--nameflag position and command-substring matching.This is reasonable for a hermetic fake, but the implementation implicitly assumes the production rebuild/exec path always passes
--name <sandbox>toopenshell sandbox execand that the probe/marker commands are recognizable purely via substring matching (rebuild-atomicity-marker.txt,https://inference.local/). If the real invocation format changes (flag order, alternate flag name, or command wrapping), this fixture could silently start returning wrong exit codes rather than failing loudly, masking a regression instead of catching it.As per path instructions for
**/*.test.{ts,js,mts,mjs,cts,cjs}, flag broad mocks/fakes whose internal logic embeds assumptions about the shape of the production call rather than only its externally observable inputs/outputs.🤖 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/rebuild-credential-preflight.test.ts` around lines 261 - 279, The fake sandbox exec handler in rebuild-credential-preflight.test.ts is too tightly coupled to the current command shape. Update the test stub around the sandbox exec branch to validate only the observable inputs it truly needs, and avoid relying on fixed `--name` position or substring-only command detection in the fake. Keep the logic localized to the sandbox exec mock and the related marker/probe handling so the fixture fails clearly if the production invocation format changes.Source: Path instructions
src/lib/actions/sandbox/rebuild-gpu-opt-out.ts (1)
214-216: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winShallow copy, not deep — verify
CustomPolicyEntryhas no nested mutable fields.
{ ...policy }only clones top-level properties. The line-range summary describes this as "deep-copied", which doesn't match the code. If policy entries hold nested arrays/objects (rules, matchers, etc.), the "authoritative" copy still aliases those nested references to the original persisted entry, risking unintended mutation of source state during rebuild.🔧 Proposed fix (if nested structures exist)
authoritativeCustomPolicies: Array.isArray(args.sb?.customPolicies) - ? args.sb.customPolicies.map((policy) => ({ ...policy })) + ? structuredClone(args.sb.customPolicies) : [],🤖 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-gpu-opt-out.ts` around lines 214 - 216, The authoritativeCustomPolicies copy in rebuild-gpu-opt-out is only a shallow clone via the map spread, so verify whether CustomPolicyEntry contains any nested mutable fields such as rules or matchers. If it does, update the rebuild path to perform a true deep copy for each policy entry in the authoritativeCustomPolicies mapping, or otherwise use an explicit cloning strategy in rebuild-gpu-opt-out to avoid aliasing nested state from args.sb.customPolicies.src/lib/onboard/docker-driver-gateway-start.test.ts (1)
131-153: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCoverage gap: fresh-start/spawn/poll path untested.
All four scenarios exit early via reuse/adopt/drift-restart-throw/missing-binary. None exercise the actual fresh-start path (spawn,
writeDockerDriverGatewayRuntimeMarkerForStateDir, health-poll loop) or a drift-restart that succeeds and falls through to the port-check/reuse logic below it. Given this module now centralizes the rebuild's runtime start orchestration, that path deserves direct coverage.🤖 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/onboard/docker-driver-gateway-start.test.ts` around lines 131 - 153, Add test coverage for the unexercised fresh-start flow in docker-driver-gateway-start.test.ts by driving the start path through spawn, writeDockerDriverGatewayRuntimeMarkerForStateDir, and the health-poll loop, not just the early-return cases. Use createHarness and start to assert the fresh-start orchestration when no recorded/adoptable PID is available, and add a case where restartForDrift succeeds so execution continues into the subsequent port-check/reuse logic instead of throwing.src/lib/onboard/authoritative-rebuild-messaging.ts (1)
40-63: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate sync/async gateway-scoping helpers.
withTargetGatewayandwithTargetGatewayAsyncare copy-pasted except forawait/Promisehandling, and the same restore-on-finally pattern is repeated a third time in the sibling test file (which is currently tripping a CI guardrail). Extracting one sharedrestoreEnvVar/scoping primitive would remove the duplication here and unblock the test-file guardrail failure in one change.♻️ Proposed consolidation
-function withTargetGateway<T>(targetGatewayName: string, action: () => T): T { - const previousGateway = process.env.OPENSHELL_GATEWAY; - process.env.OPENSHELL_GATEWAY = targetGatewayName; - try { - return action(); - } finally { - if (previousGateway === undefined) delete process.env.OPENSHELL_GATEWAY; - else process.env.OPENSHELL_GATEWAY = previousGateway; - } -} - -async function withTargetGatewayAsync<T>( - targetGatewayName: string, - action: () => Promise<T>, -): Promise<T> { - const previousGateway = process.env.OPENSHELL_GATEWAY; - process.env.OPENSHELL_GATEWAY = targetGatewayName; - try { - return await action(); - } finally { - if (previousGateway === undefined) delete process.env.OPENSHELL_GATEWAY; - else process.env.OPENSHELL_GATEWAY = previousGateway; - } -} +function restoreEnvVar(name: string, previousValue: string | undefined): void { + if (previousValue === undefined) delete process.env[name]; + else process.env[name] = previousValue; +} + +function withTargetGateway<T>(targetGatewayName: string, action: () => T): T { + const previousGateway = process.env.OPENSHELL_GATEWAY; + process.env.OPENSHELL_GATEWAY = targetGatewayName; + try { + return action(); + } finally { + restoreEnvVar("OPENSHELL_GATEWAY", previousGateway); + } +} + +async function withTargetGatewayAsync<T>( + targetGatewayName: string, + action: () => Promise<T>, +): Promise<T> { + const previousGateway = process.env.OPENSHELL_GATEWAY; + process.env.OPENSHELL_GATEWAY = targetGatewayName; + try { + return await action(); + } finally { + restoreEnvVar("OPENSHELL_GATEWAY", previousGateway); + } +}🤖 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/onboard/authoritative-rebuild-messaging.ts` around lines 40 - 63, The gateway-scoping logic is duplicated across withTargetGateway, withTargetGatewayAsync, and the sibling test helper, and the repeated restore-on-finally pattern is what is triggering the CI guardrail. Extract a shared environment-variable scoping/restore helper (for example, a single restoreEnvVar or generic wrapper used by both withTargetGateway and withTargetGatewayAsync), then update these helpers and the test-file copy to call that shared primitive so the OPENSHELL_GATEWAY setup/teardown lives in one place.src/lib/actions/sandbox/rebuild-flow-helpers.test.ts (1)
10-34: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor: duplicated dynamic-require loader scaffolding.
This file re-implements its own
requireDist/loadAgentDefs/loadAgentOnboardhelpers, while a siblingrebuild-flow.test-support.tsalready exposes an equivalentrequireDist. Consider consolidating into the shared test-support module to avoid two independently-maintained dynamic-require entry points drifting apart.🤖 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-flow-helpers.test.ts` around lines 10 - 34, The test file has duplicated dynamic-require loader scaffolding, including requireDist, loadAgentDefs, and loadAgentOnboard, which should be consolidated with the shared rebuild-flow.test-support helper. Update the rebuild-flow-helpers test to import and reuse the existing shared requireDist/exported loader utilities instead of maintaining its own entry points, and keep the unique symbols loadRebuildFlowHelpers, loadAgentDefs, and loadAgentOnboard aligned with the shared module.
🤖 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/onboard/authoritative-rebuild-messaging.test.ts`:
- Around line 72-76: The `afterEach` block in
`authoritative-rebuild-messaging.test.ts` adds a new inline conditional that
triggers the `test-conditionals:scan` guardrail. Remove the `if/else` from the
test by using a shared env restore helper, and reuse that helper in the
source-side gateway restore paths as well. Locate the cleanup logic around
`afterEach`, `withTargetGateway`, and `withTargetGatewayAsync`, and centralize
the previous-value restore behavior so the test file no longer contains the
conditional.
In `@src/lib/onboard/authoritative-rebuild-runtime.ts`:
- Around line 152-173: The `authoritativeRebuildRuntime` flow mutates
`env.OPENSHELL_LOCAL_TLS_DIR` before the readiness checks, but only
`deps.setRuntimeState(previous)` is restored in the `finally`, so the TLS
directory can leak after a failure. Update the `authoritativeRebuildRuntime`
cleanup to capture the prior `OPENSHELL_LOCAL_TLS_DIR` value and restore it in
the same `finally` block alongside the runtime-state rollback, and add/update a
test in `authoritative-rebuild-runtime.test.ts` to verify the env value is
restored when the transport-rejection path fails.
---
Nitpick comments:
In `@src/lib/actions/sandbox/rebuild-flow-helpers.test.ts`:
- Around line 10-34: The test file has duplicated dynamic-require loader
scaffolding, including requireDist, loadAgentDefs, and loadAgentOnboard, which
should be consolidated with the shared rebuild-flow.test-support helper. Update
the rebuild-flow-helpers test to import and reuse the existing shared
requireDist/exported loader utilities instead of maintaining its own entry
points, and keep the unique symbols loadRebuildFlowHelpers, loadAgentDefs, and
loadAgentOnboard aligned with the shared module.
In `@src/lib/actions/sandbox/rebuild-gpu-opt-out.ts`:
- Around line 214-216: The authoritativeCustomPolicies copy in
rebuild-gpu-opt-out is only a shallow clone via the map spread, so verify
whether CustomPolicyEntry contains any nested mutable fields such as rules or
matchers. If it does, update the rebuild path to perform a true deep copy for
each policy entry in the authoritativeCustomPolicies mapping, or otherwise use
an explicit cloning strategy in rebuild-gpu-opt-out to avoid aliasing nested
state from args.sb.customPolicies.
In `@src/lib/onboard/authoritative-rebuild-messaging.ts`:
- Around line 40-63: The gateway-scoping logic is duplicated across
withTargetGateway, withTargetGatewayAsync, and the sibling test helper, and the
repeated restore-on-finally pattern is what is triggering the CI guardrail.
Extract a shared environment-variable scoping/restore helper (for example, a
single restoreEnvVar or generic wrapper used by both withTargetGateway and
withTargetGatewayAsync), then update these helpers and the test-file copy to
call that shared primitive so the OPENSHELL_GATEWAY setup/teardown lives in one
place.
In `@src/lib/onboard/docker-driver-gateway-start.test.ts`:
- Around line 131-153: Add test coverage for the unexercised fresh-start flow in
docker-driver-gateway-start.test.ts by driving the start path through spawn,
writeDockerDriverGatewayRuntimeMarkerForStateDir, and the health-poll loop, not
just the early-return cases. Use createHarness and start to assert the
fresh-start orchestration when no recorded/adoptable PID is available, and add a
case where restartForDrift succeeds so execution continues into the subsequent
port-check/reuse logic instead of throwing.
In `@test/rebuild-credential-preflight.test.ts`:
- Around line 261-279: The fake sandbox exec handler in
rebuild-credential-preflight.test.ts is too tightly coupled to the current
command shape. Update the test stub around the sandbox exec branch to validate
only the observable inputs it truly needs, and avoid relying on fixed `--name`
position or substring-only command detection in the fake. Keep the logic
localized to the sandbox exec mock and the related marker/probe handling so the
fixture fails clearly if the production invocation format changes.
🪄 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: 50f2573a-842c-4dcf-bcff-7ac56638075d
📒 Files selected for processing (37)
docs/manage-sandboxes/lifecycle.mdxdocs/reference/commands-nemohermes.mdxdocs/reference/commands.mdxscripts/checks/run.tssrc/lib/actions/sandbox/rebuild-atomic-preflight.test.tssrc/lib/actions/sandbox/rebuild-custom-image-preflight.test.tssrc/lib/actions/sandbox/rebuild-custom-image-preflight.tssrc/lib/actions/sandbox/rebuild-env-isolation.test.tssrc/lib/actions/sandbox/rebuild-flow-helpers.test.tssrc/lib/actions/sandbox/rebuild-flow-helpers.tssrc/lib/actions/sandbox/rebuild-flow.test-support.tssrc/lib/actions/sandbox/rebuild-flow.test.tssrc/lib/actions/sandbox/rebuild-gateway-drift.test.tssrc/lib/actions/sandbox/rebuild-gpu-opt-out.tssrc/lib/actions/sandbox/rebuild-shields-finally.test.tssrc/lib/actions/sandbox/rebuild-signal-cleanup.test.tssrc/lib/actions/sandbox/rebuild-signal-cleanup.tssrc/lib/actions/sandbox/rebuild.tssrc/lib/onboard.tssrc/lib/onboard/authoritative-rebuild-messaging.test.tssrc/lib/onboard/authoritative-rebuild-messaging.tssrc/lib/onboard/authoritative-rebuild-policy.tssrc/lib/onboard/authoritative-rebuild-runtime.test.tssrc/lib/onboard/authoritative-rebuild-runtime.tssrc/lib/onboard/docker-driver-gateway-start.test.tssrc/lib/onboard/docker-driver-gateway-start.tssrc/lib/onboard/gateway-bound-operations.test.tssrc/lib/onboard/gateway-bound-operations.tssrc/lib/onboard/gateway-health-wait.tssrc/lib/onboard/gateway-start-failure.tssrc/lib/onboard/initial-policy.test.tssrc/lib/onboard/machine/handlers/provider-inference.test.tssrc/lib/onboard/machine/handlers/provider-inference.tssrc/lib/onboard/options.tssrc/lib/onboard/policy-selection-runtime.tstest/no-direct-credential-env.test.tstest/rebuild-credential-preflight.test.ts
✅ Files skipped from review due to trivial changes (2)
- src/lib/onboard/options.ts
- docs/reference/commands.mdx
🚧 Files skipped from review as they are similar to previous changes (13)
- src/lib/onboard/initial-policy.test.ts
- src/lib/onboard/machine/handlers/provider-inference.test.ts
- src/lib/actions/sandbox/rebuild-env-isolation.test.ts
- docs/manage-sandboxes/lifecycle.mdx
- src/lib/onboard/machine/handlers/provider-inference.ts
- src/lib/actions/sandbox/rebuild-atomic-preflight.test.ts
- src/lib/actions/sandbox/rebuild-flow-helpers.ts
- src/lib/actions/sandbox/rebuild-custom-image-preflight.ts
- src/lib/actions/sandbox/rebuild-shields-finally.test.ts
- src/lib/actions/sandbox/rebuild-flow.test-support.ts
- src/lib/actions/sandbox/rebuild-gateway-drift.test.ts
- src/lib/actions/sandbox/rebuild-flow.test.ts
- src/lib/onboard.ts
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
This was referenced Jul 3, 2026
Contributor
Vitest E2E Target Results — ✅ All selected jobs passedRun: 28630986293
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/lib/actions/sandbox/rebuild-custom-image-preflight.test.ts (1)
17-20: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: hoist the duplicated
restoreEnvhelper into shared test-support.This identical 3-line helper is copied across several rebuild/onboard test files (e.g.
rebuild-flow.test.ts,bridge-dns-preflight-exit.test.ts,authoritative-rebuild-*.test.ts). Consider exporting it once from a shared test-support module to keep env-restore semantics consistent.🤖 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-custom-image-preflight.test.ts` around lines 17 - 20, The duplicated restoreEnv helper used in rebuild/onboard tests should be centralized into shared test-support so env restoration stays consistent across files. Move the shared logic out of rebuild-custom-image-preflight.test.ts into a common exported helper in the test-support area, then update rebuild-flow.test.ts, bridge-dns-preflight-exit.test.ts, and the authoritative-rebuild tests to import and use it instead of copying the same implementation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lib/actions/sandbox/rebuild-flow.test.ts`:
- Around line 1259-1263: The sandbox list test stub currently calls the real
process.exit(1), which can kill the Vitest worker instead of letting the
assertion fail. In rebuild-flow.test.ts, update the harness used around
harness.captureOpenshellSpy so process.exit is stubbed/spied to throw or
otherwise remain test-safe before the sandbox list branch is exercised. Keep the
fix scoped to the test harness setup so the sandbox list path can be asserted
without exiting the test process.
In `@src/lib/actions/sandbox/rebuild-provider-detach-transaction.ts`:
- Around line 76-86: The signal cleanup path in rollbackForSignal only calls
options.relock() and rollback(), so SIGINT/SIGTERM can be swallowed and leave
the CLI running. Update rebuild-provider-detach-transaction’s signal handlers so
onSigint and onSigterm perform the existing cleanup and then re-raise the same
signal or terminate with the corresponding exit behavior, matching the rebuild
signal-recovery pattern while keeping the onExit cleanup intact.
---
Nitpick comments:
In `@src/lib/actions/sandbox/rebuild-custom-image-preflight.test.ts`:
- Around line 17-20: The duplicated restoreEnv helper used in rebuild/onboard
tests should be centralized into shared test-support so env restoration stays
consistent across files. Move the shared logic out of
rebuild-custom-image-preflight.test.ts into a common exported helper in the
test-support area, then update rebuild-flow.test.ts,
bridge-dns-preflight-exit.test.ts, and the authoritative-rebuild tests to import
and use it instead of copying the same implementation.
🪄 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: 3664f26a-3664-4cb7-af95-a9e2c0993e46
📒 Files selected for processing (14)
src/lib/actions/sandbox/rebuild-atomic-preflight.test.tssrc/lib/actions/sandbox/rebuild-custom-image-preflight.test.tssrc/lib/actions/sandbox/rebuild-custom-image-preflight.tssrc/lib/actions/sandbox/rebuild-flow.test.tssrc/lib/actions/sandbox/rebuild-inference-preflight.test.tssrc/lib/actions/sandbox/rebuild-provider-detach-transaction.tssrc/lib/actions/sandbox/rebuild-signal-cleanup.test.tssrc/lib/actions/sandbox/rebuild-web-search-preflight.test.tssrc/lib/actions/sandbox/rebuild.tssrc/lib/onboard/authoritative-rebuild-messaging.test.tssrc/lib/onboard/authoritative-rebuild-runtime.test.tssrc/lib/onboard/authoritative-rebuild-runtime.tssrc/lib/onboard/authoritative-rebuild-target.test.tssrc/lib/onboard/bridge-dns-preflight-exit.test.ts
✅ Files skipped from review due to trivial changes (1)
- src/lib/actions/sandbox/rebuild-web-search-preflight.test.ts
🚧 Files skipped from review as they are similar to previous changes (9)
- src/lib/actions/sandbox/rebuild-inference-preflight.test.ts
- src/lib/onboard/authoritative-rebuild-runtime.test.ts
- src/lib/actions/sandbox/rebuild-signal-cleanup.test.ts
- src/lib/onboard/authoritative-rebuild-target.test.ts
- src/lib/actions/sandbox/rebuild-atomic-preflight.test.ts
- src/lib/onboard/authoritative-rebuild-messaging.test.ts
- src/lib/actions/sandbox/rebuild-custom-image-preflight.ts
- src/lib/onboard/authoritative-rebuild-runtime.ts
- src/lib/onboard/bridge-dns-preflight-exit.test.ts
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Contributor
Vitest E2E Target Results — ❌ Some jobs failedRun: 28630986312
|
Contributor
Vitest E2E Target Results — ❌ Some jobs failedRun: 28632214086
|
Collaborator
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Collaborator
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/lib/onboard/prepared-dcode-rebuild.ts (1)
17-17: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse the existing DCode agent-name constant
src/lib/onboard/prepared-dcode-rebuild.ts:17should useDCODE_AGENT_NAMEfromsrc/lib/actions/sandbox/rebuild-dcode-target.tsinstead of repeating"langchain-deepagents-code"here, so the rebuild path keeps one source of truth.🤖 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/onboard/prepared-dcode-rebuild.ts` at line 17, The prepared DCode rebuild path is hardcoding the agent name instead of reusing the shared constant. Update the `DCODE_AGENT` definition in `prepared-dcode-rebuild` to reference `DCODE_AGENT_NAME` from `rebuild-dcode-target.ts`, so both `prepared-dcode-rebuild` and `rebuild-dcode-target` use the same source of truth.
🤖 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/onboard/prepared-dcode-rebuild.test.ts`:
- Around line 79-95: The test in createPreparedDcodeRebuildRuntime’s
bindCreateSandbox mock currently uses a manual conditional inside the create fn,
which violates the growth-guardrail. Replace the branching on attempt with
explicit mockImplementationOnce chaining on the vi.fn in
prepared-dcode-rebuild.test.ts so the first call throws and the second resolves,
while still asserting the contexts array behavior.
In `@src/lib/onboard/prepared-dcode-rebuild.ts`:
- Around line 136-151: resolveSandboxBuildId currently trusts
preparedBuildContext.buildId without enforcing the same target check as
resolveSandboxBuildContext. Mirror that guard in resolveSandboxBuildId by
validating the input’s agent.name and fromDockerfile before returning
preparedBuildContext.buildId, and reject mismatched/non-dcode targets the same
way as resolveSandboxBuildContext. Use the existing resolveSandboxBuildId and
resolveSandboxBuildContext flow in prepared-dcode-rebuild to keep the
atomic-rebuild path consistent and prevent stale build IDs from being accepted.
---
Nitpick comments:
In `@src/lib/onboard/prepared-dcode-rebuild.ts`:
- Line 17: The prepared DCode rebuild path is hardcoding the agent name instead
of reusing the shared constant. Update the `DCODE_AGENT` definition in
`prepared-dcode-rebuild` to reference `DCODE_AGENT_NAME` from
`rebuild-dcode-target.ts`, so both `prepared-dcode-rebuild` and
`rebuild-dcode-target` use the same source of truth.
🪄 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: 8796da7f-6d7e-43c2-afcc-f664e02fa5ff
📒 Files selected for processing (7)
src/lib/actions/sandbox/rebuild-flow.test.tssrc/lib/actions/sandbox/rebuild-gpu-opt-out.tssrc/lib/actions/sandbox/rebuild.tssrc/lib/onboard.tssrc/lib/onboard/build-context-stage.tssrc/lib/onboard/prepared-dcode-rebuild.test.tssrc/lib/onboard/prepared-dcode-rebuild.ts
💤 Files with no reviewable changes (1)
- src/lib/onboard/build-context-stage.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/lib/actions/sandbox/rebuild-gpu-opt-out.ts
- src/lib/onboard.ts
- src/lib/actions/sandbox/rebuild.ts
- src/lib/actions/sandbox/rebuild-flow.test.ts
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Collaborator
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
test/helpers/rebuild-flow-harness.ts (1)
291-299: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
getSandboxmock silently falls back oncesandboxEntryReadsis exhausted.The mock consumes
overrides.sandboxEntryReadsstrictly by call order/count and falls back tosandboxEntryafter the list is exhausted, with no distinction between "not overridden" and "read past expected count." If a future implementation change adds/removes an intermediateregistry.getSandbox()call, consuming tests (which hard-code exact read sequences, e.g. inrebuild-dcode-flow.test.ts) would silently receive drifted/stale data instead of failing loudly, undermining confidence that the "drift" assertions are actually exercising the intended call site.Consider throwing (or returning an obviously-invalid sentinel) once reads exceed
configuredReads.lengthwhensandboxEntryReadswas explicitly provided, so an unexpected extra/missing call fails the test instead of passing silently.🤖 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/helpers/rebuild-flow-harness.ts` around lines 291 - 299, The getSandbox mock in rebuild-flow-harness currently falls back to sandboxEntry after consuming all configured sandboxEntryReads, which can hide unexpected extra or missing calls. Update the mock around registry.getSandbox so that when overrides.sandboxEntryReads is explicitly provided, any read beyond the configured sequence fails loudly (for example by throwing or returning a clearly invalid sentinel) instead of silently returning sandboxEntry. Keep the normal fallback only when sandboxEntryReads was not supplied, so tests like rebuild-dcode-flow.test.ts continue to validate the intended call order.src/lib/actions/sandbox/rebuild-dcode-flow.test.ts (1)
149-175: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffTests are coupled to the exact number/order of internal
getSandbox()calls.Both tests hard-code the precise sequence of
registry.getSandbox()reads viasandboxEntryReads, annotated with comments describing internal orchestration steps ("Messaging-conflict gateway selection", "Prepared DCode target capture", "Delete-edge target verification input", etc.). This asserts drift-detection by exploiting the harness's positional-array fallback (see companion comment inrebuild-flow-harness.ts) rather than through an observable public-boundary signal. If the implementation's internal read count changes for unrelated reasons, these tests would silently consume the wrong drifted/undrifted entry at the wrong step instead of failing with a clear signal.As per path instructions, "Prefer observable outcomes through the public boundary over source-text, private-shape, or mock-call assertions" for
**/*.test.{ts,js,mts,mjs,cts,cjs}. Consider having the drift live only on the read(s) that matter and letting the harness fail loudly if the read count assumption is exceeded (see companion refactor suggestion in the harness file).Also applies to: 200-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/actions/sandbox/rebuild-dcode-flow.test.ts` around lines 149 - 175, The DCode rebuild tests are coupling drift detection to the exact sequence of internal getSandbox reads via sandboxEntryReads, which makes them brittle. Update the test to assert the observable failure at the public rebuildSandbox boundary and avoid relying on positional fallback comments for orchestration steps; keep drift only on the read that matters, and let createRebuildFlowHarness or the registry access fail loudly if extra reads occur. Use the existing rebuildSandbox, configureDcodeSession, and createRebuildFlowHarness setup to preserve the same behavior without hard-coding internal call order.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.
Nitpick comments:
In `@src/lib/actions/sandbox/rebuild-dcode-flow.test.ts`:
- Around line 149-175: The DCode rebuild tests are coupling drift detection to
the exact sequence of internal getSandbox reads via sandboxEntryReads, which
makes them brittle. Update the test to assert the observable failure at the
public rebuildSandbox boundary and avoid relying on positional fallback comments
for orchestration steps; keep drift only on the read that matters, and let
createRebuildFlowHarness or the registry access fail loudly if extra reads
occur. Use the existing rebuildSandbox, configureDcodeSession, and
createRebuildFlowHarness setup to preserve the same behavior without hard-coding
internal call order.
In `@test/helpers/rebuild-flow-harness.ts`:
- Around line 291-299: The getSandbox mock in rebuild-flow-harness currently
falls back to sandboxEntry after consuming all configured sandboxEntryReads,
which can hide unexpected extra or missing calls. Update the mock around
registry.getSandbox so that when overrides.sandboxEntryReads is explicitly
provided, any read beyond the configured sequence fails loudly (for example by
throwing or returning a clearly invalid sentinel) instead of silently returning
sandboxEntry. Keep the normal fallback only when sandboxEntryReads was not
supplied, so tests like rebuild-dcode-flow.test.ts continue to validate the
intended call order.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: bd08cdf5-d27b-4a20-9cb0-32cc1723221a
📒 Files selected for processing (7)
src/lib/actions/sandbox/rebuild-dcode-flow.test.tssrc/lib/actions/sandbox/rebuild-dcode-orchestrator.tssrc/lib/actions/sandbox/rebuild-flow.test.tssrc/lib/actions/sandbox/rebuild-managed-image-preflight.test.tssrc/lib/actions/sandbox/rebuild-managed-image-preflight.tssrc/lib/actions/sandbox/rebuild.tstest/helpers/rebuild-flow-harness.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib/actions/sandbox/rebuild.ts
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Collaborator
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/lib/actions/sandbox/rebuild-dcode-flow.test.ts (1)
297-349: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract shared assertion helper for "preserved after backup" tests.
These two tests (and per the line-range summary, likely 230-296 as well) repeat the same block:
openShieldsSpy/backupSandboxStateSpycalled once,runOpenshellSpynot called with delete,removeSandboxRegistryEntrySpy/onboardSpynot called,relockSpycalled with fixed args, anddisposePreparedDcodeRebuildImageSpycalled with the prepared context. Consider factoring this into a helper alongside the existingexpectNoDcodeMutation(lines 15-59) to reduce duplication and keep future assertion changes in one place.♻️ Suggested helper
+function expectDcodePreservedAfterBackup(harness: ReturnType<typeof createRebuildFlowHarness>): void { + expect(harness.openShieldsSpy).toHaveBeenCalledOnce(); + expect(harness.backupSandboxStateSpy).toHaveBeenCalledOnce(); + expect(harness.runOpenshellSpy).not.toHaveBeenCalledWith( + ["sandbox", "delete", "alpha"], + expect.anything(), + ); + expect(harness.removeSandboxRegistryEntrySpy).not.toHaveBeenCalled(); + expect(harness.onboardSpy).not.toHaveBeenCalled(); + expect(harness.relockSpy).toHaveBeenCalledWith("alpha", expect.any(Object), true, "nemoclaw"); + expect(harness.disposePreparedDcodeRebuildImageSpy).toHaveBeenCalledWith( + harness.preparedDcodeBuildContext, + ); +}🤖 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-dcode-flow.test.ts` around lines 297 - 349, The “preserved after backup” rebuild tests are duplicating the same assertion block for the no-mutation path. Factor those repeated checks into a shared helper near the existing expectNoDcodeMutation helper, and have the affected tests call it instead of inlining the repeated assertions. Keep the helper centered on the rebuildSandbox harness spies: openShieldsSpy, backupSandboxStateSpy, runOpenshellSpy, removeSandboxRegistryEntrySpy, onboardSpy, relockSpy, and disposePreparedDcodeRebuildImageSpy.
🤖 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.
Nitpick comments:
In `@src/lib/actions/sandbox/rebuild-dcode-flow.test.ts`:
- Around line 297-349: The “preserved after backup” rebuild tests are
duplicating the same assertion block for the no-mutation path. Factor those
repeated checks into a shared helper near the existing expectNoDcodeMutation
helper, and have the affected tests call it instead of inlining the repeated
assertions. Keep the helper centered on the rebuildSandbox harness spies:
openShieldsSpy, backupSandboxStateSpy, runOpenshellSpy,
removeSandboxRegistryEntrySpy, onboardSpy, relockSpy, and
disposePreparedDcodeRebuildImageSpy.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: bc507122-133b-4972-8b2c-1c883d37a011
📒 Files selected for processing (6)
src/lib/actions/sandbox/rebuild-dcode-flow.test.tssrc/lib/actions/sandbox/rebuild-managed-image-preflight.test.tssrc/lib/actions/sandbox/rebuild-managed-image-preflight.tssrc/lib/onboard/prepared-dcode-rebuild.test.tssrc/lib/onboard/prepared-dcode-rebuild.tstest/helpers/rebuild-flow-harness.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/lib/onboard/prepared-dcode-rebuild.ts
- src/lib/onboard/prepared-dcode-rebuild.test.ts
- test/helpers/rebuild-flow-harness.ts
Collaborator
Author
|
Final-head review disposition for
Nemotron's remaining required classifications do not identify a current defect:
The final tests independently cover an unrecoverable recorded gateway, retained-context drift after backup, and pinned-base-image identity drift after backup; all preserve the live sandbox and block delete/recreate. |
9 of 21 tasks
ericksoa
added a commit
that referenced
this pull request
Jul 4, 2026
<!-- markdownlint-disable MD041 --> ## Summary This PR prepares the user-facing documentation for v0.0.74 before the release plan is frozen. It expands the release notes across the 56-commit train and closes durable documentation gaps found during the pre-tag commit scan. ## Changes - Expand the `v0.0.74` release notes to cover OpenShell 0.0.72, managed MCP, progressive tool disclosure, LangChain Deep Agents Code, onboarding, local inference, messaging, recovery, and contributor workflows. - Correct the `destroy` contract for retained per-name volumes, gateway-unreachable `--force` cleanup, managed MCP ownership, and same-name recovery. - Document separate remediation for an unreachable container DNS resolver versus one that answers with `NXDOMAIN` or `REFUSED`. - Document the Windows on Arm N1X automatic Ollama safeguard and its remaining large-model limitations. - State that messaging conflicts abort rebuild before backup or deletion, leaving the original sandbox intact. - Link the agent-runnable value benchmark from the contributor task index. - Synchronize generated agent command variants. - Validate with `npm run docs:sync-agent-variants` and `npm run docs`; Fern completed with 0 errors and 2 existing warnings. - Source summary: - [#6020](#6020) and [#5876](#5876) -> `docs/about/release-notes.mdx`: Consolidate the OpenShell 0.0.72 policy boundary and managed MCP lifecycle. - [#6251](#6251) and [#5989](#5989) -> `docs/about/release-notes.mdx`: Summarize progressive tool disclosure and sandbox-first inference controls. - [#6232](#6232), [#6082](#6082), [#6219](#6219), [#6214](#6214), [#6215](#6215), [#6230](#6230), and [#6260](#6260) -> `docs/about/release-notes.mdx`: Summarize the experimental LangChain Deep Agents Code status, secret, version, rebuild, snapshot, and MCP boundaries. - [#6166](#6166), [#6254](#6254), [#6265](#6265), [#6164](#6164), and [#6017](#6017) -> `docs/about/release-notes.mdx`: Summarize BuildKit prebuild, validated image reuse, bounded readiness, and preflight improvements. - [#6150](#6150) -> `docs/about/release-notes.mdx` and `docs/reference/troubleshooting.mdx`: Separate unreachable-resolver remediation from reachable-but-rejected DNS responses. - [#6234](#6234) -> `docs/about/release-notes.mdx`, `docs/inference/use-local-inference.mdx`, and `docs/get-started/windows-preparation.mdx`: Document N1X automatic 9B selection and the remaining explicit-large-model boundary. - [#6129](#6129), [#5987](#5987), [#5955](#5955), and [#6220](#6220) -> `docs/about/release-notes.mdx`, `docs/manage-sandboxes/messaging-channels.mdx`, `docs/reference/commands.mdx`, and `docs/reference/commands-nemohermes.mdx`: Document messaging policy persistence, status, and the pre-destructive conflict check. - [#5963](#5963), [#6050](#6050), [#6094](#6094), [#6238](#6238), [#5988](#5988), [#6235](#6235), [#6181](#6181), and [#5986](#5986) -> `docs/about/release-notes.mdx`, `docs/reference/commands.mdx`, and `docs/reference/commands-nemohermes.mdx`: Summarize day-two recovery and clarify retained-volume and local-only destroy semantics. - [#6200](#6200), [#6248](#6248), [#6168](#6168), [#6270](#6270), and [#5649](#5649) -> `docs/about/release-notes.mdx` and `CONTRIBUTING.md`: Summarize contributor setup and verification improvements and expose the advisory value benchmark. ## Type of Change - [ ] Code change (feature, bug fix, or refactor) - [ ] Code change with doc updates - [ ] Doc only (prose changes, no code sample modifications) - [x] Doc only (includes code sample changes) ## Quality Gates <!-- Check exactly one tests line and one docs line. Check other lines when applicable. Add every requested justification or approval reference. --> - [ ] Tests added or updated for changed behavior - [ ] Existing tests cover changed behavior — justification: - [x] Tests not applicable — justification: documentation-only release preparation; generated-variant synchronization and the Fern docs build validate the changed pages and routes. - [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: ## Verification <!-- Check each applicable item only when supported by the requested evidence. Run targeted tests once per relevant change set and rerun after later edits or hook autofixes that can affect the tested behavior. Do not rerun hook-covered checks. --> - [x] PR description includes the DCO sign-off declaration and every commit appears as `Verified` in GitHub - [x] Normal `pre-commit`, `commit-msg`, and `pre-push` hooks passed, or `npm run check:diff` passed 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: tests are not applicable to this documentation-only change; `npm run docs` validates the source and generated routes. - [ ] Applicable broad gate passed — `npm test` for broad runtime/test-harness changes; `npm run check` for repo-wide validation/coverage changes — command/result: - [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) --- <!-- DCO sign-off is required in this PR description, and every commit must appear as Verified in GitHub. Run: git config user.name && git config user.email --> Signed-off-by: Aaron Erickson <aerickson@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Expanded setup guidance for Windows on Arm devices with safer default local model selection. * Clarified local inference and sandbox messaging behavior, including conflict checks before rebuilds and safer recovery steps. * Updated destroy/rebuild/reference docs with more detailed warnings, failure handling, and volume-retention guidance. * Improved troubleshooting instructions for Docker DNS issues with clearer paths for unreachable vs. blocked resolvers. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
20 tasks
Hadar301
pushed a commit
to Hadar301/NemoClaw-OpenShift
that referenced
this pull request
Jul 12, 2026
<!-- markdownlint-disable MD041 --> ## Summary <!-- 1-3 sentences: what this PR does and why. --> Prevents a Ready LangChain Deep Agents Code sandbox from being deleted until its recorded OpenShell gateway, live inference route, managed base image, and staged build context have been validated and sealed. A DCode-only, process-local orchestration facade carries that verified gateway/context pair through recreation, fixing the post-delete port-8080 false conflict from NVIDIA#6195 without adding generalized persisted replay or FSM state. ## Related Issue <!-- Fixes #NNN or Closes #NNN. Remove this section if none. --> Fixes NVIDIA#6195 Refs NVIDIA#6218 Refs NVIDIA#6224 Refs NVIDIA#6226 Refs NVIDIA#5801 ## Changes <!-- Bullet list of key changes. --> - Bind DCode rebuilds to the sandbox's recorded gateway before credential validation, probe the live `https://inference.local` route without exposing credentials, and revalidate the gateway, registry entry, route, schema, base image, and staged context immediately before deletion. - Build a disposable DCode image and fingerprint its managed build context before mutation, then pass the retained context and recorded gateway as a one-shot handoff through the create-intent seam landed in NVIDIA#6218. Ordinary onboarding and non-DCode rebuild behavior remain unchanged. - Put the DCode lifecycle in `rebuild-dcode-orchestrator.ts`: it owns scoped gateway cleanup, target preflight, replacement preparation, mutation-edge revalidation, recovery behavior, and one-shot handoff cleanup while depending on injected generic rebuild callbacks. This reduced `rebuild.ts` from 1,599 to 1,460 lines and split its 1,477-line flow spec into focused generic and DCode suites plus a shared harness. - Extract the onboarding handoff into `prepared-dcode-rebuild.ts`, which owns gateway validation, ordinary-versus-prepared staging/patching, and consume-before-call semantics. The top-level `onboard.ts` entrypoint is four lines smaller than `main`. - Preserve the existing sandbox when initial preflight fails; if mutation-edge validation fails after backup, keep the harmless backup but do not delete or recreate the sandbox. Prepared-backup recovery skips unavailable live-route probes and does not take a second backup. Messaging-preflight aborts also restore the prior process gateway. - Harden managed-context sealing against symlink substitution, pathname replacement, and in-place mutation by reading regular files through `O_NOFOLLOW` file descriptors and checking identity/metadata before and after each read. - Add a live DCode lifecycle fixture that rotates the credential stored in the real gateway, waits for a 401, runs rebuild without a host credential, and proves there was no backup/delete/create while container identity, readiness, and a workspace marker remain intact. - Document the DCode rebuild preflight and backup boundary in the quickstart. A documentation review confirmed no further user-facing changes are needed. - Keep generalized FSM/session capture and cross-agent replay out of scope for NVIDIA#6224/NVIDIA#6226; true build/health/swap replacement remains NVIDIA#5801. ## 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 <!-- Check all that apply. For any "covered by existing tests", "not applicable", or waiver entry, add a brief justification on the same line or in the Changes section. --> - [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 final architecture and nine-category security reviews both passed the narrowed DCode-only design with no blocker; the security review included the final orchestration extraction and managed-context TOCTOU hardening. - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## Verification <!-- Check each item you ran and confirmed. Leave unchecked items you skipped. Doc-only changes do not require npm test unless you ran it. --> - [x] PR description includes the DCO sign-off declaration and every commit appears as `Verified` in GitHub - [x] Git hooks passed during commit and push, or `npx prek run --from-ref main --to-ref HEAD` passes - [x] Targeted tests pass for changed behavior - [x] Full `npm test` passes (broad runtime changes only) - [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) Commands and evidence: - `make check` — passed on the final narrowed implementation under the repository-expected `umask 022`. - `npm test` — 998 files passed, 2 platform suites skipped; 11,277 tests passed, 35 skipped. - Post-merge focused/current-main set — 96/96 passed across generic rebuild flow, DCode flow, managed-image preflight, prepared handoff, inference-provider compatibility, and messaging setup; the final descriptor-pinning, target-guard, and fail-closed review deltas passed 27/27 focused cases. - Additional affected environment-failure audit — 121/121 passed; E2E support — 26/26 passed. - `npm run build:cli` and `npm run typecheck:cli` — passed. - Current-tree Vitest project-membership check — 1,075 files across eight disjoint projects; source-shape, test-file-size, and changed-test conditional budgets passed. - `npm run docs` — passed with 0 errors and 2 pre-existing warnings. - Pre-push plugin and CLI TypeScript gates — passed. - GitHub commit verification — 18/18 commits report `verified=true`, reason `valid`. --- <!-- DCO sign-off is required in this PR description, and every commit must appear as Verified in GitHub. Run: git config user.name && git config user.email --> Signed-off-by: Carlos Villela <cvillela@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added support for a new DCode rebuild flow during onboarding and sandbox recreation. * Introduced a preflight check for inference routing before rebuilds proceed. * **Bug Fixes** * Rebuilds now fail safely when credentials or routing are invalid, helping prevent unsafe sandbox changes. * Improved rebuild handling so existing sandboxes and backups are preserved if a later check fails. * **Documentation** * Updated the quickstart guide with clearer rebuild and backup behavior for managed DCode sandboxes. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Hadar301
pushed a commit
to Hadar301/NemoClaw-OpenShift
that referenced
this pull request
Jul 12, 2026
<!-- markdownlint-disable MD041 --> ## Summary This PR prepares the user-facing documentation for v0.0.74 before the release plan is frozen. It expands the release notes across the 56-commit train and closes durable documentation gaps found during the pre-tag commit scan. ## Changes - Expand the `v0.0.74` release notes to cover OpenShell 0.0.72, managed MCP, progressive tool disclosure, LangChain Deep Agents Code, onboarding, local inference, messaging, recovery, and contributor workflows. - Correct the `destroy` contract for retained per-name volumes, gateway-unreachable `--force` cleanup, managed MCP ownership, and same-name recovery. - Document separate remediation for an unreachable container DNS resolver versus one that answers with `NXDOMAIN` or `REFUSED`. - Document the Windows on Arm N1X automatic Ollama safeguard and its remaining large-model limitations. - State that messaging conflicts abort rebuild before backup or deletion, leaving the original sandbox intact. - Link the agent-runnable value benchmark from the contributor task index. - Synchronize generated agent command variants. - Validate with `npm run docs:sync-agent-variants` and `npm run docs`; Fern completed with 0 errors and 2 existing warnings. - Source summary: - [NVIDIA#6020](NVIDIA#6020) and [NVIDIA#5876](NVIDIA#5876) -> `docs/about/release-notes.mdx`: Consolidate the OpenShell 0.0.72 policy boundary and managed MCP lifecycle. - [NVIDIA#6251](NVIDIA#6251) and [NVIDIA#5989](NVIDIA#5989) -> `docs/about/release-notes.mdx`: Summarize progressive tool disclosure and sandbox-first inference controls. - [NVIDIA#6232](NVIDIA#6232), [NVIDIA#6082](NVIDIA#6082), [NVIDIA#6219](NVIDIA#6219), [NVIDIA#6214](NVIDIA#6214), [NVIDIA#6215](NVIDIA#6215), [NVIDIA#6230](NVIDIA#6230), and [NVIDIA#6260](NVIDIA#6260) -> `docs/about/release-notes.mdx`: Summarize the experimental LangChain Deep Agents Code status, secret, version, rebuild, snapshot, and MCP boundaries. - [NVIDIA#6166](NVIDIA#6166), [NVIDIA#6254](NVIDIA#6254), [NVIDIA#6265](NVIDIA#6265), [NVIDIA#6164](NVIDIA#6164), and [NVIDIA#6017](NVIDIA#6017) -> `docs/about/release-notes.mdx`: Summarize BuildKit prebuild, validated image reuse, bounded readiness, and preflight improvements. - [NVIDIA#6150](NVIDIA#6150) -> `docs/about/release-notes.mdx` and `docs/reference/troubleshooting.mdx`: Separate unreachable-resolver remediation from reachable-but-rejected DNS responses. - [NVIDIA#6234](NVIDIA#6234) -> `docs/about/release-notes.mdx`, `docs/inference/use-local-inference.mdx`, and `docs/get-started/windows-preparation.mdx`: Document N1X automatic 9B selection and the remaining explicit-large-model boundary. - [NVIDIA#6129](NVIDIA#6129), [NVIDIA#5987](NVIDIA#5987), [NVIDIA#5955](NVIDIA#5955), and [NVIDIA#6220](NVIDIA#6220) -> `docs/about/release-notes.mdx`, `docs/manage-sandboxes/messaging-channels.mdx`, `docs/reference/commands.mdx`, and `docs/reference/commands-nemohermes.mdx`: Document messaging policy persistence, status, and the pre-destructive conflict check. - [NVIDIA#5963](NVIDIA#5963), [NVIDIA#6050](NVIDIA#6050), [NVIDIA#6094](NVIDIA#6094), [NVIDIA#6238](NVIDIA#6238), [NVIDIA#5988](NVIDIA#5988), [NVIDIA#6235](NVIDIA#6235), [NVIDIA#6181](NVIDIA#6181), and [NVIDIA#5986](NVIDIA#5986) -> `docs/about/release-notes.mdx`, `docs/reference/commands.mdx`, and `docs/reference/commands-nemohermes.mdx`: Summarize day-two recovery and clarify retained-volume and local-only destroy semantics. - [NVIDIA#6200](NVIDIA#6200), [NVIDIA#6248](NVIDIA#6248), [NVIDIA#6168](NVIDIA#6168), [NVIDIA#6270](NVIDIA#6270), and [NVIDIA#5649](NVIDIA#5649) -> `docs/about/release-notes.mdx` and `CONTRIBUTING.md`: Summarize contributor setup and verification improvements and expose the advisory value benchmark. ## Type of Change - [ ] Code change (feature, bug fix, or refactor) - [ ] Code change with doc updates - [ ] Doc only (prose changes, no code sample modifications) - [x] Doc only (includes code sample changes) ## Quality Gates <!-- Check exactly one tests line and one docs line. Check other lines when applicable. Add every requested justification or approval reference. --> - [ ] Tests added or updated for changed behavior - [ ] Existing tests cover changed behavior — justification: - [x] Tests not applicable — justification: documentation-only release preparation; generated-variant synchronization and the Fern docs build validate the changed pages and routes. - [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: ## Verification <!-- Check each applicable item only when supported by the requested evidence. Run targeted tests once per relevant change set and rerun after later edits or hook autofixes that can affect the tested behavior. Do not rerun hook-covered checks. --> - [x] PR description includes the DCO sign-off declaration and every commit appears as `Verified` in GitHub - [x] Normal `pre-commit`, `commit-msg`, and `pre-push` hooks passed, or `npm run check:diff` passed 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: tests are not applicable to this documentation-only change; `npm run docs` validates the source and generated routes. - [ ] Applicable broad gate passed — `npm test` for broad runtime/test-harness changes; `npm run check` for repo-wide validation/coverage changes — command/result: - [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) --- <!-- DCO sign-off is required in this PR description, and every commit must appear as Verified in GitHub. Run: git config user.name && git config user.email --> Signed-off-by: Aaron Erickson <aerickson@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Expanded setup guidance for Windows on Arm devices with safer default local model selection. * Clarified local inference and sandbox messaging behavior, including conflict checks before rebuilds and safer recovery steps. * Updated destroy/rebuild/reference docs with more detailed warnings, failure handling, and volume-retention guidance. * Improved troubleshooting instructions for Docker DNS issues with clearer paths for unreachable vs. blocked resolvers. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
12 of 23 tasks
9 of 13 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Prevents a Ready LangChain Deep Agents Code sandbox from being deleted until its recorded OpenShell gateway, live inference route, managed base image, and staged build context have been validated and sealed. A DCode-only, process-local orchestration facade carries that verified gateway/context pair through recreation, fixing the post-delete port-8080 false conflict from #6195 without adding generalized persisted replay or FSM state.
Related Issue
Fixes #6195
Refs #6218
Refs #6224
Refs #6226
Refs #5801
Changes
https://inference.localroute without exposing credentials, and revalidate the gateway, registry entry, route, schema, base image, and staged context immediately before deletion.rebuild-dcode-orchestrator.ts: it owns scoped gateway cleanup, target preflight, replacement preparation, mutation-edge revalidation, recovery behavior, and one-shot handoff cleanup while depending on injected generic rebuild callbacks. This reducedrebuild.tsfrom 1,599 to 1,460 lines and split its 1,477-line flow spec into focused generic and DCode suites plus a shared harness.prepared-dcode-rebuild.ts, which owns gateway validation, ordinary-versus-prepared staging/patching, and consume-before-call semantics. The top-levelonboard.tsentrypoint is four lines smaller thanmain.O_NOFOLLOWfile descriptors and checking identity/metadata before and after each read.Type of Change
Quality Gates
Verification
Verifiedin GitHubnpx prek run --from-ref main --to-ref HEADpassesnpm testpasses (broad runtime changes only)npm run docsbuilds without warnings (doc changes only)Commands and evidence:
make check— passed on the final narrowed implementation under the repository-expectedumask 022.npm test— 998 files passed, 2 platform suites skipped; 11,277 tests passed, 35 skipped.npm run build:cliandnpm run typecheck:cli— passed.npm run docs— passed with 0 errors and 2 pre-existing warnings.verified=true, reasonvalid.Signed-off-by: Carlos Villela cvillela@nvidia.com
Summary by CodeRabbit
New Features
Bug Fixes
Documentation