refactor(onboard): require a journal-bound source before pre-upgrade backup - #8073
refactor(onboard): require a journal-bound source before pre-upgrade backup#8073laitingsheng wants to merge 15 commits into
Conversation
…backup Pre-upgrade backup selection took unbound registry and gateway booleans and answered a registry/OpenShell disagreement with no backup, hiding a protocol violation behind a temporary guard. Selection now consumes the recreate transaction and rejects an absent transaction, another sandbox or gateway, a missing or changed source registry row, and a live sandbox that is not the recorded source, always before backup lookup or any mutation. The installer upgrade path, where the registry row survives an absent sandbox, replaced that row with no journal at all. It now opens the canonical journal first and abandons it when the custom-image plugin-provenance check blocks recreation, so a blocked run leaves no transaction behind. Once a run binds a gateway authority, a same-name replacement that cannot open a journal stops instead of deleting the sandbox or removing its registry row. The stale-entry prune, which dropped a registry row with no ownership proof and had no production caller, is gone, and a repository check pins every openshell sandbox delete call site so a new caller cannot bypass the transaction unnoticed. Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
laitingsheng
added
refactor
|
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:
📝 WalkthroughWalkthroughSandbox recreation now uses journal-bound source proofs for backup selection and replacement. Recreate journals can be abandoned safely when unused. Same-name replacement requires gateway-authorized ownership. Legacy stale-sandbox pruning is removed. ChangesSandbox replacement lifecycle
Estimated code review effort: 4 (Complex) | ~45 minutes 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 4e869ff in the TypeScript / code-coverage/cliThe overall coverage in commit 4e869ff in the Show a code coverage summary of the most impacted files.
Updated |
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported Model lanes
Second-opinion terminology and E2E selections are advisory. They do not change the primary assessment or E2E / PR Gate. 3 semantic terminology decisionsTerminology decisions are advisory. They affect the assessment only when a separate finding identifies concrete semantic impact.
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: 2
🧹 Nitpick comments (2)
test/sandbox-replacement-journal-discovery.test.ts (1)
34-35: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd the tracked issue suffix to the suite title.
Append
(#7736)to the parentdescribetitle. Child tests inherit the parent issue reference.As per coding guidelines, “Use behavior-oriented test titles and put local issue references in a final
(#1234)suffix.” Based on learnings, a parentdescribe(...)suffix is sufficient for child tests.🤖 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/sandbox-replacement-journal-discovery.test.ts` around lines 34 - 35, Update the parent describe title in the “same-name sandbox replacement audit” suite to append the tracked issue suffix “(`#7736`)”, leaving the child test title unchanged.Sources: Coding guidelines, Learnings
src/lib/onboard/machine/handlers/sandbox-resume.ts (1)
94-97: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winMake
replacesSameNameSandboxexhaustive.A
recreatedecision withremoveRegistryEntry: falsecan reachcreateSandbox, but that path records a recreate journal before deletion. Keep this decision returningfalse. Use an exhaustiveswitchso new decision kinds cannot silently bypass the guard.🤖 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/sandbox-resume.ts` around lines 94 - 97, Update replacesSameNameSandbox to use an exhaustive switch over SandboxResumeDecision.kind, returning true for repair-and-recreate, returning decision.removeRegistryEntry for recreate, and preserving false when recreate has removeRegistryEntry false. Ensure the switch cannot silently accept newly added decision kinds.
🤖 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 `@scripts/checks/sandbox-replacement-journal.mts`:
- Around line 102-113: The countSandboxDeleteCalls audit must count only
executed, transaction-authorized sandbox deletion commands, not inert array
literals. Update countSandboxDeleteCalls to inspect the command execution
boundary and require the replacement deletion to remain guarded by the
transaction authority while preserving approved path and count checks. In
test/sandbox-replacement-journal-discovery.test.ts lines 39-73, add coverage for
inert arrays and for a same-count replacement that removes the transaction
guard.
In `@src/lib/onboard/machine/handlers/sandbox-recreate-journal.test.ts`:
- Around line 395-437: Rename the test title around handleSandboxState to
explicitly describe the gateway-authority/name mismatch that triggers the
rejection, rather than implying any bound authority is sufficient. Also add
companion coverage for a non-replacing decision, verifying that create or reuse
paths where replacesSameNameSandbox returns false do not trigger the
no-transaction guard.
---
Nitpick comments:
In `@src/lib/onboard/machine/handlers/sandbox-resume.ts`:
- Around line 94-97: Update replacesSameNameSandbox to use an exhaustive switch
over SandboxResumeDecision.kind, returning true for repair-and-recreate,
returning decision.removeRegistryEntry for recreate, and preserving false when
recreate has removeRegistryEntry false. Ensure the switch cannot silently accept
newly added decision kinds.
In `@test/sandbox-replacement-journal-discovery.test.ts`:
- Around line 34-35: Update the parent describe title in the “same-name sandbox
replacement audit” suite to append the tracked issue suffix “(`#7736`)”, leaving
the child test title unchanged.
🪄 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: b4b60c5c-7e90-4aaa-b6f6-41a78b4cf036
📒 Files selected for processing (17)
scripts/checks/run.mtsscripts/checks/sandbox-replacement-journal.mtssrc/lib/onboard.tssrc/lib/onboard/lifecycle-contracts.mdsrc/lib/onboard/machine/handlers/sandbox-recreate-journal.test.tssrc/lib/onboard/machine/handlers/sandbox-resume.tssrc/lib/onboard/machine/handlers/sandbox.tssrc/lib/onboard/not-ready-recreate.test.tssrc/lib/onboard/not-ready-recreate.tssrc/lib/onboard/onboard-recreate-journal.tssrc/lib/onboard/sandbox-lifecycle.tssrc/lib/onboard/sandbox-recreate-protection.test.tssrc/lib/onboard/sandbox-recreate-protection.tssrc/lib/onboard/sandbox-recreate-transaction.test.tssrc/lib/onboard/sandbox-recreate-transaction.tstest/onboard-sandbox-build.test.tstest/sandbox-replacement-journal-discovery.test.ts
💤 Files with no reviewable changes (2)
- test/onboard-sandbox-build.test.ts
- src/lib/onboard/sandbox-lifecycle.ts
…nal-bound-backup-select
Selecting a pre-upgrade backup asked for the source proof before reading the installer restore signal, so a run that restores nothing still opened a transaction the create path then had to complete. Ask for the proof after the signal, and drop the sandbox-delete source scan in favour of the behavior tests that already refuse an unjournaled same-name replacement. Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
…nal-bound-backup-select
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/lib/onboard/sandbox-recreate-protection.ts (1)
85-99: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winTrack whether this call opened the journal.
selectPreUpgradeBackupForCreaterequests the proof once today, butsourceProofis a callback. A second request would open and replace the journal runtime. The resume path passes an already-journaled runtime withopenJournal: null; the catch block must not abandon caller-owned runtimes. Restrict abandonment and journal opening to the runtime created here.♻️ Proposed change
let runtime = binding.runtime; + let openedHere = false; try { const backupPath = selectPreUpgradeBackup({ sourceProof: () => { - if (binding.openJournal) runtime = binding.openJournal(); + if (binding.openJournal && !openedHere) { + runtime = binding.openJournal(); + openedHere = true; + } return runtime.sourceProof; }, ... - if ("abandon" in runtime) (runtime as { abandon(): void }).abandon(); + if (openedHere && "abandon" in runtime) (runtime as { abandon(): void }).abandon();🤖 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-recreate-protection.ts` around lines 85 - 99, Update the runtime handling around selectPreUpgradeBackup to track whether this call opened a journal. In sourceProof, invoke binding.openJournal only once and record that the runtime was created here; reuse it on subsequent proof requests. In the catch block, call abandon only for that locally created runtime, never for an already-journaled caller-owned runtime.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/lib/onboard/sandbox-recreate-protection.ts`:
- Around line 85-99: Update the runtime handling around selectPreUpgradeBackup
to track whether this call opened a journal. In sourceProof, invoke
binding.openJournal only once and record that the runtime was created here;
reuse it on subsequent proof requests. In the catch block, call abandon only for
that locally created runtime, never for an already-journaled caller-owned
runtime.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: dac7bde9-5c19-4717-a442-bc1684382a1c
📒 Files selected for processing (7)
src/lib/onboard.tssrc/lib/onboard/lifecycle-contracts.mdsrc/lib/onboard/machine/handlers/sandbox-recreate-journal.test.tssrc/lib/onboard/not-ready-recreate.test.tssrc/lib/onboard/not-ready-recreate.tssrc/lib/onboard/sandbox-recreate-protection.test.tssrc/lib/onboard/sandbox-recreate-protection.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/lib/onboard/lifecycle-contracts.md
- src/lib/onboard.ts
- src/lib/onboard/not-ready-recreate.test.ts
Source-proof validation folded a live sandbox with no reported Id into the same null identity a journal records for an absent source, so unknown OpenShell state could pass as proof before backup selection. Reject it instead of comparing it. Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Every same-name replacement deletes through SandboxRecreateRuntime.beginDelete, but the no-transaction runtime reported the source as present, so a caller that never opened the journal would delete an unproven sandbox. Refuse at that edge and cover the refusal. Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
apurvvkumaria
left a comment
apurvvkumaria
left a comment
There was a problem hiding this comment.
Reviewed current head b55e942. Journal-bound source proof validates gateway authority, registry fingerprint, and live OpenShell identity before backup selection, and deletion re-observes identity at the destructive boundary. Missing IDs and transaction-less same-name replacement fail closed. Ordinary required CI passes; the cloud-onboard E2E failure is an unrelated hosted endpoint HTTP 403. I found no blocking defect.
|
Exact-head disposition for b55e942: the selected cloud-onboard lane failed during Other OpenAI-compatible endpoint validation, before the journal-bound pre-upgrade recreate path changed by this PR was exercised. That makes the failure appear external/adjacent, but the required gate is still terminal red and I am not rerunning it without authoritative endpoint-health evidence or a maintainer waiver. Separately, the PR body still marks sensitive-path security review pending and the documentation-writer review result blocked, so the existing approval does not establish merge readiness. Please add exact-head security and documentation-writer receipts and reconcile the live failure through the supported gate path. |
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
cjagwani
left a comment
cjagwani
left a comment
There was a problem hiding this comment.
Reviewed exact head 4e869ffadee58d8aa8719c06c0e41dd80f3195e9. The advisor's ownership TOCTOU blocker remains in current code: createSandboxRecreateProtection closes over sandboxEntry before the journal opens and passes that same object as registryEntry after binding.openJournal() returns. The only commits after the advisor-reviewed head are base refreshes, and there is still no post-journal registry lookup.
Please pass a current registry lookup into the selection boundary, re-read the source row after the recreate journal opens and immediately before assertSandboxRecreateSourceProof, then add a regression test that changes/removes the row between journal creation and proof validation and proves backup lookup never runs. The exact-head docs/security receipts are present, but they do not resolve this correctness and source-ownership defect.
5 participants
Summary
Pre-upgrade backup selection took unbound registry and gateway booleans and answered a registry/OpenShell disagreement by selecting no backup.
It now consumes the canonical recreate transaction and rejects an unproven source before reading a backup or mutating anything.
Related Issue
Fixes #7736
Changes
selectPreUpgradeBackupForCreatetakes a journal-bound source proof instead ofliveExistsandhasExistingRegistryEntry.assertSandboxRecreateSourceProofrejects an absent transaction, another sandbox or gateway, an absent or changed source registry row, and a live sandbox that is not the recorded source.abandonSandboxRecreateTransactionclears it when plugin provenance blocks recreation. Abandon refuses past revision zero.pruneStaleSandboxEntryis removed because it dropped a registry row without ownership evidence.The installer upgrade path now writes a journal it never had, so an interrupted run is told to resume or repair rather than silently retargeting.
The live recreate path has behaved this way since #7788.
Type of Change
Quality Gates
4e869ffadee58d8aa8719c06c0e41dd80f3195e9and base83870a9ef040ffe8792f82744176a023ecfcaa60; no findings.Documentation Writer Review
no-docs-needed83870a9ef040ffe8792f82744176a023ecfcaa60through head4e869ffadee58d8aa8719c06c0e41dd80f3195e9, withAGENTS.mdblob3dd7c2425b70934b540c404d3939e3321f5c7558. Reviewed @cv commitsf746c5378600c4ba724cc6857ddb9ebd357b991cand05e8ea4832125fc8bce1b4b85dcda916eb3e223b, including production, fixture, lifecycle-trace, registry-preservation, repair-event, comment, and test-title changes. The production change removes pre-journal registry deletion and legacy repair side effects so same-name replacement remains owned by the recreate journal. This conforms to the existing documented contract rather than adding a command, flag, configuration, supported surface, or user workflow. Existing documentation already describes journal-bound same-name replacement, preservation until replacement commits, gateway and identity validation, registry mismatch rejection, and nonzero installer recovery failures. No user-facingdocs/file changed. The later base-only Hermes session-deletion and Jetson E2E-planning commits share no changed paths with this PR. Both @cv refresh merges have no conflict-resolution delta, and the current PR patch has the same stable patch ID,7b50aa5d9ccb3a9b909af46c76bfaecac7dfabbd, as the fully reviewed previous range. Reviewed all changed explanatory text, comments, errors, internal contract text, and test titles; no findings remain. Ten focused CLI files passed 178/178,npm run validate:prpassed,npm run docspassed with 0 errors and 2 existing warnings,git diff --checkpassed, and the tracked tree is clean on the exact head.Security Review
PASS; no findings.4e869ffadee58d8aa8719c06c0e41dd80f3195e9.83870a9ef040ffe8792f82744176a023ecfcaa60.PASS; no credential material or credential logging changed.PASS; the transaction proof validates sandbox, gateway, registry fingerprint, and OpenShell identity before the operation proceeds.PASS; no authentication surface changed, and source ownership evidence is strengthened.PASS; the PR adds no dependency.PASS; missing, stale, foreign, unknown, or failed replacement evidence stops with bounded non-secret errors while preserving the source registry row.PASS; the PR adds no cryptography and persists only fingerprints and secret-free journal metadata.PASS; the PR changes no service exposure, container permission, network policy, or HTTP configuration.PASS; denial paths cover absent journals, foreign identities, changed registry rows, wrong gateways, missing OpenShell IDs, unjournaled legacy repair, failed creation, and persisted journal tracing.PASS; destructive same-name replacement now fails closed before repair or registry mutation, and transaction phase and identity checks prevent stale-source and time-of-check/time-of-use bypasses.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 unavailable4e869ffadee58d8aa8719c06c0e41dd80f3195e9;npm run validate:prpassed on the exact head.npm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result: local default and one-worker runs were stopped after unrelated host/process timing failures; required GitHub CI remains authoritative.npm run docsbuilds without warnings (doc changes only)Signed-off-by: Tinson Lai tinsonl@nvidia.com
Summary by CodeRabbit
Bug Fixes
Tests