fix(onboard): preserve Station Express choices on resume - #7059
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. |
senthilr-nv
added
platform: dgx-station
PR Review Advisor — InformationalAdvisor assessment: Informational / high confidence 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. |
|
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:
📝 WalkthroughWalkthroughStation Express onboarding now records validated, secret-free provider/model intent before managed setup, restores compatible environment state during resume, preserves intent across failures, and clears it after completion. Installer wiring marks DGX Station express mode, with expanded tests and updated resume guidance. ChangesStation Express resume
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Installer
participant Onboard
participant Session
participant ProviderSetup
Installer->>Onboard: set Station Express environment
Onboard->>Session: checkpoint provider/model intent
Onboard->>ProviderSetup: start managed setup
ProviderSetup-->>Onboard: fail or complete
Onboard->>Session: preserve intent on failure or clear on completion
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Signed-off-by: Senthil Ravichandran <senthilr@nvidia.com>
e39aedc to
2895d55
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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-station-express.test.ts`:
- Around line 37-87: Update the parent describe title for “Station Express
onboarding session state” to append the issue reference as the final “(`#7048`)”
suffix; leave the child test titles unchanged.
In `@src/lib/state/onboard-session.ts`:
- Around line 730-737: Validate the raw resumable value before the compatibility
check around stationExpressIntent: when stationExpressIntent is present, require
data.resumable to be a boolean and reject invalid values, including string
"false", by returning null. Preserve the existing checks for non-interactive
mode, resumable === false, and complete status.
🪄 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: acb70a1c-45a1-43da-972c-1b8fc6c6324b
📒 Files selected for processing (13)
docs/reference/commands.mdxscripts/install.shsrc/lib/onboard.tssrc/lib/onboard/entry-options.tssrc/lib/onboard/machine/handlers/provider-inference.test.tssrc/lib/onboard/session-bootstrap.test.tssrc/lib/onboard/session-bootstrap.tssrc/lib/onboard/station-express-resume.test.tssrc/lib/onboard/station-express-resume.tssrc/lib/state/onboard-session-station-express.test.tssrc/lib/state/onboard-session.tstest/install-express-prompt.test.tstest/install-station-host-preparation.test.ts
apurvvkumaria
left a comment
apurvvkumaria
left a comment
There was a problem hiding this comment.
Four correctness gaps block this revision:
-
Completed provider selection is trusted without binding it to the persisted Express intent.
StationExpressSessionLikeomitsproviderandmodel, and the completed path deliberately skips restoration without checking those values. A malformed session can carry Ultra intent while resuming another provider/model. Validate completed state againstvllm-localand the intent's served model, and add negative mismatch tests. -
Explicit-resume lifecycle parsing is not fail-closed. Raw
"false"currently becomes truthy, and arbitrary status strings are accepted unless they equalcomplete. Requireresumable === trueand an allowed resumable status (in_progressorfailed), with malformed and missing-field coverage. -
A failed Express intent can be erased by a plain invocation. Failed sessions restore only under explicit
--resume, while auto-resume recognizes onlyin_progress; session bootstrap can then overwrite the failed checkpoint without--fresh. Either fail with clear--resume/--freshguidance or intentionally auto-resume, and test that boundary. -
The acceptance-level failure-to-resume regression is still missing. Add one chained test that checkpoints the intent, injects managed-vLLM failure, verifies the persisted failed session, invokes the real wrapper with
--resume, and confirms the same provider/model is reused without the generic prompt.
The current CI and signature/DCO gates are otherwise clean. Please also resolve the two current CodeRabbit threads; the malformed resumable finding is part of blocker 2, while the suite-title suffix is nonblocking but straightforward.
Co-authored-by: Senthil Ravichandran <senthilr@nvidia.com> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
|
Maintainer follow-up pushed in This revision:
Validation completed before push:
GitHub reports the commit signature Verified, and DCO has passed. CI and automated reviewers are now running on the exact new head. The fork E2E controller recorded its credentialed-job skip path for this revision; no protected E2E retry or duplicate dispatch was made. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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-station-express.test.ts`:
- Around line 145-184: Update the failure-persistence test around
handleProviderInferenceState to exercise the scenario through the authoritative
OnboardRuntime or public onboarding lifecycle instead of invoking the handler
directly. Remove manual use of LEGACY_MACHINE_STEP_MUTATION_OPTIONS and
finalizeIncompleteOnboardStep, then assert that the runtime persists the
provider failure and that resuming follows the expected behavior through the
public entrypoint.
🪄 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: 82995a27-f6e7-4326-9ec5-4f142c9a689d
📒 Files selected for processing (4)
src/lib/onboard/station-express-resume.test.tssrc/lib/onboard/station-express-resume.tssrc/lib/state/onboard-session-station-express.test.tssrc/lib/state/onboard-session.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib/onboard/station-express-resume.ts
Co-authored-by: Senthil Ravichandran <senthilr@nvidia.com> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
|
Follow-up for the new runtime-level review finding is in The chained regression now:
Validation:
GitHub reports this follow-up commit Verified. Protected E2E skip approval was deliberately not recorded for the superseded revision; refreshed CI and exact-diff coordination are now running on |
Signed-off-by: Senthil Ravichandran <senthilr@nvidia.com>
Signed-off-by: Senthil Ravichandran <senthilr@nvidia.com>
|
Current-main follow-up is now at exact head
@apurvvkumaria, please rereview this exact head because the prior approval was bound to |
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
Signed-off-by: Senthil Ravichandran <senthilr@nvidia.com>
|
Explicit Verification:
@apurvvkumaria, please rereview exact head |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/onboard/station-express-resume.ts`:
- Around line 147-155: Update the servedModel validation in both the parsing and
binding paths around servedModelValue, BOUND_INTENT_KEYS, and isSafeModelId to
resolve the alias and require it to identify the Station model associated with
the intent. Preserve acceptance of compatible aliases while rejecting safe but
incompatible model IDs, including during completed-selection matching. Add a
regression test covering a canonical Ultra intent with a safe, unrelated
servedModel alias.
In `@src/lib/state/onboard-session-station-express.test.ts`:
- Around line 344-354: Update the test around session.markStepComplete and the
following requireLoadedSession assertion to explicitly verify
steps.provider_selection.status is "completed". Preserve the existing provider,
model, and stationExpressIntent assertions while asserting the step completion
outcome as part of the expected session state.
- Around line 366-377: Update the failed provider-selection assertion in the
session test to capture a pre-save snapshot of stationExpressIntent and compare
the post-failure stationExpressIntent with toEqual. Keep the existing
validation-error and pending-step assertions, while ensuring the comparison
detects any mutation, including servedModel or other fields.
🪄 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: ebd1e66a-b5a9-4af8-8298-c65d76e3ebca
📒 Files selected for processing (6)
src/lib/onboard/machine/handlers/provider-inference.test.tssrc/lib/onboard/station-express-resume.test.tssrc/lib/onboard/station-express-resume.tssrc/lib/state/onboard-session-station-express.test.tssrc/lib/state/onboard-session.tstest/install-station-host-preparation.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/lib/onboard/machine/handlers/provider-inference.test.ts
- src/lib/state/onboard-session.ts
Signed-off-by: Senthil Ravichandran <senthilr@nvidia.com>
|
Addressed the remaining exact-head findings in
Current-head verification: 28 Station wrapper/session tests, 68 general session tests, all 59 Station installer integration tests, CLI typecheck, normal commit hooks, and push-time CLI typecheck passed. Commit signature is GitHub Verified. The docs-impact review found no additional update needed because the documented resume/fresh contract already describes these outcomes. @apurvvkumaria, please re-review exact head |
apurvvkumaria
left a comment
apurvvkumaria
left a comment
There was a problem hiding this comment.
Re-review of exact head 3dd789c5fea041dbea825917d8b59a8d97af4a34 found three blockers introduced by the follow-up commits:
-
Arbitrary verified vLLM served aliases regress.
identifiesModel()only accepts registry IDs/env values, but merged base #7044 intentionally accepts an arbitrary served name (for examplenemotron-ultra) after/v1/models.rootproves the underlying Ultra checkpoint; managed--served-model-nameoverrides have the same shape. The provider setup can therefore succeed and thenmarkStepComplete("provider_selection", ...)throws while binding the checkpoint. The test was changed fromnemotron-ultrato a registry identifier, masking the regression. Carry the route-validated model identity into the atomic binding and add an arbitrary-alias persistence/failure/resume test. -
Malformed binding lifecycle is accepted. A bound intent with a non-complete provider step and null provider/model passes normalization and wrapper matching, then installation is replayed with the registry canonical alias rather than the recorded one. Conversely, an unbound intent with a complete provider step is invalid. Enforce the invariant after parsing steps: complete provider step => bound intent +
vllm-local+ exact session model; non-complete provider step => unbound intent + null provider/model. Add normalization and wrapper negatives for both directions. -
Receipt retirement precedes durable completion.
completeSession()unlinksstation-express-resumeinside the update mutator beforesaveSession()writes/renames the completed session. A persistence failure or crash in that window leaves the old resumable session on disk but deletes the reboot receipt, violating #7048's clear-only-after-success criterion. Coordinate these two durable states with a recoverable journal/tombstone (or a single authoritative state), and inject a post-cleanup save failure/crash-recovery regression.
The current static symlink check also has an lstat-then-path-unlink directory-swap race; please either close it with an fd/rename-based boundary or document why the verified owner-only directory makes it out of scope. Protected E2E must remain undispatched until these blockers and the exact head/base/review gate are rechecked.
|
Update: exact-head review at Main CI is green, but the PR remains blocked on the served-model alias, lifecycle invariant, and durable receipt-retirement findings. The earlier approval request is superseded until those findings are addressed and the exact head is re-reviewed. |
Signed-off-by: Senthil Ravichandran <senthilr@nvidia.com>
|
Exact-head follow-up is ready at The arbitrary vLLM alias binding, lifecycle normalization, receipt durability, and owner-only path-boundary findings are addressed with regression coverage. All normal CI and both review advisors are green with zero findings. Protected E2E remains undispatched pending exact-head approval. @apurvvkumaria, please rereview this head. |
apurvvkumaria
left a comment
apurvvkumaria
left a comment
There was a problem hiding this comment.
Re-review of exact head : the arbitrary-alias proof, provider lifecycle invariants, save-before-clear ordering, and owner-only pathname boundary address the prior findings. One correctness blocker remains.\n\n treats any existing session as proof that a loaded Station reboot receipt belongs to that completed attempt, deletes the receipt, and returns without invoking onboarding. The receipt is created before reboot/CLI onboarding, so an unrelated older completed session can legitimately preexist; after reboot, the newly accepted Station attempt is then silently discarded. The new case currently encodes this false positive by using a completed session with no Station provenance and expecting no CLI invocation. The TypeScript cleanup predicate is likewise uncorrelated.\n\nPlease bind the receipt to the exact Station attempt/generation (not just provider/model heuristics) and retire it only when the durable completion marker matches that generation. Add regressions for: (1) old unrelated completed session + new receipt must not skip onboarding or retire the new receipt; (2) matching Station completion + injected post-save cleanup failure must retire only that receipt on re-entry without replaying onboarding; and (3) a replaced/mismatched receipt fails closed. Protected E2E should remain held until this exact-head blocker is addressed and re-reviewed.\n\nThe residual TypeScript lstat/unlink race is reduced to a same-UID principal by the new owner-only ancestor checks and is not blocking this review.
apurvvkumaria
left a comment
apurvvkumaria
left a comment
There was a problem hiding this comment.
Re-review of exact head 070e4af7d94385bde2a5626fdb6c7f729d2894ed: the arbitrary-alias proof, provider lifecycle invariants, save-before-clear ordering, and owner-only pathname boundary address the prior findings. One correctness blocker remains.
scripts/install.sh:2560-2562,2593-2597 treats any existing { status: "complete", resumable: false } session as proof that a loaded Station reboot receipt belongs to that completed attempt, deletes the receipt, and returns without invoking onboarding. The receipt is created before reboot and CLI onboarding, so an unrelated older completed session can legitimately preexist; after reboot, the newly accepted Station attempt is then silently discarded. The new test/install-onboard-yes.test.ts case currently encodes this false positive by using a completed session with no Station provenance and expecting no CLI invocation. The TypeScript cleanup predicate is likewise uncorrelated.
Please bind the receipt to the exact Station attempt or generation—not just provider/model heuristics—and retire it only when the durable completion marker matches that generation. Add regressions for:
- An old unrelated completed session plus a new receipt must not skip onboarding or retire the new receipt.
- A matching Station completion plus an injected post-save cleanup failure must retire only that receipt on re-entry without replaying onboarding.
- A replaced or mismatched receipt must fail closed.
Protected E2E should remain held until this exact-head blocker is addressed and re-reviewed.
The residual TypeScript lstat/unlink race is reduced to a same-UID principal by the new owner-only ancestor checks and is not blocking this review.
Co-authored-by: Senthil Ravichandran <senthilr@nvidia.com> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Co-authored-by: Senthil Ravichandran <senthilr@nvidia.com> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Co-authored-by: Senthil Ravichandran <senthilr@nvidia.com> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Co-authored-by: Senthil Ravichandran <senthilr@nvidia.com> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Co-authored-by: Senthil Ravichandran <senthilr@nvidia.com> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Co-authored-by: Senthil Ravichandran <senthilr@nvidia.com> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Co-authored-by: Senthil Ravichandran <senthilr@nvidia.com> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Co-authored-by: Senthil Ravichandran <senthilr@nvidia.com> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Accept supported full model identities and keep fresh cleanup safe for legacy state directories. Co-authored-by: Senthil Ravichandran <senthilr@nvidia.com> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Co-authored-by: Senthil Ravichandran <senthilr@nvidia.com> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
apurvvkumaria
left a comment
apurvvkumaria
left a comment
There was a problem hiding this comment.
Re-reviewed exact head 439d38c70aa6731c1bc66a329cc5961a969f4198 against base aa3dd78a2c64d59e6d5367e14e24c377c56c66b4. The Station receipt is now bound to the exact installer generation, completion persists the matching retirement marker before receipt cleanup, recovery reconciles only that generation, unrelated completed sessions do not suppress a new attempt, and replaced or mismatched receipts fail closed. All inline threads are resolved. Local exact-head verification passed 61 focused CLI lifecycle tests, 18 installer wrapper tests, 60 Station installer tests, and CLI typecheck. Current canonical advisor output reports zero actionable findings; the failed Nemotron lane is an advisor infrastructure failure and its partial output is explicitly non-canonical.
4 participants
Summary
DGX Station Express now records its secret-free provider, model, sandbox, and interaction intent before managed vLLM setup can fail. Running
nemoclaw onboard --resumerestores those choices and retries the failed Express step instead of returning to the generic provider and model prompts.This follow-up is refreshed onto current
mainafter #6991 merged and contains only the Station Express resume fix.Supersedes #7053, which GitHub automatically closed when its stacked base branch was removed after merge.
Related Issue
Fixes #7048
Changes
--fresh.onboard --fresh, so a later installer run cannot restore stale Express choices.Type of Change
Quality Gates
98d6f503; exact-head rereview requested for3dd789c5after the alias-validation and success/fresh receipt-lifecycle fixesVerification
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 typecheck:cli, normal commit hooks, and push-time CLI typecheck passednpx vitest run --project clicompleted with 9,084 passed, 20 skipped, and four unrelated local failures (missing Pythonyaml, a temp-directory cleanup race, and two existing five-second timeouts)npm run docsbuilds without warnings (doc changes only) — passed with zero errors and two existing Fern warningsSigned-off-by: Senthil Ravichandran senthilr@nvidia.com
Summary by CodeRabbit
onboard --resumeguidance to cover resumable interrupted or failed sessions (keeping existing restrictions).STATION_EXPRESS.