refactor(onboard): make FSM resume recovery a single explicit path (#6227)

[atulya-singh]

Summary

Foundational slice of #6227: replace the three implicit onboarding-recovery mechanisms with explicit, deterministic FSM semantics. Resume recovery becomes one validated, side-effect-free pass; terminal failures get a single idempotent owner; and the legal transition graph and terminality invariant are documented and enforced. This is the first of a planned phased series for #6227 and ships independently of the (out-of-scope) persisted create checkpoint.

Related Issue

Part of #6227. Also adds explicit enforcement for the invalid failed -> <agent> transition described in #6179.

Changes

• Single recovery path — new src/lib/onboard/session-recovery.ts (planSessionRecovery / applySessionRecovery) classifies the durable snapshot, computes and validates a single legal non-terminal entry, re-seats the snapshot, and surfaces the decision so the caller emits exactly one explicit state.repair.completed event. Replaces the implicit repairResumeMachineSnapshot rewrite, which is removed; resumeMachineState is retained as an internal building block.
• Single terminal-failure owner — new synchronous finalizeIncompleteOnboardStep in onboard-session.ts for exception/signal/nonzero-exit paths. It validates the <non-terminal> -> failed transition and is idempotent against an already-terminal machine, so exactly one failed transition and one terminal event are recorded. This removes the last production use of LEGACY_MACHINE_STEP_MUTATION_OPTIONS (the process-exit backstop in exit-step-failure.ts).
• Documented + enforced graph — added a transition-graph/terminality-invariant doc block in transitions.ts and a negative test asserting a terminal failed state can never re-enter an agent/flow state ([DGX Spark][Onboard] nemoclaw onboard exits with InvalidOnboardMachineTransitionError after Ollama sandbox creation succeeds #6179).
• Test callers migrated from repairResumeMachineSnapshot to applySessionRecovery; rebuild-flow terminal-failure mock updated to the new owner.

No persisted SandboxCreateIntent or public schema is introduced (explicit non-goal of #6227).

Type of Change

• Code change (feature, bug fix, or refactor)

Quality Gates

• Tests added or updated for changed behavior — new session-recovery.test.ts; new [DGX Spark][Onboard] nemoclaw onboard exits with InvalidOnboardMachineTransitionError after Ollama sandbox creation succeeds #6179 negative test; new idempotency test in exit-step-failure.test.ts; migrated resume/bootstrap/rebuild tests.
• Docs not applicable — justification: internal FSM refactor with no user-facing behavior change (recovery outcomes and CLI surface are unchanged; only their internal mechanism is made explicit).
• Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging) — onboarding FSM recovery.
• Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: requesting maintainer review of the onboarding recovery paths.

Verification

• PR description includes the DCO sign-off declaration and every commit appears as Verified in GitHub
• Targeted tests pass for changed behavior — full src/lib/onboard unit suite (203 files) plus onboard FSM/resume/lifecycle/rollback integration tests: 1899 passing. npm run typecheck:cli clean; Biome clean; npm run test:projects:check disjoint.
• No secrets, API keys, or credentials committed

Note: commit/push git hooks were bypassed for the slow plugin-Vitest/plugin-typecheck steps only — the plugin (nemoclaw/) is untouched by this CLI-only diff and its node_modules is not installed in this environment. The CLI typecheck passed in the pre-push hook and in manual runs.

---

Signed-off-by: Atulya Singh atulyarajsingh@gmail.com

Summary by CodeRabbit

• New Features

  • Added a more reliable session recovery flow during onboarding, helping resumed sessions continue from the correct step.
  • Onboarding now records recovery decisions more clearly, including whether a session was kept as-is or repaired.

• Bug Fixes

  • Improved handling of interrupted or failed onboarding so terminal failures are finalized consistently.
  • Prevented invalid transitions out of terminal failure states and made repeat failure handling idempotent.

[copy-pr-bot]

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

[coderabbitai]

📝 Walkthrough

Walkthrough

This PR replaces repairResumeMachineSnapshot with a new session-recovery.ts module exposing planSessionRecovery/applySessionRecovery, and replaces markStepFailed with a new finalizeIncompleteOnboardStep terminal-failure backstop, updating onboarding bootstrap, exit-handling, and dependent tests accordingly.

Changes

Session recovery and terminal failure finalization

Layer / File(s)	Summary
Session recovery module and transition invariants src/lib/onboard/session-recovery.ts, src/lib/onboard/session-recovery.test.ts, src/lib/onboard/resume-machine-repair.ts, src/lib/onboard/machine/transitions.ts, src/lib/onboard/machine/transitions.test.ts	New module defines SessionRecoveryPlan, UnrecoverableSessionError, planSessionRecovery, applySessionRecovery; repairResumeMachineSnapshot removed; transitions doc/tests enforce that failed/complete are terminal.
Session bootstrap wiring src/lib/onboard/session-bootstrap.ts, src/lib/onboard/session-bootstrap.test.ts	OnboardSessionBootstrapDeps/Result gain applySessionRecovery/recovery; prepareResumeSession and prepareFreshSession updated and tested.
Onboard.ts recovery event recording src/lib/onboard.ts, src/lib/actions/sandbox/rebuild-resume-snapshot.test.ts, src/lib/onboard/resume-machine-repair.test.ts	onboard.ts wires applySessionRecovery and records a state.repair.completed event on recovery; dependent tests switch from repairResumeMachineSnapshot to applySessionRecovery.
Terminal failure finalization src/lib/state/onboard-session.ts, src/lib/onboard/exit-step-failure.ts, src/lib/onboard/exit-step-failure.test.ts	New finalizeIncompleteOnboardStep validates non-terminal→failed transitions and finalizes a step; exit-step-failure.ts routes through it instead of markStepFailed.
Rebuild flow test alignment src/lib/actions/sandbox/rebuild-flow.test.ts	Test harness spies and assertions updated from markStepFailed to finalizeIncompleteOnboardStep; mock always sets machine state to failed.

Estimated code review effort: 4 (Complex) | ~60 minutes

Sequence Diagram(s)

sequenceDiagram
  participant OnboardTs as onboard.ts
  participant Bootstrap as session-bootstrap.ts
  participant Recovery as session-recovery.ts
  participant Session

  OnboardTs->>Bootstrap: prepareOnboardSession(deps with applySessionRecovery)
  Bootstrap->>Recovery: applySessionRecovery(currentSession)
  Recovery->>Recovery: planSessionRecovery (classify + validate entry)
  alt recover
    Recovery->>Session: set machine state/stateEnteredAt/revision
  end
  Recovery-->>Bootstrap: recovery plan
  Bootstrap-->>OnboardTs: {session, fromDockerfile, recovery}
  alt recovery.action == recover
    OnboardTs->>OnboardTs: record state.repair.completed event
  end

Loading

sequenceDiagram
  participant ExitStepFailure as exit-step-failure.ts
  participant OnboardSession as onboard-session.ts
  participant Session

  ExitStepFailure->>OnboardSession: finalizeIncompleteOnboardStep(stepName, message)
  OnboardSession->>Session: loadSession()
  alt machine already terminal
    OnboardSession-->>ExitStepFailure: unchanged session
  else
    OnboardSession->>OnboardSession: assertValidOnboardMachineTransition(-> failed)
    OnboardSession->>Session: mark step failed, set machine.state=failed
    OnboardSession->>OnboardSession: emit state.failed, onboard.failed
    OnboardSession-->>ExitStepFailure: updated session
  end

Loading

Possibly related PRs

• NVIDIA/NemoClaw#4472: Both PRs modify onboarding resume initialization in src/lib/onboard.ts, directly overlapping in the resume-machine recovery wiring that this PR replaces with applySessionRecovery.

Suggested labels: refactor

Suggested reviewers: jyaunches, ericksoa, cjagwani

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly matches the main change: consolidating onboarding FSM resume recovery into a single explicit path.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 2

🧹 Nitpick comments (3)

src/lib/actions/sandbox/rebuild-flow.test.ts (1)

141-166: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Mock diverges from the real terminal owner on the missing-step branch.

installTerminalStepFailureMock reimplements finalizeIncompleteOnboardStep, but where the real function returns early without transitioning when session.steps[stepName] is absent (if (!step) return session;), this mock instead synthesizes a createStep("pending") and drives the machine to failed. If a rebuild-flow test ever exercises a not-yet-recorded step, the harness will record a terminal failure the production code would not, masking a real gap. Consider aligning the mock's missing-step branch with production (early return, no transition).

As per path instructions: "Flag copied production algorithms, broad mocks that bypass the behavior under test."

♻️ Align missing-step handling

       const stepKey = String(stepName);
-      const step = session.steps[stepKey] ?? createStep("pending");
-      session.steps[stepKey] = step;
+      const step = session.steps[stepKey];
+      if (!step) return session;
       step.status = "failed";

🤖 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-flow.test.ts` around lines 141 - 166, The
`installTerminalStepFailureMock` helper is diverging from
`finalizeIncompleteOnboardStep` in the missing-step path by creating a synthetic
pending step and forcing the session to `failed`. Update the mock to match
production behavior in that branch: if `session.steps[stepName]` is absent,
return the session immediately without changing `session.status`,
`session.failure`, or `session.machine.state`. Keep the rest of the
failure-transition logic unchanged so the test harness mirrors
`finalizeIncompleteOnboardStep` accurately.

src/lib/onboard/resume-machine-repair.ts (1)

33-38: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Link the tracked follow-up for removing the resumeMachineState bridge.

The comment documents resumeMachineState as a transitional bridge to be removed once step fields stop being used, but doesn't reference a tracking issue/PR — unlike transitions.ts, which links #6179 for its own invariant. As per path instructions, "If a PR intentionally migrates only a slice, it must say so and link the remaining work in GitHub."

🤖 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/resume-machine-repair.ts` around lines 33 - 38, The doc
comment for the `resumeMachineState` bridge should explicitly link the tracked
follow-up work for removing it, since it’s a temporary transition and currently
lacks a GitHub reference. Update the comment in `resume-machine-repair.ts` near
`resumeMachineState` to say this slice is intentional and add the remaining-work
issue/PR link, matching the style used in `transitions.ts` for its invariant
reference.

Source: Path instructions

src/lib/onboard/session-bootstrap.test.ts (1)

122-123: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Consider adding coverage for the action: "recover" branch.

Only the keep recovery outcome is exercised here. Since recovery.action === "recover" is what triggers the new state.repair.completed event in onboard.ts, a test asserting result.recovery for the recover case (with entry/reason) would meaningfully guard this critical path.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/lib/onboard/session-bootstrap.test.ts` around lines 122 - 123, Add test
coverage in session-bootstrap.test.ts for the recovery branch where
applySessionRecovery leads to action: "recover", not just the current keep case.
Update or add a test around applySessionRecovery and result.recovery to assert
the recover outcome includes the expected entry and reason, so the onboard.ts
path that emits state.repair.completed is covered.

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.ts`:
- Around line 4796-4804: Move the `state.repair.completed` emission into the
recovery helper path so the repair write and audit record are applied together.
Update the `applySessionRecovery` flow and its caller in
`onboardRuntimeBoundary.recordOnboardStarted` handling so the helper persists
the repaired snapshot and immediately records the
`recordRepairEvent("state.repair.completed", ...)` when `recovery.action ===
"recover"`, preserving the existing state and metadata payload.

In `@src/lib/onboard/session-recovery.test.ts`:
- Around line 29-75: Add a dedicated test in planSessionRecovery to cover the
reopened_complete_snapshot path, since the current cases only exercise
failed_terminal_snapshot and nonterminal_snapshot. Create a session fixture with
a complete machine snapshot that is reopened (status not complete or resumable
not false), then assert planSessionRecovery returns action keep with reason
reopened_complete_snapshot and that applySessionRecovery preserves the expected
entry. Use the existing planSessionRecovery, applySessionRecovery, and
createSession helpers so the new test directly protects the
assertRecoverableEntry behavior from regressions.

---

Nitpick comments:
In `@src/lib/actions/sandbox/rebuild-flow.test.ts`:
- Around line 141-166: The `installTerminalStepFailureMock` helper is diverging
from `finalizeIncompleteOnboardStep` in the missing-step path by creating a
synthetic pending step and forcing the session to `failed`. Update the mock to
match production behavior in that branch: if `session.steps[stepName]` is
absent, return the session immediately without changing `session.status`,
`session.failure`, or `session.machine.state`. Keep the rest of the
failure-transition logic unchanged so the test harness mirrors
`finalizeIncompleteOnboardStep` accurately.

In `@src/lib/onboard/resume-machine-repair.ts`:
- Around line 33-38: The doc comment for the `resumeMachineState` bridge should
explicitly link the tracked follow-up work for removing it, since it’s a
temporary transition and currently lacks a GitHub reference. Update the comment
in `resume-machine-repair.ts` near `resumeMachineState` to say this slice is
intentional and add the remaining-work issue/PR link, matching the style used in
`transitions.ts` for its invariant reference.

In `@src/lib/onboard/session-bootstrap.test.ts`:
- Around line 122-123: Add test coverage in session-bootstrap.test.ts for the
recovery branch where applySessionRecovery leads to action: "recover", not just
the current keep case. Update or add a test around applySessionRecovery and
result.recovery to assert the recover outcome includes the expected entry and
reason, so the onboard.ts path that emits state.repair.completed is covered.

🪄 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: 532d200a-6719-474c-8034-225f4bb459fe

📥 Commits

Reviewing files that changed from the base of the PR and between 6092ad2 and f96da05.

📒 Files selected for processing (14)

• src/lib/actions/sandbox/rebuild-flow.test.ts
• src/lib/actions/sandbox/rebuild-resume-snapshot.test.ts
• src/lib/onboard.ts
• src/lib/onboard/exit-step-failure.test.ts
• src/lib/onboard/exit-step-failure.ts
• src/lib/onboard/machine/transitions.test.ts
• src/lib/onboard/machine/transitions.ts
• src/lib/onboard/resume-machine-repair.test.ts
• src/lib/onboard/resume-machine-repair.ts
• src/lib/onboard/session-bootstrap.test.ts
• src/lib/onboard/session-bootstrap.ts
• src/lib/onboard/session-recovery.test.ts
• src/lib/onboard/session-recovery.ts
• src/lib/state/onboard-session.ts

[wscurran]

✨ Thanks for the refactor. This foundational slice for #6227 replaces three implicit recovery mechanisms with a single explicit FSM path — maintainers can review the new session-recovery module and transition enforcement.

---

Related open issues:

• #6227 refactor(onboard): harden FSM recovery and transition semantics
• #6179 [DGX Spark][Onboard] nemoclaw onboard exits with InvalidOnboardMachineTransitionError after Ollama sandbox creation succeeds

[apurvvkumaria]

Maintainer salvage update:

• Credit: @atulya-singh authored the original explicit FSM recovery design; the original commit is preserved and Atulya is a co-author on the salvage commit.
• Synced the branch to current main without force-pushing. All three PR commits are GitHub Verified and carry DCO sign-offs.
• Closed the two outstanding review gaps with a durable, secret-free recovery receipt plus explicit reopened_complete_snapshot coverage. Runtime dispatch now follows onboard.resumed; retries keep a stable receipt ID and observer delivery remains accurately documented as best-effort.
• Final scope remains 21 onboarding/recovery files (+817/-97), with no dependency, policy, credential, workflow, or configuration changes.
• Exact-head validation: 278 focused tests passed, CLI type-check passed, plugin/JS/CLI pre-push type-checks passed, static gates passed, and the nine-category security review returned PASS with no findings.

CI and the refreshed automated review are still running; this is not being marked ready until they settle.

[apurvvkumaria]

Exact head 6d8da3d503a5cdf731bbd66398de7bc46f36151d has now settled fully green: all required checks pass, CodeRabbit has no current actionable findings, both review threads are resolved, and the PR remains mergeable.

The prior maintainer update’s recovery receipt, reopened_complete_snapshot coverage, author credit, signed/Verified history, and validation evidence are unchanged. An exact-head feedback audit found no remaining code, test, documentation, or security action.

This PR is ready for independent human review. No approval or merge action was taken.