refactor(onboard): apply strict step results directly - #5708
Conversation
…rd-fsm-live-slice-inputs
…live-slice-inputs # Conflicts: # src/lib/onboard/machine/core-flow-phases.ts # src/lib/onboard/machine/final-flow-phases.ts # src/lib/onboard/machine/initial-flow-phases.ts # src/lib/onboard/machine/live-flow-slice.test.ts
…live-slice-inputs # Conflicts: # src/lib/onboard/machine/core-flow-phases.ts # src/lib/onboard/machine/initial-flow-phases.ts
…nboard-fsm-compat-result-loop
…onboard-fsm-drop-record-only-runner
…ctor/onboard-fsm-trim-recorders
…ard-fsm-record-only-boundary
…r/onboard-fsm-runtime-record-only
…/onboard-fsm-name-legacy-step-mutation
…st/onboard-explicit-legacy-step-mutation
…nboard-fsm-compat-result-loop
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (3)
📝 WalkthroughWalkthroughUpdated onboarding step-completion handling to bypass the compatibility bridge for one path, added a test for that behavior, and rewrote related resume-compatibility comments in the onboarding flow slices and runtime boundary. ChangesOnboard resume path and comment wording
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
E2E Advisor RecommendationRequired E2E: Dispatch hint: Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
Dispatch hint
|
Vitest E2E Scenario RecommendationRequired Vitest E2E scenarios: Dispatch required Vitest E2E scenarios:
Full Vitest E2E advisor summaryVitest E2E Scenario AdvisorBase: Required Vitest E2E scenarios
Optional Vitest E2E scenarios
Relevant changed files
|
PR Review Advisor — No blocking findingsMerge posture: No blocking advisor findings This is an automated, non-binding review; it still expects maintainers and agents to respond to each required or warning item. Treat suggestions as current-PR improvements when they touch changed code; defer only with maintainer rationale or a linked follow-up. A human maintainer must make the final merge decision. |
…-legacy-step-mutation # Conflicts: # src/lib/state/onboard-step-mutation.test.ts
…p-mutation' into refactor/onboard-fsm-session-record-only
…ord-only' into test/onboard-record-only-boundary-stale-results
…session-record-only # Conflicts: # src/lib/state/onboard-step-mutation.test.ts
…ord-only' into test/onboard-record-only-boundary-stale-results
…ord-only' into test/onboard-record-only-boundary-stale-results
## Summary Refresh the onboarding FSM compatibility comments after the record-only migration. The comments now describe the remaining replay bridge as persisted resume/ahead-state tolerance instead of default legacy step-helper machine mutation. ## Changes - Update initial, core, and final flow-slice compatibility comments with current source-of-truth wording. - Narrow runtime-boundary bridge comments to explicit legacy/test overrides and resume/ahead replay. - Document removal conditions around first-class FSM recovery states. ## Type of Change - [x] Code change (feature, bug fix, or refactor) - [ ] Code change with doc updates - [ ] Doc only (prose changes, no code sample modifications) - [ ] Doc only (includes code sample changes) ## Verification - [x] PR description includes the DCO sign-off declaration and every commit appears as `Verified` in GitHub - [x] Git hooks passed during commit and push, or `npx prek run --from-ref main --to-ref HEAD` passes - [x] Targeted tests pass for changed behavior - [ ] Full `npm test` passes (broad runtime changes only) - [ ] Tests added or updated for new or changed behavior - [x] No secrets, API keys, or credentials committed - [ ] Docs updated for user-facing behavior changes - [ ] `npm run docs` builds without warnings (doc changes only) - [ ] Doc pages follow the [style guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md) (doc changes only) - [ ] New doc pages include SPDX header and frontmatter (new pages only) --- Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in the Show a code coverage summary of the most covered files.
TypeScript / code-coverage/cliThe overall coverage in the Show a code coverage summary of the most covered files.
Updated |
## Summary Route record-only step/result pairing through the strict runtime result path. This keeps the stale-result compatibility bridge reserved for explicit legacy or resume replay paths. ## Changes - Change `recordStepCompleteWithStateResult` to call `recordStateResult` after its preflight validation. - Leave legacy/resume compatible result handling unchanged. - Preserve existing runtime-boundary coverage. ## Type of Change - [x] Code change (feature, bug fix, or refactor) - [ ] Code change with doc updates - [ ] Doc only (prose changes, no code sample modifications) - [ ] Doc only (includes code sample changes) ## Verification - [x] PR description includes the DCO sign-off declaration and every commit appears as `Verified` in GitHub - [x] Git hooks passed during commit and push, or `npx prek run --from-ref main --to-ref HEAD` passes - [x] Targeted tests pass for changed behavior - [ ] Full `npm test` passes (broad runtime changes only) - [ ] Tests added or updated for new or changed behavior - [x] No secrets, API keys, or credentials committed - [ ] Docs updated for user-facing behavior changes - [ ] `npm run docs` builds without warnings (doc changes only) - [ ] Doc pages follow the [style guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md) (doc changes only) - [ ] New doc pages include SPDX header and frontmatter (new pages only) --- Signed-off-by: Carlos Villela <cvillela@nvidia.com>
## Summary Route validated record-only step completion results through the strict state-result path instead of the compatibility bridge. This makes `recordStepCompleteWithStateResult()` apply a checked transition directly, while keeping legacy/repaired-resume compatibility paths explicitly documented and isolated. ## Changes - Apply validated record-only step completion results directly after the step status write, avoiding `state.result.skipped` compatibility diagnostics on the strict path. - Add focused runtime-boundary coverage for a `preflight` → `gateway` step completion result that applies directly without compatibility events. - Clarify initial/core/final flow-slice compatibility comments with the tolerated ahead-state snapshots, their source boundaries, existing regression coverage, and removal condition. ## Type of Change - [x] Code change (feature, bug fix, or refactor) - [ ] Code change with doc updates - [ ] Doc only (prose changes, no code sample modifications) - [ ] Doc only (includes code sample changes) ## Verification - [x] PR description includes the DCO sign-off declaration and every commit appears as `Verified` in GitHub - [x] Git hooks passed during commit and push, or `npx prek run --from-ref main --to-ref HEAD` passes - [x] Targeted tests pass for changed behavior - [ ] Full `npm test` passes (broad runtime changes only) - [x] Tests added or updated for new or changed behavior - [x] No secrets, API keys, or credentials committed - [ ] Docs updated for user-facing behavior changes - [ ] `npm run docs` builds without warnings (doc changes only) - [ ] Doc pages follow the [style guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md) (doc changes only) - [ ] New doc pages include SPDX header and frontmatter (new pages only) Targeted local checks run: - `npx vitest run --project cli src/lib/onboard/machine/initial-flow-phases.test.ts src/lib/onboard/machine/core-flow-phases.test.ts src/lib/onboard/machine/final-flow-phases.test.ts src/lib/onboard/runtime-boundary-record-only.test.ts src/lib/onboard/runtime-boundary.test.ts` - `npm run typecheck:cli` --- Signed-off-by: Carlos Villela <cvillela@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved onboarding resume and step-completion handling so validated results follow the direct path, reducing inconsistent “stale” transition behavior. * Clarified and reinforced resume behavior for onboarding flows, helping ensure repair and verification steps still run in the correct order. * **Tests** * Added coverage to verify direct record-only step completion does not use the compatibility path. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Summary
Route validated record-only step completion results through the strict state-result path instead of the compatibility bridge. This makes
recordStepCompleteWithStateResult()apply a checked transition directly, while keeping legacy/repaired-resume compatibility paths explicitly documented and isolated.Changes
state.result.skippedcompatibility diagnostics on the strict path.preflight→gatewaystep completion result that applies directly without compatibility events.Type of Change
Verification
Verifiedin GitHubnpx prek run --from-ref main --to-ref HEADpassesnpm testpasses (broad runtime changes only)npm run docsbuilds without warnings (doc changes only)Targeted local checks run:
npx vitest run --project cli src/lib/onboard/machine/initial-flow-phases.test.ts src/lib/onboard/machine/core-flow-phases.test.ts src/lib/onboard/machine/final-flow-phases.test.ts src/lib/onboard/runtime-boundary-record-only.test.ts src/lib/onboard/runtime-boundary.test.tsnpm run typecheck:cliSigned-off-by: Carlos Villela cvillela@nvidia.com
Summary by CodeRabbit