refactor(onboard): add versioned resume checkpoint with tri-state decisions - #7022
Merged
Conversation
…isions Add a dedicated, secret-free onboarding checkpoint with an explicit schemaVersion, migration from legacy sessions, and fail-closed handling of an unknown future version. Model sandbox name, web search, messaging, and resource choices as explicit unset/declined/selected decisions instead of overloading null, capture durable sandbox identity before create, and add the resumable-replay and stale-binding revalidation decision layer. Wire the resume-entry fail-safe and durable-identity capture into the live flow; consuming replay decisions at the create executor and recording effect groups at every apply boundary remain follow-ups. Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Contributor
|
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:
📝 WalkthroughWalkthroughAdds a versioned, persisted onboarding checkpoint model with tri-state decisions, migration and fail-closed validation, replay planning, binding revalidation, resume guards, and sandbox crash-recovery integration. ChangesOnboarding checkpoint lifecycle
Estimated code review effort: 4 (Complex) | ~60 minutes Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
laitingsheng
added
refactor
Contributor
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage remains at 96%, unchanged from the TypeScript / code-coverage/cliThe overall coverage in the Show a code coverage summary of the most impacted files.
Updated |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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/checkpoint-replay.test.ts`:
- Around line 85-117: The crash-then-resume coverage must include the
receipt-loss state where the sandbox exists but sandbox_create is unrecorded,
expecting reuse rather than create. Extend tests around planSandboxCreateReplay
and the public resume entrypoint to prove resumed execution reaches this planner
path and that the superseded creation path cannot run; otherwise narrow the
at-most-once guarantee claim until executor integration is covered.
In `@src/lib/onboard/checkpoint-replay.ts`:
- Around line 42-55: Update planSandboxCreateReplay to reuse the selected
sandbox whenever observed.liveSandboxExists is true, regardless of whether
checkpoint.effectGroups.sandbox_create is recorded; retain the
capture_identity_first path and create action when no live sandbox exists.
In `@src/lib/onboard/session-bootstrap.ts`:
- Line 49: Make resolveResumeCheckpoint required in session-bootstrap.ts and
invoke it unconditionally before resume recovery, removing the optional
compatibility path. In checkpoint-resume-guard.test.ts, replace
backward-compatibility coverage with a test proving resume validation cannot be
omitted; update all affected entrypoints to use this single authoritative path.
In `@src/lib/state/onboard-checkpoint-migrate.ts`:
- Around line 81-96: Update resolveCheckpointForResume to normalize the
containing session before accepting an inspected loaded checkpoint, then compare
its sessionId with checkpoint.sessionId and return corrupt when they differ.
Preserve the existing unsupported_future and corrupt handling, and only return
the loaded checkpoint after this identity validation; reuse the normalized
session for migration.
In `@src/lib/state/onboard-checkpoint.ts`:
- Around line 81-98: Update parseEffectGroups and parseBindings to return null
when their containers or any current-schema entries are malformed, rather than
silently omitting invalid values or defaulting to empty collections. Propagate
these null results through the checkpoint parsing/validation path so malformed
effect groups or bindings classify the checkpoint as corrupt, while preserving
normal parsing for valid data.
🪄 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: 4b4d14b6-941d-4961-a077-2c046c224e58
📒 Files selected for processing (15)
src/lib/onboard.tssrc/lib/onboard/checkpoint-record.tssrc/lib/onboard/checkpoint-replay.test.tssrc/lib/onboard/checkpoint-replay.tssrc/lib/onboard/checkpoint-resume-guard.test.tssrc/lib/onboard/checkpoint-revalidate.tssrc/lib/onboard/lifecycle-contracts.mdsrc/lib/onboard/session-bootstrap.tssrc/lib/state/onboard-checkpoint-decision.tssrc/lib/state/onboard-checkpoint-migrate.test.tssrc/lib/state/onboard-checkpoint-migrate.tssrc/lib/state/onboard-checkpoint-types.tssrc/lib/state/onboard-checkpoint.test.tssrc/lib/state/onboard-checkpoint.tssrc/lib/state/onboard-session.ts
Contributor
PR Review Advisor — InformationalAdvisor assessment: Informational / high confidence 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: 3 optional E2E recommendations
This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
Give recordCheckpointEffectGroup, revalidateCheckpointBindings, and planSandboxCreateReplay real production call sites instead of leaving them covered only by unit tests. A "create" resume decision only means the sandbox step was never marked complete; it does not check whether a previous run already executed the destructive create effect before crashing. The sandbox handler now disambiguates that narrow window using the durable checkpoint identity and effect-group receipt, revalidates recorded bindings before any mutation, and reuses a surviving sandbox or recreates only under its recorded identity, never a new one. Record the sandbox_create effect group at its real success boundary, and capture durable sandbox identity from the same FSM path that names the sandbox, not only the bootstrap resume guard. Move the checkpoint resolver default into session-bootstrap.ts so the production wiring lives under src/lib/onboard/, keeping src/lib/onboard.ts net-neutral. Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/onboard/machine/handlers/sandbox.ts (1)
985-1019: 🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy liftPersist the creation receipt at the external-effect success boundary.
The receipt is written only after
recordStepComplete. A crash aftercreateSandbox()succeeds but before step completion leaves an incomplete step with no receipt—the exact state that can create a second sandbox. Conversely, once this receipt exists, the step is already complete and normally will not produce a"create"decision.Record the effect immediately after
createSandbox()returns, then make replay finish any remaining registry/session finalization. Add crash-injection coverage between creation success and every subsequent write.Based on the PR objectives, interrupted resume must not create a second sandbox.
🤖 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.ts` around lines 985 - 1019, Move persistence of the sandbox creation receipt to immediately after createSandbox() succeeds, before recordStepComplete or any registry/session writes, using the existing checkpoint mechanism around the sandbox_create effect. Update replay handling to recognize the receipt and complete any remaining registry and session finalization without creating another sandbox. Add crash-injection coverage for the interval after creation success and before each subsequent write, ensuring interrupted resume never issues a second create operation.Source: Path instructions
🧹 Nitpick comments (2)
src/lib/onboard/machine/handlers/sandbox-checkpoint-crash-recovery.test.ts (2)
71-83: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPositional mock-call argument assertion is brittle.
(calls.createSandbox.mock.calls[0] as unknown[] | undefined)?.[4]locks the test to the exact parameter position ofcreateSandbox. Any reordering of that call's arguments silently breaks this assertion without signaling an actual behavior regression, and the index4doesn't self-document what's being checked.As per path instructions, tests should "Prefer observable outcomes through the public boundary over source-text, private-shape, or mock-call assertions." Consider destructuring the call with a named/typed helper (or asserting via an options object if
createSandbox's signature uses one) so the intent ("identity name passed to sandbox creation is the recorded durable identity") is explicit and resilient to signature changes.- expect((calls.createSandbox.mock.calls[0] as unknown[] | undefined)?.[4]).toBe("my-assistant"); + const [, , , , sandboxNameArg] = calls.createSandbox.mock.calls[0] ?? []; + expect(sandboxNameArg).toBe("my-assistant"); // TODO: replace with a named-arg assertion once createSandbox's signature is confirmed🤖 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-checkpoint-crash-recovery.test.ts` around lines 71 - 83, Replace the positional mock-call assertion in the “recreates only under the recorded durable identity” test with a named, typed extraction or options-based assertion that explicitly verifies the sandbox identity passed to createSandbox is “my-assistant”. Keep the assertion focused on the observable creation identity while avoiding dependence on argument index 4.Source: Path instructions
56-114: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMissing coverage for the "identity not yet captured" resume branch.
Per
planSandboxCreateReplay(context snippet 1), there are three replay outcomes:capture_identity_first(sandboxIdentity not yet selected),reuse, andcreate. This suite only exercisesreuse(test 1) andcreate(test 2); thecapture_identity_firstpath — the exact scenario where a crash occurs before durable identity capture — has no test here. That branch is the one most tied to PR objective#6228("interrupted resume must not create a second sandbox"), so leaving it uncovered is a gap on the most safety-critical path this PR introduces.🧪 Suggested additional test (adjust expected behavior to match the real handler contract)
+ it("does not create a sandbox before durable identity has been captured", async () => { + const { deps, calls } = createDeps({ getSandboxReuseState: () => "missing" }); + const session = sessionWithCheckpoint( + crashedCheckpoint({ sandboxIdentity: decisionUnset(), effectGroups: {} }), + ); + + await handleSandboxState({ + ...baseOptions(deps, session), + resume: true, + sandboxName: "my-assistant", + }); + + // Assert the actual capture_identity_first behavior (e.g. identity gets + // captured/persisted before create proceeds, or a single sandbox_create + // occurs and no duplicate). + });Since the exact handler semantics for
capture_identity_firstaren't visible in the files provided for this review, please confirm the intended behavior for that branch before finalizing the 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/machine/handlers/sandbox-checkpoint-crash-recovery.test.ts` around lines 56 - 114, Add coverage in the “sandbox crash-recovery replay (`#5961`, `#6228`)” suite for the `capture_identity_first` outcome returned by `planSandboxCreateReplay` when the checkpoint lacks `sandboxIdentity`. Verify the handler follows its intended contract for this branch—confirm the expected result and mutation behavior from `handleSandboxState` before asserting—and ensure the test proves resume does not create a second sandbox before identity capture.Source: Path instructions
🤖 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/machine/handlers/sandbox.ts`:
- Around line 532-555: Update the checkpoint replay flow around
revalidateCheckpointBindings and planSandboxCreateReplay to validate the
checkpoint’s recorded sandbox identity against the live sandbox name and agent
before any reuse decision or side effects. Reject identity drift, or query live
sandbox state using the checkpoint identity, and preserve the requirement that
stale results are rejected first; do not discard replay.identity. Add coverage
for mismatched session/checkpoint sandbox names and agents.
- Around line 791-795: Move the recordCheckpointSandboxIdentity call out of the
resumesSandboxPrompts-only branch in the surrounding creation flow so every
fresh and resumed sandbox lifecycle records identity, including non-OpenClaw
agents. Preserve the existing agent-name fallback and ensure the authoritative
checkpoint path is invoked before the later effect-receipt recovery logic; add
coverage for an interrupted non-OpenClaw resume.
- Around line 536-541: Update the bindingCheck setup in the sandbox handler to
derive liveRegisteredProviders from the current provider registry rather than
state.session.stagedCredentialProviders. Ensure deleted or otherwise
unregistered providers cannot satisfy revalidateCheckpointBindings, while
preserving the existing availableCredentialEnvs validation.
---
Outside diff comments:
In `@src/lib/onboard/machine/handlers/sandbox.ts`:
- Around line 985-1019: Move persistence of the sandbox creation receipt to
immediately after createSandbox() succeeds, before recordStepComplete or any
registry/session writes, using the existing checkpoint mechanism around the
sandbox_create effect. Update replay handling to recognize the receipt and
complete any remaining registry and session finalization without creating
another sandbox. Add crash-injection coverage for the interval after creation
success and before each subsequent write, ensuring interrupted resume never
issues a second create operation.
---
Nitpick comments:
In `@src/lib/onboard/machine/handlers/sandbox-checkpoint-crash-recovery.test.ts`:
- Around line 71-83: Replace the positional mock-call assertion in the
“recreates only under the recorded durable identity” test with a named, typed
extraction or options-based assertion that explicitly verifies the sandbox
identity passed to createSandbox is “my-assistant”. Keep the assertion focused
on the observable creation identity while avoiding dependence on argument index
4.
- Around line 56-114: Add coverage in the “sandbox crash-recovery replay (`#5961`,
`#6228`)” suite for the `capture_identity_first` outcome returned by
`planSandboxCreateReplay` when the checkpoint lacks `sandboxIdentity`. Verify
the handler follows its intended contract for this branch—confirm the expected
result and mutation behavior from `handleSandboxState` before asserting—and
ensure the test proves resume does not create a second sandbox before identity
capture.
🪄 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: f827ffaf-657d-417c-98a4-815141ca3288
📒 Files selected for processing (3)
src/lib/onboard/machine/handlers/sandbox-checkpoint-crash-recovery.test.tssrc/lib/onboard/machine/handlers/sandbox.tssrc/lib/onboard/session-bootstrap.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib/onboard/session-bootstrap.ts
Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Address PR review advisor blockers and CodeRabbit findings on the versioned checkpoint work: reuse a live sandbox even when its create receipt was lost to a mid-create crash, bind reuse to the checkpoint's recorded identity, revalidate credential bindings against the live gateway registry instead of a stale session receipt, record sandbox identity on every creation path (not only resumable-prompt agents), treat checkpoint machine-state progress as resume authority alongside the legacy step status, bind a loaded checkpoint to its parent session, reject malformed checkpoint effect groups and bindings instead of silently dropping them, and make resume checkpoint validation a mandatory dependency with no compatibility bypass. Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/machine/handlers/sandbox-checkpoint-crash-recovery.test.ts`:
- Around line 191-204: Update the test named “records durable sandbox identity
for a non-OpenClaw agent create so a crash can still be recovered” to make the
mocked createSandbox operation throw and, from that mock or the caught failure
path, assert that checkpoint.sandboxIdentity was persisted before createSandbox
began. Preserve the successful identity expectation as appropriate, and add
public-boundary coverage for fresh, resumed, repair, and failure onboarding
phases if not already present.
In `@src/lib/onboard/machine/handlers/sandbox.ts`:
- Around line 351-357: Update checkpointProvesSandboxStepComplete and the
checkpoint reuse path to require authoritative identity and binding validation,
not merely a later machine state; reject unset, mismatched, and stale
checkpoints before authorizing reuse. In
src/lib/onboard/machine/handlers/sandbox.ts lines 351-357 and 506-508, apply the
existing selected-identity and binding checks. In
src/lib/onboard/machine/handlers/sandbox.test.ts lines 515-552, use a matching
selected identity for the valid case and add coverage proving unset, mismatched,
stale, and unsafe legacy checkpoints cannot execute.
🪄 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: d443b353-c45a-4dd9-99a0-8fd72a3b0b78
📒 Files selected for processing (15)
src/lib/onboard.tssrc/lib/onboard/checkpoint-replay.test.tssrc/lib/onboard/checkpoint-replay.tssrc/lib/onboard/checkpoint-resume-guard.test.tssrc/lib/onboard/machine/core-flow-phases.test.tssrc/lib/onboard/machine/handlers/sandbox-checkpoint-crash-recovery.test.tssrc/lib/onboard/machine/handlers/sandbox-test-fixtures.tssrc/lib/onboard/machine/handlers/sandbox.test.tssrc/lib/onboard/machine/handlers/sandbox.tssrc/lib/onboard/session-bootstrap.test.tssrc/lib/onboard/session-bootstrap.tssrc/lib/state/onboard-checkpoint-migrate.test.tssrc/lib/state/onboard-checkpoint-migrate.tssrc/lib/state/onboard-checkpoint.test.tssrc/lib/state/onboard-checkpoint.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- src/lib/state/onboard-checkpoint-migrate.test.ts
- src/lib/state/onboard-checkpoint.test.ts
- src/lib/onboard/session-bootstrap.ts
- src/lib/state/onboard-checkpoint-migrate.ts
- src/lib/state/onboard-checkpoint.ts
- src/lib/onboard/checkpoint-replay.test.ts
- src/lib/onboard/checkpoint-replay.ts
…rd.ts Wire the checkpoint-validation and live-provider-registry dependencies without growing the capped onboard.ts entrypoint: route the default checkpoint resolver through a small session-bootstrap helper referenced by name, and make the live gateway provider check an optional dependency that falls back to the prior staged-provider heuristic when unwired. Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Contributor
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/session-bootstrap.ts`:
- Around line 58-59: Update defaultResolveResumeCheckpoint and the underlying
loadResumeCheckpoint behavior so unreadable or malformed checkpoint data returns
status "corrupt", while only a genuinely missing session file returns "none";
preserve the fail-safe guard’s fatal corrupt branch. Add a regression test
through the validated entrypoint covering malformed or unreadable checkpoint
data.
🪄 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: cd923927-bf51-4db8-a51a-bbac71fd4aaf
📒 Files selected for processing (4)
src/lib/onboard.tssrc/lib/onboard/machine/handlers/sandbox-checkpoint-crash-recovery.test.tssrc/lib/onboard/machine/handlers/sandbox.tssrc/lib/onboard/session-bootstrap.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/lib/onboard/machine/handlers/sandbox-checkpoint-crash-recovery.test.ts
- src/lib/onboard/machine/handlers/sandbox.ts
Extend the crash-recovery fix so the versioned checkpoint, not the legacy step and prompt-progress fields, decides resume behaviour whenever a checkpoint is present: the sandbox step-complete signal and the recoverable resume sandbox name now come exclusively from the checkpoint when one exists, with legacy fields used only as a migration fallback for sessions that never acquired one. A migrated legacy checkpoint is now attached to the session and persisted during the resume guard, so it is loaded rather than re-derived on every subsequent resume. Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
… decision boundary Read the checkpoint-authority signal from the session's live machine state instead of a snapshot the checkpoint only captured while recording sandbox identity or effects, since that snapshot goes stale the moment the FSM advances past the sandbox step and would otherwise send a later-phase interruption back through recreation instead of reuse. Record the web-search, messaging, and resource-profile decisions plus credential-provider bindings into the checkpoint at their own write boundaries, alongside the existing sandbox identity and create-effect receipts, so resume can reconstruct and revalidate those choices instead of leaving the checkpoint's bindings and decisions empty after they have already taken effect. Also drop a test fixture default that made the optional live-provider-registry check always report nothing live, which caused stale-binding rejection for provider names that were never meant to be checked live in that scenario. Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/lib/onboard/checkpoint-record.ts (1)
51-88: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: extract shared decision-recording helper to reduce duplication.
recordCheckpointWebSearch,recordCheckpointMessaging, andrecordCheckpointResourceProfilefollow an identical pattern: spread base checkpoint, updatemachineState/updatedAt, set one field todecisionSelected(value)ordecisionDeclined(). A single generic helper would eliminate the copy-paste and ensure future decision fields get the same write-boundary semantics automatically.♻️ Optional consolidation
+function recordCheckpointDecisionField<K extends keyof OnboardCheckpoint>( + session: Session, + field: K, + value: OnboardCheckpoint[K] extends CheckpointDecision<infer V> ? V | null : never, +): void { + const base = baseCheckpoint(session); + session.checkpoint = { + ...base, + machineState: session.machine.state, + updatedAt: new Date().toISOString(), + [field]: value ? decisionSelected(value) : decisionDeclined(), + }; +} + -export function recordCheckpointWebSearch(session: Session, webSearchConfig: WebSearchConfig | null): void { - // ... body replaced by: - recordCheckpointDecisionField(session, "webSearch", webSearchConfig); -}🤖 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/checkpoint-record.ts` around lines 51 - 88, Optionally extract the duplicated checkpoint update logic from recordCheckpointWebSearch, recordCheckpointMessaging, and recordCheckpointResourceProfile into a shared generic decision-recording helper. Have the helper preserve baseCheckpoint, machineState, updatedAt, and the selected-versus-declined decision semantics while accepting the target decision field and value; update the three functions to delegate to it.
🤖 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/checkpoint-record.ts`:
- Around line 51-88: Optionally extract the duplicated checkpoint update logic
from recordCheckpointWebSearch, recordCheckpointMessaging, and
recordCheckpointResourceProfile into a shared generic decision-recording helper.
Have the helper preserve baseCheckpoint, machineState, updatedAt, and the
selected-versus-declined decision semantics while accepting the target decision
field and value; update the three functions to delegate to it.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: dfe53b58-e2db-4470-906d-b44a356a243a
📒 Files selected for processing (5)
src/lib/onboard/checkpoint-record.tssrc/lib/onboard/checkpoint-replay.tssrc/lib/onboard/machine/handlers/sandbox.test.tssrc/lib/onboard/machine/handlers/sandbox.tssrc/lib/onboard/session-bootstrap.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/lib/onboard/session-bootstrap.ts
- src/lib/onboard/machine/handlers/sandbox.ts
…ift before reuse Route the credential-provider registration boundary through the declared effect-group replay contract instead of leaving it unused: record a web_search_provider or messaging_providers receipt after a successful registration, and skip re-registering only when that receipt exists and every bound provider is confirmed live in the gateway, so an interrupted run reruns registration whenever the live postcondition cannot be confirmed. Record a sandbox_register receipt alongside sandbox_create once the registry entry is written. Extend the sandbox-create effect-group fingerprint to cover both the built agent version and the resolved policy tier, and revalidate it before a crash-recovery reuse decision: a recorded fingerprint that no longer matches what the current run would build is treated as drift and rejected with guidance to pass --recreate-sandbox, rather than silently reusing a sandbox built from different inputs. Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
…, not name alone Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Contributor
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/machine/handlers/sandbox-checkpoint-crash-recovery.test.ts`:
- Around line 254-295: Update the crash-recovery tests around the public
handleSandboxState flow to use the real sandbox credential-provider registration
adapter instead of mocking stageSandboxCredentialProviders. Mock only the
Openshell boundary, configure its first staging attempt to create the matching
provider before throwing, and ensure the resumed flow discovers or upserts that
live provider, persists the registeredProviders binding, and avoids duplication;
apply the same change to the related test at the additional referenced section.
🪄 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: 499568b2-97dd-456d-b504-1a13acbaa66f
📒 Files selected for processing (15)
src/lib/onboard/checkpoint-record.tssrc/lib/onboard/checkpoint-replay.test.tssrc/lib/onboard/checkpoint-revalidate.tssrc/lib/onboard/credential-provider-registration.test.tssrc/lib/onboard/credential-provider-registration.tssrc/lib/onboard/machine/handlers/sandbox-checkpoint-crash-recovery.test.tssrc/lib/onboard/machine/handlers/sandbox-create-intent-boundary.test.tssrc/lib/onboard/machine/handlers/sandbox-test-fixtures.tssrc/lib/onboard/machine/handlers/sandbox.test.tssrc/lib/onboard/machine/handlers/sandbox.tssrc/lib/state/onboard-checkpoint-migrate.test.tssrc/lib/state/onboard-checkpoint-migrate.tssrc/lib/state/onboard-checkpoint-types.tssrc/lib/state/onboard-checkpoint.test.tssrc/lib/state/onboard-checkpoint.ts
🚧 Files skipped from review as they are similar to previous changes (10)
- src/lib/onboard/checkpoint-revalidate.ts
- src/lib/state/onboard-checkpoint-types.ts
- src/lib/state/onboard-checkpoint-migrate.test.ts
- src/lib/onboard/checkpoint-replay.test.ts
- src/lib/state/onboard-checkpoint.test.ts
- src/lib/onboard/checkpoint-record.ts
- src/lib/state/onboard-checkpoint-migrate.ts
- src/lib/onboard/machine/handlers/sandbox.test.ts
- src/lib/state/onboard-checkpoint.ts
- src/lib/onboard/machine/handlers/sandbox.ts
…cisions Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/machine/handlers/sandbox-messaging.test.ts`:
- Around line 413-416: Update the test for the divergent messaging selection to
assert that the setupMessagingChannels dependency is called, rather than relying
only on the reconciliation note and returned channels. Keep the existing result
assertions, and verify the setup call uses the expected reconciled Discord
selection so the public entrypoint’s new path is exercised.
In `@src/lib/onboard/machine/handlers/sandbox.test.ts`:
- Around line 657-662: Update the legacy completion flags in the
sandboxPromptProgress fixtures for both affected tests to true, ensuring they
conflict with the checkpoint state and actually exercise checkpoint precedence
rather than passing without testing the claimed regression.
🪄 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: 500fb00f-1519-4bd2-b4bb-4812227f2280
📒 Files selected for processing (9)
src/lib/onboard/checkpoint-record.tssrc/lib/onboard/machine/handlers/sandbox-checkpoint-crash-recovery.test.tssrc/lib/onboard/machine/handlers/sandbox-messaging.test.tssrc/lib/onboard/machine/handlers/sandbox-messaging.tssrc/lib/onboard/machine/handlers/sandbox.test.tssrc/lib/onboard/machine/handlers/sandbox.tssrc/lib/state/onboard-checkpoint-migrate.tssrc/lib/state/onboard-checkpoint-types.tssrc/lib/state/onboard-checkpoint.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- src/lib/onboard/checkpoint-record.ts
- src/lib/state/onboard-checkpoint-types.ts
- src/lib/onboard/machine/handlers/sandbox-checkpoint-crash-recovery.test.ts
- src/lib/state/onboard-checkpoint.ts
- src/lib/state/onboard-checkpoint-migrate.ts
- src/lib/onboard/machine/handlers/sandbox.ts
Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
cv
requested changes
Jul 18, 2026
cv
left a comment
cv
left a comment
Collaborator
There was a problem hiding this comment.
Requesting changes on cadd549fb. Issue #6228 requires interruption coverage at every apply boundary and at-most-once destructive effects, but the live handler has an uncovered crash window after updateSandboxRegistry and successful recordStepComplete and before the updateSession call writes sandbox_create / sandbox_register receipts. The existing lost-receipt test starts from a hand-built checkpoint, while the live crash test throws earlier from recordStepComplete; neither proves this exact post-registration state converges. Add a handler-level failure injection in that final window, resume from the actually persisted session against the surviving sandbox, and assert no second create or duplicate registration plus durable backfilled receipts. The versioned/fail-closed schema, secret-free bindings, locked revalidation, and earlier crash cases otherwise review coherently.
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
…gistration-receipt
cv
requested changes
Jul 18, 2026
cv
left a comment
cv
left a comment
Collaborator
There was a problem hiding this comment.
Maintainer follow-up on exact head 2621de767: I directly fixed the crash-window receipt backfill, exact registered-credential revalidation after secret scrubbing, the growth-guardrail test shape, and the stale lifecycle-contract row. The focused checkpoint suite is green (88 tests), and the prior live E2E failure exposed and verified the credential fix. One acceptance gap remains: #6228 requires interactive and non-interactive process-termination coverage at every apply boundary, while web-search and messaging provider-registration interruption is still covered only through handler fixtures. Closing that safely needs deterministic entry-path failure injection plus both real CLI modes, so it is not a small maintainer patch. Please add that matrix (or explicitly narrow the accepted scope) before approval.
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
cv
approved these changes
Jul 18, 2026
cv
left a comment
cv
left a comment
Collaborator
There was a problem hiding this comment.
Approved. Security review: PASS. The versioned checkpoint remains secret-free, rejects unsupported future schemas, and revalidates durable registrations before replay. Sandbox bindings are limited to web-search and messaging providers; primary inference remains owned and revalidated by its existing phases. Interactive and non-interactive crash recovery now cover registration, sandbox creation, and registration boundaries. Focused suites, type-checking, docs audit, conventional CI, and the canonical advisor pass; selected E2E remains a merge gate.
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
22 tasks
This was referenced Jul 28, 2026
cv
added a commit
that referenced
this pull request
Jul 28, 2026
<!-- markdownlint-disable MD041 --> ## Summary Updates the internal onboarding lifecycle contract map to match the implemented create-intent, FSM recovery, checkpoint, and replay behavior. The map now records completed linked issues and separates remaining cross-effect gaps from the completed #6224 scope. ## Related Issue Closes #6224 ## Changes - Update the onboarding flow and contract matrix for versioned checkpoints, durable sandbox identity, effect receipts, and live postcondition revalidation. - Correct the status and implementation evidence for #5961, #6040, #6179, and #6099. - Replace stale child-issue ownership and coverage gaps with the current owners, tests, and remaining boundaries. - [#6253](#6253) -> `src/lib/onboard/lifecycle-contracts.md`: Record explicit terminal recovery and transition validation. - [#6742](#6742) -> `src/lib/onboard/lifecycle-contracts.md`: Record complete create-intent validation before destructive effects. - [#7022](#7022) -> `src/lib/onboard/lifecycle-contracts.md`: Record versioned checkpoint migration, replay, and crash-recovery coverage. ## Type of Change - [ ] Code change (bug fix, feature, refactor) - [ ] Test only - [ ] Build/CI - [x] Doc only (prose changes, no code sample modifications) - [ ] Release ## Quality Gates - [ ] Tests added or updated for changed behavior - [ ] Existing tests cover changed behavior - [x] Tests not applicable — justification: Documentation-only internal contract inventory; no runtime behavior changes. - [ ] Docs updated for user-facing behavior - [x] Docs not applicable — justification: This PR updates an internal lifecycle contract map and does not change user-facing behavior. - [ ] Sensitive paths reviewed (`nemoclaw-blueprint/`, `.github/workflows/`, `scripts/`, install scripts) - [ ] Exception or waiver documented ## Documentation Writer Review - [x] Documentation writer subagent reviewed the completed changes - Result: `docs-updated` - Evidence: `src/lib/onboard/lifecycle-contracts.md`; the subagent reviewed the writing rules, documentation style, terminology, structure, voice, code-sample presentation, and issue/coverage accuracy. - Agent: `Codex Desktop` <!-- docs-review-head-sha: 0062efc --> <!-- docs-review-agents-blob-sha: be20a09 --> ## DGX Station Hardware Evidence - [ ] This PR changes `scripts/prepare-dgx-station-host.sh` - Tested commit: - Station profile or scenario: - Result: - Supporting link: - Reviewer: ## Verification - [x] DCO declaration is present below and the pushed commit is Verified on GitHub. - [x] Normal pre-commit, commit-msg, and pre-push hooks passed. - [x] Targeted behavior tests passed or are not applicable — Tests are not applicable because this PR changes only the internal contract map. - [ ] Applicable broad test or release gate passed. - [x] Quality gates above are complete. - [x] No secrets, API keys, or credentials are committed. - [ ] `npm run docs` builds without warnings (doc changes only) — Exited 0; Fern reported unrelated warnings for unauthenticated redirect checks and existing light-mode accent contrast. - [x] Doc pages follow the NemoClaw writing and documentation style guides. - [ ] New docs pages are added to the Fern navigation. --- Signed-off-by: Carlos Villela <cvillela@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Updated onboarding and recovery lifecycle contracts to reflect current checkpoint, replay, and validation behavior. * Clarified resume flows, including live postcondition checks and credential or binding revalidation. * Expanded create/register flow details and receipt tracking. * Refreshed journey coverage, known gaps, ownership references, and characterization evidence. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
23 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds a dedicated, versioned onboarding resume checkpoint that models each secret-free operator choice as an explicit
unset/declined/selecteddecision instead of overloadingnull, fails closed on an unknown future checkpoint version instead of silently starting fresh, and captures durable sandbox identity before creation so an interrupted run never opens a second sandbox.Related Issue
Resolves #6227
Resolves #6228
Changes
src/lib/state/onboard-checkpoint-types.ts,onboard-checkpoint-decision.ts,onboard-checkpoint.ts, andonboard-checkpoint-migrate.ts: a versionedOnboardCheckpointdata-transfer contract with an explicitschemaVersion, a tri-stateCheckpointDecision, schema validation that fails closed on an unknown future version (rather than treating it as a missing/fresh session), andderiveCheckpointFromSession, which reconstructs the same tri-state from legacy sessions using the existing completion markers.checkpointfield onSession;normalizeSessionandserializeSessionForDiskround-trip it through the existing atomic session write.src/lib/onboard/checkpoint-replay.tsandcheckpoint-revalidate.ts: durable-identity replay decisions (reuse a surviving sandbox, otherwise recreate under the same recorded identity — never a new name), skip-only-after-postcondition-revalidated effect groups, and stale-binding revalidation that fails closed and reports only names, never values.prepareResumeSession(abort with guidance on an unsupported-future or corrupt checkpoint) and record durable sandbox identity atcheckpointSandboxName.src/lib/onboard/lifecycle-contracts.mdto record the new contract and remaining follow-ups.This is a migration/compatibility path required by #6228. Its current consumer is the onboarding resume flow; a direct change is insufficient because legacy on-disk sessions collapse "never reached", "declined", and "cleared" into one
null, which the checkpoint's tri-state resolves. It is protected by the newonboard-checkpoint*,checkpoint-replay, andcheckpoint-resume-guardtests.Scope note: consuming the replay decisions at the live sandbox-create executor and recording effect groups at every apply boundary are staged behind this tested decision layer and remain follow-ups; the decision, migration, fail-safe, and revalidation contracts land here in full.
Type of Change
Quality Gates
lifecycle-contracts.mdmap is updated in this PR, and there is no user-facing behavior or documentation changeVerification
Signed-off-by:line and every commit appears asVerifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run check:diffpassed when hooks were skipped or unavailablenpx vitest runfor the checkpoint suites (35 pass);src/lib/state/(357 pass);src/lib/onboard/(3051 pass). The two failures in the onboard directory are pre-existing and unrelated to this change (an unbuiltnemoclaw/distmodule and a NodeDEP0205stderr deprecation warning); neither imports the changed modules.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: Tinson Lai tinsonl@nvidia.com
Summary by CodeRabbit