feat(onboard): gate buildless managed workloads - #8261
Conversation
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
|
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 change adds managed-image onboarding, transactional startup profiles, Docker bootstrap authority storage, managed rebuild mutation validation, buildless end-to-end coverage, and updated documentation. Legacy Dockerfile behavior remains available through ChangesManaged image lifecycle
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in commit da4fce1 in the TypeScript / code-coverage/cliThe overall coverage in commit da4fce1 in the Show a code coverage summary of the most impacted files.
Updated |
|
🌿 Preview your docs: https://nvidia-preview-pr-8261.docs.buildwithfern.com/nemoclaw |
PR Review Advisor — InformationalAdvisor assessment: Informational / low confidence Model lanes
Second-opinion terminology and E2E selections are advisory. They do not change the primary assessment or E2E / PR Gate. E2E guidanceAdvisory only. E2E / PR Gate selects and runs jobs independently. Recommended E2E: This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
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-dcode-orchestrator.ts (1)
252-284: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winReport the managed path failure with an accurate message.
The managed branch prepares no replacement, but a failure still reports
"DCode replacement validation failed before sandbox deletion.". The operator sees a replacement-artifact message for a managed workload authority failure at the delete edge. Select the message frommanagedWorkloadRebuild.🐛 Proposed message fix
if (!valid) { scope.cleanup(); return { ok: false, - message: "DCode replacement validation failed before sandbox deletion.", + message: managedWorkloadRebuild + ? "Managed DCode workload validation failed before sandbox deletion." + : "DCode replacement validation failed before sandbox deletion.", }; }🤖 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-orchestrator.ts` around lines 252 - 284, Update the failure message in the !valid branch of the managedWorkloadRebuild/revalidateDcodeReplacementAtMutationEdge flow to select an accurate message based on managedWorkloadRebuild, using a managed-workload authority failure message for the managed path and retaining the existing replacement validation message for the replacement path.
🧹 Nitpick comments (14)
tools/advisors/risk-plan.mts (2)
485-485: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse the existing multiarch job constant.
Line 464 refers to the same job through
PROTECTED_MANAGED_IMAGE_MULTIARCH_JOB_ID. Line 485 spells the identifier as a literal. Two spellings for one job id can drift silently, and the advisor guidance requires deriving inventories from a canonical source.♻️ Proposed constant reuse
- requiredJobs: [MANAGED_IMAGE_PROTECTED_RUNTIME_JOB_ID, "managed-image-multiarch-startup"], + requiredJobs: [ + MANAGED_IMAGE_PROTECTED_RUNTIME_JOB_ID, + PROTECTED_MANAGED_IMAGE_MULTIARCH_JOB_ID, + ],Based on path instructions for
tools/{advisors,pr-review-advisor}/**: "Derive inventories and limits from a canonical source where possible; flag duplicated lists that can silently drift", and the coding guideline "Use existing repository vocabulary and one name per concept".🤖 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 `@tools/advisors/risk-plan.mts` at line 485, Replace the literal "managed-image-multiarch-startup" in the requiredJobs list with the existing PROTECTED_MANAGED_IMAGE_MULTIARCH_JOB_ID constant used elsewhere in this advisor, while leaving MANAGED_IMAGE_PROTECTED_RUNTIME_JOB_ID unchanged.Sources: Coding guidelines, Path instructions
79-87: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBound the
rebuildprefix and note the workflow boundary.Two points apply to this prefix list.
First,
"src/lib/actions/sandbox/rebuild"has no separator or dot boundary. The other entries end with/or., so they bind to a directory or a filename stem. This entry matches any future path whose basename merely starts withrebuild, and it therefore selects a tier-3 protected job for unrelated files.Second, the sibling
managed-image-multiarchfamily carries a comment at lines 471-474 that requires keeping its source boundary synchronized with the managed-image workflow path filter. This new set is a second hand-maintained copy of a workflow path filter with no equivalent note. Add the same synchronization note, or derive both boundaries from one source.♻️ Proposed prefix boundary
const MANAGED_IMAGE_PROTECTED_RUNTIME_INPUT_PREFIXES = [ "scripts/checks/run-managed-image-openshell-e2e.", - "src/lib/actions/sandbox/rebuild", + "src/lib/actions/sandbox/rebuild-", "src/lib/onboard/managed-bootstrap/",Based on path instructions for
tools/{advisors,pr-review-advisor}/**: "Derive inventories and limits from a canonical source where possible; flag duplicated lists that can silently drift."🤖 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 `@tools/advisors/risk-plan.mts` around lines 79 - 87, Bound the "src/lib/actions/sandbox/rebuild" entry with the appropriate directory or filename separator so it cannot match unrelated paths beginning with “rebuild”. Add the workflow path-filter synchronization note used by the managed-image-multiarch family, or refactor both lists to derive their boundaries from a shared canonical source; update MANAGED_IMAGE_PROTECTED_RUNTIME_INPUT_PREFIXES and the related workflow boundary together.Source: Path instructions
test/helpers/managed-image-buildless-e2e.ts (2)
80-92: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAlign the
workloadpayload type with the asserted fields.The declared
workloadshape omitsplatform,sourceCohort, andencodedProfile. Lines 716-730 assert all three withtoEqual. The interface no longer documents the receipt that this fixture validates. Add the missing optional fields.♻️ Proposed type completion
workload?: { schemaVersion?: number; kind?: string; reference?: string; + platform?: string; release?: string; sourceRevision?: string; + sourceCohort?: string; capabilityContractVersion?: number; startupProfileContractVersion?: number; + encodedProfile?: string; startupProfileSha256?: string; credentialProxyReplayRequired?: boolean; shared?: boolean; };🤖 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/managed-image-buildless-e2e.ts` around lines 80 - 92, Update the workload payload type in the managed image fixture to include optional platform, sourceCohort, and encodedProfile fields, matching the fields asserted later in the fixture while preserving the existing workload properties.
31-31: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSelect the fixture platform by value.
MANAGED_IMAGE_PLATFORMS[0]currently matches the forced"x64"platform. If the array order changes, the fixture can select"linux/arm64"while runtime negotiation resolves"x64"to"linux/amd64", causing the registration assertion to fail. Set the fixture platform explicitly to"linux/amd64".🤖 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/managed-image-buildless-e2e.ts` at line 31, Update MANAGED_IMAGE_PLATFORM to explicitly use the "linux/amd64" platform value instead of selecting MANAGED_IMAGE_PLATFORMS[0], so the fixture remains aligned with the runtime’s x64 platform resolution regardless of array order.test/pr-risk-plan.test.ts (1)
386-417: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a negative case for the protected-runtime prefixes.
This test proves detection for four in-scope paths. It does not prove that an out-of-scope path stays out of the family. The new prefixes in
tools/advisors/risk-plan.mtsare broad."src/lib/actions/sandbox/rebuild"has no trailing separator, and"src/lib/onboard/workload/"covers a whole directory. Add one assertion that a near-miss path does not activatemanaged-image-protected-runtime.🧪 Proposed false-positive assertion
expect(riskPlanRequiredJobIds(activatedImplementation)).toEqual( expect.arrayContaining([ "managed-image-multiarch-startup", "managed-image-protected-runtime", ]), ); + const nearMiss = plan("src/lib/actions/sandbox/status-phase.ts"); + expect( + nearMiss.families.some((family) => family.id === "managed-image-protected-runtime"), + ).toBe(false);Based on path instructions for
tools/{advisors,pr-review-advisor}/**: "Require focused tests for both detection and false-positive behavior."🤖 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/pr-risk-plan.test.ts` around lines 386 - 417, Add a negative assertion to the test covering the protected-runtime family: run plan with a near-miss path such as “src/lib/actions/sandbox/rebuild” or an out-of-scope path under the broad workload prefix, then verify the resulting families do not contain “managed-image-protected-runtime”. Keep the existing positive detection and required-job assertions unchanged.Source: Path instructions
src/lib/onboard/managed-workload/onboard-orchestration.ts (2)
157-190: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove the inference resolution inside the memoization.
preparedProfileis memoized, but lines 157-169 runresolveAgentInferenceApiandgetSandboxInferenceConfigon every call.ensurePreparedProfileis called at least three times per onboarding run (pre-delete, post-delete, and receipt construction), so the resolution repeats without effect. Compute it only when the profile is not yet built.♻️ Proposed refactor
- const inferenceApi = - input.agentName === "langchain-deepagents-code" - ? "openai-completions" - : dependencies.resolveAgentInferenceApi( - input.agentName, - input.provider, - input.preferredInferenceApi, - ); - const inference: SandboxInferenceConfig = dependencies.getSandboxInferenceConfig( - input.model, - input.provider, - inferenceApi, - ); - preparedProfile ??= buildManagedStartupOnboardProfile({ + if (preparedProfile) return preparedProfile; + const inferenceApi = + input.agentName === "langchain-deepagents-code" + ? "openai-completions" + : dependencies.resolveAgentInferenceApi( + input.agentName, + input.provider, + input.preferredInferenceApi, + ); + const inference: SandboxInferenceConfig = dependencies.getSandboxInferenceConfig( + input.model, + input.provider, + inferenceApi, + ); + preparedProfile = buildManagedStartupOnboardProfile({🤖 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/managed-workload/onboard-orchestration.ts` around lines 157 - 190, Move the inference resolution block containing resolveAgentInferenceApi and getSandboxInferenceConfig inside the preparedProfile ??= initialization in ensurePreparedProfile. Ensure both calls execute only when the memoized profile is first constructed, while preserving the existing profile fields and return behavior for subsequent calls.
192-194: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the
resolveCreateIntentidentity seam or give it a current consumer.
resolveCreateIntentreturns its argument unchanged, andprepareOnboardSandboxWorkloadLaunchcalls it at line 281 only to pass the intent through. It is an extension point with no current requirement and no protecting test. Either delete it and passinput.plan.intentdirectly, or implement the managed-image intent adjustment it is intended to own.As per coding guidelines: "Do not add configuration, fallback, migration, compatibility, or extension layers without a current requirement; identify the current consumer and protecting test."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/onboard/managed-workload/onboard-orchestration.ts` around lines 192 - 194, Remove the unused resolveCreateIntent identity seam and update prepareOnboardSandboxWorkloadLaunch to pass input.plan.intent directly, eliminating the helper and its call without adding replacement extension logic.Source: Coding guidelines
src/lib/onboard/sandbox-create-plan.test.ts (1)
88-88: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a case that passes a managed image reference as
fromRef.All three fixtures still use a Dockerfile path, so the tests only prove the previous behavior through the renamed field. The reason for the rename is that
materializeSandboxCreatePlanmust now emit an image reference verbatim without appending/Dockerfile. Add one case that passes a managed image reference and assertscreateArgscontains--fromfollowed by that exact reference.As per path instructions: "Migration tests must prove the superseded path is unreachable or removed, not merely prove that the new path also works."
Also applies to: 260-260, 331-331
🤖 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-create-plan.test.ts` at line 88, Extend the fixtures in sandbox-create-plan tests to include a managed image reference as fromRef, and assert materializeSandboxCreatePlan produces createArgs with --from followed by the exact reference unchanged. Ensure the test demonstrates no /Dockerfile suffix is appended, while preserving the existing Dockerfile-path cases.Source: Path instructions
src/lib/onboard/machine/handlers/provider-inference.ts (1)
974-974: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the unused provider-inference estimate dependencies.
provider-inference.tsno longer callsassessHostorformatSandboxBuildEstimateNote. Remove both dependency members, theironboard.tswiring, and the corresponding test fixture fields. Keep the estimate owned byfallbackBuildEstimate.🤖 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.ts` at line 974, Remove the unused assessHost and formatSandboxBuildEstimateNote dependency members from the provider-inference handler, along with their wiring in onboard.ts and corresponding test fixture fields. Preserve estimate ownership through fallbackBuildEstimate and remove only the obsolete dependency paths.Source: Path instructions
src/lib/onboard/sandbox-create-plan.ts (1)
155-157: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDelete the unused
prepareSandboxCreatePlanwrapper and its wrapper-specific tests.
src/lib/onboard.tspassesmaterializeSandboxCreatePlandirectly to the managed workload orchestration path, and no production code callsprepareSandboxCreatePlan. Remove its test cases andsandbox-create-plan-extra-providers.test.ts; this also removes the duplicate${buildCtx}/Dockerfileconstruction. Keep the shared intent and materialization exports.🤖 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-create-plan.ts` around lines 155 - 157, Remove the unused prepareSandboxCreatePlan wrapper and its wrapper-specific tests, including sandbox-create-plan-extra-providers.test.ts. Update related imports and references so the managed workload path continues using materializeSandboxCreatePlan directly, while preserving the shared intent and materialization exports and eliminating duplicate ${buildCtx}/Dockerfile construction.Source: Path instructions
src/lib/actions/sandbox/rebuild-managed-workload-mutation-guard.test.ts (2)
20-22: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop the redundant mock teardown.
The
cliVitest project already enablesrestoreMocks, sovi.restoreAllMocks()inafterEachrepeats project-level isolation. Remove the hook.Based on learnings: "Vitest test files under src (e.g.,
*.test.ts) are executed by thecliVitest project, which importstest/helpers/vitest-state-isolation.tsand enablesclearMocks,restoreMocks,unstubEnvs, andunstubGlobals."🤖 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-managed-workload-mutation-guard.test.ts` around lines 20 - 22, Remove the redundant afterEach hook containing vi.restoreAllMocks() from the test file; rely on the cli Vitest project's existing restoreMocks configuration for mock teardown.Source: Learnings
24-43: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the fail-closed branches instead of stubbing the decision itself.
The first two tests stub
managedWorkloadRebuildHandoffMatchesEntry, which is the exact comparison the guard exists to enforce. They prove only that the guard forwards a boolean. The behavior that protects the delete edge is untested:
registry.getSandboxreturnsnull, soproviderstaysnulland the guard must fail closed.requireRuntimeProviderBundleForSandboxthrows for an unrecognizedopenshellDriver, so thecatchmust fail closed.- The persisted receipt, contract, or profile differs from the handoff, so the real matcher must return
false.Add cases for the two branches above with the real matcher, and drive the third case through a persisted entry rather than a stub.
💚 Proposed additional cases
+ it("blocks deletion when the sandbox entry disappeared", () => { + vi.spyOn(registry, "getSandbox").mockReturnValue(null); + + expect(revalidateManagedWorkloadRebuildBeforeDelete("alpha", handoff)).toEqual({ + ok: false, + message: "Managed workload authority changed before sandbox deletion.", + }); + }); + + it("blocks deletion when the recorded runtime provider is unknown", () => { + vi.spyOn(registry, "getSandbox").mockReturnValue({ + ...entry, + openshellDriver: "not-a-provider", + } as SandboxEntry); + + expect(revalidateManagedWorkloadRebuildBeforeDelete("alpha", handoff)).toEqual({ + ok: false, + message: "Managed workload authority changed before sandbox deletion.", + }); + });As per path instructions: "Flag copied production algorithms, broad mocks that bypass the behavior under test, and conditionals that make a test pass without exercising its claim."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/actions/sandbox/rebuild-managed-workload-mutation-guard.test.ts` around lines 24 - 43, Replace the boolean stubs in the tests around revalidateManagedWorkloadRebuildBeforeDelete with real matcher coverage: add a case where registry.getSandbox returns null and assert fail-closed rejection, add a case where requireRuntimeProviderBundleForSandbox throws for an unknown openshellDriver and assert the catch rejects, and add a mismatch case using a persisted entry whose receipt, contract, or profile differs from handoff. Keep the legacy undefined-handoff case unchanged.Source: Path instructions
src/lib/actions/sandbox/rebuild-dcode-orchestrator.test.ts (1)
119-151: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtend managed coverage to the delete-edge branches.
This test covers
prepareImageonly. The same change adds managed branches torevalidateBeforeDeleteandcheckAtDeleteEdge, andcheckAtDeleteEdgenow permits anullpreparedReplacement. Those branches guard sandbox deletion, and they are untested. Add cases that assert:
revalidateBeforeDeletereturns the managed revalidation result and never reaches the"DCode replacement preflight was not retained."bail whenmanagedWorkloadRebuildistrue.checkAtDeleteEdgereturns{ ok: false }when the managed revalidation resolvesfalse, and returns a captured bail message when the managed revalidation callsbail.As per path instructions: "Migration tests must prove the superseded path is unreachable or removed, not merely prove that the new path also works."
🤖 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-orchestrator.test.ts` around lines 119 - 151, Extend the managed-workload tests around revalidateBeforeDelete and checkAtDeleteEdge to cover both delete-edge branches. Assert revalidateBeforeDelete returns the managed revalidation result without invoking the “DCode replacement preflight was not retained.” bail; assert checkAtDeleteEdge returns { ok: false } when revalidation resolves false and captures the bail message when revalidation invokes bail, including the null preparedReplacement path.Source: Path instructions
src/lib/actions/sandbox/agents/managed-workload-rebuild-profile.ts (1)
89-92: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winReplace the
asassertion with a validated narrowing.
inference.inferenceApiis asserted into the three-member union without a runtime check. IfresolveManagedStartupInferenceRoutereturns any other API string, the invalid value is frozen into the replacement profile and only fails later, after the rebuild has committed to the handoff. Narrow the value with an explicit check so the failure occurs while the old workload is still authoritative.♻️ Proposed validated narrowing
+const MANAGED_STARTUP_INFERENCE_APIS = [ + "openai-completions", + "openai-responses", + "anthropic-messages", +] as const; +type ManagedStartupInferenceApi = (typeof MANAGED_STARTUP_INFERENCE_APIS)[number]; + +function requireManagedStartupInferenceApi(api: string): ManagedStartupInferenceApi { + if (!(MANAGED_STARTUP_INFERENCE_APIS as readonly string[]).includes(api)) { + throw new Error(`Unsupported managed startup inference API '${api}'.`); + } + return api as ManagedStartupInferenceApi; +}- api: inference.inferenceApi as - | "openai-completions" - | "openai-responses" - | "anthropic-messages", + api: requireManagedStartupInferenceApi(inference.inferenceApi),🤖 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/agents/managed-workload-rebuild-profile.ts` around lines 89 - 92, In the replacement-profile construction around resolveManagedStartupInferenceRoute, replace the inference.inferenceApi type assertion with an explicit runtime validation against the supported "openai-completions", "openai-responses", and "anthropic-messages" values. Reject or propagate an error for any other value before constructing or committing the replacement profile, while preserving the narrowed union for valid APIs.
🤖 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/managed-bootstrap/docker-authority-store.ts`:
- Around line 87-102: Update recordPreparedAuthority() to load the existing
journal before generating a new receipt or calling journalStore.create(): return
the existing preparationReceipt when sameJournal-compatible prepared authority
data matches, and reject an existing identity with mismatched data. Preserve the
current durability verification for newly created journals, and add a retry test
using two distinct clocks with a fixture that rejects duplicate creation instead
of overwriting journals.
In `@src/lib/onboard/managed-bootstrap/docker-runtime.ts`:
- Around line 281-288: Update createDockerManagedBootstrapSurface so the
lifecycle adapter receives the canonical Docker journal store, using the same
stateRoot-derived store passed by createAuthorityStore. Ensure
createDockerLifecycle’s adapter creation includes the required journalStore (and
stateRoot where applicable), and add coverage for both activation and resume
recovery.
In `@test/helpers/managed-image-buildless-e2e.ts`:
- Line 582: Update the onboarding setup around NEMOCLAW_TEST_SECRET_CANARY so
the canary is consumed by a real startup-profile reader. Either add the
onboarding read path for this variable or replace it with the existing
environment variable consumed by the startup profile builder, ensuring the test
validates actual secret propagation rather than only injection.
In `@test/onboard-managed-image-buildless-e2e.test.ts`:
- Around line 11-12: Increase the timeout for the “launches every shipped agent
by immutable image and startup profile without Dockerfile work (`#7744`)” test
above the combined 180-second child budget, leaving sufficient margin for
fixture setup and teardown so each child’s spawnSync timeout is reported first.
In `@test/onboard-messaging.test.ts`:
- Around line 47-49: Restore NEMOCLAW_TEST_MANAGED_IMAGE_FALLBACK after each
test instead of leaving the raw process.env assignment in place. Update the
hooks in test/onboard-messaging.test.ts lines 47-49,
test/onboard-sandbox-build.test.ts lines 19-21, and
test/onboard-sandbox-recreation.test.ts lines 14-16 to use vi.stubEnv with
existing cleanup or explicitly restore/delete the prior value in afterEach.
---
Outside diff comments:
In `@src/lib/actions/sandbox/rebuild-dcode-orchestrator.ts`:
- Around line 252-284: Update the failure message in the !valid branch of the
managedWorkloadRebuild/revalidateDcodeReplacementAtMutationEdge flow to select
an accurate message based on managedWorkloadRebuild, using a managed-workload
authority failure message for the managed path and retaining the existing
replacement validation message for the replacement path.
---
Nitpick comments:
In `@src/lib/actions/sandbox/agents/managed-workload-rebuild-profile.ts`:
- Around line 89-92: In the replacement-profile construction around
resolveManagedStartupInferenceRoute, replace the inference.inferenceApi type
assertion with an explicit runtime validation against the supported
"openai-completions", "openai-responses", and "anthropic-messages" values.
Reject or propagate an error for any other value before constructing or
committing the replacement profile, while preserving the narrowed union for
valid APIs.
In `@src/lib/actions/sandbox/rebuild-dcode-orchestrator.test.ts`:
- Around line 119-151: Extend the managed-workload tests around
revalidateBeforeDelete and checkAtDeleteEdge to cover both delete-edge branches.
Assert revalidateBeforeDelete returns the managed revalidation result without
invoking the “DCode replacement preflight was not retained.” bail; assert
checkAtDeleteEdge returns { ok: false } when revalidation resolves false and
captures the bail message when revalidation invokes bail, including the null
preparedReplacement path.
In `@src/lib/actions/sandbox/rebuild-managed-workload-mutation-guard.test.ts`:
- Around line 20-22: Remove the redundant afterEach hook containing
vi.restoreAllMocks() from the test file; rely on the cli Vitest project's
existing restoreMocks configuration for mock teardown.
- Around line 24-43: Replace the boolean stubs in the tests around
revalidateManagedWorkloadRebuildBeforeDelete with real matcher coverage: add a
case where registry.getSandbox returns null and assert fail-closed rejection,
add a case where requireRuntimeProviderBundleForSandbox throws for an unknown
openshellDriver and assert the catch rejects, and add a mismatch case using a
persisted entry whose receipt, contract, or profile differs from handoff. Keep
the legacy undefined-handoff case unchanged.
In `@src/lib/onboard/machine/handlers/provider-inference.ts`:
- Line 974: Remove the unused assessHost and formatSandboxBuildEstimateNote
dependency members from the provider-inference handler, along with their wiring
in onboard.ts and corresponding test fixture fields. Preserve estimate ownership
through fallbackBuildEstimate and remove only the obsolete dependency paths.
In `@src/lib/onboard/managed-workload/onboard-orchestration.ts`:
- Around line 157-190: Move the inference resolution block containing
resolveAgentInferenceApi and getSandboxInferenceConfig inside the
preparedProfile ??= initialization in ensurePreparedProfile. Ensure both calls
execute only when the memoized profile is first constructed, while preserving
the existing profile fields and return behavior for subsequent calls.
- Around line 192-194: Remove the unused resolveCreateIntent identity seam and
update prepareOnboardSandboxWorkloadLaunch to pass input.plan.intent directly,
eliminating the helper and its call without adding replacement extension logic.
In `@src/lib/onboard/sandbox-create-plan.test.ts`:
- Line 88: Extend the fixtures in sandbox-create-plan tests to include a managed
image reference as fromRef, and assert materializeSandboxCreatePlan produces
createArgs with --from followed by the exact reference unchanged. Ensure the
test demonstrates no /Dockerfile suffix is appended, while preserving the
existing Dockerfile-path cases.
In `@src/lib/onboard/sandbox-create-plan.ts`:
- Around line 155-157: Remove the unused prepareSandboxCreatePlan wrapper and
its wrapper-specific tests, including
sandbox-create-plan-extra-providers.test.ts. Update related imports and
references so the managed workload path continues using
materializeSandboxCreatePlan directly, while preserving the shared intent and
materialization exports and eliminating duplicate ${buildCtx}/Dockerfile
construction.
In `@test/helpers/managed-image-buildless-e2e.ts`:
- Around line 80-92: Update the workload payload type in the managed image
fixture to include optional platform, sourceCohort, and encodedProfile fields,
matching the fields asserted later in the fixture while preserving the existing
workload properties.
- Line 31: Update MANAGED_IMAGE_PLATFORM to explicitly use the "linux/amd64"
platform value instead of selecting MANAGED_IMAGE_PLATFORMS[0], so the fixture
remains aligned with the runtime’s x64 platform resolution regardless of array
order.
In `@test/pr-risk-plan.test.ts`:
- Around line 386-417: Add a negative assertion to the test covering the
protected-runtime family: run plan with a near-miss path such as
“src/lib/actions/sandbox/rebuild” or an out-of-scope path under the broad
workload prefix, then verify the resulting families do not contain
“managed-image-protected-runtime”. Keep the existing positive detection and
required-job assertions unchanged.
In `@tools/advisors/risk-plan.mts`:
- Line 485: Replace the literal "managed-image-multiarch-startup" in the
requiredJobs list with the existing PROTECTED_MANAGED_IMAGE_MULTIARCH_JOB_ID
constant used elsewhere in this advisor, while leaving
MANAGED_IMAGE_PROTECTED_RUNTIME_JOB_ID unchanged.
- Around line 79-87: Bound the "src/lib/actions/sandbox/rebuild" entry with the
appropriate directory or filename separator so it cannot match unrelated paths
beginning with “rebuild”. Add the workflow path-filter synchronization note used
by the managed-image-multiarch family, or refactor both lists to derive their
boundaries from a shared canonical source; update
MANAGED_IMAGE_PROTECTED_RUNTIME_INPUT_PREFIXES and the related workflow boundary
together.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: dee4ce77-54b5-44d6-9770-890d82044252
📒 Files selected for processing (42)
docs/get-started/quickstart-hermes.mdxdocs/get-started/quickstart-langchain-deepagents-code.mdxdocs/get-started/quickstart.mdxdocs/manage-sandboxes/recover-rebuild-sandboxes.mdxdocs/reference/commands.mdxsrc/lib/actions/sandbox/agents/managed-workload-rebuild-profile.tssrc/lib/actions/sandbox/rebuild-dcode-orchestrator.test.tssrc/lib/actions/sandbox/rebuild-dcode-orchestrator.tssrc/lib/actions/sandbox/rebuild-dcode-preflight.tssrc/lib/actions/sandbox/rebuild-gpu-opt-out.tssrc/lib/actions/sandbox/rebuild-managed-workload-mutation-guard.test.tssrc/lib/actions/sandbox/rebuild-pipeline.tssrc/lib/actions/sandbox/rebuild-preflight-guards.tssrc/lib/actions/sandbox/rebuild-preflight-phase.tssrc/lib/actions/sandbox/rebuild-preflight-target-phase.tssrc/lib/onboard.tssrc/lib/onboard/machine/handlers/provider-inference.tssrc/lib/onboard/managed-bootstrap/docker-authority-store.test.tssrc/lib/onboard/managed-bootstrap/docker-authority-store.tssrc/lib/onboard/managed-bootstrap/docker-runtime.tssrc/lib/onboard/managed-bootstrap/docker.tssrc/lib/onboard/managed-workload/onboard-orchestration.tssrc/lib/onboard/runtime-provider/contract.tssrc/lib/onboard/runtime-provider/docker.tssrc/lib/onboard/runtime-provider/registry.tssrc/lib/onboard/runtime-provider/runtime-provider-contract.test.tssrc/lib/onboard/sandbox-create-intent-types.tssrc/lib/onboard/sandbox-create-plan-materialization.tssrc/lib/onboard/sandbox-create-plan.test.tssrc/lib/onboard/sandbox-create-plan.tssrc/lib/onboard/sandbox-gpu-create-flow.test.tssrc/lib/onboard/types.tstest/e2e/support/e2e-cross-runtime-compatibility.test.tstest/helpers/managed-image-buildless-e2e.tstest/helpers/onboard-script-mocks.cjstest/onboard-managed-image-buildless-e2e.test.tstest/onboard-messaging.test.tstest/onboard-sandbox-build.test.tstest/onboard-sandbox-recreation.test.tstest/pr-e2e-gate.test.tstest/pr-risk-plan.test.tstools/advisors/risk-plan.mts
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
wscurran
added
area: onboarding
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/onboard/managed-bootstrap/docker-runtime.test.ts (1)
36-110: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winAlways remove the temporary state directory.
If setup or an assertion after Line 36 throws, Line 110 does not run. The test then leaves a temporary directory on the test host. Put the lifecycle setup and assertions in
try/finally, or remove the directory fromafterEach.Based on learnings, only clean up resources Vitest does not manage, such as temporary directories and files.
🤖 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/managed-bootstrap/docker-runtime.test.ts` around lines 36 - 110, Ensure the temporary directory created by stateRoot is removed regardless of setup, lifecycle execution, or assertion failures. Wrap the lifecycle setup and assertions in a try/finally block that always calls fs.rmSync for stateRoot, or use an equivalent test cleanup hook; only add cleanup for this manually managed filesystem resource.Source: Learnings
🤖 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/onboard/managed-bootstrap/docker-runtime.test.ts`:
- Around line 36-110: Ensure the temporary directory created by stateRoot is
removed regardless of setup, lifecycle execution, or assertion failures. Wrap
the lifecycle setup and assertions in a try/finally block that always calls
fs.rmSync for stateRoot, or use an equivalent test cleanup hook; only add
cleanup for this manually managed filesystem resource.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: bd6bfd92-fcf4-4e4b-b650-f06fdd7a01f1
📒 Files selected for processing (36)
ci/source-shape-test-budget.jsonscripts/checks/run-managed-image-openshell-e2e.tssrc/lib/actions/sandbox/agents/managed-workload-rebuild-profile.tssrc/lib/actions/sandbox/rebuild-dcode-orchestrator.test.tssrc/lib/actions/sandbox/rebuild-dcode-orchestrator.tssrc/lib/actions/sandbox/rebuild-managed-workload-mutation-guard.test.tssrc/lib/onboard.tssrc/lib/onboard/machine/core-flow-phases.test.tssrc/lib/onboard/machine/handlers/provider-inference-route-containment.test.tssrc/lib/onboard/machine/handlers/provider-inference.test-support.tssrc/lib/onboard/machine/handlers/provider-inference.tssrc/lib/onboard/managed-bootstrap/docker-authority-store.test.tssrc/lib/onboard/managed-bootstrap/docker-authority-store.tssrc/lib/onboard/managed-bootstrap/docker-runtime.test.tssrc/lib/onboard/managed-bootstrap/docker-runtime.tssrc/lib/onboard/managed-bootstrap/docker-test-fixture.tssrc/lib/onboard/managed-bootstrap/runtime-create.tssrc/lib/onboard/managed-workload/onboard-orchestration.tssrc/lib/onboard/sandbox-create-intent-types.tssrc/lib/onboard/sandbox-create-plan-extra-providers.test.tssrc/lib/onboard/sandbox-create-plan.test.tssrc/lib/onboard/sandbox-create-plan.tssrc/lib/onboard/sandbox-gpu-create-flow.test.tssrc/lib/onboard/sandbox-gpu-create-flow.tssrc/lib/onboard/sandbox-gpu-create-run-attempt.tstest/helpers/managed-image-buildless-e2e.tstest/onboard-managed-image-buildless-e2e.test.tstest/onboard-messaging.test.tstest/onboard-prepared-build-context.test.tstest/onboard-sandbox-build.test.tstest/onboard-sandbox-recreation.test.tstest/onboard-terminal-dashboard.test.tstest/pr-e2e-gate-signal-shards.test.tstest/pr-risk-plan.test.tstest/runtime-provider-source-shape.test.tstools/advisors/risk-plan.mts
💤 Files with no reviewable changes (7)
- src/lib/onboard/sandbox-create-plan-extra-providers.test.ts
- src/lib/onboard/machine/core-flow-phases.test.ts
- src/lib/onboard/machine/handlers/provider-inference.test-support.ts
- src/lib/onboard/sandbox-create-plan.ts
- src/lib/onboard/machine/handlers/provider-inference.ts
- src/lib/onboard/machine/handlers/provider-inference-route-containment.test.ts
- src/lib/onboard.ts
🚧 Files skipped from review as they are similar to previous changes (10)
- test/onboard-sandbox-recreation.test.ts
- src/lib/onboard/sandbox-gpu-create-flow.test.ts
- src/lib/onboard/managed-bootstrap/docker-runtime.ts
- test/onboard-sandbox-build.test.ts
- test/pr-risk-plan.test.ts
- tools/advisors/risk-plan.mts
- test/onboard-messaging.test.ts
- src/lib/onboard/managed-bootstrap/docker-authority-store.ts
- test/helpers/managed-image-buildless-e2e.ts
- src/lib/onboard/managed-workload/onboard-orchestration.ts
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
ericksoa
changed the title
## Summary Adds the dormant, provider-scoped Podman command/preflight/start-stop boundary and proves it against a real rootless Podman 5 service with Docker disabled. The provider remains absent from the production registry: this PR does not activate or advertise Podman support. Stacked on #8261. Part of #7744. ## Related Issue Part of #7744. ## Changes - Adds an immutable operation-scoped container-engine command contract and a Podman adapter pinned to one qualified Unix-socket authority. - Adds Linux amd64/arm64 rootless Podman 5 preflight, subordinate UID/GID and cgroups v2 validation, and exact labeled-container start/stop semantics. - Adds an inert Podman runtime bundle with only host doctor and direct CPU lifecycle capabilities; managed bootstrap, snapshots, recovery, cleanup, GPU, local inference, and production selection remain explicitly unsupported for later slices. - Adds unit coverage across OpenClaw, Hermes, and Deep Agents Code while keeping the production registry limited to qualified providers. - Adds a credential-free Ubuntu 26.04 PR proof that disables and masks Docker, guards every Docker CLI resolution, starts one exact rootless Podman API socket, and proves all three agents preserve immutable container identity across stop/start/restart. - The abstraction is required so Podman and future MXC-style providers can inject engine-specific operations without central Podman switches. Directly changing existing Docker helpers would violate the runtime-provider capability boundary; the registry/source-shape and rootless workflow tests protect that seam. ## Type of Change - [x] Code change (feature, bug fix, or refactor) - [ ] Code change with doc updates - [ ] Doc only (prose changes, no code sample modifications) - [ ] Doc only (includes code sample changes) ## Quality Gates - [x] Tests added or updated for changed behavior - [ ] Existing tests cover changed behavior — justification: - [ ] Tests not applicable — justification: - [ ] Docs updated for user-facing behavior changes - [x] Docs not applicable — justification: the Podman bundle is deliberately absent from production selection and this PR exposes no user-facing runtime option. - [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: endpoint authority is pinned before and after every command; Docker is disabled and guarded in the live proof; the provider remains dormant pending later qualification slices. - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## Documentation Writer Review - [x] Documentation writer subagent reviewed the completed changes - Result: `no-docs-needed` - Evidence: `.github/workflows/podman-cpu-proof.yaml`; `src/lib/onboard/runtime-provider/podman.ts`; the bundle remains non-selectable and no user-visible behavior is documented in this slice. - Agent: Codex Desktop <!-- docs-review-head-sha: a254cf1 --> <!-- docs-review-agents-blob-sha: 3dd7c24 --> ## DGX Station Hardware Evidence - [ ] Tested on DGX Station - Tested commit: - Station profile/scenario: - Result: - Supporting evidence: ## Verification - [x] PR description includes a `Signed-off-by:` line and every commit appears as `Verified` in GitHub - [x] Normal `pre-commit`, `commit-msg`, and `pre-push` hooks passed, or `npm run validate:pr` passed after refreshing `origin/main` when hooks were skipped or unavailable - [x] Targeted behavior tests pass for the current change set, or tests are marked not applicable above — `60/60` focused Podman adapter/provider/workflow/parity tests passed on the restacked head; the advisor follow-up adds `10/10` focused tests and passes source-shape, repository, and CLI pre-push gates on exact head `a254cf1cc306`. - [x] Applicable broad gate passed — `prek run --files <complete slice>` passed repository checks, semantic E2E phases, source-shape, test-size, formatting, YAML, secret scan, and all other applicable hooks. - [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) - [ ] 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) --- Signed-off-by: Aaron Erickson <aerickson@nvidia.com> --------- Signed-off-by: Aaron Erickson <aerickson@nvidia.com> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Apurv Kumaria <akumaria@nvidia.com>
2 participants
Summary
Adds an intentionally hidden experimental activation gate for buildless managed-image onboarding on the current Docker runtime. Normal onboarding remains on the existing Dockerfile path. Passing
--temp-managed-runtimeopts a new OpenClaw, Hermes, or Deep Agents Code sandbox into the all-agent managed-image and transactional startup-profile path without making that behavior a documented or supported default.Existing sandboxes that already record managed-image workload authority retain that authority through rebuild and recovery without requiring the temporary flag again.
Related Issue
Refs #7744
Changes
--temp-managed-runtimeonboarding flag without adding it to usage, examples, command documentation, or quickstartsmainso this PR makes no support claimThe same hidden gate can guard candidate Podman selection while the PR4 implementation is qualified. Removing or replacing the temporary gate and documenting support remains a later evidence-backed activation decision.
Type of Change
Quality Gates
Documentation Writer Review
no-docs-neededmain; the hidden experimental flag is absent from usage, examples, and user-facing documentationDGX Station Hardware Evidence
scripts/prepare-dgx-station-host.shis unchangedVerification
Signed-off-by:line and every commit appears asVerifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run validate:prpassed after refreshingorigin/mainwhen hooks were skipped or unavailablenpm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — pending exact-head CI and protected E2Enpm run docsbuilds without warnings (doc changes only)Signed-off-by: Aaron Erickson aerickson@nvidia.com