fix(onboard): preserve truthful review recovery - #8724
Conversation
Signed-off-by: Yimo Jiang <yimoj@nvidia.com>
|
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:
📝 WalkthroughWalkthroughOnboarding now derives resume and non-interactive state, persists sandbox identity asynchronously, records rejected provider selections as failed sessions, and recovers provider reviews before inference setup. Tests cover TTY, resume, rejection, checkpoint ordering, and Ollama preparation. ChangesOnboarding recovery
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Operator
participant runOnboard
participant ProviderInference
participant SessionState
participant OllamaProxy
Operator->>runOnboard: start or resume onboarding
runOnboard->>ProviderInference: pass review context and dependencies
ProviderInference->>SessionState: checkpoint sandbox and provider selection
ProviderInference->>SessionState: record rejection or accepted selection
ProviderInference->>OllamaProxy: prepare local provider when configured
OllamaProxy-->>ProviderInference: return prepared inference route
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![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
🧹 Nitpick comments (1)
src/lib/onboard/session-bootstrap.ts (1)
95-95: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefix the retained unused parameter.
The changed recovery logic no longer reads
agent. Rename the parameter on Line 86 to_agentif the three-argument API must remain compatible.As per coding guidelines, “prefix intentionally unused variables with
_.”Proposed fix
export function getCheckpointedSandboxName( resume: boolean, - agent: { name?: string } | null, + _agent: { name?: string } | null, session: Session | null, ): string | null {🤖 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/session-bootstrap.ts` at line 95, Rename the unused agent parameter in the recovery function to _agent while preserving the existing three-argument API and behavior.Source: Coding guidelines
🤖 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.ts`:
- Around line 4189-4204: Make prepareLocalProviderForInference the only Ollama
proxy preparation path: remove or bypass the proxy startup in
startProxyOrAnnounceDirect and the
ensureOllamaAuthProxy/persistAndProbeOllamaProxy calls in
setupOllamaLocalInference. Ensure proxy preparation occurs only after
configuration review acceptance through the existing
prepareLocalProviderForInference hook.
- Around line 3770-3807: Add public CLI coverage for the onboarding entry flow
around isOnboardAutoYesNonInteractive: exercise onboard --resume --yes with
stdin/stdout reported as non-TTY, and verify fresh --yes behavior without a TTY.
Do not pass --non-interactive in these cases, and assert the resulting mode and
behavior through the public CLI interface.
---
Nitpick comments:
In `@src/lib/onboard/session-bootstrap.ts`:
- Line 95: Rename the unused agent parameter in the recovery function to _agent
while preserving the existing three-argument API and behavior.
🪄 Autofix
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: e7f3c176-57b9-49be-9fbf-92ea467636de
📒 Files selected for processing (21)
src/lib/core/non-interactive.test.tssrc/lib/onboard.tssrc/lib/onboard/entry-options.test.tssrc/lib/onboard/machine/core-flow-phases.test.tssrc/lib/onboard/machine/handlers/provider-inference-route-containment.test.tssrc/lib/onboard/machine/handlers/provider-inference.test-support.tssrc/lib/onboard/machine/handlers/provider-inference.test.tssrc/lib/onboard/machine/handlers/provider-inference.tssrc/lib/onboard/machine/runtime.tssrc/lib/onboard/no-tty-auto-yes.test.tssrc/lib/onboard/no-tty-auto-yes.tssrc/lib/onboard/resume-config.test.tssrc/lib/onboard/resume-config.tssrc/lib/onboard/runtime-boundary.test.tssrc/lib/onboard/runtime-boundary.tssrc/lib/onboard/sandbox-agent.test.tssrc/lib/onboard/sandbox-agent.tssrc/lib/onboard/session-bootstrap.test.tssrc/lib/onboard/session-bootstrap.tssrc/lib/state/onboard-session.test.tssrc/lib/state/onboard-session.ts
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported Model lanes
1 terminology difference from the second opinionAdvisory only. These are normalized differences from the primary terminology receipt.
1 additional E2E selection from the second opinionAdvisory only. The primary lane did not select these E2E jobs or targets.
Second-opinion terminology and E2E selections are advisory. Live E2E does not run automatically for pull requests. 2 semantic terminology decisionsTerminology decisions are advisory. They affect the assessment only when a separate finding identifies concrete semantic impact.
E2E guidanceAdvisory only. A maintainer can dispatch the default E2E suite for the commit under review. Recommended E2E: Manual-only E2E: 1 optional E2E recommendation
This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
wscurran
added
area: cli
Signed-off-by: Yimo Jiang <yimoj@nvidia.com>
Signed-off-by: Yimo Jiang <yimoj@nvidia.com>
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/lib/onboard/machine/handlers/provider-inference.test.ts (1)
1451-1483: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the replacement selection outcome.
This test passes if
handleProviderInferenceStatecallssetupNimbut returns without configuring a replacement inference route. Assert that the result contains the newly selected provider and model. Also assert thatsetupInferenceruns for that replacement. Keep the negative assertion for the rejected selection.Proposed test strengthening
- await handleProviderInferenceState({ + const result = await handleProviderInferenceState({ ...baseOptions(deps, session), resume: true, sandboxName: "rejected-review", }); expect(calls.setupNim).toHaveBeenCalled(); + expect(calls.setupInference).toHaveBeenCalled(); + expect(result).toMatchObject({ + provider: "nvidia-prod", + model: "nvidia/test", + }); expect(calls.setupInference).not.toHaveBeenCalledWith(As per path instructions, “Prefer observable outcomes through the public boundary over source-text, private-shape, or mock-call assertions.”
🤖 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/handlers/provider-inference.test.ts` around lines 1451 - 1483, Strengthen the test around handleProviderInferenceState by capturing its returned result and asserting it contains the newly selected provider and model. Verify setupInference is called for that replacement selection, while retaining the existing negative assertion that it is not called with the explicitly rejected provider/model.Source: Path instructions
🤖 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/handlers/provider-inference.test.ts`:
- Around line 1451-1483: Strengthen the test around handleProviderInferenceState
by capturing its returned result and asserting it contains the newly selected
provider and model. Verify setupInference is called for that replacement
selection, while retaining the existing negative assertion that it is not called
with the explicitly rejected provider/model.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: ff9807aa-783c-4d01-9e6b-97cabb001c6f
📒 Files selected for processing (22)
src/lib/core/non-interactive.test.tssrc/lib/onboard.tssrc/lib/onboard/entry-options.test.tssrc/lib/onboard/machine/core-flow-phases.test.tssrc/lib/onboard/machine/handlers/provider-inference-route-containment.test.tssrc/lib/onboard/machine/handlers/provider-inference.test-support.tssrc/lib/onboard/machine/handlers/provider-inference.test.tssrc/lib/onboard/machine/handlers/provider-inference.tssrc/lib/onboard/machine/runtime.tssrc/lib/onboard/no-tty-auto-yes.test.tssrc/lib/onboard/no-tty-auto-yes.tssrc/lib/onboard/provider-review-wiring.tssrc/lib/onboard/resume-config.test.tssrc/lib/onboard/resume-config.tssrc/lib/onboard/runtime-boundary.test.tssrc/lib/onboard/runtime-boundary.tssrc/lib/onboard/sandbox-agent.test.tssrc/lib/onboard/sandbox-agent.tssrc/lib/onboard/session-bootstrap.test.tssrc/lib/onboard/session-bootstrap.tssrc/lib/state/onboard-session.test.tssrc/lib/state/onboard-session.ts
🚧 Files skipped from review as they are similar to previous changes (21)
- src/lib/onboard/entry-options.test.ts
- src/lib/onboard/no-tty-auto-yes.ts
- src/lib/onboard/no-tty-auto-yes.test.ts
- src/lib/state/onboard-session.test.ts
- src/lib/core/non-interactive.test.ts
- src/lib/onboard/sandbox-agent.ts
- src/lib/onboard/sandbox-agent.test.ts
- src/lib/onboard/resume-config.ts
- src/lib/onboard/runtime-boundary.test.ts
- src/lib/onboard/machine/core-flow-phases.test.ts
- src/lib/onboard/resume-config.test.ts
- src/lib/state/onboard-session.ts
- src/lib/onboard/machine/handlers/provider-inference-route-containment.test.ts
- src/lib/onboard/session-bootstrap.test.ts
- src/lib/onboard/runtime-boundary.ts
- src/lib/onboard/machine/runtime.ts
- src/lib/onboard/session-bootstrap.ts
- src/lib/onboard.ts
- src/lib/onboard/provider-review-wiring.ts
- src/lib/onboard/machine/handlers/provider-inference.ts
- src/lib/onboard/machine/handlers/provider-inference.test-support.ts
Signed-off-by: Yimo Jiang <yimoj@nvidia.com>
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/onboard/machine/handlers/provider-review-recovery.test.ts`:
- Around line 66-81: In both test cases in
src/lib/onboard/machine/handlers/provider-review-recovery.test.ts (lines 66-81
and 121-136), add an assertion verifying checkpointSandboxIdentity is called
before promptYesNoOrDefault. Preserve the existing behavioral assertions while
covering the required checkpoint ordering for both prompted identity paths.
In `@src/lib/onboard/setup-inference.ts`:
- Around line 29-58: Complete the provider-preparation cutover by removing the
proxy-start and ensure/persist calls from setup-nim-ollama.ts and
inference-providers/ollama-local.ts, unless an explicitly bounded recovery
window is required. Route Ollama preparation through
createProviderReviewDeps.prepareLocalProviderForInference, and add a
production-wiring test that exercises createProviderReviewDeps rather than only
mocked handlers.
🪄 Autofix
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: 199aa907-c9de-4779-b676-a6dfe23684a1
📒 Files selected for processing (7)
src/lib/onboard.tssrc/lib/onboard/entry-options.test.tssrc/lib/onboard/entry-options.tssrc/lib/onboard/machine/handlers/provider-inference.test.tssrc/lib/onboard/machine/handlers/provider-review-recovery.test.tssrc/lib/onboard/setup-inference.tstest/onboard-inference-reconciliation.test.ts
💤 Files with no reviewable changes (1)
- src/lib/onboard/machine/handlers/provider-inference.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib/onboard.ts
Signed-off-by: Yimo Jiang <yimoj@nvidia.com>
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/onboard/entry-options.ts`:
- Around line 67-70: Update resolveOnboardEntryOptions so resume auto-confirm
treats either non-TTY stream as non-interactive, matching the cannotPrompt
predicate rather than checking only stdinIsTty. Preserve existing explicit --yes
and environment-based behavior, and add tests covering both mixed-stream cases
where exactly one of stdin or stdout is not a TTY.
🪄 Autofix
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: a325b990-32a7-4ac7-9832-2ea2b2abce9d
📒 Files selected for processing (4)
src/lib/onboard.tssrc/lib/onboard/entry-options.test.tssrc/lib/onboard/entry-options.tssrc/lib/onboard/setup-inference.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/lib/onboard.ts
- src/lib/onboard/entry-options.test.ts
- src/lib/onboard/setup-inference.ts
Signed-off-by: Yimo Jiang <yimoj@nvidia.com>
Signed-off-by: Yimo Jiang <yimoj@nvidia.com>
Signed-off-by: Yimo Jiang <yimoj@nvidia.com>
Signed-off-by: Yimo Jiang <yimoj@nvidia.com>
Signed-off-by: Yimo Jiang <yimoj@nvidia.com>
cv
left a comment
cv
left a comment
There was a problem hiding this comment.
src/lib/onboard/setup-inference.ts:48-56 and src/lib/onboard/machine/handlers/provider-inference.ts:1118 add a second Ollama proxy preparation owner. After this hook starts the proxy and persists and probes its token, src/lib/onboard/inference-providers/ollama-local.ts:68-79 persists and probes it again. src/lib/onboard/setup-nim-ollama.ts:309,355 also still starts the detached proxy before configuration review. An accepted Ollama flow therefore performs duplicate reachability work, increasing latency and transient-failure exposure, while a declined review still leaves host proxy state despite the new post-acceptance contract. Make one path own proxy startup, token persistence, and reachability probing, or define a bounded fallback that does not repeat successful work. Add production-wiring regression evidence that rejection creates no proxy state and acceptance performs each operation once; the current tests mock prepareLocalProviderForInference and cannot detect either path.
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Security reviewVerdict: PASS — no findings. I reviewed branch revision
Large-change flag: +1,060/−132 across 24 files. The size is primarily recovery-state coverage; no large generated or dependency payload is added. All contributor and maintainer commits are signed off, and GitHub marks the current maintainer merge Verified. |
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Security review — PASSReviewed branch revision Large-change flag: 28 files, +1,191/-159. The review surface is substantial and requires the full repository gates plus maintainer rereview before merge. FindingsNo findings. Category review
Files reviewed
|
4 participants
Summary
Onboarding now reports configuration-review cancellation as incomplete and preserves interrupted review state for resume. A no-TTY
onboard --resume --yesreuses the saved selection without an inappropriate prompt, while interactive resume presents the review again.Related Issue
Fixes #8686
Fixes #8687
Changes
--yesauto-confirms it.Type of Change
Quality Gates
--resume --yes-i-accept-third-party-software --yesworkflow. No flag, command, default, or documented workflow changed.Documentation Writer Review
no-docs-needednpm run docs:sync-agent-variantspassed;npm run docscompleted with 0 errors and 2 existing Fern warnings.DGX Station Hardware Evidence
Verification
Signed-off-by:line and every commit appears asVerifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run validate:prpassed after refreshingorigin/mainwhen hooks were skipped or unavailablenpm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result: the earlier broad run completed 27,693 tests with 78 unrelated integration failures; the current branch must still pass all required GitHub gates before mergenpm run docsbuilds without warnings (doc changes only) — the build completed with 0 errors and 2 existing Fern warnings unrelated to this changeSigned-off-by: Yimo Jiang yimoj@nvidia.com