Skip to content

refactor(onboard): apply explicit state results through runtime#4376

Draft
cv wants to merge 1 commit into
stack/onboard-fsm-state-resultsfrom
stack/onboard-fsm-apply-result
Draft

refactor(onboard): apply explicit state results through runtime#4376
cv wants to merge 1 commit into
stack/onboard-fsm-state-resultsfrom
stack/onboard-fsm-apply-result

Conversation

@cv
Copy link
Copy Markdown
Collaborator

@cv cv commented May 28, 2026

Summary

Teach OnboardRuntime how to apply explicit state handler results. Transition results are validated against the FSM graph and expected transition kind before any context updates are persisted.

Changes

  • Add OnboardRuntime.applyResult() for transition, complete, and failed state results.
  • Validate optional expected transition kinds such as retry and branch before mutating session context.
  • Extend runtime tests for advance, retry, branch, completion, failure, and invalid transition-kind handling.

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 28, 2026
@copy-pr-bot
Copy link
Copy Markdown

copy-pr-bot Bot commented May 28, 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 28, 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: 3b77b720-49dc-4b5b-bd6b-571eccbd87c6

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-apply-result

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: cloud-onboard-e2e
Optional E2E: onboard-resume-e2e, onboard-negative-paths-e2e

Dispatch hint: cloud-onboard-e2e,onboard-resume-e2e,onboard-negative-paths-e2e

Workflow run

Full advisor summary

E2E Recommendation Advisor

Base: origin/stack/onboard-fsm-state-results
Head: HEAD
Confidence: medium

Required E2E

  • cloud-onboard-e2e (high): Runs the real non-interactive cloud onboarding flow and verifies that core onboarding/session completion still works after changes to the onboarding FSM runtime.

Optional E2E

  • onboard-resume-e2e (high): Useful adjacent coverage for durable onboarding session state, skipped/cached phases, and completion after an interrupted run; applyResult is part of the same runtime/session boundary even though current live handlers may not yet call it directly.
  • onboard-negative-paths-e2e (high): Exercises onboarding validation and failure/edge paths, providing confidence that runtime transition/failure changes do not surface stack traces or unsafe user-visible behavior.

New E2E recommendations

  • onboarding FSM state-result application (medium): Existing E2E jobs validate live onboarding flows, but the new OnboardRuntime.applyResult API is not yet directly exercised through a real state-handler runner path. Add coverage once handlers return OnboardStateResult in production flow to assert advance/retry/branch/complete/failed results persist expected state and redact metadata in artifacts.
    • Suggested test: Add an E2E scenario that drives a machine-handler-based onboard run through explicit OnboardStateResult transitions, including an invalid transition-kind failure that proves session context is not mutated.

Dispatch hint

  • Workflow: E2E / Nightly
  • jobs input: cloud-onboard-e2e,onboard-resume-e2e,onboard-negative-paths-e2e

@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-state-results
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, 1 worth checking, 0 nice ideas
Top item: Preserve completion-result metadata

Review findings

🛠️ Needs attention

  • None.

🔎 Worth checking

  • Completion results drop their metadata (src/lib/onboard/machine/runtime.ts:203): `OnboardStateCompleteResult` and `completeOnboardMachine()` both carry optional `metadata`, but `applyResult()` delegates completion to `this.complete(result.updates ?? {})` without passing that metadata. Transition and failure results preserve metadata into emitted runtime events, so completion handlers using the same result contract would lose diagnostic context silently.
    • Recommendation: Either extend `complete()` to accept runtime options and pass `result.metadata` through to its `context.updated`, `state.completed`, `state.entered`, and/or `onboard.completed` events as appropriate, or remove `metadata` from complete results if completion metadata is intentionally unsupported. Add a test covering `completeOnboardMachine(..., metadata)` through `applyResult()`.
    • Evidence: In `applyResult()`, the complete branch returns `this.complete(result.updates ?? {})`; nearby branches pass `result.metadata` to `fail()` and `transition()`. `result.ts` defines `OnboardStateCompleteResult.metadata` and `completeOnboardMachine(updates, metadata)`.

🌱 Nice ideas

  • None.

Workflow run details

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

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant