refactor(onboard): give post-verify its own phase - #7673
Merged
cv merged 5 commits intoJul 28, 2026
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
📝 WalkthroughWalkthroughOnboarding FSM wiring now uses explicit core phase objects, strict initial and final slice execution, prerequisite repair events, and a separate post-verification state handler. Runtime-boundary hooks for initial preflight and post-verify startup are removed, with updated tests and lifecycle documentation. ChangesOnboarding FSM flow
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Onboard
participant CoreFlow
participant FinalFlow
participant RuntimeBoundary
Onboard->>CoreFlow: run provider inference and sandbox phases
CoreFlow-->>Onboard: return updated onboarding context
Onboard->>FinalFlow: run final phases and prerequisite repairs
FinalFlow->>RuntimeBoundary: record repair and state events
FinalFlow-->>Onboard: complete or pause post_verify
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in commit 34df2af in the TypeScript / code-coverage/cliThe overall coverage in commit 34df2af in the Show a code coverage summary of the most impacted files.
Updated |
Contributor
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported Model lanes
Nemotron output stays in workflow artifacts and does not change the assessment above. E2E guidanceAdvisory only. E2E / PR Gate selects and runs jobs independently. Recommended E2E: 1 optional E2E recommendation
This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
23 tasks
<!-- markdownlint-disable MD041 --> ## Summary This cumulative stack change replaces final-flow compatibility replay with explicit prerequisite repairs, gives the strict FSM runner transition authority at exact `init`, `preflight`, and `provider_selection` entries, and separates provider/inference construction from sandbox construction. Saved final states at `policies`, `finalizing`, or `post_verify` run earlier phases as evented, update-free repairs before strict entry. Initial and core snapshots already downstream retain compatibility repair. CLI commands, terminal output, configuration, persisted contracts, and supported onboarding behavior are unchanged. Stack layers 3–6 of 6. Base: `codex/onboard-fsm-post-verify-phase`. ## Related Issue Refs #6224 ## Changes - Canonicalize and validate the four final-flow phases before any state read or effect. - Run only earlier branch, policy, or finalization phases as prerequisite repairs for downstream saved states. - Emit `state.repair.started`, followed by `state.repair.completed` or `state.repair.failed`, for each repair. - Reject repairs that update the session, return multiple results, transition incorrectly, or change the durable entry state. - Remove the entrypoint-owned `init -> preflight` transition and let the strict initial-flow runner own exact `init` and `preflight` entry. - Split the combined core-phase factory into provider/inference and sandbox factories with phase-specific dependencies. - Give the strict runner exact `provider_selection` ownership while preserving downstream resume compatibility. - Preserve rollback timing, repaired context propagation, endpoint provenance, and explicit stale-result invalidation. - Update the lifecycle contract and strengthen final-flow, initial-flow, core-flow, runtime-boundary, exit-handler, and live-slice tests. ## Type of Change - [ ] Code change (feature, bug fix, or refactor) - [x] Code change with doc updates - [ ] Doc only (prose changes, no code sample modifications) - [ ] Doc only (includes code sample changes) ## Quality Gates - [x] Tests added or updated for changed behavior - [ ] Existing tests cover changed behavior — justification: - [ ] Tests not applicable — justification: - [ ] Docs updated for user-facing behavior changes - [x] Docs not applicable — justification: The internal repair contract and lifecycle ownership map are updated; commands, output, configuration, persisted contracts, and supported onboarding behavior are unchanged. - [x] Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging) - [x] Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: Independent Codex review of the exact six-commit stack found no actionable findings and verified strict entry, compatibility bounds, state durability, recovery, rollback, and fail-closed behavior. - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## Documentation Writer Review - [x] Documentation writer subagent reviewed the completed changes - Result: `docs-updated` - Evidence: `src/lib/onboard/lifecycle-contracts.md` documents the final-flow prerequisite repair lifecycle, canonical update-free advances, unchanged durable entry state, unapplied repair transitions, and repaired context passed to the strict runner. It also assigns `init` to the synthetic phase applied by the strict initial-flow runner. The core construction split and exact `provider_selection` runner ownership are internal and require no user-documentation changes. - Agent: Codex Desktop documentation-writer subagent <!-- docs-review-head-sha: e5ba515 --> <!-- docs-review-agents-blob-sha: be20a09 --> ## DGX Station Hardware Evidence - [ ] Tested on DGX Station - Tested commit: Not applicable - Station profile/scenario: Not applicable - Result: Not applicable; `scripts/prepare-dgx-station-host.sh` is unchanged. - Supporting evidence: Not applicable ## Verification - [x] PR description includes a `Signed-off-by:` line and every commit appears as `Verified` in GitHub - [x] Normal `pre-commit`, `commit-msg`, and `pre-push` hooks passed, or `npm run check:diff` passed when hooks were skipped or unavailable - [x] Targeted behavior tests pass for the current change set, or tests are marked not applicable above — exact stack-head FSM selection: PASS (9 files, 112 tests); focused CLI Vitest: PASS (18 tests); focused integration Vitest: PASS (12 tests); CLI type-check: PASS. - [x] Applicable broad gate passed — exact-head GitHub CLI, platform, security, and selected live E2E gates passed; `cloud-onboard`, `onboard-repair`, and `onboard-resume` all succeeded. - [x] Quality Gates section completed with required justifications or waivers - [x] No secrets, API keys, or credentials committed - [ ] `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> --------- Signed-off-by: Carlos Villela <cvillela@nvidia.com>
cv
marked this pull request as ready for review
July 28, 2026 03:15
Collaborator
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/lib/onboard/machine/final-flow-phases.runtime.test.ts (1)
134-155: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefer literal expectations over recomputing the production slice logic.
prerequisiteStatesre-derives the repair set with the same index arithmeticrunFinalFlowPrerequisiteRepairsuses, so a mistake in the ordering constant can be mirrored on both sides. The case table already knows its answer — carrying an explicitexpectedRepairsper row (as thestate.exitedassertion at Lines 157-165 already does) keeps the test independent of the implementation.As per path instructions, "Flag copied production algorithms, broad mocks that bypass the behavior under test".
🤖 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/onboard/machine/final-flow-phases.runtime.test.ts` around lines 134 - 155, Replace the computed prerequisiteStates expectation in the final-flow test with an explicit expectedRepairs value in each case-table row. Update the recordRepairEvent assertion to use that fixture data, matching the existing literal state.exited expectation pattern and removing the duplicated slice/index logic.Source: Path instructions
src/lib/onboard/machine/initial-flow-phases.ts (1)
197-231: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueComment's "tolerated downstream family" no longer matches the list.
The prose enumerates "inference, sandbox, openclaw/agent_setup, policies, finalizing, and post_verify", but
compatibilityWhenStatealso includesgatewayandprovider_selection(and those two are the entire non-resume list). Worth aligning so the rationale stays trustworthy after the init/preflight removal.♻️ Suggested wording tweak
- // The tolerated downstream family is every nonterminal state after the initial - // slice: inference, sandbox, openclaw/agent_setup, policies, finalizing, and - // post_verify. Phase tests cover ahead-state resume and terminal-state + // The tolerated family is every nonterminal state after the `init`/`preflight` + // entries: gateway, provider_selection, inference, sandbox, + // openclaw/agent_setup, policies, finalizing, and post_verify. + // Phase tests cover ahead-state resume and terminal-state🤖 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/onboard/machine/initial-flow-phases.ts` around lines 197 - 231, The comments above runLiveOnboardFlowSlice describe only downstream resume states, but compatibilityWhenState also includes gateway and provider_selection. Update the rationale to explicitly include these states and accurately distinguish the resume and non-resume compatibility lists; leave the state arrays and behavior unchanged.
🤖 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/onboard/machine/final-flow-phases.runtime.test.ts`:
- Around line 134-155: Replace the computed prerequisiteStates expectation in
the final-flow test with an explicit expectedRepairs value in each case-table
row. Update the recordRepairEvent assertion to use that fixture data, matching
the existing literal state.exited expectation pattern and removing the
duplicated slice/index logic.
In `@src/lib/onboard/machine/initial-flow-phases.ts`:
- Around line 197-231: The comments above runLiveOnboardFlowSlice describe only
downstream resume states, but compatibilityWhenState also includes gateway and
provider_selection. Update the rationale to explicitly include these states and
accurately distinguish the resume and non-resume compatibility lists; leave the
state arrays and behavior unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 4cd08eaa-fba1-4fcd-9d35-355c65627646
📒 Files selected for processing (17)
src/lib/onboard.tssrc/lib/onboard/lifecycle-contracts.mdsrc/lib/onboard/machine/core-flow-phases.test.tssrc/lib/onboard/machine/core-flow-phases.tssrc/lib/onboard/machine/final-flow-phases.runtime.test.tssrc/lib/onboard/machine/final-flow-phases.test.tssrc/lib/onboard/machine/final-flow-phases.tssrc/lib/onboard/machine/handlers/finalization.test.tssrc/lib/onboard/machine/handlers/finalization.tssrc/lib/onboard/machine/initial-flow-phases.test.tssrc/lib/onboard/machine/initial-flow-phases.tssrc/lib/onboard/runtime-boundary.test.tssrc/lib/onboard/runtime-boundary.tstest/credential-migration-reconciliation.test.tstest/helpers/onboard-final-flow-phases.tstest/onboard-exit-handler.test.tstest/onboard-fsm-live-slices.test.ts
💤 Files with no reviewable changes (3)
- test/credential-migration-reconciliation.test.ts
- test/helpers/onboard-final-flow-phases.ts
- src/lib/onboard/runtime-boundary.ts
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Collaborator
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Collaborator
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Collaborator
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Collaborator
Author
|
Nemotron warning disposition for exact head |
cv
added a commit
that referenced
this pull request
Jul 28, 2026
<!-- markdownlint-disable MD041 --> ## Summary This cumulative change gives `post_verify` its own final-flow phase, replaces final-flow compatibility replay with explicit prerequisite repairs, gives the strict FSM runner transition authority at exact `init`, `preflight`, and `provider_selection` entries, and separates provider/inference construction from sandbox construction. `finalizing` performs final preparation and advances; `post_verify` owns deployment verification and completion or pause. Saved final states run earlier phases as evented, update-free repairs before strict entry. Initial and core snapshots already downstream retain compatibility repair. CLI commands, terminal output, configuration, persisted contracts, and supported onboarding behavior are unchanged. The lifecycle map now also reconciles completed checkpoint and recovery issues with the remaining cross-effect gaps. This is the final cumulative result of all six reviewed stack layers. ## Related Issue Refs #6224 ## Changes - Split `handleFinalizationState` and `handlePostVerifyState` at the existing FSM boundary. - Add a `post_verify` sequence phase and remove the bespoke runtime-boundary mutation. - Canonicalize final-flow phases and replace downstream compatibility replay with validated, evented prerequisite repairs. - Keep repair transitions unapplied and the durable entry state unchanged until strict entry. - Remove the entrypoint-owned `init -> preflight` transition and give the strict initial runner exact `init` and `preflight` ownership. - Split provider/inference and sandbox phase construction into effect-specific factories. - Give the strict runner exact `provider_selection` ownership while preserving downstream resume compatibility. - Preserve rollback timing, repaired context propagation, endpoint provenance, and explicit stale-result invalidation. - Update the lifecycle contract and strengthen finalization, repair, initial-flow, core-flow, runtime-boundary, credential-reconciliation, exit-handler, and live-slice tests. - Reconcile completed recovery issues, checkpoint replay coverage, and unresolved cross-effect ownership in the lifecycle map. ## Type of Change - [ ] Code change (feature, bug fix, or refactor) - [x] Code change with doc updates - [ ] Doc only (prose changes, no code sample modifications) - [ ] Doc only (includes code sample changes) ## Quality Gates - [x] Tests added or updated for changed behavior - [ ] Existing tests cover changed behavior — justification: - [ ] Tests not applicable — justification: - [ ] Docs updated for user-facing behavior changes - [x] Docs not applicable — justification: The internal lifecycle ownership map and repair contract are updated; commands, output, configuration, persisted contracts, and supported onboarding behavior are unchanged. - [x] Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging) - [x] Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: Independent Codex review of the exact cumulative change found no actionable findings and verified strict entry, compatibility bounds, state durability, recovery, rollback, and fail-closed behavior. Nemotron's unpublished lifecycle-map warning was stale against the reviewed tree; disposition: #7673 (comment). - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## Documentation Writer Review - [x] Documentation writer subagent reviewed the completed changes - Result: `docs-updated` - Evidence: `src/lib/onboard/lifecycle-contracts.md` accurately documents strict initial and final runner ownership, distinct `finalizing` and `post_verify` handlers, and evented update-free prerequisite repairs while preserving the reconciled checkpoint/recovery status and remaining cross-effect gaps. Adjacent machine and user-facing resume documentation remain consistent. No CLI contract, terminal-output, persistence, or supported-behavior change requires additional user-facing documentation. - Agent: Codex Desktop documentation-writer subagent <!-- docs-review-head-sha: af9e5bc --> <!-- docs-review-agents-blob-sha: be20a09 --> ## DGX Station Hardware Evidence - [ ] Tested on DGX Station - Tested commit: Not applicable - Station profile/scenario: Not applicable - Result: Not applicable; `scripts/prepare-dgx-station-host.sh` is unchanged. - Supporting evidence: Not applicable ## Verification - [x] PR description includes a `Signed-off-by:` line and every commit appears as `Verified` in GitHub - [x] Normal `pre-commit`, `commit-msg`, and `pre-push` hooks passed, or `npm run check:diff` passed when hooks were skipped or unavailable - [x] Targeted behavior tests pass for the current change set, or tests are marked not applicable above — exact cumulative-tree FSM selection: PASS (9 files, 112 tests); focused CLI Vitest: PASS (18 tests); focused integration Vitest: PASS (12 tests); CLI type-check: PASS. Comment-only follow-up: normal hooks and CLI pre-push typecheck PASS; Biome PASS; initial-flow Vitest PASS (17 tests). Lifecycle-map follow-up: markdownlint PASS; `npm run docs` PASS with zero errors and two pre-existing Fern warnings; normal hooks PASS. Base refresh: focused FSM/integration PASS (9 files, 112 tests); CLI type-check PASS; merge-commit and pre-push hooks PASS. - [x] Applicable broad gate passed — promoted head `af9e5bc8` is tree-identical to fully gated head `34df2af2`. Credentialed `cloud-onboard`, `onboard-repair`, and `onboard-resume` E2Es passed. The one allowed build/package-contract retry passed after a non-reproducible CommonJS initialization error; the exact local full package-contract project passed 26 files and 327 tests. - [x] Quality Gates section completed with required justifications or waivers - [x] No secrets, API keys, or credentials committed - [ ] `npm run docs` builds without warnings (doc changes only) — Exited 0 with zero errors; Fern reported two pre-existing warnings. - [x] 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> --------- Signed-off-by: Carlos Villela <cvillela@nvidia.com>
This was referenced Jul 29, 2026
wscurran
added
area: onboarding
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
3 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
This cumulative stack change gives
post_verifyits own final-flow phase, replaces final-flow compatibility replay with explicit prerequisite repairs, gives the strict FSM runner transition authority at exactinit,preflight, andprovider_selectionentries, and separates provider/inference construction from sandbox construction.finalizingperforms final preparation and advances;post_verifyowns deployment verification and completion or pause. Saved final states run earlier phases as evented, update-free repairs before strict entry. Initial and core snapshots already downstream retain compatibility repair. CLI commands, terminal output, configuration, persisted contracts, and supported onboarding behavior are unchanged. The lifecycle map now also reconciles completed checkpoint and recovery issues with the remaining cross-effect gaps.Stack layers 2–6 of 6. Base:
codex/onboard-fsm-final-entry.Related Issue
Refs #6224
Changes
handleFinalizationStateandhandlePostVerifyStateat the existing FSM boundary.post_verifysequence phase and remove the bespoke runtime-boundary mutation.init -> preflighttransition and give the strict initial runner exactinitandpreflightownership.provider_selectionownership while preserving downstream resume compatibility.Type of Change
Quality Gates
Documentation Writer Review
docs-updatedsrc/lib/onboard/lifecycle-contracts.mdassignsfinalizingtohandleFinalizationStateandpost_verifytohandlePostVerifyState. It documents final-flow prerequisite repairs, canonical update-free advances, unchanged durable entry state, unapplied repair transitions, and repaired context passed to the strict runner. It also assignsinitto the synthetic phase applied by the strict initial-flow runner. The core construction split and exactprovider_selectionownership are internal and require no user-documentation changes. The initial-flow rationale now enumerates the complete downstream compatibility family and limits non-resume compatibility togatewayandprovider_selection. The lifecycle map records completed checkpoint and recovery work, current replay coverage, and the still-unresolved cross-effect and atomic-swap gaps without assigning closed issues as current owners.DGX Station Hardware Evidence
scripts/prepare-dgx-station-host.shis unchanged.Verification
Signed-off-by:line and every commit appears asVerifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run check:diffpassed when hooks were skipped or unavailablenpm run docsPASS with zero errors and two pre-existing Fern warnings; normal hooks PASS. Base refresh: focused FSM/integration PASS (9 files, 112 tests); CLI type-check PASS; merge-commit and pre-push hooks PASS.cloud-onboard,onboard-repair, andonboard-resumeE2Es passed. The one allowed build/package-contract retry passed after a non-reproducible CommonJS initialization error; the exact local full package-contract project passed 26 files and 327 tests.npm run docsbuilds without warnings (doc changes only) — Exited 0 with zero errors; Fern reported two pre-existing warnings.Signed-off-by: Carlos Villela cvillela@nvidia.com