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 PR adds managed-workload onboarding and rebuild handoff preparation, Docker bootstrap authority persistence, dormant Podman runtime and managed-bootstrap support, and new E2E, workflow, and risk-plan coverage for managed-image and Podman lifecycle paths. ChangesManaged workload lifecycle
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant CLI as Onboard CLI
participant Onboard as createSandboxWithBaseImageResolution
participant MW as managedWorkloadOnboard
participant GPU as sandboxGpuCreateFlow
participant Registry as sandboxRegistration
CLI->>Onboard: resolveOnboardOptions()
Onboard->>MW: prepareOnboardSandboxWorkloadLaunch(...)
MW-->>Onboard: launch metadata + managedBootstrap
Onboard->>GPU: runSandboxGpuCreateFlow(...)
GPU-->>Onboard: created sandbox
Onboard->>MW: resolveOnboardSandboxWorkloadReceipt(...)
MW-->>Onboard: workloadReceipt
Onboard->>Registry: registerCreatedSandbox(workloadReceipt)
sequenceDiagram
participant Rebuild as rebuild preflight/pipeline
participant Profile as prepareManagedRebuildProfileHandoff
participant Guard as managed workload guards
participant Runtime as runtime provider
Rebuild->>Profile: prepare managed rebuild handoff
Profile-->>Rebuild: staged rebuild profile
Rebuild->>Guard: revalidate before delete
Guard->>Runtime: reload provider-bound authority
Runtime-->>Guard: current managed workload state
Guard-->>Rebuild: ok or fail-closed result
Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Comment |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in commit ee19578 in the TypeScript / code-coverage/cliThe overall coverage in commit ee19578 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>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (7)
.github/workflows/podman-cpu-proof.yaml (1)
29-29: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMake
ubuntu-26.04known to actionlint.GitHub supports this label in public preview, but actionlint 1.7.12 rejects it. Add it to the
runner-labellist inactionlint.yaml, or update actionlint before workflow validation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/podman-cpu-proof.yaml at line 29, Update the workflow validation setup so actionlint accepts the ubuntu-26.04 runner label used in the podman-cpu-proof workflow. The smallest fix is to add ubuntu-26.04 to the runner-label list in actionlint.yaml; if that validation config is intentionally versioned elsewhere, update the actionlint configuration there instead so the new runner label is recognized during checks.Source: Linters/SAST tools
src/lib/onboard/command-support.ts (1)
89-89: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLink a retirement issue for the temporary gate.
The flag name declares itself temporary. The declaration carries no retirement reference and no exit criteria. Add the tracking issue link in a code comment next to the flag, so the removal is tracked in GitHub rather than in code archaeology.
Based on learnings from the path instructions for
src/**: "Retain an old path only for a demonstrated external/persisted-data contract or a bounded confidence/rollback window... link the retirement issue or PR in GitHub, and state observable exit criteria."🤖 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/command-support.ts` at line 89, Add a code comment adjacent to the temp-managed-runtime flag declaration linking the GitHub retirement issue and stating the observable exit criteria for removing the temporary gate. Keep the flag behavior unchanged and use the existing issue-tracking reference if one is available.Source: Path instructions
src/lib/onboard/managed-workload/onboard-orchestration.ts (1)
113-120: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExplain why the disabled gate keeps
prefer-managed.When the gate is off, the code sets
managedImageSelectionPolicy: "prefer-managed"together withmanagedImages: null. The actual disable comes frommanagedImages: null, not from the policy value. A reader can misread this branch as the enabled path. Add a short comment that statesmanagedImages: nullforces the legacy Dockerfile path.🤖 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 113 - 120, In the fallback branch of the runtimeCapabilities assignment, add a short comment explaining that managedImages: null disables managed images and forces the legacy Dockerfile path, while managedImageSelectionPolicy remains prefer-managed.src/lib/onboard/managed-bootstrap/podman-held-workload.test.ts (1)
259-267: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test that reaches the
sameObservationstability check.This test drifts
Imageon the second inspect.parseObservationcompares the observed image againstexpectedImageContentIdatpodman-held-workload.tsline 245, so the second parse throws beforesameObservationruns. The assertion passes without exercising the double-inspect stability invariant that the title claims.No test in this file asserts the "changed during stable identity capture" message. Add a case that drifts a field which
parseObservationdoes not compare against an expected input, so the two observations differ only at line 304.💚 Proposed additional test
+ it("rejects a held workload whose evidence changed between the two inspections", () => { + const fake = engineWith([ + result(listOutput()), + result(inspectOutput()), + result( + inspectOutput({ + labels: { + [PODMAN_MANAGED_LABEL]: "true", + [PODMAN_SANDBOX_ID_LABEL]: SANDBOX_ID, + [PODMAN_SANDBOX_NAME_LABEL]: SANDBOX_NAME, + [PODMAN_SANDBOX_NAMESPACE_LABEL]: SANDBOX_NAMESPACE, + "openshell.extra": "added-between-inspections", + }, + }), + ), + ]); + + expect(() => inspect(fake.engine)).toThrow("changed during stable identity capture"); + });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/onboard/managed-bootstrap/podman-held-workload.test.ts` around lines 259 - 267, Update the stable-capture drift test around inspect(fake.engine) to vary a parseObservation field that is not validated against expectedImageContentId, allowing both parses to complete and sameObservation to detect the difference. Assert the “changed during stable identity capture” error so the test exercises the double-inspect stability invariant rather than the earlier image-content validation.Source: Path instructions
src/lib/onboard/runtime-provider/podman.test.ts (1)
195-198: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop the order-coupled registry assertion.
Line 196 asserts the exact key order of
CURRENT_RUNTIME_PROVIDER_BUNDLES. Registering an unrelated provider, or reordering the source object, breaks this test for a reason that has nothing to do with Podman. Line 197 already states the real claim.♻️ Proposed change
it("stays outside the production-selectable registry", () => { - expect(Object.keys(CURRENT_RUNTIME_PROVIDER_BUNDLES)).toEqual(["docker", "kubernetes"]); expect(CURRENT_RUNTIME_PROVIDER_BUNDLES).not.toHaveProperty("podman"); });🤖 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/runtime-provider/podman.test.ts` around lines 195 - 198, Remove the exact Object.keys(...).toEqual(["docker", "kubernetes"]) assertion from the test “stays outside the production-selectable registry”; retain the CURRENT_RUNTIME_PROVIDER_BUNDLES.not.toHaveProperty("podman") assertion as the only required check.src/lib/onboard/managed-bootstrap/podman-bootstrap-journal.test.ts (1)
157-172: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the
listUnfinishedtemp-file tolerance.
podman-bootstrap-journal.tsline 484 tolerates a leftover.<identity>.json[.decision].<uuid>.tmpentry, and line 487 fails on any other entry. Neither branch is tested. The tolerance regex is load-bearing: if it stops matching the names thatatomicWriteproduces at line 377, a crash during a write leaves an entry that makeslistUnfinishedthrow for every bootstrap identity, which blocks all recovery.💚 Proposed additions
+ it("tolerates a crash-left temporary file and rejects a foreign entry", () => { + const root = temporaryRoot(); + const store = createFilePodmanBootstrapJournalStore(root); + store.create(journal); + const directory = path.join(root, PODMAN_BOOTSTRAP_JOURNAL_DIRECTORY); + + fs.writeFileSync( + path.join(directory, `.${BOOTSTRAP_IDENTITY}.json.123e4567-e89b-42d3-a456-426614174000.tmp`), + "partial", + { mode: 0o600 }, + ); + expect(store.listUnfinished()).toEqual([journal]); + + fs.writeFileSync(path.join(directory, "unexpected.txt"), "x", { mode: 0o600 }); + expect(() => store.listUnfinished()).toThrow("unsupported entry"); + }); + it("rejects an orphan rollback decision during recovery enumeration", () => {🤖 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/podman-bootstrap-journal.test.ts` around lines 157 - 172, Add tests for listUnfinished in createFilePodmanBootstrapJournalStore covering both directory-entry branches: confirm atomicWrite-style temporary files matching the .<identity>.json[.decision].<uuid>.tmp pattern are ignored, and confirm unrelated entries still throw. Use the existing journal directory and bootstrap identity constants, and preserve the current recovery behavior.src/lib/onboard/runtime-provider/podman-preflight.test.ts (1)
181-193: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a success-path assertion for
inspectPodmanHost.The suite exercises only the failure branch of
inspectPodmanHost. Thestatus: "ok"branch inpodman-preflight.tsat lines 240-245 builds adetailtemplate from the receipt, and no test asserts it. A regression in that template would not fail the suite.💚 Proposed addition
+ it("reports an ok doctor check for a qualified rootless host", () => { + expect(inspectPodmanHost(engine(), { platform: "linux", architecture: "x64" })).toEqual({ + group: "Host", + label: "Podman runtime", + status: "ok", + detail: "rootless server 5.6.2 (client 5.6.2), cgroups v2, linux/amd64", + }); + }); + it("reports a bounded doctor failure without throwing", () => {🤖 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/runtime-provider/podman-preflight.test.ts` around lines 181 - 193, Add a success-path test for inspectPodmanHost using a successful engine receipt, and assert the complete status: "ok" result including the detail template populated from that receipt. Keep the existing failure assertion unchanged and cover the Linux x64 context and expected hint as appropriate.
🤖 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/podman-bootstrap-replacement.ts`:
- Around line 349-364: Update runtimeArguments to skip the
FORBIDDEN_ATTACHED_SHORT_FLAGS prefix check for arguments beginning with "--",
while preserving exact FORBIDDEN_RUNTIME_FLAGS rejection. Extend the relevant
tests to verify --device and --log-driver are accepted and passed through, while
-eSECRET=1 remains rejected.
In `@src/lib/onboard/managed-bootstrap/podman-watcher-lease.ts`:
- Around line 55-64: Add a concrete file-backed implementation of
PodmanGatewayWatcherLeaseStore, using the existing podman-bootstrap-journal.ts
durability and permission patterns. Implement read to open with O_NOFOLLOW,
require a regular single-link file with mode bits restricted by (mode & 0o077n)
=== 0n, and reject corrupt or ambiguous contents. Implement acquire with
exclusive creation only when absent, and implement advance and clear as leaseId
compare-and-swap operations; perform atomic replacement/removal as appropriate
and fsyncSync after every mutation.
In `@test/e2e/live/podman-cpu-lifecycle.test.ts`:
- Around line 114-123: Update the podman CPU lifecycle test in the same
`lifecycle.start`/`lifecycle.stop` flow to inspect the container after the final
`lifecycle.stop` call and assert the stopped state explicitly. Use
`inspectContainer` with `agentEngines.sandboxLifecycle` and `sandboxName`, then
verify the container reports `Running: false` and `Status: "exited"` in addition
to the existing stop result checks so the test confirms the container actually
reached rest state.
In `@test/helpers/managed-image-buildless-e2e.ts`:
- Line 711: Update the assertions in the managed-image buildless E2E test to
avoid fallback values that mask missing fields: assert registration?.agent
directly against agent, and ensure bootstrapRequest?.encodedProfile is present
before passing it to decodeManagedStartupProfile. Preserve an explicit
expectation for any intentionally omitted default-agent field rather than using
a fallback.
---
Nitpick comments:
In @.github/workflows/podman-cpu-proof.yaml:
- Line 29: Update the workflow validation setup so actionlint accepts the
ubuntu-26.04 runner label used in the podman-cpu-proof workflow. The smallest
fix is to add ubuntu-26.04 to the runner-label list in actionlint.yaml; if that
validation config is intentionally versioned elsewhere, update the actionlint
configuration there instead so the new runner label is recognized during checks.
In `@src/lib/onboard/command-support.ts`:
- Line 89: Add a code comment adjacent to the temp-managed-runtime flag
declaration linking the GitHub retirement issue and stating the observable exit
criteria for removing the temporary gate. Keep the flag behavior unchanged and
use the existing issue-tracking reference if one is available.
In `@src/lib/onboard/managed-bootstrap/podman-bootstrap-journal.test.ts`:
- Around line 157-172: Add tests for listUnfinished in
createFilePodmanBootstrapJournalStore covering both directory-entry branches:
confirm atomicWrite-style temporary files matching the
.<identity>.json[.decision].<uuid>.tmp pattern are ignored, and confirm
unrelated entries still throw. Use the existing journal directory and bootstrap
identity constants, and preserve the current recovery behavior.
In `@src/lib/onboard/managed-bootstrap/podman-held-workload.test.ts`:
- Around line 259-267: Update the stable-capture drift test around
inspect(fake.engine) to vary a parseObservation field that is not validated
against expectedImageContentId, allowing both parses to complete and
sameObservation to detect the difference. Assert the “changed during stable
identity capture” error so the test exercises the double-inspect stability
invariant rather than the earlier image-content validation.
In `@src/lib/onboard/managed-workload/onboard-orchestration.ts`:
- Around line 113-120: In the fallback branch of the runtimeCapabilities
assignment, add a short comment explaining that managedImages: null disables
managed images and forces the legacy Dockerfile path, while
managedImageSelectionPolicy remains prefer-managed.
In `@src/lib/onboard/runtime-provider/podman-preflight.test.ts`:
- Around line 181-193: Add a success-path test for inspectPodmanHost using a
successful engine receipt, and assert the complete status: "ok" result including
the detail template populated from that receipt. Keep the existing failure
assertion unchanged and cover the Linux x64 context and expected hint as
appropriate.
In `@src/lib/onboard/runtime-provider/podman.test.ts`:
- Around line 195-198: Remove the exact Object.keys(...).toEqual(["docker",
"kubernetes"]) assertion from the test “stays outside the production-selectable
registry”; retain the
CURRENT_RUNTIME_PROVIDER_BUNDLES.not.toHaveProperty("podman") assertion as the
only required check.
🪄 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: 1ac40196-2b34-4689-b433-503d8221dedf
📒 Files selected for processing (99)
.github/workflows/e2e.yaml.github/workflows/podman-cpu-proof.yamlci/protected-managed-image-runtime-activation-v1.jsonci/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-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/adapters/container-engine.test.tssrc/lib/adapters/container-engine.tssrc/lib/adapters/podman/index.test.tssrc/lib/adapters/podman/index.tssrc/lib/adapters/podman/socket-authority.test.tssrc/lib/adapters/podman/socket-authority.tssrc/lib/onboard.tssrc/lib/onboard/build-context-stage.test.tssrc/lib/onboard/build-context-stage.tssrc/lib/onboard/command-support.test.tssrc/lib/onboard/command-support.tssrc/lib/onboard/command.test.tssrc/lib/onboard/command.tssrc/lib/onboard/lifecycle-contracts.mdsrc/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/README.mdsrc/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/docker.tssrc/lib/onboard/managed-bootstrap/podman-bootstrap-journal.test.tssrc/lib/onboard/managed-bootstrap/podman-bootstrap-journal.tssrc/lib/onboard/managed-bootstrap/podman-bootstrap-replacement.test.tssrc/lib/onboard/managed-bootstrap/podman-bootstrap-replacement.tssrc/lib/onboard/managed-bootstrap/podman-held-workload.test.tssrc/lib/onboard/managed-bootstrap/podman-held-workload.tssrc/lib/onboard/managed-bootstrap/podman-image-transaction.test.tssrc/lib/onboard/managed-bootstrap/podman-image-transaction.tssrc/lib/onboard/managed-bootstrap/podman-watcher-lease.test.tssrc/lib/onboard/managed-bootstrap/podman-watcher-lease.tssrc/lib/onboard/managed-bootstrap/runtime-create.tssrc/lib/onboard/managed-workload/onboard-orchestration.tssrc/lib/onboard/runtime-provider/contract.tssrc/lib/onboard/runtime-provider/docker.tssrc/lib/onboard/runtime-provider/podman-lifecycle.test.tssrc/lib/onboard/runtime-provider/podman-lifecycle.tssrc/lib/onboard/runtime-provider/podman-preflight.test.tssrc/lib/onboard/runtime-provider/podman-preflight.tssrc/lib/onboard/runtime-provider/podman.test.tssrc/lib/onboard/runtime-provider/podman.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-extra-providers.test.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/sandbox-gpu-create-flow.tssrc/lib/onboard/sandbox-gpu-create-run-attempt.tssrc/lib/onboard/types.tstest/e2e/fixtures/shell-probe.tstest/e2e/fixtures/workload-source-env.tstest/e2e/live/bedrock-runtime-compatible-anthropic-raw-command.tstest/e2e/live/podman-cpu-lifecycle.test.tstest/e2e/mock-parity.jsontest/e2e/support/bedrock-runtime-compatible-anthropic-progress.test.tstest/e2e/support/e2e-cross-runtime-compatibility.test.tstest/e2e/support/podman-cpu-proof-workflow.test.tstest/e2e/support/workload-source-env.test.tstest/helpers/managed-image-buildless-e2e.tstest/helpers/onboard-script-mocks.cjstest/onboard-extra-provider-reconciliation.test.tstest/onboard-installer-restore-intent.test.tstest/onboard-managed-image-buildless-e2e.test.tstest/onboard-mcp-observability-redirect.test.tstest/onboard-messaging.test.tstest/onboard-prepared-build-context.test.tstest/onboard-reservation-recreate.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-e2e-gate.test.tstest/pr-risk-plan.test.tstest/runtime-provider-source-shape.test.tstest/shellquote-sandbox.test.tstools/advisors/risk-plan.mtstools/e2e/managed-image-protected-runtime-workflow-boundary.mts
💤 Files with no reviewable changes (5)
- src/lib/onboard/sandbox-create-plan-extra-providers.test.ts
- src/lib/onboard/machine/handlers/provider-inference-route-containment.test.ts
- src/lib/onboard/machine/handlers/provider-inference.test-support.ts
- src/lib/onboard/machine/core-flow-phases.test.ts
- src/lib/onboard/sandbox-create-plan.ts
🚧 Files skipped from review as they are similar to previous changes (48)
- src/lib/actions/sandbox/rebuild-gpu-opt-out.ts
- scripts/checks/run-managed-image-openshell-e2e.ts
- src/lib/onboard/runtime-provider/registry.ts
- src/lib/onboard/sandbox-create-intent-types.ts
- src/lib/onboard/managed-bootstrap/docker-authority-store.test.ts
- src/lib/onboard/sandbox-gpu-create-run-attempt.ts
- test/onboard-messaging.test.ts
- test/e2e/fixtures/shell-probe.ts
- test/onboard-sandbox-recreation.test.ts
- src/lib/actions/sandbox/rebuild-dcode-orchestrator.test.ts
- test/pr-risk-plan.test.ts
- test/e2e/support/e2e-cross-runtime-compatibility.test.ts
- src/lib/onboard/types.ts
- test/onboard-terminal-dashboard.test.ts
- test/onboard-sandbox-build.test.ts
- src/lib/onboard/sandbox-create-plan-materialization.ts
- src/lib/onboard/managed-bootstrap/docker-test-fixture.ts
- src/lib/actions/sandbox/rebuild-dcode-orchestrator.ts
- test/onboard-managed-image-buildless-e2e.test.ts
- test/onboard-prepared-build-context.test.ts
- test/onboard-reservation-recreate.test.ts
- src/lib/onboard/runtime-provider/docker.ts
- test/shellquote-sandbox.test.ts
- test/pr-e2e-gate.test.ts
- test/onboard-installer-restore-intent.test.ts
- src/lib/onboard/machine/handlers/provider-inference.ts
- src/lib/onboard/runtime-provider/runtime-provider-contract.test.ts
- src/lib/onboard/managed-bootstrap/docker-runtime.test.ts
- test/pr-e2e-gate-signal-shards.test.ts
- src/lib/actions/sandbox/rebuild-preflight-target-phase.ts
- src/lib/actions/sandbox/rebuild-preflight-guards.ts
- src/lib/actions/sandbox/agents/managed-workload-rebuild-profile.ts
- src/lib/onboard/managed-bootstrap/runtime-create.ts
- test/runtime-provider-source-shape.test.ts
- src/lib/actions/sandbox/rebuild-managed-workload-mutation-guard.test.ts
- test/onboard-mcp-observability-redirect.test.ts
- src/lib/onboard/sandbox-gpu-create-flow.test.ts
- tools/advisors/risk-plan.mts
- src/lib/actions/sandbox/rebuild-pipeline.ts
- test/onboard-extra-provider-reconciliation.test.ts
- src/lib/actions/sandbox/rebuild-preflight-phase.ts
- src/lib/onboard/managed-bootstrap/docker-authority-store.ts
- test/helpers/onboard-script-mocks.cjs
- src/lib/onboard/managed-bootstrap/docker.ts
- src/lib/onboard/sandbox-gpu-create-flow.ts
- src/lib/onboard/managed-bootstrap/docker-runtime.ts
- src/lib/onboard/sandbox-create-plan.test.ts
- src/lib/onboard.ts
Signed-off-by: Aaron Erickson <aerickson@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 LangChain 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-runtimewithout usage text, examples, command documentation, or quickstarts.Type of Change
Quality Gates
Documentation Writer Review
docs-updatedsrc/lib/onboard/lifecycle-contracts.mdandsrc/lib/onboard/managed-bootstrap/README.mddocument the folded dormant Podman CPU lifecycle and managed-bootstrap authority. Publicdocs/remains intentionally unchanged because--temp-managed-runtimeis hidden and default-off, while Podman remains absent from the production provider registry.Verification
Exact head:
79530819d68d2db8b713a47f1b802db2fa9e1be0.Signed-off-by: Aaron Erickson aerickson@nvidia.com
Summary by CodeRabbit
New Features
Bug Fixes
Tests