refactor(onboard): make FSM resume recovery a single explicit path (#6227) - #6252
refactor(onboard): make FSM resume recovery a single explicit path (#6227)#6252atulya-singh wants to merge 1 commit into
Conversation
…VIDIA#6227) Replace the three implicit onboarding recovery mechanisms with explicit, deterministic FSM semantics, the foundational slice of NVIDIA#6227. - Consolidate resume repair into one validated, side-effect-free recovery pass (planSessionRecovery/applySessionRecovery in session-recovery.ts). It 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. - Introduce a single synchronous terminal-failure owner (finalizeIncompleteOnboardStep) for exception/signal/nonzero-exit paths. It validates the failed transition and is idempotent against an already-terminal machine, so exactly one failed transition and one terminal event are recorded. Removes the last production use of LEGACY_MACHINE_STEP_MUTATION_OPTIONS. - Document the legal transition graph and terminality invariant, and assert that a terminal failed state can never re-enter an agent/flow state (NVIDIA#6179). No persisted SandboxCreateIntent or schema change is introduced. Signed-off-by: Atulya Singh <atulyarajsingh@gmail.com> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR replaces two legacy onboarding mechanisms with new dedicated modules: ChangesOnboard session recovery and terminal failure refactor
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant applySessionRecovery
participant planSessionRecovery
participant classifyResumeMachineRepair
participant resumeMachineState
Caller->>applySessionRecovery: applySessionRecovery(session, stateEnteredAt)
applySessionRecovery->>planSessionRecovery: planSessionRecovery(session)
planSessionRecovery->>classifyResumeMachineRepair: classifyResumeMachineRepair(session)
classifyResumeMachineRepair-->>planSessionRecovery: decision
alt decision requires recovery
planSessionRecovery->>resumeMachineState: resumeMachineState(session)
resumeMachineState-->>planSessionRecovery: candidate entry
planSessionRecovery-->>applySessionRecovery: recover plan (entry, reason)
applySessionRecovery->>applySessionRecovery: mutate session.machine
else keep
planSessionRecovery-->>applySessionRecovery: keep plan
end
applySessionRecovery-->>Caller: SessionRecoveryPlan
sequenceDiagram
participant Caller
participant markLastStartedStepFailed
participant finalizeIncompleteOnboardStep
participant SessionStore
participant EventEmitter
Caller->>markLastStartedStepFailed: markLastStartedStepFailed(message)
markLastStartedStepFailed->>finalizeIncompleteOnboardStep: finalizeIncompleteOnboardStep(stepName, message)
finalizeIncompleteOnboardStep->>SessionStore: load session
alt session missing or machine terminal
finalizeIncompleteOnboardStep-->>markLastStartedStepFailed: null
else
finalizeIncompleteOnboardStep->>SessionStore: updateSession(mark step failed, machine -> failed)
SessionStore-->>finalizeIncompleteOnboardStep: updated session
finalizeIncompleteOnboardStep->>EventEmitter: emit state.failed, onboard.failed
finalizeIncompleteOnboardStep-->>markLastStartedStepFailed: updated session
end
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
Superseded by #6253 (identical change, reopened to drop the co-author trailer). |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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.ts (1)
4769-4804: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winMove the exit-failure backstop before session recovery
applySessionRecovery()can throwUnrecoverableSessionErrorduring resume bootstrap, butregisterIncompleteOnboardExitHandlerForSession()is only installed later. That leaves an unrecoverable resumed session to exit without the single failure owner recording a failed transition or terminal event. Move the backstop earlier, or route recovery errors through the same failure 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.ts` around lines 4769 - 4804, The resume bootstrap in onboardSessionBootstrap.prepareOnboardSession can throw UnrecoverableSessionError before registerIncompleteOnboardExitHandlerForSession is installed, leaving recovery failures without the normal terminal failure handling. Move the exit-failure backstop to run before prepareOnboardSession, or make applySessionRecovery / the recovery path in onboard.ts route errors through the same failure owner used by the session exit handler so failed resumed sessions always record a failed transition or terminal event.
🧹 Nitpick comments (3)
src/lib/actions/sandbox/rebuild-flow.test.ts (1)
140-167: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMock re-implements production mutation logic and diverges from it on the missing-step path.
This mock duplicates
finalizeIncompleteOnboardStep's failure-recording/transition logic instead of delegating to it, and it diverges on the missing-step case: the real implementation no-ops entirely whensession.steps[stepName]is absent (seesrc/lib/state/onboard-session.tsLine 1256), but this mock creates a pending step and proceeds to mark it failed anyway (Lines 152-154). Tests built on this double can't catch the missing-step no-op gap flagged inonboard-session.ts.As per path instructions, "Flag copied production algorithms, broad mocks that bypass the behavior under test, and conditionals that make a test pass without exercising its claim."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/actions/sandbox/rebuild-flow.test.ts` around lines 140 - 167, The test helper installTerminalStepFailureMock is re-implementing finalizeIncompleteOnboardStep instead of exercising the real behavior, so it hides the missing-step no-op path. Update the mock to delegate to the onboardSession.finalizeIncompleteOnboardStep implementation (or assert its call behavior) rather than mutating session state directly, and remove the createStep("pending") fallback so absent session.steps[stepName] remains a no-op just like production.Source: Path instructions
src/lib/onboard/session-recovery.test.ts (1)
29-133: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd integration coverage for the unrecoverable-session throw path.
assertRecoverableEntrythrowing is tested standalone, but there's no test provingplanSessionRecovery/applySessionRecoveryactually surfaceUnrecoverableSessionErrorfor a session whose derived resume entry resolves to a terminal state (e.g., afailedsession whose last completed step is the final step). This is the critical safety net the whole recovery redesign depends on.🤖 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-recovery.test.ts` around lines 29 - 133, Add an integration-style test in session-recovery.test.ts that exercises the unrecoverable path through planSessionRecovery and/or applySessionRecovery, not just assertRecoverableEntry directly. Create a session whose derived resume entry resolves to a terminal state (for example, a failed session where the last completed step is the final step) and verify the call throws UnrecoverableSessionError. Use the existing helpers and the planSessionRecovery/applySessionRecovery symbols to confirm the throw is surfaced end-to-end.src/lib/onboard/session-bootstrap.test.ts (1)
122-123: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winPrefer an observable-outcome assertion over a mock-call assertion.
expect(deps.applySessionRecovery).toHaveBeenCalledWith(initial)locks in an implementation detail (that the dependency was invoked with this reference) rather than an observable behavior. Since the actual correctness invariant here is that recovery must see the session's failure/lastStepStarted fields before they're cleared, consider having the mock capture and assert on the state it was invoked with (e.g., itsfailure/statussnapshot) so the test fails if that ordering regresses, instead of just checking the call occurred.As per path instructions, "Prefer observable outcomes through the public boundary over source-text, private-shape, or mock-call assertions."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/onboard/session-bootstrap.test.ts` around lines 122 - 123, The test is asserting a mock call detail instead of the observable recovery state, which makes it brittle to implementation changes. Update the session bootstrap test around applySessionRecovery so it captures the argument passed into that dependency and asserts the session snapshot still contains the expected failure/status/lastStepStarted information before cleanup, using the session bootstrap flow and recovery result as the public behavior check rather than only toHaveBeenCalledWith(initial).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.
Outside diff comments:
In `@src/lib/onboard.ts`:
- Around line 4769-4804: The resume bootstrap in
onboardSessionBootstrap.prepareOnboardSession can throw
UnrecoverableSessionError before registerIncompleteOnboardExitHandlerForSession
is installed, leaving recovery failures without the normal terminal failure
handling. Move the exit-failure backstop to run before prepareOnboardSession, or
make applySessionRecovery / the recovery path in onboard.ts route errors through
the same failure owner used by the session exit handler so failed resumed
sessions always record a failed transition or terminal event.
---
Nitpick comments:
In `@src/lib/actions/sandbox/rebuild-flow.test.ts`:
- Around line 140-167: The test helper installTerminalStepFailureMock is
re-implementing finalizeIncompleteOnboardStep instead of exercising the real
behavior, so it hides the missing-step no-op path. Update the mock to delegate
to the onboardSession.finalizeIncompleteOnboardStep implementation (or assert
its call behavior) rather than mutating session state directly, and remove the
createStep("pending") fallback so absent session.steps[stepName] remains a no-op
just like production.
In `@src/lib/onboard/session-bootstrap.test.ts`:
- Around line 122-123: The test is asserting a mock call detail instead of the
observable recovery state, which makes it brittle to implementation changes.
Update the session bootstrap test around applySessionRecovery so it captures the
argument passed into that dependency and asserts the session snapshot still
contains the expected failure/status/lastStepStarted information before cleanup,
using the session bootstrap flow and recovery result as the public behavior
check rather than only toHaveBeenCalledWith(initial).
In `@src/lib/onboard/session-recovery.test.ts`:
- Around line 29-133: Add an integration-style test in session-recovery.test.ts
that exercises the unrecoverable path through planSessionRecovery and/or
applySessionRecovery, not just assertRecoverableEntry directly. Create a session
whose derived resume entry resolves to a terminal state (for example, a failed
session where the last completed step is the final step) and verify the call
throws UnrecoverableSessionError. Use the existing helpers and the
planSessionRecovery/applySessionRecovery symbols to confirm the throw is
surfaced end-to-end.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 7f3dc939-7947-42e2-87cb-5b397c7b231f
📒 Files selected for processing (14)
src/lib/actions/sandbox/rebuild-flow.test.tssrc/lib/actions/sandbox/rebuild-resume-snapshot.test.tssrc/lib/onboard.tssrc/lib/onboard/exit-step-failure.test.tssrc/lib/onboard/exit-step-failure.tssrc/lib/onboard/machine/transitions.test.tssrc/lib/onboard/machine/transitions.tssrc/lib/onboard/resume-machine-repair.test.tssrc/lib/onboard/resume-machine-repair.tssrc/lib/onboard/session-bootstrap.test.tssrc/lib/onboard/session-bootstrap.tssrc/lib/onboard/session-recovery.test.tssrc/lib/onboard/session-recovery.tssrc/lib/state/onboard-session.ts
2 participants
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
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 explicitstate.repair.completedevent. Replaces the implicitrepairResumeMachineSnapshotrewrite, which is removed;resumeMachineStateis retained as an internal building block.finalizeIncompleteOnboardStepinonboard-session.tsfor exception/signal/nonzero-exit paths. It validates the<non-terminal> -> failedtransition 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 ofLEGACY_MACHINE_STEP_MUTATION_OPTIONS(the process-exit backstop inexit-step-failure.ts).transitions.tsand a negative test asserting a terminalfailedstate can never re-enter an agent/flow state ([DGX Spark][Onboard] nemoclaw onboard exits with InvalidOnboardMachineTransitionError after Ollama sandbox creation succeeds #6179).repairResumeMachineSnapshottoapplySessionRecovery; rebuild-flow terminal-failure mock updated to the new owner.No persisted
SandboxCreateIntentor public schema is introduced (explicit non-goal of #6227).Type of Change
Quality Gates
session-recovery.test.ts; new [DGX Spark][Onboard] nemoclaw onboard exits with InvalidOnboardMachineTransitionError after Ollama sandbox creation succeeds #6179 negative test; new idempotency test inexit-step-failure.test.ts; migrated resume/bootstrap/rebuild tests.Verification
Verifiedin GitHubsrc/lib/onboardunit suite (203 files) plus onboard FSM/resume/lifecycle/rollback integration tests: 1899 passing.npm run typecheck:cliclean; Biome clean;npm run test:projects:checkdisjoint.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 itsnode_modulesis 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
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes