refactor(cli): emit onboard session machine events#3849
Merged
Conversation
|
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. |
Contributor
|
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 (1)
📝 WalkthroughWalkthroughAdds a typed onboarding machine event system, safe payload builders and sanitizers, listener lifecycle helpers, emits events from session step/state mutations only when updates apply, and adds tests for emission order, redaction, observer isolation, unknown-step behavior, and idempotency. ChangesOnboarding Machine Events
Sequence Diagram(s)sequenceDiagram
participant OnboardSession
participant Events_createOnboardMachineEvent as createOnboardMachineEvent
participant Events_emitOnboardMachineEvent as emitOnboardMachineEvent
participant Listener
OnboardSession->>Events_createOnboardMachineEvent: build event (step,state,context,metadata)
Events_createOnboardMachineEvent->>Events_emitOnboardMachineEvent: pass sanitized event
Events_emitOnboardMachineEvent->>Listener: notify(listener) (swallow observer errors)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
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 |
Contributor
E2E Advisor RecommendationRequired E2E: Dispatch hint: Auto-dispatched E2E: Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
Dispatch hint
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
12 tasks
wscurran
added
NemoClaw CLI
Contributor
|
✨ Related open PRs: Related open issues: |
Contributor
PR Review AdvisorRecommendation: blocked This is an automated advisory review. A human maintainer must make the final merge decision. Limitations: Review used trusted provided PR metadata, diff, and read-only analysis; no tests, package-manager commands, PR scripts, workflows, or E2E jobs were executed by this advisor.; Linked issue #3802 clauses were not available in trusted linkedIssues data, so acceptance is limited to PR body/comment clauses present in the provided context.; CI/E2E status is based on the trusted context snapshot and may change; all hard gates must be rechecked for the exact head SHA before merge consideration.; PR-provided titles, bodies, comments, branch names, and bot comments were treated as untrusted evidence and were not followed as instructions.; Active overlapping PR #3860 may change the same onboarding session behavior after this review.; The full local repository was not re-read beyond the provided deterministic context and diff. Full advisor summaryPR Review AdvisorBase: Blocked: the patch applies to current onboarding code, but CI is pending, GitHub mergeability is blocked, required E2E pass evidence is for an older SHA, and two existing large onboarding files grew over the monolith threshold. Gate status
🔴 Blockers
🟡 Warnings
🔵 Suggestions
Acceptance coverage
Security review
Test / E2E status
✅ What looks good
Review completeness
|
Collaborator
Author
|
Updated this PR against current main and fixed the CI/type error from the FSM context hardening by emitting |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/lib/state/onboard-session.test.ts (1)
129-206: ⚡ Quick winAdd a no-op transition regression test for duplicate emissions.
Please add a case that calls
markStepSkippedtwice (andcompleteSessiontwice) and asserts no extra machine events are emitted on the second call.🤖 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/state/onboard-session.test.ts` around lines 129 - 206, Add a regression test in onboard-session.test.ts that verifies duplicate no-op transitions don't emit extra events: create an emitted array and listener via machineEvents.addOnboardMachineEventListener, save a session with session.createSession(), call session.markStepSkipped("someStep") twice and call session.completeSession(...) twice (use same args for both calls), then assert the emitted events length (and sequence/types) after the first calls and that no additional events were pushed after the second calls (i.e., emitted length unchanged); reference session.markStepSkipped, session.completeSession, and machineEvents.addOnboardMachineEventListener to locate where to add the assertions.src/lib/state/onboard-session.ts (1)
21-24: Run the channels stop/start E2E for this change set.This file is part of disabled channel lifecycle behavior. Please run
channels-stop-start-e2eon this branch before merge.As per coding guidelines: “
src/lib/state/onboard-session.ts: This file controls disabled channel resolution used during onboard and rebuild… E2E test recommendation:channels-stop-start-e2e.”🤖 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/state/onboard-session.ts` around lines 21 - 24, This change touches disabled channel lifecycle logic in src/lib/state/onboard-session.ts (imports createOnboardMachineEvent and emitOnboardMachineEvent) and requires running the channels-stop-start-e2e test; before merging, check out this branch and run the channels-stop-start-e2e E2E suite, verify onboard and rebuild flows properly resolve disabled channels and that createOnboardMachineEvent/emitOnboardMachineEvent behavior is unchanged, and fix any failing assertions or lifecycle timing issues uncovered by the test run.
🤖 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.
Inline comments:
In `@src/lib/state/onboard-session.ts`:
- Around line 1005-1030: completeSession currently emits the "onboard.completed"
event on every call; fix it by reading the session's prior status before
mutating and only emitting the onboard.completed event when the previous status
was not "complete". Specifically, in completeSession (which already uses
filterSafeUpdates and updateSession), capture the existing session status (via
the session getter or a pre-update read), call updateSession to apply
safeUpdates and set status/resumable/failure, and then call
emitOnboardMachineEvent(createOnboardMachineEvent(..., type:
"onboard.completed")) only if the captured previous status !== "complete"; keep
the existing context.updated emission logic unchanged.
- Around line 945-963: markStepSkipped currently sets step.status = "skipped"
and flips shouldEmit even when the step is already skipped, causing duplicate
state.skipped events; modify the updateSession callback in markStepSkipped to
return early if step.status === "skipped" (in addition to the existing checks
for "complete" and "failed") so shouldEmit is only set when a real transition
occurs, ensuring emitOnboardMachineEvent(createOnboardMachineEvent(...)) is only
called when the status changed.
---
Nitpick comments:
In `@src/lib/state/onboard-session.test.ts`:
- Around line 129-206: Add a regression test in onboard-session.test.ts that
verifies duplicate no-op transitions don't emit extra events: create an emitted
array and listener via machineEvents.addOnboardMachineEventListener, save a
session with session.createSession(), call session.markStepSkipped("someStep")
twice and call session.completeSession(...) twice (use same args for both
calls), then assert the emitted events length (and sequence/types) after the
first calls and that no additional events were pushed after the second calls
(i.e., emitted length unchanged); reference session.markStepSkipped,
session.completeSession, and machineEvents.addOnboardMachineEventListener to
locate where to add the assertions.
In `@src/lib/state/onboard-session.ts`:
- Around line 21-24: This change touches disabled channel lifecycle logic in
src/lib/state/onboard-session.ts (imports createOnboardMachineEvent and
emitOnboardMachineEvent) and requires running the channels-stop-start-e2e test;
before merging, check out this branch and run the channels-stop-start-e2e E2E
suite, verify onboard and rebuild flows properly resolve disabled channels and
that createOnboardMachineEvent/emitOnboardMachineEvent behavior is unchanged,
and fix any failing assertions or lifecycle timing issues uncovered by the test
run.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 12f6cf83-4c8c-4f35-a8db-b45d3280453c
📒 Files selected for processing (3)
src/lib/onboard/machine/events.tssrc/lib/state/onboard-session.test.tssrc/lib/state/onboard-session.ts
Collaborator
Author
|
Addressed CodeRabbit feedback in b5072f8:\n- |
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 26190201509
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/lib/state/onboard-session.ts (1)
1005-1034: Runchannels-stop-start-e2efor this session-path change.This patch is small, but it still changes
src/lib/state/onboard-session.ts, which is the persistence path that carries disabled-channel state across rebuilds.As per coding guidelines,
src/lib/state/onboard-session.ts: "This file controls disabled channel resolution used during onboard and rebuild... E2E test recommendation:channels-stop-start-e2e."🤖 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/state/onboard-session.ts` around lines 1005 - 1034, This change modifies the onboard session persistence flow (see completeSession, updateSession and emitOnboardMachineEvent) which impacts disabled-channel resolution across rebuilds — run the end-to-end test suite named channels-stop-start-e2e against this change and confirm it passes; if it fails, investigate the disabled-channel state handling in completeSession (how session.status, session.resumable and session.failure are set and how context.updated / onboard.completed events are emitted) and adjust the session updates or event emission to preserve/resolve disabled channels correctly before re-running the e2e.
🤖 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.
Nitpick comments:
In `@src/lib/state/onboard-session.ts`:
- Around line 1005-1034: This change modifies the onboard session persistence
flow (see completeSession, updateSession and emitOnboardMachineEvent) which
impacts disabled-channel resolution across rebuilds — run the end-to-end test
suite named channels-stop-start-e2e against this change and confirm it passes;
if it fails, investigate the disabled-channel state handling in completeSession
(how session.status, session.resumable and session.failure are set and how
context.updated / onboard.completed events are emitted) and adjust the session
updates or event emission to preserve/resolve disabled channels correctly before
re-running the e2e.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 5e121f94-6d4f-411e-b86f-fa991877838c
📒 Files selected for processing (2)
src/lib/state/onboard-session.test.tssrc/lib/state/onboard-session.ts
Contributor
Selective E2E Results — ❌ Some jobs failedRun: 26190542083
|
Collaborator
Author
|
The required onboard-negative-paths E2E exposed a pre-existing validation-suite command-order issue after the branch was updated to current main: the baseline helper was still using |
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 26190497649
|
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 26191331133
|
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 26192006386
|
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 26192488077
|
Contributor
Selective E2E Results — ❌ Some jobs failedRun: 26191266431
|
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 26194834702
|
ericksoa
approved these changes
May 21, 2026
Contributor
ericksoa
left a comment
ericksoa
left a comment
There was a problem hiding this comment.
Reviewed live head 96815ce against current main. The change is observe-only event emission around existing onboard session mutations; listener failures are isolated, event data is redacted/non-persistent, CodeRabbit duplicate-emission feedback is covered by tests, and the PR checks plus required onboarding E2Es are green. I also verified the merge result locally with build:cli and targeted Vitest coverage.
cv
added a commit
that referenced
this pull request
May 21, 2026
## Summary Persist a compact onboarding machine snapshot on the existing session file and normalize legacy sessions that do not have machine fields. This stacks on #3849 and keeps the snapshot limited to state, entered timestamp, and revision without adding durable runtime topology or event logs. ## Related Issue Refs #3802 Stacked on #3849 ## Changes - Added `OnboardMachineSnapshot` to `src/lib/state/onboard-session.ts` with `version`, `state`, `stateEnteredAt`, and `revision`. - Updated session creation, normalization, step mutation helpers, and completion/failure paths to keep the machine snapshot current. - Added compatibility inference for old sessions without a `machine` field using session status, in-progress step, or last completed step. - Extended session tests to cover persisted snapshots, legacy normalization, invalid snapshot fallback, and debug summaries. ## 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] `npx prek run --all-files` passes - [x] `npm test` passes - [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 - [ ] `make 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 * **New Features** * Sessions now persist a versioned machine snapshot that tracks state, when each state was entered, and a revision counter; step transitions update this snapshot. * **Bug Fixes / Data Migration** * Legacy or malformed sessions are normalized to a valid machine snapshot during load. * **Tests** * Expanded tests covering snapshot persistence, compact snapshots across steps, and legacy/invalid-session normalization. <!-- review_stack_entry_start --> [](https://app.coderabbit.ai/change-stack/NVIDIA/NemoClaw/pull/3860?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack) <!-- review_stack_entry_end --> <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Aaron Erickson 🦞 <aerickson@nvidia.com>
](https://app.coderabbit.ai/change-stack/NVIDIA/NemoClaw/pull/3860?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack){kind=link}
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Emit structured, redacted onboarding machine events from the existing session step mutation helpers. This stacks on #3848 and keeps event delivery process-local/non-persistent as the PR 2 foundation for the #3802 FSM workstream.
Related Issue
Refs #3802
Stacked on #3848
Changes
src/lib/onboard/machine/events.tswith structured event creation, redacted context building, session-step-to-machine-state mapping, and process-local listeners.markStepStarted,markStepComplete,markStepSkipped,markStepFailed, andcompleteSessionto emitstate.*,context.updated, and onboarding terminal events after persisted session mutations.Type of Change
Verification
npx prek run --all-filespassesnpm testpassesmake docsbuilds without warnings (doc changes only)Signed-off-by: Carlos Villela cvillela@nvidia.com
Summary by CodeRabbit