refactor(runtime): add provider lifecycle and mutation parity - #7990
Conversation
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>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com> (cherry picked from commit f99197b)
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com> (cherry picked from commit 7436f35)
Signed-off-by: Aaron Erickson <aerickson@nvidia.com> (cherry picked from commit 98f17dbb8409f4320763c4ad653bcda6b554e601)
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
added
area: onboarding
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe pull request adds provider-neutral runtime contracts, Docker and Kubernetes bundles, validated workload receipts, and provider-driven sandbox lifecycle, diagnosis, cleanup, rebuild, snapshot restore, and inference-set authority checks. ChangesRuntime provider migration
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in commit 75730cf in the TypeScript / code-coverage/cliThe overall coverage in commit 75730cf in the Show a code coverage summary of the most impacted files.
Updated |
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (11)
test/runtime-provider-source-shape.test.ts (1)
24-42: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKey the guarded sources by path instead of array position.
nonSnapshotActions,centralConsumers[4], andproviderContract[1]depend on the literal order of the arrays above. If a contributor inserts or reorders a path,slice(0, 6)silently stops coveringstop.tsand the provider-neutrality guard keeps passing with reduced coverage. Read the sources into a keyed record so each assertion names its file.♻️ Proposed keyed lookup
- const centralConsumers = [ - readFileSync(join(repoRoot, "src/lib/actions/inference-set.ts"), "utf8"), - readFileSync(join(repoRoot, "src/lib/actions/sandbox/destroy-execution.ts"), "utf8"), - readFileSync(join(repoRoot, "src/lib/actions/sandbox/destroy.ts"), "utf8"), - readFileSync(join(repoRoot, "src/lib/actions/sandbox/runtime/lifecycle-runtime.ts"), "utf8"), - readFileSync(join(repoRoot, "src/lib/actions/sandbox/start.ts"), "utf8"), - readFileSync(join(repoRoot, "src/lib/actions/sandbox/stop.ts"), "utf8"), - readFileSync(join(repoRoot, "src/lib/onboard/compute/plan.ts"), "utf8"), - readFileSync(join(repoRoot, "src/lib/onboard/sandbox-registration.ts"), "utf8"), - readFileSync(join(repoRoot, "src/lib/onboard/workload/runtime.ts"), "utf8"), - ]; - const nonSnapshotActions = centralConsumers.slice(0, 6); - const providerContract = [ - readFileSync(join(repoRoot, "src/lib/onboard/runtime-provider/contract.ts"), "utf8"), - readFileSync(join(repoRoot, "src/lib/onboard/runtime-provider/current.ts"), "utf8"), - readFileSync(join(repoRoot, "src/lib/onboard/runtime-provider/docker.ts"), "utf8"), - readFileSync(join(repoRoot, "src/lib/onboard/runtime-provider/registry.ts"), "utf8"), - ]; + const read = (relativePath: string) => readFileSync(join(repoRoot, relativePath), "utf8"); + const driverNeutralActions = { + "actions/inference-set.ts": read("src/lib/actions/inference-set.ts"), + "actions/sandbox/destroy-execution.ts": read("src/lib/actions/sandbox/destroy-execution.ts"), + "actions/sandbox/destroy.ts": read("src/lib/actions/sandbox/destroy.ts"), + "actions/sandbox/runtime/lifecycle-runtime.ts": read( + "src/lib/actions/sandbox/runtime/lifecycle-runtime.ts", + ), + "actions/sandbox/start.ts": read("src/lib/actions/sandbox/start.ts"), + "actions/sandbox/stop.ts": read("src/lib/actions/sandbox/stop.ts"), + }; + const onboardConsumers = { + "onboard/compute/plan.ts": read("src/lib/onboard/compute/plan.ts"), + "onboard/sandbox-registration.ts": read("src/lib/onboard/sandbox-registration.ts"), + "onboard/workload/runtime.ts": read("src/lib/onboard/workload/runtime.ts"), + }; + const providerContract = { + contract: read("src/lib/onboard/runtime-provider/contract.ts"), + current: read("src/lib/onboard/runtime-provider/current.ts"), + docker: read("src/lib/onboard/runtime-provider/docker.ts"), + registry: read("src/lib/onboard/runtime-provider/registry.ts"), + }; - for (const source of nonSnapshotActions) { + for (const source of Object.values(driverNeutralActions)) { expect(source).not.toMatch(/\b(?:docker|podman)\b/iu); expect(source).not.toMatch(/(?:adapters\/docker|docker-driver-sandbox-recovery)/u); } - for (const source of centralConsumers) { + for (const source of [ + ...Object.values(driverNeutralActions), + ...Object.values(onboardConsumers), + ]) { expect(source).not.toMatch(/\b(?:openshellDriver|driverName)\s*={2,3}\s*["'][^"']+["']/u); expect(source).not.toMatch(/switch\s*\([^)]*\b(?:openshellDriver|driverName)\b[^)]*\)/u); } - expect(centralConsumers[4]).toMatch(/resolved\.lifecycle\.verifyStarted\(/u); - expect(providerContract.join("\n")).not.toMatch(/managed-bootstrap/u); - expect(providerContract[1]).not.toMatch(/\b(?:podman|mxc)\b/iu); + expect(driverNeutralActions["actions/sandbox/start.ts"]).toMatch( + /resolved\.lifecycle\.verifyStarted\(/u, + ); + expect(Object.values(providerContract).join("\n")).not.toMatch(/managed-bootstrap/u); + expect(providerContract.current).not.toMatch(/\b(?:podman|mxc)\b/iu);🤖 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/runtime-provider-source-shape.test.ts` around lines 24 - 42, Replace positional source selection in the test with a path-keyed record for the runtime-provider files and central consumers. Update the assertions using nonSnapshotActions, centralConsumers[4], and providerContract[1] to retrieve sources by their explicit file keys, preserving coverage for stop.ts regardless of array order or insertions.src/lib/actions/sandbox/runtime/lifecycle-runtime.ts (1)
21-30: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCarry the resolved
sandboxon theok: truebranch.
SandboxLifecycleProviderResolutiondoes not relateok: trueto a non-nullsandbox. The caller must therefore re-assert the value it already passed in.src/lib/actions/sandbox/stop.tsline 64 usessandbox: sandbox!for exactly this reason. A future edit that changes the null check in this function would not be caught at that call site, because the assertion suppresses the error.♻️ Proposed resolution shape
export type SandboxLifecycleProviderResolution = | { readonly ok: true; + readonly sandbox: SandboxEntry; readonly bundle: RuntimeProviderBundle; readonly lifecycle: Extract<RuntimeProviderBundle["lifecycle"], { readonly supported: true }>; }Then return it at line 83:
- return { ok: true, bundle, lifecycle: bundle.lifecycle }; + return { ok: true, sandbox, bundle, lifecycle: bundle.lifecycle };
src/lib/actions/sandbox/stop.tsthen usessandbox: resolved.sandboxand drops the!.🤖 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/runtime/lifecycle-runtime.ts` around lines 21 - 30, Update SandboxLifecycleProviderResolution so its ok: true branch carries a non-null sandbox value, return that sandbox from the resolution function after its null check, and update the caller in stop.ts to use resolved.sandbox instead of sandbox!.src/lib/tunnel/sandbox-gateway-stop.test.ts (1)
130-133: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAssert the script constant instead of a substring of it.
expect.stringContaining("find_gateway_pids")couples the assertion to a shell function name insideGATEWAY_STOP_SCRIPT. A rename of that internal function breaks the test without any behavior change. Compare against the exported constant.♻️ Proposed assertion
expect.objectContaining({ - input: expect.stringContaining("find_gateway_pids"), + input: GATEWAY_STOP_SCRIPT, timeout: 20000, }),Import the constant from
src/lib/tunnel/gateway-stop-script.ts.As per path instructions: "Prefer observable outcomes through the public boundary over source-text, private-shape, or mock-call assertions."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/tunnel/sandbox-gateway-stop.test.ts` around lines 130 - 133, Update the test assertion around the gateway stop input to compare against the exported GATEWAY_STOP_SCRIPT constant rather than using expect.stringContaining with the internal find_gateway_pids function name. Import and reuse that constant from gateway-stop-script.ts while preserving the existing timeout assertion.Source: Path instructions
src/lib/tunnel/sandbox-gateway-stop.ts (1)
27-27: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse the contract transport type instead of restating the union.
src/lib/onboard/runtime-provider/contract.tsline 181 declareschannelStopTransport: "docker-kubectl-first" | "openshell"on the supported lifecycle surface.src/lib/actions/sandbox/stop.tsline 77 forwards that exact value into this dependency. Two independent declarations of one value can drift. If the contract gains a third transport, this file compiles and silently applies the kubectl-first branch.Export a named type from the contract and reference it here.
♻️ Proposed shared type
In
src/lib/onboard/runtime-provider/contract.ts:+export type RuntimeProviderChannelStopTransport = "docker-kubectl-first" | "openshell";Then in this file:
- channelStopTransport?: "docker-kubectl-first" | "openshell"; + channelStopTransport?: RuntimeProviderChannelStopTransport;🤖 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/tunnel/sandbox-gateway-stop.ts` at line 27, Export a named type for the channelStopTransport value from the runtime-provider contract, then update the channelStopTransport declaration in the sandbox gateway stop module to reference that shared type instead of repeating the string union. Preserve the existing optional property shape and values.src/lib/actions/sandbox/destroy-flow.test.ts (1)
107-118: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the provider-authority reason, not only
exit(1).The title claims the refusal comes from unknown runtime authority. The body proves only that destruction exited with code 1, skipped
sandbox delete, and kept the registry row. Other refusal paths in this flow also exit with 1, for example the force/MCP path at line 194. A regression that rejects"unknown-runtime"for an unrelated reason keeps this test green.
destroy-execution.tssurfaces theRuntimeProviderSelectionErrormessage asdeleteOutput, so the reason is observable at the boundary.💚 Proposed additional assertion
expect(harness.removeSandboxSpy).not.toHaveBeenCalled(); + const errorOutput = harness.errorSpy.mock.calls.map((call) => String(call[0])).join("\n"); + expect(errorOutput).toContain("unknown-runtime"); + expect(errorOutput).toContain("is not registered for this operation"); });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/destroy-flow.test.ts` around lines 107 - 118, Strengthen the test “preserves provider and registry ownership when runtime authority is unknown” by asserting that the rejected error or surfaced deleteOutput contains the unknown-runtime provider-authority reason, not just “process.exit(1)”. Keep the existing assertions that no sandbox delete occurs and the registry row remains, using the RuntimeProviderSelectionError message exposed by destroy-execution.ts.Source: Path instructions
src/lib/onboard/runtime-provider/registry.ts (1)
240-360: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffSplit
validateSupportedSurfaceSchemasinto per-surface validators.The function validates twelve surfaces plus two cross-surface agreement rules in one body. Each new surface adds another branch to the same function, and a reader must scan the whole body to find the rule for one surface.
Extract one small validator per surface, then keep the cross-surface agreement checks (lines 345-359) in this function.
validateWorkloadProfilealready shows the pattern.As per coding guidelines: "Prefix intentionally unused variables with
_and keep function complexity low."🤖 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/registry.ts` around lines 240 - 360, Split validateSupportedSurfaceSchemas into dedicated validators for each surface, moving the existing plan, capabilities, preflightDoctor, gateway, workload, lifecycle, mutationAuthority, bootstrap, snapshot, recovery, cleanup, and containerEngine checks into small named functions. Have the main function invoke those validators and retain only the two cross-surface agreement checks there. Keep validateWorkloadProfile’s existing pattern, and prefix any intentionally unused parameters with an underscore while keeping each validator simple.Source: Coding guidelines
src/lib/onboard/runtime-provider/contract.ts (1)
134-136: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlign
detachProvidersarity with the caller.The contract declares
detachProviders(sandboxName: string). The known caller insrc/lib/actions/sandbox/destroy-execution.tssupplies a zero-argument closure that already capturessandboxName:const detachProviders = (): DetachSandboxProvidersResult => runSandboxProviderPreDeleteCleanup(sandboxName, { runOpenshell, redact });TypeScript accepts this, so a provider that calls
operations.detachProviders(otherName)gets cleanup for the captured name instead. Either drop the parameter from the contract or make the caller forward the argument. A zero-argument shape matches the current single consumer.♻️ Proposed contract change
export interface RuntimeProviderCleanupOperations { - readonly detachProviders: (sandboxName: string) => RuntimeProviderProviderDetachResult; + readonly detachProviders: () => RuntimeProviderProviderDetachResult; }🤖 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/contract.ts` around lines 134 - 136, Update RuntimeProviderCleanupOperations.detachProviders to a zero-argument function matching the closure created by the destroy execution caller, which already captures sandboxName. Remove the sandboxName parameter from the contract and preserve the existing cleanup behavior.src/lib/onboard/compute/plan.ts (1)
4-12: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse explicit type-only imports in the three runtime-provider consumers.
access.tsexports these contracts withexport type, and all three consumers use them only as types. The current CommonJS configuration elides these imports, so this is not a current build failure. Addtypemodifiers to make the import contract explicit and prevent future compiler changes from introducing errors.🤖 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/compute/plan.ts` around lines 4 - 12, Update the runtime-provider imports in src/lib/onboard/compute/plan.ts lines 4-12, src/lib/onboard/workload/runtime.ts lines 6-12, and src/lib/actions/sandbox/runtime/lifecycle-runtime.ts lines 4-11 to mark every imported type-only contract with an explicit type modifier, while leaving runtime imports such as resolveRuntimeProviderBundle, resolveCurrentRuntimeProviderBundle, runtimeProviderContainerEngineIdentity, and RuntimeProviderGatewayLauncher unchanged.src/lib/onboard/runtime-provider/docker.ts (1)
357-437: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider moving the Kubernetes bundle out of
docker.ts.
createKubernetesRuntimeProviderBundlelives in a module nameddocker.ts, acceptsDockerRuntimeProviderDependencies, and reusesinspectDockerHostandremoveOwnedDockerWorkload. The shared host inspection also reports the label "Docker daemon" for the Kubernetes provider, which will surface as a Docker-named doctor row for Kubernetes sandboxes. A separatekubernetes.tsmodule plus a sharedcontainer-enginehelper module would make provider ownership explicit and keep the doctor label provider-accurate. This is a structural cleanup, not a behavior fix, so it can be deferred to the next slice if the current label is intended.🤖 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/docker.ts` around lines 357 - 437, Defer this structural cleanup unless the current Docker-labeled doctor row is not acceptable: otherwise move createKubernetesRuntimeProviderBundle out of docker.ts into a dedicated kubernetes.ts module, extract shared host-inspection and workload-cleanup logic into a container-engine helper, and update the Kubernetes dependencies and doctor labeling so it no longer reports “Docker daemon” under Kubernetes.src/lib/actions/sandbox/destroy-execution.ts (1)
234-240: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAlign
detachProviderswith its contract.
RuntimeProviderCleanupOperationsacceptssandboxName, butdestroy-execution.tsignores it and always uses the captured name. Accept the argument and pass it torunSandboxProviderPreDeleteCleanup, or remove the parameter from the contract and its callers.🤖 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/destroy-execution.ts` around lines 234 - 240, Update the detachProviders callback in the destroy execution flow to accept the sandboxName argument required by RuntimeProviderCleanupOperations, and pass that argument to runSandboxProviderPreDeleteCleanup instead of always using the captured name. Preserve the existing runOpenshell and redact options and all detachOutcome branching.test/image-cleanup.test.ts (1)
109-137: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd coverage for the
authority-unprovenblocking path.Add tests for both cleanup helpers. Assert that they return
falseornulland do not remove the registry entry when cleanup returns{ status: "skipped", reason: "authority-unproven" }.🤖 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/image-cleanup.test.ts` around lines 109 - 137, Add test coverage for the authority-unproven cleanup path in removeSandboxImage and removeSandboxRegistryEntry. Mock cleanup to return { status: "skipped", reason: "authority-unproven" }, then assert removeSandboxImage returns the skipped result without invoking image removal and removeSandboxRegistryEntry returns false or null without invoking the registry-removal callback.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lib/actions/sandbox/destroy.ts`:
- Around line 270-273: The authority-unproven image-cleanup outcome is currently
silent and reported as success. In src/lib/actions/sandbox/destroy.ts lines
270-273, propagate the skipped result through destroySandboxUnlocked, warn with
a recovery hint when removed is false around line 461, and suppress the success
message around line 500; in lines 244-250, log the provider id and caught
RuntimeProviderSelectionError message through deps.warn before returning the
skipped result. In test/image-cleanup.test.ts lines 109-137, add coverage
asserting authority-unproven makes removeSandboxRegistryEntry return false
without calling removeSandbox.
In `@src/lib/onboard/runtime-provider/runtime-provider-contract.test.ts`:
- Around line 582-587: Update the runOpenshell mock to remove the argument-based
if conditional and record the sandbox deletion event with a direct linear
expression before returning the existing response object.
In `@src/lib/onboard/workload/runtime.ts`:
- Around line 28-44: Remove the unused projectRuntimeProviderWorkloadProfiles
function, CURRENT_MANAGED_IMAGE_RUNTIME_PROFILES constant, and
ManagedImageRuntimeProfile-related type aliases. In cloneRuntimeSupport, replace
ManagedImageRuntimeSupport with RuntimeProviderManagedImageSupport while
preserving the existing cloning behavior.
In `@src/lib/state/registry/workload.ts`:
- Around line 21-22: Update the constants used by workload registry profile
validation to import and reuse MANAGED_STARTUP_PROFILE_MAX_BYTES and
MANAGED_STARTUP_PROFILE_MAX_ENCODED_BYTES from managed-startup/profile.ts,
removing the local MAX_PROFILE_BYTES and MAX_PROFILE_ENCODED_BYTES definitions.
---
Nitpick comments:
In `@src/lib/actions/sandbox/destroy-execution.ts`:
- Around line 234-240: Update the detachProviders callback in the destroy
execution flow to accept the sandboxName argument required by
RuntimeProviderCleanupOperations, and pass that argument to
runSandboxProviderPreDeleteCleanup instead of always using the captured name.
Preserve the existing runOpenshell and redact options and all detachOutcome
branching.
In `@src/lib/actions/sandbox/destroy-flow.test.ts`:
- Around line 107-118: Strengthen the test “preserves provider and registry
ownership when runtime authority is unknown” by asserting that the rejected
error or surfaced deleteOutput contains the unknown-runtime provider-authority
reason, not just “process.exit(1)”. Keep the existing assertions that no sandbox
delete occurs and the registry row remains, using the
RuntimeProviderSelectionError message exposed by destroy-execution.ts.
In `@src/lib/actions/sandbox/runtime/lifecycle-runtime.ts`:
- Around line 21-30: Update SandboxLifecycleProviderResolution so its ok: true
branch carries a non-null sandbox value, return that sandbox from the resolution
function after its null check, and update the caller in stop.ts to use
resolved.sandbox instead of sandbox!.
In `@src/lib/onboard/compute/plan.ts`:
- Around line 4-12: Update the runtime-provider imports in
src/lib/onboard/compute/plan.ts lines 4-12, src/lib/onboard/workload/runtime.ts
lines 6-12, and src/lib/actions/sandbox/runtime/lifecycle-runtime.ts lines 4-11
to mark every imported type-only contract with an explicit type modifier, while
leaving runtime imports such as resolveRuntimeProviderBundle,
resolveCurrentRuntimeProviderBundle, runtimeProviderContainerEngineIdentity, and
RuntimeProviderGatewayLauncher unchanged.
In `@src/lib/onboard/runtime-provider/contract.ts`:
- Around line 134-136: Update RuntimeProviderCleanupOperations.detachProviders
to a zero-argument function matching the closure created by the destroy
execution caller, which already captures sandboxName. Remove the sandboxName
parameter from the contract and preserve the existing cleanup behavior.
In `@src/lib/onboard/runtime-provider/docker.ts`:
- Around line 357-437: Defer this structural cleanup unless the current
Docker-labeled doctor row is not acceptable: otherwise move
createKubernetesRuntimeProviderBundle out of docker.ts into a dedicated
kubernetes.ts module, extract shared host-inspection and workload-cleanup logic
into a container-engine helper, and update the Kubernetes dependencies and
doctor labeling so it no longer reports “Docker daemon” under Kubernetes.
In `@src/lib/onboard/runtime-provider/registry.ts`:
- Around line 240-360: Split validateSupportedSurfaceSchemas into dedicated
validators for each surface, moving the existing plan, capabilities,
preflightDoctor, gateway, workload, lifecycle, mutationAuthority, bootstrap,
snapshot, recovery, cleanup, and containerEngine checks into small named
functions. Have the main function invoke those validators and retain only the
two cross-surface agreement checks there. Keep validateWorkloadProfile’s
existing pattern, and prefix any intentionally unused parameters with an
underscore while keeping each validator simple.
In `@src/lib/tunnel/sandbox-gateway-stop.test.ts`:
- Around line 130-133: Update the test assertion around the gateway stop input
to compare against the exported GATEWAY_STOP_SCRIPT constant rather than using
expect.stringContaining with the internal find_gateway_pids function name.
Import and reuse that constant from gateway-stop-script.ts while preserving the
existing timeout assertion.
In `@src/lib/tunnel/sandbox-gateway-stop.ts`:
- Line 27: Export a named type for the channelStopTransport value from the
runtime-provider contract, then update the channelStopTransport declaration in
the sandbox gateway stop module to reference that shared type instead of
repeating the string union. Preserve the existing optional property shape and
values.
In `@test/image-cleanup.test.ts`:
- Around line 109-137: Add test coverage for the authority-unproven cleanup path
in removeSandboxImage and removeSandboxRegistryEntry. Mock cleanup to return {
status: "skipped", reason: "authority-unproven" }, then assert
removeSandboxImage returns the skipped result without invoking image removal and
removeSandboxRegistryEntry returns false or null without invoking the
registry-removal callback.
In `@test/runtime-provider-source-shape.test.ts`:
- Around line 24-42: Replace positional source selection in the test with a
path-keyed record for the runtime-provider files and central consumers. Update
the assertions using nonSnapshotActions, centralConsumers[4], and
providerContract[1] to retrieve sources by their explicit file keys, preserving
coverage for stop.ts regardless of array order or insertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 48e5b907-df46-403d-b417-9609cccb5309
📒 Files selected for processing (37)
ci/source-architecture-budget.jsonci/source-shape-test-budget.jsonsrc/lib/actions/inference-set-failure-handling.test.tssrc/lib/actions/inference-set-provider.tssrc/lib/actions/inference-set.tssrc/lib/actions/sandbox/destroy-execution.tssrc/lib/actions/sandbox/destroy-flow.test.tssrc/lib/actions/sandbox/destroy.tssrc/lib/actions/sandbox/doctor-system-checks.tssrc/lib/actions/sandbox/doctor.tssrc/lib/actions/sandbox/runtime/lifecycle-runtime.tssrc/lib/actions/sandbox/start.test.tssrc/lib/actions/sandbox/start.tssrc/lib/actions/sandbox/stop.test.tssrc/lib/actions/sandbox/stop.tssrc/lib/onboard/compute/plan.tssrc/lib/onboard/runtime-provider/access.tssrc/lib/onboard/runtime-provider/contract.tssrc/lib/onboard/runtime-provider/current.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-registration.test.tssrc/lib/onboard/sandbox-registration.tssrc/lib/onboard/sandbox-workload-preparation.test.tssrc/lib/onboard/sandbox-workload-runtime.test.tssrc/lib/onboard/workload/runtime.tssrc/lib/state/registry.tssrc/lib/state/registry/persistence.tssrc/lib/state/registry/types.tssrc/lib/state/registry/workload.tssrc/lib/tunnel/sandbox-gateway-stop.test.tssrc/lib/tunnel/sandbox-gateway-stop.tstest/helpers/destroy-flow-test-harness.tstest/helpers/runtime-provider-bundle.tstest/image-cleanup.test.tstest/runtime-provider-source-shape.test.ts
|
@coderabbitai review |
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported Model lanes
Nemotron output stays in workflow artifacts and does not change the assessment above. E2E guidanceAdvisory only. E2E / PR Gate selects and runs jobs independently. Recommended E2E: 1 optional E2E recommendation
This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
|
✅ Action performedReview finished.
|
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
|
@coderabbitai review Exact-head follow-up at 8f8bcd5 addresses the remaining advisor findings: the cleanup-authority recovery boundary is documented with its source, repair constraint, regression proof, and removal condition; a repaired matching workload receipt now proves registry/session retirement; and table-driven unknown-provider start/stop tests prove failure before Docker, channel, dashboard, or gateway side effects. Focused suites and npm run validate:pr pass on this head. |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
target='8f8bcd5a18ae270538015e7f3772adf3a39d52fe'
git rev-parse --verify "${target}^{commit}"
git show --no-ext-diff --format='commit %H%nparent %P%nsubject %s' --no-patch "$target"
git diff --no-ext-diff --name-status "${target}^" "$target"
git diff --no-ext-diff --stat "${target}^" "$target"
echo '--- TypeScript outlines for changed source/test files ---'
git diff --no-ext-diff --name-only "${target}^" "$target" \
| grep -E '\.(ts|tsx)$' \
| while IFS= read -r file; do
echo "### $file"
ast-grep outline "$file" --items all --type function,class,interface,type 2>/dev/null || true
doneLength of output: 1875 🏁 Script executed: #!/bin/bash
set -euo pipefail
target='8f8bcd5a18ae270538015e7f3772adf3a39d52fe'
parent="${target}^"
echo '--- Full follow-up diff ---'
git diff --no-ext-diff --unified=80 "$parent" "$target" -- \
src/lib/actions/sandbox/destroy.ts \
src/lib/actions/sandbox/destroy-flow.test.ts \
src/lib/actions/sandbox/start.test.ts \
src/lib/actions/sandbox/stop.test.ts \
test/helpers/destroy-flow-test-harness.ts
echo '--- Current line-numbered changed files ---'
for file in \
src/lib/actions/sandbox/destroy.ts \
src/lib/actions/sandbox/destroy-flow.test.ts \
src/lib/actions/sandbox/start.test.ts \
src/lib/actions/sandbox/stop.test.ts \
test/helpers/destroy-flow-test-harness.ts
do
echo "### $file"
cat -n "$file"
doneLength of output: 146153 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '--- Implementation outlines ---'
for file in \
src/lib/actions/sandbox/start.ts \
src/lib/actions/sandbox/stop.ts \
src/lib/actions/sandbox/runtime/lifecycle-runtime.ts \
src/lib/onboard/runtime-provider/docker.ts \
src/lib/onboard/runtime-provider/registry.ts \
src/lib/actions/sandbox/destroy-execution.ts \
src/lib/state/registry.ts
do
echo "### $file"
ast-grep outline "$file" --items all 2>/dev/null || true
done
echo '--- Provider-resolution and cleanup implementations ---'
rg -n -A90 -B15 \
'function (startSandbox|stopSandbox|resolveSandboxLifecycleProvider|removeOwnedWorkload|removeSandboxWithReceipt)|const (startSandbox|stopSandbox|removeOwnedWorkload)' \
src/lib/actions/sandbox/start.ts \
src/lib/actions/sandbox/stop.ts \
src/lib/actions/sandbox/runtime/lifecycle-runtime.ts \
src/lib/onboard/runtime-provider/docker.ts \
src/lib/state/registry.ts
echo '--- Lifecycle and cleanup call sites ---'
rg -n -A8 -B8 \
'\b(resolveSandboxLifecycleProvider|removeSandboxRegistryEntryOutcome|removeOwnedWorkload|requireRuntimeProviderBundleForSandbox)\s*\(' \
src/lib/actions/sandbox \
src/lib/onboard/runtime-provider \
src/lib/state/registry.tsLength of output: 47470 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '--- Docker provider: lifecycle and owned-workload cleanup ---'
sed -n '105,275p' src/lib/onboard/runtime-provider/docker.ts
echo '--- Destroy execution: authority checks before live deletion ---'
sed -n '188,360p' src/lib/actions/sandbox/destroy-execution.ts
echo '--- Related lower-level cleanup tests ---'
rg -n -A55 -B12 \
'(authority-unproven|legacy-dockerfile|removeSandboxImage|removeSandboxRegistryEntry|shared)' \
test/image-cleanup.test.ts \
src/lib/actions/sandbox/destroy-flow.test.ts
echo '--- Exact follow-up patch whitespace check ---'
git diff --check d73f13a08688df4a61e00426d2206e1d7175d678 8f8bcd5a18ae270538015e7f3772adf3a39d52feLength of output: 45948
The start and stop actions resolve the persisted provider before the tested lifecycle side effects. The repaired legacy Dockerfile receipt matches the recorded image tag. The provider then removes the image, removes the registry entry, and retires the onboarding session. An unproven receipt still blocks registry and session retirement. The follow-up patch has no whitespace errors. ✅ Action performedReview finished.
|
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
|
@coderabbitai review Exact head bf2fd19 closes both retained-ownership callers: snapshot restore and rebuild now exit nonzero with repair guidance instead of reporting deletion success when provider-owned cleanup cannot retire the registry row. The mismatched legacy workload path and rebuild receipt-denial path have regression proofs. The Nemotron null/vm note is dispositioned as the pre-existing Docker compatibility contract; any writer able to set that local authority field can already set docker directly, while unknown registered identities still fail closed. Its destroy-race note is bounded by the outer per-sandbox lifecycle lock across the whole destroy operation, with the later provider check intentionally remaining fail closed. 228 focused tests and npm run validate:pr pass on this exact head. |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
base='8f8bcd5a18ae270538015e7f3772adf3a39d52fe'
head='bf2fd190113428cd9c452fdda1476a0a5c8eb439'
git rev-parse --verify "${base}^{commit}"
git rev-parse --verify "${head}^{commit}"
echo '--- commits in reviewed range ---'
git log --no-ext-diff --format='%H %s' "${base}..${head}"
echo '--- changed files ---'
git diff --no-ext-diff --name-status "$base" "$head"
echo '--- change statistics ---'
git diff --no-ext-diff --stat "$base" "$head"
echo '--- TypeScript outlines for changed files ---'
git diff --no-ext-diff --name-only "$base" "$head" \
| grep -E '\.(ts|tsx)$' \
| while IFS= read -r file; do
echo "### $file"
ast-grep outline "$file" --items all --type function,class,interface,type 2>/dev/null || true
done
echo '--- relevant diff ---'
git diff --no-ext-diff --unified=90 "$base" "$head" -- \
src/lib/actions/sandbox \
src/lib/onboard \
src/lib/state \
test \
ci
echo '--- whitespace check ---'
git diff --no-ext-diff --check "$base" "$head"Length of output: 49109 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '--- registry removal contracts ---'
ast-grep outline src/lib/actions/sandbox/destroy.ts --items all --type function --match 'removeSandboxRegistryEntry|removeSandboxRegistryEntryWithReceipt'
rg -n -A100 -B20 \
'export function (removeSandboxRegistryEntry|removeSandboxRegistryEntryWithReceipt)\s*\(' \
src/lib/actions/sandbox/destroy.ts
echo '--- rebuild destroy phase and direct callers ---'
ast-grep outline src/lib/actions/sandbox/rebuild-destroy-phase.ts --items all --type function --match 'runRebuildDestroyPhase|waitForRebuildDeleteAbsence'
rg -n -A55 -B15 \
'\brunRebuildDestroyPhase\s*\(' \
src/lib/actions/sandbox
echo '--- snapshot removal helper and restore error handling ---'
ast-grep outline src/lib/actions/sandbox/snapshot.ts --items all --type function,class --match 'requireSnapshotDestinationRegistryRemoval|deleteSandboxForRestore|snapshot|restore'
rg -n -A65 -B20 \
'(function (deleteSandboxForRestore|requireSnapshotDestinationRegistryRemoval)|deleteSandboxForRestore\s*\(|SnapshotCommandError|snapshotExit\s*\()' \
src/lib/actions/sandbox/snapshot.ts
echo '--- tests that cover snapshot restore destination deletion ---'
rg -n -A35 -B15 \
'(deleteSandboxForRestore|snapshot restore|Destination.*deleted|SnapshotCommandError|requireSnapshotDestinationRegistryRemoval)' \
src/lib/actions/sandbox --glob '*test.ts' \
test --glob '*test.ts'Length of output: 203629
Snapshot restore now throws Rebuild now calls The regression coverage verifies retained registry ownership, repair guidance, backup preservation, and false-success suppression. The reviewed diff has no whitespace errors. ✅ Action performedReview finished.
|
Require a side-effect-free provider cleanup plan before destructive sandbox actions. Validate provider and workload ownership before destroy, rebuild, or force-restore deletes. Preserve ownership state and give fail-closed guidance when authority is ambiguous. Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
|
Advisor remediation on exact head
@coderabbitai review |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
base='4788d287b8672be1b44999e78e094b2221303bd1'
head='4316a18daa2827442fe9786a8c943a1b29938bce'
git rev-parse --verify "${base}^{commit}"
git rev-parse --verify "${head}^{commit}"
echo '--- commits ---'
git log --no-ext-diff --format='%H %s' "${base}..${head}"
echo '--- changed files ---'
git diff --no-ext-diff --name-status "$base" "$head"
echo '--- change statistics ---'
git diff --no-ext-diff --stat "$base" "$head"
echo '--- TypeScript outlines for changed files ---'
git diff --no-ext-diff --name-only "$base" "$head" |
grep -E '\.(ts|tsx)$' |
while IFS= read -r file; do
echo "### $file"
ast-grep outline "$file" --items all --type function,class,interface,type 2>/dev/null || true
done
echo '--- authority, destroy, rebuild, and snapshot diff ---'
git diff --no-ext-diff --unified=60 "$base" "$head" -- \
src/lib/actions/sandbox \
src/lib/onboard/runtime-provider \
test \
ci
echo '--- whitespace ---'
git diff --no-ext-diff --check "$base" "$head"Length of output: 93054 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '--- relevant outlines ---'
for file in \
src/lib/actions/sandbox/destroy.ts \
src/lib/actions/sandbox/destroy-execution.ts \
src/lib/actions/sandbox/snapshot.ts \
src/lib/actions/sandbox/rebuild-destroy-phase.ts \
src/lib/onboard/runtime-provider/registry.ts \
src/lib/actions/sandbox/destroy-flow.test.ts \
src/lib/actions/sandbox/rebuild-destroy-phase.test.ts
do
echo "### $file"
ast-grep outline "$file" --items all --type function,class,interface,type 2>/dev/null || true
done
echo '--- destructive authority definitions and call sites ---'
rg -n -A75 -B20 \
'\b(requireRuntimeProviderDestructiveCleanupAuthority|requireSandboxDestructiveCleanupAuthority)\s*\(' \
src/lib/actions/sandbox \
src/lib/onboard/runtime-provider \
test
echo '--- snapshot destructive sequencing ---'
rg -n -A140 -B30 \
'(function (deleteSandboxForRestore|restoreSnapshot|requireSnapshotDestinationRegistryRemoval)|\b(deleteSandboxForRestore|requireSandboxDestructiveCleanupAuthority)\s*\()' \
src/lib/actions/sandbox/snapshot.ts
echo '--- rebuild destructive sequencing ---'
rg -n -A150 -B30 \
'(function (runRebuildDestroyPhase|deleteSandboxForRebuild)|\b(requireSandboxDestructiveCleanupAuthority|prepareMcpBridgesForDestroy|restoreMcpBridgesAfterDestroyAbort)\s*\()' \
src/lib/actions/sandbox/rebuild-destroy-phase.ts \
src/lib/actions/sandbox/rebuild-destroy-phase.test.ts
echo '--- snapshot and destroy regression cases ---'
rg -n -A100 -B20 \
'(unknown provider|mismatched legacy|authority.*change|before.*side effect|destructive cleanup|doctor --json)' \
src/lib/actions/sandbox --glob '*test.ts' \
test --glob '*test.ts'Length of output: 202226 🏁 Script executed: #!/bin/bash
set -euo pipefail
show_matches() {
local file="$1"
local pattern="$2"
echo "### $file :: $pattern"
rg -n -P "$pattern" "$file" | while IFS=: read -r line _; do
start=$(( line > 35 ? line - 35 : 1 ))
end=$(( line + 125 ))
sed -n "${start},${end}p" "$file" | nl -ba -v "$start"
echo '---'
done
}
show_matches src/lib/actions/sandbox/destroy.ts \
'\b(requireSandboxDestructiveCleanupAuthority|removeSandboxRegistryEntryWithReceipt|destroySandbox)\s*\('
show_matches src/lib/actions/sandbox/snapshot.ts \
'\b(requireSandboxDestructiveCleanupAuthority|deleteSandboxForRestore|runSandboxSnapshot)\s*\('
show_matches src/lib/actions/sandbox/rebuild-destroy-phase.ts \
'\b(requireSandboxDestructiveCleanupAuthority|runRebuildDestroyPhase|prepareMcpBridgesForDestroy|restoreMcpBridgesAfterDestroyAbort)\s*\('
show_matches src/lib/actions/sandbox/rebuild-destroy-phase.test.ts \
'(authority|receipt|restoreMcp|relock|delete)'
echo '--- all imports and direct authority calls, constrained to relevant sources ---'
rg -n -C4 \
'\b(requireRuntimeProviderDestructiveCleanupAuthority|requireSandboxDestructiveCleanupAuthority)\b' \
src/lib/actions/sandbox/destroy.ts \
src/lib/actions/sandbox/snapshot.ts \
src/lib/actions/sandbox/rebuild-destroy-phase.ts \
src/lib/onboard/runtime-provider/registry.tsLength of output: 1842 🏁 Script executed: #!/bin/bash
set -euo pipefail
show_matches() {
local file="$1"
local pattern="$2"
echo "### $file :: $pattern"
rg -n -P "$pattern" "$file" | while IFS=: read -r line _; do
start=$(( line > 35 ? line - 35 : 1 ))
end=$(( line + 125 ))
awk -v start="$start" -v end="$end" \
'NR >= start && NR <= end { printf "%6d %s\n", NR, $0 }' "$file"
echo '---'
done
}
show_matches src/lib/actions/sandbox/destroy.ts \
'\b(requireSandboxDestructiveCleanupAuthority|removeSandboxRegistryEntryWithReceipt|destroySandbox)\s*\('
show_matches src/lib/actions/sandbox/snapshot.ts \
'\b(requireSandboxDestructiveCleanupAuthority|deleteSandboxForRestore|runSandboxSnapshot)\s*\('
show_matches src/lib/actions/sandbox/rebuild-destroy-phase.ts \
'\b(requireSandboxDestructiveCleanupAuthority|runRebuildDestroyPhase|prepareMcpBridgesForDestroy|restoreMcpBridgesAfterDestroyAbort)\s*\('
show_matches src/lib/actions/sandbox/rebuild-destroy-phase.test.ts \
'(authority|receipt|restoreMcp|relock|delete)'
echo '--- all imports and direct authority calls, constrained to relevant sources ---'
rg -n -C4 \
'\b(requireRuntimeProviderDestructiveCleanupAuthority|requireSandboxDestructiveCleanupAuthority)\b' \
src/lib/actions/sandbox/destroy.ts \
src/lib/actions/sandbox/snapshot.ts \
src/lib/actions/sandbox/rebuild-destroy-phase.ts \
src/lib/onboard/runtime-provider/registry.tsLength of output: 119036 🏁 Script executed: #!/bin/bash
set -euo pipefail
print_range() {
local file="$1"
local start="$2"
local end="$3"
echo "### $file lines $start-$end"
awk -v start="$start" -v end="$end" \
'NR >= start && NR <= end { printf "%6d %s\n", NR, $0 }' "$file"
}
print_range src/lib/actions/sandbox/destroy.ts 235 325
print_range src/lib/actions/sandbox/snapshot.ts 450 560
print_range src/lib/actions/sandbox/rebuild-destroy-phase.ts 95 300
print_range src/lib/actions/sandbox/rebuild-destroy-phase.test.ts 620 815
print_range src/lib/actions/sandbox/destroy-flow.test.ts 1 220
echo '--- check for destructive calls before the snapshot authority call ---'
awk 'NR >= 430 && NR <= 520 { printf "%6d %s\n", NR, $0 }' \
src/lib/actions/sandbox/snapshot.ts |
grep -nE '(requireSandboxDestructiveCleanupAuthority|stopNim|runOpenshell|cleanupShields|streamSandboxCreate|registerSandbox)' || true
echo '--- check for destructive calls around rebuild authority revalidation ---'
awk 'NR >= 95 && NR <= 300 { printf "%6d %s\n", NR, $0 }' \
src/lib/actions/sandbox/rebuild-destroy-phase.ts |
grep -nE '(requireSandboxDestructiveCleanupAuthority|prepareMcp|restoreMcp|relock|runOpenshell|stopNim)' || trueLength of output: 40943 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '--- rebuild: MCP preparation through delete edge ---'
awk 'NR >= 300 && NR <= 450 { printf "%6d %s\n", NR, $0 }' \
src/lib/actions/sandbox/rebuild-destroy-phase.ts
echo '--- rebuild: changed-authority regression cases ---'
rg -n -A110 -B20 \
'(authority changes|authority.*preparation|cleanup authority|revalidateBeforeDelete|assertDeleteEdgeUnchanged|reattachMcp)' \
src/lib/actions/sandbox/rebuild-destroy-phase.test.tsLength of output: 46896
The shared destructive-cleanup authority boundary checks registered provider identity, required mutation authority, supported cleanup, and workload ownership before destructive cleanup. Snapshot force-restore checks authority before NIM stop and OpenShell deletion. The regression cases verify that unknown providers and mismatched receipts cause no destructive side effects. Rebuild checks authority before MCP preparation. It checks authority again at the delete edge. If the second check fails, it restores MCP state, relocks shields, and stops before sandbox deletion. Destroy preserves registry and session ownership when cleanup authority is unproven. It permits retirement after a matching repaired receipt. The reviewed range has no whitespace errors. ✏️ Learnings added
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
src/lib/actions/sandbox/rebuild-resume-snapshot.test.ts (1)
186-194: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueType the receipt entry instead of casting to
never.
{ name: "alpha", agent: null }satisfiesSandboxEntry. Theas nevercast suppresses future shape drift inSandboxRemovalReceipt, so a contract change will not fail this test.♻️ Proposed change
- entry: { - name: "alpha", - agent: null, - } as never, + entry: { name: "alpha", agent: null } satisfies SandboxEntry,🤖 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-resume-snapshot.test.ts` around lines 186 - 194, Update the mock receipt in the test around removeSandboxRegistryEntryWithReceipt to type entry as the expected SandboxEntry shape instead of casting { name: "alpha", agent: null } to never, preserving compile-time detection of future SandboxRemovalReceipt contract changes.src/lib/actions/sandbox/rebuild-destroy-phase.test.ts (2)
409-411: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAvoid asserting the exact registry read count.
toHaveBeenCalledTimes(3)locks the number ofregistry.getSandboxreads inrunRebuildDestroyPhase. Any added read fails this test without a behavior change. Keep the ordering assertion and drop the exact count, or assert a lower bound.As per path instructions: "Prefer observable outcomes through the public boundary over source-text, private-shape, or mock-call assertions."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/actions/sandbox/rebuild-destroy-phase.test.ts` around lines 409 - 411, Remove the exact getSandbox call-count assertion from the test around runRebuildDestroyPhase, since it over-specifies internal reads. Preserve the prepareMcpForRebuild ordering assertion and, if needed, replace the count check with a lower-bound assertion while continuing to validate the observable ordering behavior.Source: Path instructions
265-274: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBoth assertions couple to the number of
registry.getSandboxreads.runRebuildDestroyPhasereads the registry at several points, so ordinal-keyed mocks and exact call counts break or silently weaken when a read is added.
src/lib/actions/sandbox/rebuild-destroy-phase.test.ts#L265-L274: return the drifted entry based on a flag set whenprepareMcpForRebuildresolves, instead ofmockReturnValueOncetwice.src/lib/actions/sandbox/rebuild-destroy-phase.test.ts#L409-L411: droptoHaveBeenCalledTimes(3)and keep only the invocation-order assertion.As per path instructions: "Prefer observable outcomes through the public boundary over source-text, private-shape, or mock-call assertions."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/actions/sandbox/rebuild-destroy-phase.test.ts` around lines 265 - 274, Decouple both assertions from the number of registry reads in rebuild-destroy-phase.test.ts: at lines 265-274, have getSandbox return the drifted entry based on a flag set when prepareMcpForRebuild resolves instead of using two mockReturnValueOnce calls; at lines 409-411, remove the exact toHaveBeenCalledTimes(3) assertion and retain only the invocation-order assertion.Source: Path instructions
src/lib/actions/sandbox/snapshot.ts (1)
481-492: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueRedact the authority failure detail before printing it.
src/lib/actions/sandbox/rebuild-destroy-phase.tsline 135 wraps the same class of cleanup-authority detail inredactFull(detail). Apply the same treatment here so both destructive paths print the detail under one redaction rule.♻️ Proposed change
- const detail = error instanceof Error ? error.message : String(error); + const detail = redactFull(error instanceof Error ? error.message : String(error));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/actions/sandbox/snapshot.ts` around lines 481 - 492, Update the cleanup-authority failure handling around requireSandboxDestructiveCleanupAuthority in the snapshot deletion path to pass the derived detail through redactFull before including it in console.error. Keep the existing error normalization, message context, and retry guidance unchanged, matching the treatment in rebuild-destroy-phase.ts.src/lib/actions/sandbox/rebuild-destroy-phase.ts (1)
133-137: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueHardcoded
nemoclawcommand name in two recovery messages. Both messages build doctor guidance from a literal instead of the sharedCLI_NAMEconstant used bysrc/lib/actions/sandbox/snapshot.tsandsrc/lib/actions/sandbox/destroy.ts.
src/lib/actions/sandbox/rebuild-destroy-phase.ts#L133-L137: replace'nemoclaw ${sandboxName} doctor --json'with'${CLI_NAME} ${sandboxName} doctor --json'and importCLI_NAMEfrom../../cli/branding.src/lib/actions/sandbox/rebuild-destroy-phase.ts#L523-L527: replace the same literal in the registry-retention message with${CLI_NAME}.🤖 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-destroy-phase.ts` around lines 133 - 137, Replace the hardcoded nemoclaw command in both recovery messages in rebuild-destroy-phase.ts (lines 133-137 and 523-527) with CLI_NAME, and import CLI_NAME from ../../cli/branding. Preserve the existing sandboxName doctor command formatting and message content at both sites.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lib/actions/sandbox/snapshot-restore-test-fixture.ts`:
- Line 314: Update the removeSandboxRegistryEntry mock in the snapshot restore
test fixture to return true, matching the production function’s boolean contract
and the success path expected by requireSnapshotDestinationRegistryRemoval.
In `@src/lib/onboard/runtime-provider/runtime-provider-contract.test.ts`:
- Around line 352-369: Update the test around planOwnedWorkloadCleanup to
snapshot the relevant sandbox and runtime state before invoking the planner,
then assert both snapshots are unchanged afterward. Keep the existing
authority-unproven plan assertion, and verify the planner remains
side-effect-free while validating the cleanup request.
---
Nitpick comments:
In `@src/lib/actions/sandbox/rebuild-destroy-phase.test.ts`:
- Around line 409-411: Remove the exact getSandbox call-count assertion from the
test around runRebuildDestroyPhase, since it over-specifies internal reads.
Preserve the prepareMcpForRebuild ordering assertion and, if needed, replace the
count check with a lower-bound assertion while continuing to validate the
observable ordering behavior.
- Around line 265-274: Decouple both assertions from the number of registry
reads in rebuild-destroy-phase.test.ts: at lines 265-274, have getSandbox return
the drifted entry based on a flag set when prepareMcpForRebuild resolves instead
of using two mockReturnValueOnce calls; at lines 409-411, remove the exact
toHaveBeenCalledTimes(3) assertion and retain only the invocation-order
assertion.
In `@src/lib/actions/sandbox/rebuild-destroy-phase.ts`:
- Around line 133-137: Replace the hardcoded nemoclaw command in both recovery
messages in rebuild-destroy-phase.ts (lines 133-137 and 523-527) with CLI_NAME,
and import CLI_NAME from ../../cli/branding. Preserve the existing sandboxName
doctor command formatting and message content at both sites.
In `@src/lib/actions/sandbox/rebuild-resume-snapshot.test.ts`:
- Around line 186-194: Update the mock receipt in the test around
removeSandboxRegistryEntryWithReceipt to type entry as the expected SandboxEntry
shape instead of casting { name: "alpha", agent: null } to never, preserving
compile-time detection of future SandboxRemovalReceipt contract changes.
In `@src/lib/actions/sandbox/snapshot.ts`:
- Around line 481-492: Update the cleanup-authority failure handling around
requireSandboxDestructiveCleanupAuthority in the snapshot deletion path to pass
the derived detail through redactFull before including it in console.error. Keep
the existing error normalization, message context, and retry guidance unchanged,
matching the treatment in rebuild-destroy-phase.ts.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 126e8cd9-ecb6-4fcc-9257-f49a85af78ed
📒 Files selected for processing (16)
src/lib/actions/sandbox/destroy-execution.tssrc/lib/actions/sandbox/destroy-flow.test.tssrc/lib/actions/sandbox/destroy.tssrc/lib/actions/sandbox/rebuild-destroy-phase.test.tssrc/lib/actions/sandbox/rebuild-destroy-phase.tssrc/lib/actions/sandbox/rebuild-resume-snapshot.test.tssrc/lib/actions/sandbox/snapshot-restore-lifecycle.test.tssrc/lib/actions/sandbox/snapshot-restore-test-fixture.tssrc/lib/actions/sandbox/snapshot.tssrc/lib/onboard/runtime-provider/access.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.tstest/helpers/runtime-provider-bundle.tstest/image-cleanup.test.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- test/helpers/runtime-provider-bundle.ts
- src/lib/onboard/runtime-provider/access.ts
- src/lib/actions/sandbox/destroy-flow.test.ts
- test/image-cleanup.test.ts
- src/lib/actions/sandbox/destroy.ts
- src/lib/actions/sandbox/destroy-execution.ts
Keep the force-restore fixture aligned with the required cleanup-authority boundary. Its success path now exercises the new guard before deletion. Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Snapshot provider and sandbox state around cleanup planning. Keep snapshot registry-removal mocks faithful to the production boolean contract. Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
b758dc7
into
feat/buildless-managed-contract-hardening
<!-- markdownlint-disable MD041 --> ## Summary Adds the inert, provider-neutral managed-workload rebuild transaction for the incremental buildless stack. The exact old workload and registry row remain authoritative through replacement preparation, readiness, state restore, and provider rebind. Only one exact compare-and-swap publishes the replacement, and old-runtime retirement happens afterward through provider-owned opaque handles. This slice does not wire a production rebuild caller or activate buildless onboarding. Snapshot/backup and durable recovery ownership remain tracked in #7744 and are required before activation. ## Related Issue Part of #7744 ## Changes - Capture a deep-frozen rebuild plan bound to the exact provider, shipped agent, platform, prior managed receipt, full durable-row revision, lifecycle generation, and live identity fingerprint. - Pre-render and validate the exact replacement image, startup profile, receipt, and safe metadata before provider mutation. - Define provider-neutral prepare, create, readiness, restore, provider-rebind, rollback, abort-preparation, and retire-previous phases using opaque exact handles rather than sandbox-name deletion. - Keep partial prepare/create cleanup transaction-idempotent and run abort cleanup even when post-prepare registry revalidation throws. - Publish only through exact old-authority CAS; reconcile ambiguous persistence against either the exact replacement or exact old row. - Preserve the staged replacement and return an immutable recovery task when publication is indeterminate, avoiding rollback of a replacement that may already be durable. - Retire the exact old runtime only after publication; return a durable-owner recovery task if retirement remains pending. - Bind replacement contracts and startup profiles to OpenClaw, Hermes, or DCode authority and reject provider, agent, platform, receipt, generation, or identity drift. - Reject malformed provider artifacts at every transition, stop before later phases, and prove exact transaction abort or exact staged-handle rollback. - Document the shared backup boundary and the durable recovery ownership tracked in #7744 before activation. ## Type of Change - [x] Code change (feature, bug fix, or refactor) - [x] Code change with doc updates - [ ] Doc only (prose changes, no code sample modifications) - [ ] Doc only (includes code sample changes) ## Quality Gates - [x] Tests added or updated for changed behavior - [ ] Existing tests cover changed behavior — justification: - [ ] Tests not applicable — justification: - [ ] Docs updated for user-facing behavior changes - [x] Docs not applicable — justification: The transaction is inert with no production caller or support claim; the internal README records ownership boundaries for later slices. - [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: Exact-head audit covers immutable authority, pre-mutation validation, CAS ambiguity, abort cleanup, exact-handle rollback, and deferred recovery ownership. - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## Documentation Writer Review - [x] Documentation writer subagent reviewed the completed changes - Result: `docs-updated` - Evidence: The reviewed 23-file, `+4,272/-0` slice remains byte-identical after the append-only current-main refresh to `0de2789608a86e580d787991e81c03c5f0b14dbf` through `e97ecce48c7fcc1dfb398e1cfae8c81a859b7dcd`; stable patch ID remains `dd1c4a899fd9a62954a00d4e2e61da445a306e03`. The only documentation path is `src/lib/onboard/managed-workload/rebuild/README.md`. It accurately states that the transaction is dormant, has no CLI command or production-action importer, and does not activate buildless rebuilds. It assigns ambiguous publication and pending retirement to durable recovery, links recovery and snapshot/backup ownership to the live accepted epic #7744, and requires normalized backup manifests, restore validation, durable reconciliation, and protected qualification for OpenClaw, Hermes, and LangChain Deep Agents Code before activation. Production-import and command/action diff scans found no activation caller. Markdownlint passed with zero issues on the exact refreshed head. The append-only parent refresh to `362a70cda` preserves the exact reviewed slice diff and changes no reviewed documentation. - Agent: Codex Desktop <!-- docs-review-head-sha: 362a70c --> <!-- docs-review-agents-blob-sha: 3dd7c24 --> ## DGX Station Hardware Evidence - [ ] Tested on DGX Station - Tested commit: - Station profile/scenario: - Result: - Supporting evidence: ## Verification - Exact locally validated head/base: `e97ecce48c7fcc1dfb398e1cfae8c81a859b7dcd` / `0de2789608a86e580d787991e81c03c5f0b14dbf` - Review budget: 23 files, `+4,272/-0`. - Stable exact-slice patch ID: `dd1c4a899fd9a62954a00d4e2e61da445a306e03`. - [x] The six implementation/review commits and both maintainer refresh commits are SSH-signed and contain DCO trailers; GitHub-generated conflict-resolution merge commits preserve append-only branch history. - [x] `npm run validate:pr` passed on the exact clean head with Node 22.16.0. - [x] 132 focused rebuild transaction, workload authority, registry CAS, and source-boundary tests passed again on the exact refreshed head; CLI typecheck and repository checks also passed; changed test files add zero `if` statements. - [x] `npm run build:cli`, CLI typecheck through `validate:pr`, exact-base pre-commit, commitlint, and pre-push gates passed. - [x] Failure tests prove prepare/create ambiguity aborts exact transaction resources, staged failures roll back only exact staging authority, and indeterminate CAS never rolls back. - [x] Agent-binding tests reject cross-agent image/profile drift for all shipped managed-image agents. - [x] No snapshot manifest dependency, production rebuild callsite, runtime selection change, or public activation exists in this slice. - [ ] Applicable broad gate passed — exact-head required CI, advisors, CodeRabbit, multiarch, and protected E2E are the broad remote gates. ## Stack - Base: live `main` at `0de2789608a86e580d787991e81c03c5f0b14dbf`; PR3.1 through PR3.6 content is already landed, with #7976, #7988, and #7990 carried once through the final #7973 aggregate tree. - This slice: PR3.7 branch `feat/managed-workload-rebuild-parity` at `e97ecce48c7fcc1dfb398e1cfae8c81a859b7dcd`. - Epic #7744 tracks shared snapshot, backup, restore, and durable recovery ownership before activation. - Buildless support remains disabled until OpenClaw, Hermes, and DCode plus required multiarch and protected qualification pass together. Signed-off-by: Aaron Erickson <aerickson@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added managed workload rebuild workflows with staged replacement, validation, rollback, recovery, and atomic commit handling. * Added authority validation for managed workloads, including receipt, image, platform, and startup configuration checks. * Added safe cloning and deep-freezing for supported immutable data. * Added safeguards against stale, conflicting, or incomplete workload state during rebuilds. * **Documentation** * Documented rebuild recovery behavior and activation requirements. * **Tests** * Added comprehensive coverage for rebuild transactions, authority validation, rollback, persistence reconciliation, and immutable data handling. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Aaron Erickson <aerickson@nvidia.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
3 participants
Summary
Introduces the driver-neutral runtime-provider lifecycle and mutation contract used by the incremental buildless/runtime stack, and closes the destructive-cleanup authority boundary identified during exact-head review. Destroy, rebuild, and snapshot force-restore must now prove provider and workload cleanup authority through a side-effect-free provider plan before deleting or stopping anything.
Production selection remains limited to the existing Docker and Kubernetes providers. This slice does not activate another runtime or expand supported lifecycle platforms.
Related Issue
Part of #7744
Changes
planOwnedWorkloadCleanupto the cleanup contract and require every supported provider to prove cleanup intent without side effects before a destructive action.nemoclaw <sandbox> doctor --jsondiagnostic path. Operators must restore trusted ownership metadata or resolve the runtime conflict and must not rewrite a receipt to match a mutable sandbox name.lifecycle.supported: false, and this PR does not claim Kubernetes lifecycle activation.Direct connect, status, logs, authenticated reconciliation, and durable crash recovery remain owned by later slices. No future provider is production-selectable or advertised by this PR.
Type of Change
Quality Gates
Documentation Writer Review
no-docs-needed+4,073/-508) tightens an inert internal provider and destructive-authority contract and replaces misleading failure text with an existing diagnostic command. It does not activate or advertise a new provider, platform, or runtime.DGX Station Hardware Evidence
Verification
75730cf09bf1a1aa901cc3b275052250f8e7d85d/4788d287b8672be1b44999e78e094b2221303bd1+4,073/-508; five files above the soft file guide to apply and prove one complete cross-cutting destructive-authority boundary, while remaining within the 2–5k line guide.eef1fddf8138d6e8a3ef4efb443aa2adc9f74fe3.Signed-off-by:line and every new commit contains an SSH signature and DCO trailer.npm run validate:prpassed on the exact clean head.git diff --checkpassed.doctor --jsonis named, unsafe receipt rewriting is rejected, and no false success is emitted.Stack
feat/buildless-runtime-e2e-foundationat4788d287b8672be1b44999e78e094b2221303bd1.feat/runtime-provider-lifecycle-parityat75730cf09bf1a1aa901cc3b275052250f8e7d85d.Signed-off-by: Aaron Erickson aerickson@nvidia.com
Summary by CodeRabbit