feat(runtime): add transactional Podman bootstrap preparation - #8055
feat(runtime): add transactional Podman bootstrap preparation#8055ericksoa wants to merge 20 commits into
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>
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>
📝 WalkthroughWalkthroughAdds a durable Podman bootstrap journal and a dormant replacement transaction. The workflow validates identities, ownership, authority, and runtime state; creates a stopped replacement; stops only the exact original; and supports authorized rollback with filesystem-backed recovery. ChangesPodman bootstrap replacement
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in commit e724196 in the TypeScript / code-coverage/cliThe overall coverage in commit e724196 in the Show a code coverage summary of the most impacted files.
Updated |
|
@coderabbitai review |
✅ Action performedReview finished.
|
PR Review Advisor — InformationalAdvisor assessment: Informational / low confidence Model lanes
Second-opinion 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: 1 warning · 0 suggestionsWarningsWarnings do not block.
|
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
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: 7
🧹 Nitpick comments (4)
src/lib/onboard/managed-bootstrap/podman-bootstrap-journal.ts (1)
559-572: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winVerify the durable read-back in
recordOriginalStopped.
recordStateVolume(Lines 525-529) andrecordReplacement(Lines 553-556) both re-read the journal and compare it withsameJournalbefore they return.recordOriginalStoppedwrites and then returns the in-memoryupdatedvalue. Theoriginal-stoppedphase is the boundary that records that the exact original container is stopped, so it carries the same durability requirement as the two preceding transitions. Apply the same read-back so the store contract stays uniform.♻️ Proposed read-back for `recordOriginalStopped`
const updated = normalizePodmanBootstrapJournal({ ...current, phase: "original-stopped" }); atomicWrite(directory, target, serializePodmanBootstrapJournal(updated), false); - return updated; + const persisted = load(bootstrapIdentity); + if (!persisted || !sameJournal(persisted, updated)) { + fail("original stop boundary was not durably re-readable"); + } + return persisted; },🤖 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.ts` around lines 559 - 572, Update recordOriginalStopped to re-read the journal from durable storage after atomicWrite, compare the read-back with updated using the existing sameJournal validation, and return the verified persisted value instead of the in-memory object. Preserve the existing phase checks and idempotent current return behavior.src/lib/onboard/managed-bootstrap/podman-bootstrap-replacement.test.ts (2)
49-51: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDo not recompute the production naming formula in the test.
Lines 49-51 rebuild the staging name and the state-volume name with the same expression that
stagingNameandstateVolumeNameuse inpodman-bootstrap-replacement.ts(Lines 346-363). The assertions at Lines 352-353 then compare the result against that recomputation. If the production suffix changes, both sides change together and the test still passes, so it cannot detect a naming regression.Use literal expected names so the deterministic naming contract is pinned.
♻️ Proposed literal name expectations
-const STAGING_NAME = `${ORIGINAL_NAME}-nemoclaw-bootstrap-${BOOTSTRAP_IDENTITY.slice(0, 12)}`; -const STATE_VOLUME_NAME = `${ORIGINAL_NAME}-nemoclaw-state-${BOOTSTRAP_IDENTITY.slice(0, 12)}`; +const STAGING_NAME = "openshell-sandbox-alpha-nemoclaw-bootstrap-111111111111"; +const STATE_VOLUME_NAME = "openshell-sandbox-alpha-nemoclaw-state-111111111111";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-bootstrap-replacement.test.ts` around lines 49 - 51, Replace the computed STAGING_NAME and STATE_VOLUME_NAME constants in the test with literal expected names derived from the current deterministic naming contract. Keep STATE_VOLUME_MOUNTPOINT based on the literal state-volume name, so the assertions test production output without duplicating the naming formula used by podman-bootstrap-replacement.ts.Source: Path instructions
574-613: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for two fail-closed branches on the critical path.
The suite covers ambiguity and engine-authority mismatch. Two adjacent guards have no test:
stopExactPodmanBootstrapOriginalrejects apreparedvalue whosereplacementStateVolumeMountpointorreplacementSpecFingerprintdiverges from the journal (Lines 1020-1028 ofpodman-bootstrap-replacement.ts). Pass a mutatedpreparedand assert "does not match the durable journal", and assert that the original still runs.rollbackPodmanBootstrapBeforeCommitfails when the journal recorded a mountpoint but the state volume is gone (Line 1087). Setharness.stateVolume = nullafter a successfulprepare, then assert "recorded state volume disappeared before rollback".Both guards protect the boundary at which the original container is stopped, so a regression in either is silent today.
Do you want me to write both test cases?
🤖 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-replacement.test.ts` around lines 574 - 613, Add tests for the two uncovered fail-closed guards: mutate the prepared replacementStateVolumeMountpoint or replacementSpecFingerprint before calling stopExactPodmanBootstrapOriginal, assert “does not match the durable journal,” and verify the original remains running; separately, after successful prepare, set harness.stateVolume to null, call rollbackPodmanBootstrapBeforeCommit, assert “recorded state volume disappeared before rollback,” and preserve the journal assertions relevant to rollback authorization.src/lib/onboard/managed-bootstrap/podman-bootstrap-replacement.ts (1)
933-943: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCover or remove the
candidate.Id ?? candidate.IDfallback.Line 936 accepts two spellings of the Podman
container ls --format jsonidentity field. The coding guidelines prohibit compatibility layers without a current requirement, and they ask for the current consumer and a protecting test. The harness inpodman-bootstrap-replacement.test.tsemits onlyId(Line 289), so theIDbranch has no coverage.If a supported Podman version emits
ID, add a test that exercises that branch. If no supported version emitsID, readIdonly.Line 940 also contains dead logic.
ids.length > 1already fails, sonew Set(ids).size !== ids.lengthcan never be reached.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-bootstrap/podman-bootstrap-replacement.ts` around lines 933 - 943, Update the staging discovery mapping in the replacement flow to read only the currently supported Podman identity field, Id, unless a supported version requires ID; if so, add a protecting test that emits ID. Remove the unreachable duplicate-identity Set check and retain the existing single-identity validation through ids.length > 1.Source: Coding guidelines
🤖 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/lifecycle-contracts.md`:
- Around line 167-177: Update the rollback requirement around
stopExactPodmanBootstrapOriginal so the caller invokes
rollbackPodmanBootstrapBeforeCommit only when the thrown
PodmanBootstrapPreparationError has rollbackRequired set. Preserve the
no-rollback path for pre-mutation validation failures where no journal exists.
In `@src/lib/onboard/managed-bootstrap/podman-bootstrap-replacement.test.ts`:
- Around line 405-414: Update the test around the first prepare call to capture
and assert on its thrown Podman create failure, rather than invoking prepare
again with the same journal state. Ensure the assertion verifies the thrown
error does not contain either credential string, and use an assertion pattern
that fails if prepare returns normally; preserve the existing status and
journal-phase checks.
- Around line 196-292: Refactor PodmanHarness.capture to remove its conditional
chain and dispatch commands through a table keyed by the first two arguments.
Move each existing branch into named private helper methods, preserving all fake
behavior and return values, and route unknown commands through a single
unsupported helper that throws the existing Unexpected Podman command error.
In `@src/lib/onboard/managed-bootstrap/podman-bootstrap-replacement.ts`:
- Around line 240-254: Update the key comparator in exactStringMap to use
deterministic code-unit ordering instead of localeCompare, ensuring
canonicalLabels and the resulting persisted fingerprints remain identical across
locales and ICU environments.
- Around line 301-314: Update assertMountDoesNotShadowState to recognize the
dest destination alias, preserve the full destination value after the first “=”
when parsing mount entries, and reject specifications that lack a recognized
destination key instead of returning. Keep pathsOverlap validation applied to
every recognized destination so nested overlaps with
PODMAN_BOOTSTRAP_STATE_DIRECTORY are rejected.
- Around line 602-623: Update the state-volume validation around modeTokens so
Mounts[].Mode is treated as non-authoritative: remove the requirement for
modeTokens to contain “z” while preserving rejection of “Z”, “ro”, and readonly
options. Add coverage for a valid mount with an empty Mode value.
- Around line 665-699: Update the environment validation in the inspect
comparison to compare validated Config.Env entries by variable name rather than
array order. Replace the sameArray check for environment with the existing
validated key-based map comparison, while preserving the current handling of
--unsetenv-all and --http-proxy=false and leaving entrypoint, command, and
supervisor comparisons unchanged.
---
Nitpick comments:
In `@src/lib/onboard/managed-bootstrap/podman-bootstrap-journal.ts`:
- Around line 559-572: Update recordOriginalStopped to re-read the journal from
durable storage after atomicWrite, compare the read-back with updated using the
existing sameJournal validation, and return the verified persisted value instead
of the in-memory object. Preserve the existing phase checks and idempotent
current return behavior.
In `@src/lib/onboard/managed-bootstrap/podman-bootstrap-replacement.test.ts`:
- Around line 49-51: Replace the computed STAGING_NAME and STATE_VOLUME_NAME
constants in the test with literal expected names derived from the current
deterministic naming contract. Keep STATE_VOLUME_MOUNTPOINT based on the literal
state-volume name, so the assertions test production output without duplicating
the naming formula used by podman-bootstrap-replacement.ts.
- Around line 574-613: Add tests for the two uncovered fail-closed guards:
mutate the prepared replacementStateVolumeMountpoint or
replacementSpecFingerprint before calling stopExactPodmanBootstrapOriginal,
assert “does not match the durable journal,” and verify the original remains
running; separately, after successful prepare, set harness.stateVolume to null,
call rollbackPodmanBootstrapBeforeCommit, assert “recorded state volume
disappeared before rollback,” and preserve the journal assertions relevant to
rollback authorization.
In `@src/lib/onboard/managed-bootstrap/podman-bootstrap-replacement.ts`:
- Around line 933-943: Update the staging discovery mapping in the replacement
flow to read only the currently supported Podman identity field, Id, unless a
supported version requires ID; if so, add a protecting test that emits ID.
Remove the unreachable duplicate-identity Set check and retain the existing
single-identity validation through ids.length > 1.
🪄 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: bd154581-9c2f-40d9-b0c3-0dfccc97d4c6
📒 Files selected for processing (5)
src/lib/onboard/lifecycle-contracts.mdsrc/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.ts
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.test.ts`:
- Around line 503-519: Update the fixture in the “recognizes the Podman dest
alias without truncating its value” test to use a destination whose prefix
before the second “=” matches the protected directory while the complete
destination does not overlap it, so truncation would cause rejection. Keep the
existing prepareStoppedPodmanBootstrapReplacement call and replacement-created
assertion.
🪄 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: ceaba696-e2e5-4cae-8744-c087ee00c9cb
📒 Files selected for processing (5)
src/lib/onboard/lifecycle-contracts.mdsrc/lib/onboard/managed-bootstrap/podman-bootstrap-journal.tssrc/lib/onboard/managed-bootstrap/podman-bootstrap-replacement.test.tssrc/lib/onboard/managed-bootstrap/podman-bootstrap-replacement.tstest/runtime-provider-source-shape.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/lib/onboard/lifecycle-contracts.md
- src/lib/onboard/managed-bootstrap/podman-bootstrap-journal.ts
- src/lib/onboard/managed-bootstrap/podman-bootstrap-replacement.ts
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
ericksoa
added
area: onboarding
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
apurvvkumaria
left a comment
apurvvkumaria
left a comment
There was a problem hiding this comment.
Comment — reviewed exact head e724196. The transactional Podman bootstrap preparation is safe to merge and I found no blocking defect. Non-blocking fast follow: replace the partial runtimeArgs denylist with an activation-time allowlist before this dormant provider is registered, so boundary-changing flags such as --cap-add, --device, --pid=host, --userns=host, and --security-opt cannot slip through. Also add a lost-ack rollback test where volume removal succeeds but receipt publication fails, to pin reconciliation rather than repeated mutation.
cv
left a comment
cv
left a comment
There was a problem hiding this comment.
Reviewed commit e724196652 in bottom-up stack order. This PR depends on root PR #8052, which is not approval-ready. The branch is behind that parent and its build, typecheck, CI, and E2E checks fail. Review remains blocked until the parent is reconstructed or repaired, this branch is restacked on the accepted parent commit, and current CI, security, and documentation evidence passes for the resulting commit. No approval is granted for the current branch.
3 participants
Summary
Add the dormant transactional preparation boundary for native Podman bootstrap. The new internal path durably owns a stopped replacement and its Podman-managed state volume, preserves the exact original until commit, and provides exact pre-commit rollback without activating Podman runtime selection.
Related Issue
Related to #7744.
Changes
podman-bootstrap-journal.test.tscovers canonical storage, monotonic phases, decision recovery, and fail-closed parsing./var/lib/nemoclaw, transports environment values only through a private temporary file, creates one final-labelled replacement, and proves the exact volume, mount, image, startup arguments, environment, and stopped identity. Direct central orchestration is insufficient because the later image-owned all-agent transaction must share state with no-exec helper containers;podman-bootstrap-replacement.test.tscovers the exact command, inspection, secret, collision, and mount-shadow boundaries.src/lib/onboard/lifecycle-contracts.md.Type of Change
Quality Gates
Documentation Writer Review
docs-updatedsrc/lib/onboard/lifecycle-contracts.md; the explanatory text insrc/lib/onboard/managed-bootstrap/podman-bootstrap-journal.ts,podman-bootstrap-journal.test.ts,podman-bootstrap-replacement.ts, andpodman-bootstrap-replacement.test.ts; and the refreshedtest/runtime-provider-source-shape.test.tsinventory were independently reviewed againstWRITING.mdand the controlled word list. The review clarified that creation requestsrelabel=sharedwhile inspection proves a writable mount with no reported privateZrelabel, and distinguishes volume labels from container labels. No findings remain, andgit diff --checkpassed. The append-only parent refresh to4b4fcef76preserves the exact reviewed slice diff and changes no reviewed documentation.DGX Station Hardware Evidence
Verification
Signed-off-by:line and every commit appears asVerifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run validate:prpassed after refreshingorigin/mainwhen hooks were skipped or unavailablenpx vitest run --project cli src/lib/onboard/managed-bootstrap/podman-bootstrap-journal.test.ts src/lib/onboard/managed-bootstrap/podman-bootstrap-replacement.test.ts(28 tests passed);npx vitest run --project integration test/runtime-provider-source-shape.test.ts(2 tests passed); CLI and plugin builds plus CLI typecheck and exact-base pre-push passed.npm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result:npm run docsbuilds without warnings (doc changes only)Signed-off-by: Aaron Erickson aerickson@nvidia.com
Summary by CodeRabbit
New Features
Bug Fixes