Skip to content

refactor(onboard): extract live FSM slice runner helper#4530

Draft
cv wants to merge 1 commit into
stack/onboard-fsm-use-final-slicefrom
stack/onboard-fsm-live-slice-helper
Draft

refactor(onboard): extract live FSM slice runner helper#4530
cv wants to merge 1 commit into
stack/onboard-fsm-use-final-slicefrom
stack/onboard-fsm-live-slice-helper

Conversation

@cv
Copy link
Copy Markdown
Collaborator

@cv cv commented May 29, 2026

Summary

Extract the common live FSM slice execution policy into a reusable helper. The helper runs the strict slice path only for fresh matching entry states and falls back to compatibility execution for resume/ahead-state flows.

Changes

  • Add runLiveOnboardFlowSlice() for strict-versus-compatibility slice execution.
  • Normalize single and ordered FSM result application for compatibility paths.
  • Add tests covering fresh strict execution and resume/ahead-state compatibility execution.

Type of Change

  • 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

  • npx prek run --all-files passes
  • npm test passes
  • Tests added or updated for new or changed behavior
  • 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 (doc changes only)
  • New doc pages include SPDX header and frontmatter (new pages only)

Signed-off-by: Carlos Villela cvillela@nvidia.com

Signed-off-by: Carlos Villela <cvillela@nvidia.com>
@cv cv self-assigned this May 29, 2026
@copy-pr-bot
Copy link
Copy Markdown

copy-pr-bot Bot commented May 29, 2026

Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually.

Contributors can view more details about this message here.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 29, 2026

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: fa2e9f74-ac33-4b4d-bdf7-cb360d4c6c61

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch stack/onboard-fsm-live-slice-helper

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions
Copy link
Copy Markdown
Contributor

E2E Advisor Recommendation

Required E2E: ubuntu-repo-cloud-openclaw, ubuntu-repo-cloud-openclaw-resume, ubuntu-repo-cloud-openclaw-repair
Optional E2E: onboard-inference-smoke-e2e

Dispatch hint: ubuntu-repo-cloud-openclaw,ubuntu-repo-cloud-openclaw-resume,ubuntu-repo-cloud-openclaw-repair

Workflow run

Full advisor summary

E2E Recommendation Advisor

Base: origin/stack/onboard-fsm-use-final-slice
Head: HEAD
Confidence: high

Required E2E

  • ubuntu-repo-cloud-openclaw (medium): Validates the fresh onboarding path that should take the strict slice runner when the machine is at the expected entry state.
  • ubuntu-repo-cloud-openclaw-resume (medium): Directly exercises interrupted onboarding followed by --resume, the main behavior affected by the compatibility execution branch in the new live flow slice helper.
  • ubuntu-repo-cloud-openclaw-repair (medium): Covers repair/backstop onboarding behavior for already-existing or drifted state, which is called out in the helper comment as relying on compatibility execution.

Optional E2E

  • onboard-inference-smoke-e2e (low): Useful additional regression coverage if the slice affects the final inference validation step, but the scenario runner baseline already gives primary end-to-end confidence.

New E2E recommendations

  • onboarding-resume-and-ahead-state-compatibility (medium): Existing resume and repair scenarios cover broad user flows, but there is no clearly named E2E that specifically seeds an already-advanced machine state and asserts all compatibility/backstop phase side effects are applied before completion.
    • Suggested test: Add a targeted onboarding scenario that resumes from an ahead-of-slice session state and verifies compatibility phase side effects and final session state.

Dispatch hint

  • Workflow: .github/workflows/e2e-scenarios.yaml
  • jobs input: ubuntu-repo-cloud-openclaw,ubuntu-repo-cloud-openclaw-resume,ubuntu-repo-cloud-openclaw-repair

@github-actions
Copy link
Copy Markdown
Contributor

E2E Scenario Advisor Recommendation

Required scenario E2E: None
Optional scenario E2E: None

Workflow run

Full scenario advisor summary

E2E Scenario Advisor

Base: origin/stack/onboard-fsm-use-final-slice
Head: HEAD
Confidence: high

Required scenario E2E

  • None. No scenario workflow, scenario metadata, scenario runtime, or validation-suite files changed.

Optional scenario E2E

  • None.

Relevant changed files

  • None.

@github-actions
Copy link
Copy Markdown
Contributor

PR Review Advisor

Findings: 0 needs attention, 5 worth checking, 0 nice ideas
Top item: Compatibility path should preserve strict FSM invariants

Review findings

🛠️ Needs attention

  • None.

🔎 Worth checking

  • Source-of-truth review needed: runLiveOnboardFlowSlice compatibility path: The advisor marked localized patch analysis as needs_followup.
    • Recommendation: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
    • Evidence: src/lib/onboard/machine/live-flow-slice.ts comment says compatibility is used for saved sessions that advanced beyond the slice; src/lib/onboard/machine/live-flow-slice.test.ts includes a fallback test but no root-boundary regression.
  • Compatibility execution can bypass strict runner invariants (src/lib/onboard/machine/live-flow-slice.ts:46): The fallback path executes every supplied phase directly and delegates result application to an arbitrary applyCompatibleResult callback. That differs from the strict runner, which selects handlers by current machine state, rejects empty result arrays, validates duplicate phases before execution, and applies results through the runtime transition checks. In live onboarding, these phases can include sandbox, provider, and policy side effects, so the helper should make the compatibility contract explicit and preserve the same safety invariants before it is wired into those flows.
    • Recommendation: Either route compatibility result application through the same runtime validation contract or constrain applyCompatibleResult's type/implementation expectations and add guards for empty result arrays and duplicate phases. Add tests proving invalid/empty/duplicate phase cases fail before side effects can silently proceed.
    • Evidence: src/lib/onboard/machine/live-flow-slice.ts runs `phase.run(nextContext)` for each phase and calls `applyCompatibleResult(result)`; nearby `runOnboardMachine` throws on empty handler results and `buildOnboardSequenceHandlers` rejects duplicate phase states.
  • Compatibility workaround lacks source-of-truth rationale (src/lib/onboard/machine/live-flow-slice.ts:30): The new helper documents a compatibility path for resume/ahead-state flows, but the diff does not establish where those ahead states are created, why that source cannot be fixed here, what regression test protects the source boundary, or when the workaround can be removed. This makes it hard to tell whether the helper is preserving a necessary migration behavior or normalizing an invalid state that should be impossible.
    • Recommendation: Document the source boundary for ahead-state sessions, why strict slice entry cannot be enforced there in this PR, and the removal/narrowing condition. Add a regression test around the source boundary or the real live-slice caller, not only a local unit test of the fallback branch.
    • Evidence: The comment says compatibility is used so phase bodies execute when a saved session has already advanced beyond the slice, and the test at live-flow-slice.test.ts line 58 covers the fallback branch, but no caller/source boundary is changed or tested.
  • Reusable extraction is not wired into an existing live flow (src/lib/onboard/machine/live-flow-slice.ts:27): The PR body says it extracts common live FSM slice execution policy into a reusable helper, but this diff only adds the helper and tests. No existing live onboarding slice is changed to call it, so the extraction/reuse part of the stated scope is only partially demonstrated.
    • Recommendation: Either wire the helper into at least one existing live slice in this PR, or narrow the PR description to say this only introduces the helper for a later caller.
    • Evidence: Repository inspection found references to runLiveOnboardFlowSlice only in the new helper and its new test file; existing slice helpers in flow-slices.ts still call runOnboardSequenceWithRunner directly.
  • Fallback tests do not cover failure and contract cases (src/lib/onboard/machine/live-flow-slice.test.ts:58): The tests cover the fresh strict path and one resume/ahead compatibility happy path, but they do not separately cover non-resume ahead-state behavior, resume at a matching state, result argument ordering, empty result arrays, duplicate phases, terminal/current-state mismatches, or whether applyCompatibleResult updates the runtime session returned by the helper.
    • Recommendation: Add focused unit tests for strict-vs-compatibility branch selection, ordered result arguments, empty and duplicate phase rejection or documented behavior, applyCompatibleResult failure propagation, and a runtime-backed validation test using the real OnboardRuntime contract.
    • Evidence: The compatibility test asserts applyCompatibleResult is called three times, but does not assert the applied result values/order or returned session effects; the mocked applyCompatibleResult returns undefined and never updates runtime.

🌱 Nice ideas

  • None.

Workflow run details

This is an automated advisory review. A human maintainer must make the final merge decision.

@cv cv added the onboarding Making the onboarding experience better label May 29, 2026
@Iamkewl
Copy link
Copy Markdown
Contributor

Iamkewl commented May 29, 2026

Unused helper — routing logic duplicated 3× in onboard.ts

runLiveOnboardFlowSlice is exported from live-flow-slice.ts but has zero production consumers — the only call sites are in live-flow-slice.test.ts. Meanwhile, onboard.ts reimplements the same !resume && state === X routing inline at three locations:

  • Line 6933 — initial slice (state === "preflight")
  • Line 7160 — core slice (state === "provider_selection")
  • Line 7372 — final slice (state === "openclaw" || "agent_setup")

The inline versions use recordStateResultWithStepCompatibility directly while this helper uses a generic applyCompatibleResult callback — functionally equivalent but code-level different.

Question: Is there a planned follow-up PR that wires onboard.ts to call runLiveOnboardFlowSlice? If so, could we note that in the PR description so reviewers know this is preparatory? If not, should the inline pattern be considered the intended long-term approach?

severity: Nit

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

onboarding Making the onboarding experience better

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants