feat(onboard): make configuration review editable - #9037
Conversation
Signed-off-by: Prekshi Vyas <prekshiv@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. |
|
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 (6)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughThe onboarding flow replaces the confirmation prompt with a four-action configuration review. It supports applying, editing inference settings, editing the sandbox name, or exiting. It preserves or clears staged state and supports recovery after inference setup failures. ChangesOnboarding configuration review
Estimated code review effort: 3 (Moderate) | ~20 minutes Mergeability Score: ⚪ Minimal · up to The configuration review changes are merge-ready after normal checks and review; no actionable merge-blocking risk remains. Sequence Diagram(s)sequenceDiagram
participant OnboardingFlow
participant reviewProviderConfiguration
participant promptOnboardConfigurationReview
participant ProviderSetup
OnboardingFlow->>reviewProviderConfiguration: submit provider, model, credential, and sandbox
reviewProviderConfiguration->>promptOnboardConfigurationReview: request review action
promptOnboardConfigurationReview-->>reviewProviderConfiguration: return action
alt apply
reviewProviderConfiguration->>ProviderSetup: continue inference setup
else edit inference
reviewProviderConfiguration->>ProviderSetup: reselect provider and model
ProviderSetup-->>reviewProviderConfiguration: return updated inference
else edit sandbox name
reviewProviderConfiguration-->>OnboardingFlow: prompt with previous name
else exit
reviewProviderConfiguration-->>OnboardingFlow: clear staged choices and exit
end
Possibly related PRs
Suggested labels: 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 |
|
🌿 Preview your docs: https://nvidia-preview-pr-9037.docs.buildwithfern.com/nemoclaw |
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in commit 224e4d5 in the TypeScript / code-coverage/cliThe overall coverage in commit 224e4d5 in the Show a code coverage summary of the most impacted files.
Updated |
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported Model lanes
5 terminology differences from the second opinionAdvisory only. These are normalized differences from the primary terminology receipt.
Second-opinion terminology and E2E selections are advisory. Live E2E does not run automatically for pull requests. 3 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. |
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
|
Maintainer review: this PR cannot pass the product-scope gate. Issue #6005 remains labeled The commit under review does not preserve that boundary:
As a result, Edit inference or Exit onboarding can leave host runtime or checkpoint effects from a discarded selection. Moving these effects after Review and Apply requires the broader draft and materialization refactor described in the issue audit. That refactor would materially change this PR's narrow intent, so I am skipping this PR instead of pushing that redesign onto its branch. |
wscurran
added
area: cli
prekshivyas
left a comment
prekshivyas
left a comment
There was a problem hiding this comment.
Reviewed exact head 224e4d570d86602b87ac079a82547852ecefd710.
No actionable findings. The four-way configuration-review loop keeps provider registration and inference setup behind Apply, preserves the current sandbox name as the edit default, records an explicit rejected state on Exit, and clears staged credential environment values through the existing finally path. The change stays within the established onboarding configuration-review surface and does not create a new supported integration.
Security review:
- Input validation — PASS: prompt aliases resolve to a closed action union, and sandbox edits reuse the validated-name prompt.
- Authentication and authorization — PASS: no authority boundary changes.
- Secrets and credentials — PASS: the review checkpoint is secret-free and staged credential environment values remain cleared on retry, exit, failure, and success paths.
- Code injection — PASS: selected actions are internal enum values and are not evaluated or passed to a shell.
- Data exposure — PASS: the summary uses the existing redacted configuration formatter.
- Dependency and supply chain — PASS: no dependency, artifact, or workflow changes.
- System interaction — PASS: Apply is the only action that advances to host/provider mutation; Edit and Exit do not register a new gateway credential.
- Error handling — PASS: exit records rejection and terminates; inference setup remains guarded by the existing cleanup path.
- Testing — PASS: focused tests cover aliases/defaults, repeated edits, provider reselection, sandbox-name reuse, exit, noninteractive behavior, and credential cleanup. The latest required CI, CodeQL, advisor, DCO, documentation-receipt, growth-guard, and managed-runtime lanes pass at this head.
Files reviewed: src/lib/onboard/prompt-helpers.ts, src/lib/onboard/prompt-helpers.test.ts, src/lib/onboard/machine/handlers/provider-inference.ts, src/lib/onboard/machine/handlers/provider-inference.test.ts, the onboarding orchestration and state helpers changed by this PR, and the related documentation and PR metadata.
This is my own PR, so GitHub does not permit an independent approval from this account. From this review, it is ready for an independent maintainer approval.
3 participants
Summary
Replace the onboarding review's yes/no confirmation with an explicit action menu so users can correct the inference provider/model or sandbox name before applying the configuration. This is the first small replacement slice after #8171: provider registration and sandbox materialization remain behind Apply configuration, while web search, messaging, resources, policy, and general post-Apply editing remain out of scope.
Related Issue
Part of #6005
Tracks NVBug 6392526.
Changes
back, users can select a provider/model again and review the updated configuration.Type of Change
Quality Gates
finallyon edit and Exit; existing provider, model, and sandbox validators remain authoritative; provider registration and sandbox materialization remain behind Apply; no authentication, authorization, dependency, cryptography, network-policy, or security-header behavior changes; focused and broad tests cover action parsing, state transitions, cleanup ordering, preservation, Exit, resume, and non-interactive behavior.Documentation Writer Review
docs-updateddocs/get-started/quickstart.mdx,docs/get-started/quickstart-hermes.mdx,docs/get-started/quickstart-langchain-deepagents-code.mdx, anddocs/reference/commands.mdxagainst the exact implementation; confirmed the four actions, credential cleanup, preserved selections, resumable interrupted-review checkpoint, non-interactive and Exit behavior, and the post-Apply inference-failure recovery exception; generated OpenClaw, Hermes, and Deep Agents command references were verified;npm run docspassed 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 unavailablenpx vitest run --project clifor the five focused onboarding suites passed 68/68;npm run build:cli,npm run typecheck:cli -- --incremental,npm run lint,git diff --check, andnpm run docspassed on the rebased head.npx vitest run --changed origin/main --project cli --project plugin --project e2e-support --maxWorkers=4passed 1,808/1,808 tests across 152 files.npm run docsbuilds without warnings (doc changes only)Signed-off-by: Prekshi Vyas prekshiv@nvidia.com