refactor(onboard): make runtime the machine authority - #7716
Conversation
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
|
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. |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in commit 8da6eb3 in the TypeScript / code-coverage/cliThe overall coverage in commit 8da6eb3 in the Show a code coverage summary of the most impacted files.
Updated |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe onboarding runtime now owns machine transitions, revisions, terminal state, events, and result validation. Session helpers persist step status and safe data. Legacy step-mutation APIs and live-flow slice execution were removed. ChangesOnboarding runtime authority
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Handler
participant OnboardRuntimeBoundary
participant OnboardRuntime
participant OnboardSession
Handler->>OnboardRuntimeBoundary: Return explicit state result
OnboardRuntimeBoundary->>OnboardRuntime: Validate result metadata and graph transition
OnboardRuntime->>OnboardSession: Persist step status and safe updates
OnboardRuntime->>OnboardRuntime: Apply machine transition and emit events
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported Model lanes
4 terminology differences from the second opinionAdvisory only. These are normalized differences from the primary terminology receipt.
5 additional E2E selections from the second opinionAdvisory only. The primary lane did not select these E2E jobs or targets.
Second-opinion terminology and E2E selections are advisory. They do not change the primary assessment or E2E / PR Gate. 2 semantic terminology decisionsTerminology decisions are advisory. They affect the assessment only when a separate finding identifies concrete semantic impact.
E2E guidanceAdvisory only. E2E / PR Gate selects and runs jobs independently. Recommended E2E: This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
wscurran
added
area: architecture
|
Draft-stack babysitting status for exact head 90dd9cd (plain comment; no Changes Requested review): this PR depends on #7715 but its recorded base is 4788d97 while #7715 is now at 8ec0d88. The draft has no current CI evidence, sensitive-path review is still marked pending, auto-sync is disabled, and maintainer edits are disabled. I will review and gate a refreshed quiet stack after #7715 is resolved and this base is updated. |
|
Security review for commit SHA 6a7a061: PASS.
Issue #7703 records the accepted scope. No security finding blocks merge. Repository checks and required reviews for this commit remain separate merge gates. |
Security review — PASSReviewed PR #7716 at commit The change removes the legacy step-owned machine-mutation path, centralizes result validation in FindingsNo findings. Detailed analysis
Files reviewed
|
Security review — PASSReviewed PR #7716 at commit bcd3c03 against base c614c0d and issue #7703. The merge resolution preserves the prior PR delta. No security finding remains.
Required GitHub checks remain a separate gate. |
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Clarify the source-shape contract and session step bookkeeping. Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Security review receipt — PASSReviewed commit SHA
Issue #7703 establishes the accepted internal refactor scope. This PR does not create a new supported surface or user-visible onboarding behavior. The final commit changes two explanatory lines after the prior PASS: The confirmed #8106 Required GitHub checks for commit SHA |
Use one term for step helper bookkeeping across the internal authority contracts. Signed-off-by: Carlos Villela <cvillela@nvidia.com>
|
Security review receipt Reviewed commit SHA Verdict: PASS. The PR-specific diff has no security finding that prevents approval.
Accepted issue #7703 establishes product scope for this change. The listener-ownership finding introduced by PR #8106 is present in the base SHA and does not originate in this PR diff. |
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Sensitive-path security review: PASSReviewed commit SHA No security findings remain for this SHA pair.
Security categories:
The skipped-step remediation reads the known step's prior status, records the status change, and emits exactly once only when the resulting status is This PASS covers the sensitive-path security review only. Required checks and required reviews remain separate merge requirements. |
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
apurvvkumaria
left a comment
apurvvkumaria
left a comment
There was a problem hiding this comment.
Reviewed current head 7dfbb44. Runtime ownership of snapshot and state mutation is consistent, and transition filtering plus event emission preserve the intended behavior. Focused testing passed 56 assertions; one suite load issue was due to a missing generated dist artifact in the temporary review worktree rather than a failing product assertion. I found no blocking defect.
<!-- markdownlint-disable MD041 --> ## Summary Stack 3 of 5; depends on #7716. Move initial-to-core and core-to-final context validation into `flow-handoff.ts`. The entrypoint retains dependency wiring instead of constructing handoff context. ## Related Issue Refs #7706 ## Changes - Validate preflight GPU output before core execution. - Preserve sandbox-name precedence and reserved-name reporting. - Validate sandbox identity and inference output before final execution. - Document the extracted handoff owner. ## Type of Change - [x] Code change with doc updates ## Quality Gates - [x] Tests added or updated for changed behavior - [x] Documentation not applicable — No supported command, configuration, default, workflow, or user-visible behavior changes. The internal ownership README was updated. - [x] Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging) - [x] Sensitive-path review completed — Independent nine-category review passed for commit SHA `74d024849fb23fca25c339f567220a937298d6c0` against base SHA `7c83651249b7a53a847a317736aa59e46ded8101`: #7717 (comment) ## Documentation Writer Review - [x] Documentation writer subagent reviewed the completed changes - Result: `docs-updated` - Evidence: Independent Codex Desktop documentation writer review passed for commit SHA `74d024849fb23fca25c339f567220a937298d6c0` against base SHA `7c83651249b7a53a847a317736aa59e46ded8101`. Reviewed all five changed files against `WRITING.md`, the NemoClaw Controlled Word List, the shared documentation-writing contract, and the documentation contributor guidance. `src/lib/onboard/machine/README.md` accurately states that `flow-handoff.ts` validates required data and constructs context at the initial-to-core and core-to-final boundaries. The entrypoint continues to supply process-bound dependencies and reserved-name output. Changed errors and behavior-oriented test titles accurately describe preflight GPU validation, sandbox-name precedence, required final context, and preservation of runner results and sessions. Issue #7706 defines this internal refactor scope and prohibits a new public API or speculative extension point. The change does not modify a supported command, output, configuration, workflow, default, or other user-visible behavior, so no user-facing `docs/` source change is required. - Agent: Codex Desktop <!-- docs-review-head-sha: 74d0248 --> <!-- docs-review-agents-blob-sha: 3dd7c24 --> ## DGX Station Hardware Evidence - [ ] Tested on DGX Station - Result: Not applicable; this PR does not change `scripts/prepare-dgx-station-host.sh`. ## 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 — CI / Pull Request test jobs passed for commit SHA `74d024849fb23fca25c339f567220a937298d6c0`. - [x] Applicable broad gate passed — Required check `E2E / PR Gate` passed for commit SHA `74d024849fb23fca25c339f567220a937298d6c0`. - [x] Quality Gates section completed with required justifications or waivers - [x] No secrets, API keys, or credentials committed - [x] `npm run docs` builds without warnings (documentation changes only) — Not applicable; no published documentation source changed. - [x] Doc pages follow the style guide (doc changes only) --- Signed-off-by: Carlos Villela <cvillela@nvidia.com> --------- Signed-off-by: Carlos Villela <cvillela@nvidia.com>
apurvvkumaria
left a comment
apurvvkumaria
left a comment
There was a problem hiding this comment.
Approve — re-reviewed exact head 49f779c after the flow-handoff extraction and main refresh. The extracted helpers preserve sandbox-name precedence, runtime session authority, selected channels, normalized GPU state, and the full final-flow context while adding explicit validation for required handoff fields. The merge resolution also retains the current source-budget changes. Exact-head handoff and context-focused tests passed 44 of 44. The one shared CI shard failure is the same managed-image registry fetch timeout seen on an unrelated PR and is not attributable to this delta. No blocking defect found.
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
6 participants
Summary
Make
OnboardRuntimethe authority for normal machine transitions, revisions, terminal state, and machine events. Extract initial-to-core and core-to-final context handoffs from the onboarding entrypoint.Related Issue
Related to #7703 and #7706
Changes
OnboardRuntime.filterSafeUpdates. Prevent them from changing the machine snapshot.state.skippedevent fromOnboardRuntimewhen a pending or in-progress step becomes skipped.flow-handoff.tswhile retaining process-bound reserved-name handling in the entrypoint.Type of Change
Scope Review
PASSverifyGpuSandboxLocalInferenceAndCommitAfterReadyblock remains unchanged by this PR. It still waits for GPU verification before committing the deferred runtime patch.Quality Gates
8da6eb3cc3ccf502547b9b08dbdf28dbf48a4081against1ee723a3c0fd61fa6c8217b81035208ef5a68e09with no findings.Documentation Writer Review
docs-updated8da6eb3cc3ccf502547b9b08dbdf28dbf48a4081against1ee723a3c0fd61fa6c8217b81035208ef5a68e09.src/lib/onboard/lifecycle-contracts.mdandsrc/lib/onboard/machine/README.mdmatch the implementation:OnboardRuntimeowns normal machine transitions, terminal state, and machine events; step helpers update step status and context accepted byfilterSafeUpdateswithout changing the machine snapshot; explicit recovery and the process-exit failure backstop remain separate boundaries; andflow-handoff.tsvalidates the required context. No user-facingdocs/update is required because supported behavior does not change. GitHub CI provides validation for this commit.Security Review
PASS8da6eb3cc3ccf502547b9b08dbdf28dbf48a40811ee723a3c0fd61fa6c8217b81035208ef5a68e09PASS— persistence and event paths retain redaction, including secret-bearing URL and error coverage.PASS— handoffs validate required context; state writes remain gated byfilterSafeUpdates; result source, target, kind, and transition are checked before mutation.PASS— no authentication or authorization surface changes.PASS— no dependency or registry changes.PASS— errors expose finite machine-state names, and failure persistence and emitted events remain redacted.PASS— no cryptographic behavior changes; secret-free session and redaction contracts remain.PASS— no network policy, container, header, port, permission, or debug setting changes.PASS— coverage checks stale-source and invalid-transition rejection, event redaction, filtered updates, and removal of the machine-mutation escape hatch.PASS— normal transition authority is centralized while fail-closed slice-entry checks and validated recovery paths remain.DGX Station Hardware Evidence
scripts/prepare-dgx-station-host.sh.Verification
Signed-off-by:line and every commit appears asVerifiedin GitHub — verification is pending after local commit8da6eb3cc3ccf502547b9b08dbdf28dbf48a4081is pushed.pre-commit,commit-msg, andpre-pushhooks passed, ornpm run validate:prpassed after refreshingorigin/mainwhen hooks were skipped or unavailable — commit hooks passed; pre-push is pending.npm run docsbuilds without warnings (documentation changes only) — Not applicable; no published documentation page changed.GitHub CI is authoritative.
No duplicate local test, documentation, build, or typecheck suite ran after the merge refresh.
Signed-off-by: Carlos Villela cvillela@nvidia.com
Summary by CodeRabbit
Bug Fixes
Documentation