Repository navigation
refactor(onboard): remove resume flag from live slices - #5643
Conversation
|
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 (2)
📝 WalkthroughWalkthroughRemoves the ChangesResume encoded via
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
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 |
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 |
PR Review Advisor — No blocking findingsMerge posture: No blocking advisor findings Action checklist
Test follow-ups to resolve or justifyIf these cover changed behavior, prefer adding them in this PR; otherwise state why existing coverage is enough or link the follow-up.
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. |
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 follow-up: The requested runtime validations are covered by existing targeted tests for this refactor layer:
I did not add another runtime test here because this PR only removes the low-level |
…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
Nightly E2E regression that #5643 can absorbI think #5643 is the right place to absorb a regression now showing up in the nightly E2E rebuild/resume lanes, while keeping your intended onboard FSM direction intact. What regressedAfter #5642, That is directionally good because compatibility is now explicit, but the declared resume states are incomplete for rebuild/onboard-resume flows. Those flows enter Failing nightly evidencePost-revert nightly run: https://github.com/NVIDIA/NemoClaw/actions/runs/28050018215 Observed failures:
The same signature was already present in the full nightly run after #5642/#5600, so this does not appear to be caused by the #5600 key-routing revert. Suggested #5643-compatible fixI would not restore a global resume bypass in For // initial wrapper, resume=true
compatibilityWhenState: [
"init", "preflight", "gateway", "provider_selection",
"inference", "sandbox", "openclaw", "agent_setup",
"policies", "finalizing", "post_verify",
]
// core wrapper, resume=true
compatibilityWhenState: [
"provider_selection", "inference", "sandbox",
"openclaw", "agent_setup", "policies",
"finalizing", "post_verify",
]
// final wrapper, resume=true
compatibilityWhenState: [
"openclaw", "agent_setup", "policies",
"finalizing", "post_verify",
]The important part is not necessarily this exact list, but that Tests to addCould you add targeted tests in this PR for the two observed regression states?
Then the focused E2E validation would be:
This should preserve your architecture goal: explicit compatibility declarations at the phase wrappers, record/FSM as the durable source of truth, and no broad resume escape hatch in the low-level slice. |
…live-slice-inputs # Conflicts: # src/lib/onboard/machine/core-flow-phases.ts # src/lib/onboard/machine/initial-flow-phases.ts
|
PR Review Advisor follow-up for latest review:
The downstream resume regression called out in the maintainer comment was handled by merged hotfix #5690 and is present in this branch via the latest |
## Summary Remove the low-level live-slice `resume` flag now that compatibility execution is driven by explicit state declarations. This shrinks the slice API and keeps resume-specific decisions in the phase wrappers that know which states are safe to replay. ## Changes - Drop `resume` from `runLiveOnboardFlowSlice` options and tests. - Make initial, core, and final wrappers choose compatibility states from their existing resume option. - Preserve fresh strict-runner behavior while keeping resume repair/backstop replay explicit. ## 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) --- Signed-off-by: Carlos Villela <cvillela@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Refactor** * Updated onboarding flow execution to choose which phases can run based on the current machine state instead of an explicit resume flag. * Resume sessions now allow a broader compatibility path, while fresh sessions restrict compatibility to reduce unintended transitions. * **Tests** * Adjusted live onboarding slice tests to rely on the new state-based behavior. * Added/expanded strict-runner coverage for fresh preflight/provider selection sessions, including branching transitions and ensuring compatibility recording is not triggered. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: Hadar Cohen <hacohen@redhat.com>
## Summary Remove the low-level live-slice `resume` flag now that compatibility execution is driven by explicit state declarations. This shrinks the slice API and keeps resume-specific decisions in the phase wrappers that know which states are safe to replay. ## Changes - Drop `resume` from `runLiveOnboardFlowSlice` options and tests. - Make initial, core, and final wrappers choose compatibility states from their existing resume option. - Preserve fresh strict-runner behavior while keeping resume repair/backstop replay explicit. ## 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) --- Signed-off-by: Carlos Villela <cvillela@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Refactor** * Updated onboarding flow execution to choose which phases can run based on the current machine state instead of an explicit resume flag. * Resume sessions now allow a broader compatibility path, while fresh sessions restrict compatibility to reduce unintended transitions. * **Tests** * Adjusted live onboarding slice tests to rely on the new state-based behavior. * Added/expanded strict-runner coverage for fresh preflight/provider selection sessions, including branching transitions and ensuring compatibility recording is not triggered. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: Hadar Cohen <hacohen@redhat.com>
## Summary Remove the low-level live-slice `resume` flag now that compatibility execution is driven by explicit state declarations. This shrinks the slice API and keeps resume-specific decisions in the phase wrappers that know which states are safe to replay. ## Changes - Drop `resume` from `runLiveOnboardFlowSlice` options and tests. - Make initial, core, and final wrappers choose compatibility states from their existing resume option. - Preserve fresh strict-runner behavior while keeping resume repair/backstop replay explicit. ## 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) --- Signed-off-by: Carlos Villela <cvillela@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Refactor** * Updated onboarding flow execution to choose which phases can run based on the current machine state instead of an explicit resume flag. * Resume sessions now allow a broader compatibility path, while fresh sessions restrict compatibility to reduce unintended transitions. * **Tests** * Adjusted live onboarding slice tests to rely on the new state-based behavior. * Added/expanded strict-runner coverage for fresh preflight/provider selection sessions, including branching transitions and ensuring compatibility recording is not triggered. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Summary
Remove the low-level live-slice
resumeflag now that compatibility execution is driven by explicit state declarations. This shrinks the slice API and keeps resume-specific decisions in the phase wrappers that know which states are safe to replay.Changes
resumefromrunLiveOnboardFlowSliceoptions and tests.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)Signed-off-by: Carlos Villela cvillela@nvidia.com
Summary by CodeRabbit