feat(onboard): add reversible configuration review - #8171
Conversation
Signed-off-by: Prekshi Vyas <prekshiv@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:
📝 WalkthroughWalkthroughThe onboarding flow now collects secret-free configuration drafts, supports review editing and backward navigation, persists partial drafts, and applies accepted choices before credential and resource setup. Post-Apply flows restrict navigation and reuse reviewed selections. Documentation and tests cover the new phases. ChangesOnboarding intent-draft workflow
Estimated code review effort: 5 (Critical) | ~120 minutes 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-8171.docs.buildwithfern.com/nemoclaw |
Sensitive-path security reviewResult: PASS on head
Reviewed the complete 41-file PR diff and the integration points for inference, web search, messaging, policy selection, credentials, session persistence, and sandbox startup. |
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported Model lanes
8 terminology differences from the second opinionAdvisory only. These are normalized differences from the primary terminology receipt.
2 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. 3 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: 3 optional E2E recommendations
This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/onboard/messaging-channel-setup.ts (1)
184-195: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winPreserve reviewed channel IDs in non-interactive setup.
An accepted
intentDraftpassesselectionProvided: true, but the non-interactive branch ignores it and derives channels from configured inputs. Use the provided channel IDs and cover this combination in a 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/messaging-channel-setup.ts` around lines 184 - 195, Update the channel selection initialization in the setup flow around statusForChannel so the non-interactive path with selectionProvided true preserves the accepted intentDraft channel IDs instead of deriving channels from configured inputs; retain filtering to availableChannels as appropriate, and add a test covering selectionProvided true with the accepted draft IDs.
🧹 Nitpick comments (14)
src/lib/state/onboard-session/intent-draft.test.ts (1)
46-64: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the accepted phase and for an unknown draft version.
The suite covers a
collectingdraft and unknown-field stripping. It does not cover the two cases that change materialization behavior:
- A
phase: "accepted"draft must survive the save and load round trip. Downstream handlers gate credential materialization and the reviewed policy tier on that exact value.- A draft with an unsupported
versionmust normalize tonullrather than persist.💚 Proposed additional cases
+ it("round-trips an accepted draft that gates materialization", async () => { + const session = await import("../onboard-session"); + const created = session.createSession(); + created.intentDraft = { + version: 1, + phase: "accepted", + answers: { agent: "openclaw", policy: "balanced" }, + }; + + session.saveSession(created); + + expect(session.loadSession()?.intentDraft?.phase).toBe("accepted"); + }); + + it("drops a draft written by an unsupported version", async () => { + const session = await import("../onboard-session"); + const created = session.createSession() as unknown as Record<string, unknown>; + created.intentDraft = { version: 2, phase: "accepted", answers: { agent: "openclaw" } }; + + expect(session.normalizeSession(created as never)?.intentDraft).toBeNull(); + });🤖 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/state/onboard-session/intent-draft.test.ts` around lines 46 - 64, Add tests alongside the existing normalizeSession coverage for both materialization boundaries: verify an intent draft with phase "accepted" survives the save/load round trip unchanged, and verify a draft with an unsupported version normalizes to null and is not persisted. Reuse the existing session creation and normalization helpers in the test.src/lib/onboard/intent-draft/ui.test.ts (2)
13-17: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFail the test when queued replies stay unconsumed.
The
promptfake throws only whenrepliesis empty. It stays silent when the flow consumes fewer replies than queued. A test that skips a step therefore still passes, and the positional reply arrays hide which step received which answer. Line 146 queues fifteen unlabeled entries, so a single misplaced reply changes the navigation path without any assertion noticing.Record the prompt text with each reply, and assert that the queue is empty after the call. The recorded pairs also let each test state the navigation path it claims to exercise.
♻️ Proposed change to `makeDeps` and a per-test assertion
-function makeDeps(replies: string[]): OnboardIntentDraftUiDeps & { lines: string[] } { +function makeDeps( + replies: string[], +): OnboardIntentDraftUiDeps & { + lines: string[]; + asked: [string, string][]; + pendingReplies: string[]; +} { const lines: string[] = []; + const asked: [string, string][] = []; return { lines, + asked, + pendingReplies: replies, - prompt: vi.fn(async () => { + prompt: vi.fn(async (question: string) => { const reply = replies.shift(); if (reply === undefined) throw new Error("Missing prompt reply"); + asked.push([question, reply]); return reply; }),Then assert full consumption in each test:
const result = await collectOnboardIntentDraft(deps); + expect(deps.pendingReplies).toEqual([]); expect(result.kind).toBe("apply");🤖 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/intent-draft/ui.test.ts` around lines 13 - 17, Update the test dependency factory’s prompt fake in makeDeps to record each prompt text alongside its consumed reply, and ensure queued replies are identified by the prompt they are intended for rather than relying on unlabeled positional entries. Add assertions after every relevant test flow that the reply queue is empty, while preserving the existing missing-reply failure behavior and using the recorded prompt/reply pairs to make each test’s navigation path explicit.
90-90: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReduce exact-string assertions on rendered layout.
Several assertions pin exact indentation, for example
" 1) Apply configuration"on line 90," Agent:"on line 151, and" Managed tools: nous-web, nous-audio"on line 205. A purely visual change to indentation or spacing breaks many tests at once without any behavior change.Match on trimmed text or on a substring where the label, not the layout, is the contract. Keep exact matching only for prompt strings that encode a default value, such as
" CPU [50%]: ".Also applies to: 151-153, 176-177, 205-205, 226-228
🤖 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/intent-draft/ui.test.ts` at line 90, Relax layout-sensitive assertions in the intent-draft UI tests, including the checks around deps.lines and the related assertions at the referenced locations, by matching trimmed text or stable label substrings instead of exact indentation. Preserve exact-string assertions only where spacing is part of the prompt contract, such as the CPU default prompt.src/lib/onboard/messaging-channel-setup.test.ts (1)
458-473: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd coverage for an already-configured credential under
selectionProvided.This test only covers the missing-token case. Add a companion test where
TELEGRAM_BOT_TOKENis already set before callingsetupMessagingChannels(..., { selectionProvided: true }), assertingpromptis not called again for the token. This locks in the resume behavior described in the linked comment onsandbox-messaging.tslines 377-383.🤖 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/messaging-channel-setup.test.ts` around lines 458 - 473, Add a companion test near “uses reviewed channel IDs while requesting their credentials after Apply (`#6005`)” that sets TELEGRAM_BOT_TOKEN before calling setupMessagingChannels with selectionProvided: true. Assert the Telegram channel remains selected and prompt is not called for the token, preserving the existing credential without invoking saveCredential again.src/lib/onboard/intent-draft/ui.ts (2)
451-462: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the duplicated multi-select parse loop.
promptMessagingandpromptManagedToolsrepeat the same token split, lookup, dedupe, and invalid-token loop. Only the label and the return wrapper differ. Extract one helper so both prompts stay consistent when the parsing rules change.♻️ Proposed helper
function parseMultiSelect( raw: string, choices: readonly OnboardIntentChoice[], ): { readonly selected: string[] } | { readonly invalid: string } { const selected: string[] = []; for (const part of raw.split(/[\s,]+/).filter(Boolean)) { const choice = findChoice(part, choices); if (!choice) return { invalid: part }; if (!selected.includes(choice.value)) selected.push(choice.value); } return { selected }; }Then each prompt becomes:
- const selected: string[] = []; - let invalid: string | null = null; - for (const part of normalized.split(/[\s,]+/).filter(Boolean)) { - const choice = findChoice(part, choices); - if (!choice) { - invalid = part; - break; - } - if (!selected.includes(choice.value)) selected.push(choice.value); - } - if (!invalid) return { kind: "answer", value: selected }; - deps.log(` Unknown messaging channel: ${invalid}`); + const parsed = parseMultiSelect(normalized, choices); + if ("selected" in parsed) return { kind: "answer", value: parsed.selected }; + deps.log(` Unknown messaging channel: ${parsed.invalid}`);Also applies to: 496-507
🤖 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/intent-draft/ui.ts` around lines 451 - 462, Extract the duplicated token parsing logic from promptMessaging and promptManagedTools into a shared parseMultiSelect helper near the existing choice utilities. The helper should split on whitespace or commas, resolve tokens with findChoice, deduplicate selected values, and return either the selected values or the first invalid token; update both prompts to use it while preserving their existing labels and answer wrappers.
608-614: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNarrow
profileValuebefore building the resource answer.
profileValueis typedstring | undefinedfromprior?.profile. Line 614 can send the stage directly to"gpu"for a Back entry, so the profile stage may never assign it. At runtime that path is safe, because Line 614 requiresprior, andprior.profileis defined. The compiler does not check the mismatch, becauseDraftStep<OnboardIntentStepId, OnboardIntentDraft, unknown>types the prompt result asunknown. A later edit to the stage machine can therefore emitprofile: undefinedintoOnboardResourceIntentwith no type error.Make the invariant explicit so the compiler enforces it.
🛡️ Proposed fix
- return { - kind: "answer", - value: { - profile: profileValue, + if (!profileValue) throw new Error("Resource profile was not collected."); + return { + kind: "answer", + value: { + profile: profileValue,Also applies to: 677-685
🤖 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/intent-draft/ui.ts` around lines 608 - 614, Make profileValue non-optional in the resource prompt by narrowing or validating prior?.profile before constructing the resource answer, while preserving the back-entry path that starts at the "gpu" stage. Apply the same invariant to the corresponding prompt logic around the second resource flow at lines 677-685, so any emitted OnboardResourceIntent always has a defined profile.src/lib/onboard/machine/handlers/sandbox-messaging.test.ts (1)
266-271: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAccepted-draft fixtures bypass
parseOnboardIntentDraftin both tests. Both tests build a session withcreateSession()and then assignsession.intentDraftdirectly.createSessionrunsintentDraftthroughparseOnboardIntentDraft(src/lib/state/onboard-session.tsLine 695), so post-construction assignment skips the parser. Neither test proves that a real persisted accepted draft reaches its handler, and a shape the parser would reject or normalize still passes.
src/lib/onboard/machine/handlers/sandbox-messaging.test.ts#L266-L271: pass the draft ascreateSession({ intentDraft: { version: 1, phase: "accepted", answers: { messaging: ["discord"] } } })and drop the direct assignment.src/lib/onboard/machine/handlers/provider-inference.test.ts#L1435-L1442: pass the draft ascreateSession({ intentDraft: { version: 1, phase: "accepted", answers: {} } })and drop the direct assignment. Confirm thatparseOnboardIntentDraftpreserves an accepted draft with an emptyanswersobject; if it does not, use a complete answer set, because production accepts only a complete draft.As per path instructions: "Resume and repair bridges must correspond to real persisted older-session shapes, be idempotent across interruption/replay, keep secrets redacted, and converge on the same authoritative path as a fresh run."
🤖 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/sandbox-messaging.test.ts` around lines 266 - 271, Update the accepted-draft fixtures in src/lib/onboard/machine/handlers/sandbox-messaging.test.ts#L266-L271 and src/lib/onboard/machine/handlers/provider-inference.test.ts#L1435-L1442 to pass intentDraft through createSession, removing direct post-construction assignment so parseOnboardIntentDraft is exercised. Preserve the intended accepted-draft shapes, and in provider-inference.test.ts use a complete answer set if parsing rejects an empty answers object.Source: Path instructions
src/lib/onboard/intent-draft/controller.test.ts (1)
178-191: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the
acceptedphase in both assertion tests.These tests exercise
"collecting"and"materializing"only.collectOnboardIntentDraftinsrc/lib/onboard/intent-draft/ui.tsLine 720 persistsphase: "accepted", so"accepted"is the phase a resumed post-Apply session actually carries. Both guards must reject it. Add that case so a later change toOnboardDraftPhasehandling cannot silently re-enable Back navigation for an accepted draft.💚 Proposed additions
it("refuses Back once materialization has started", () => { expect(() => assertDraftNavigationAllowed("collecting", "nemoclaw")).not.toThrow(); + expect(() => assertDraftNavigationAllowed("accepted", "nemoclaw")).toThrow( + "Back navigation is unavailable after Apply configuration", + ); expect(() => assertDraftNavigationAllowed("materializing", "nemoclaw")).toThrow( "Back navigation is unavailable after Apply configuration", ); }); it("refuses a post-Apply retry that would revisit an accepted choice", () => { + expect(() => assertDraftRevisionAllowed("collecting", "the Ollama model", "nemoclaw")).not.toThrow(); + expect(() => assertDraftRevisionAllowed("accepted", "the Ollama model", "nemoclaw")).toThrow( + "Cannot change the Ollama model after Apply configuration", + ); expect(() => assertDraftRevisionAllowed("materializing", "the Ollama model", "nemoclaw"), ).toThrow(🤖 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/intent-draft/controller.test.ts` around lines 178 - 191, Extend both tests around assertDraftNavigationAllowed and assertDraftRevisionAllowed to cover the "accepted" phase, asserting it throws the same post-Apply errors as "materializing". Preserve the existing collecting and materializing assertions while ensuring resumed accepted drafts cannot navigate Back or revise an accepted choice.src/lib/onboard/intent-draft/controller.ts (1)
122-129: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the idempotence requirement for
prepareReview.Reference identity is the only loop-termination guard here. If a caller's
prepareReviewreturns a new object on every call, this loop never reachesoptions.review, and the CLI hangs with no output.prepareDraftForReviewinsrc/lib/onboard/intent-draft/ui.tssatisfies the requirement because it returns the same reference when the answers are unchanged. State that contract on theprepareReviewoption so future callers keep it.♻️ Proposed doc-comment change
- /** Revalidate a complete draft against current capabilities before review. */ + /** + * Revalidate a complete draft against current capabilities before review. + * + * Must be idempotent and must return the same object reference when nothing + * changes. The collection loop repeats until the returned draft is reference + * equal to its input. + */ readonly prepareReview?: (draft: Draft) => Promise<Draft> | Draft;🤖 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/intent-draft/controller.ts` around lines 122 - 129, Document the idempotence and reference-identity contract on the prepareReview option used by the controller loop: callers must return the same draft object when no changes are needed, and only return a new object when preparation changes it. Update the option’s type or adjacent doc comment, using prepareDraftForReview as the established behavior, without changing the loop logic.src/lib/onboard/policy-selection.ts (1)
402-407: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the applied-preset preservation loop.
The loop at Lines 403-407 duplicates the loop at Lines 482-488, including the
suppressedNamesexclusion. Extract one helper that takeschosen,appliedForPreservation, andsuppressedNamesand returns the kept names. Both call sites then share one rule, and the non-interactive note can keep using the returned list.🤖 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/policy-selection.ts` around lines 402 - 407, Extract the duplicated applied-preset preservation logic into a helper that accepts chosen, appliedForPreservation, and suppressedNames and returns the names retained after excluding already chosen or suppressed entries. Replace both preservation loops with calls to this helper, preserving the existing behavior and allowing the non-interactive note to use the returned list.src/lib/onboard.ts (1)
3937-3949: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGive the
NEMOCLAW_ONBOARD_INTENT_ACCEPTEDcontract one owner. Two modules read the raw env name and compare it to"1"independently, so the accepted-intent contract has no single source of truth. Export one predicate from theintent-draftmodule and call it from both sites.
src/lib/onboard.ts#L3937-L3949: replace the literal insidehasAcceptedOnboardIntentwith a call to the exportedintent-draftpredicate.src/lib/onboard/web-search-flow.ts#L460-L478: replaceenv.NEMOCLAW_ONBOARD_INTENT_ACCEPTED === "1"with the same predicate, passing the injectedenvso the tests keep injecting it.🤖 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.ts` around lines 3937 - 3949, The NEMOCLAW_ONBOARD_INTENT_ACCEPTED check must have one shared owner. In src/lib/onboard.ts lines 3937-3949, export a predicate from the intent-draft module and update hasAcceptedOnboardIntent to call it; in src/lib/onboard/web-search-flow.ts lines 460-478, replace the direct env comparison with that same predicate, passing the injected env so tests remain isolated.src/lib/onboard/messaging-channel-setup.ts (1)
183-189: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReport reviewed channels that the selected agent does not support.
The filter drops any reviewed channel that is missing from
availableChannels, with no output. The user reviewed and applied that channel, so silence is confusing. Emit anotefor each dropped channel, matching the pattern used byproviderSupportedinsrc/lib/onboard/web-search-flow.tsLine 384.🤖 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/messaging-channel-setup.ts` around lines 183 - 189, Update the channel filtering logic in the setup flow around the enabled Set to identify reviewed channel IDs absent from availableChannels and emit a note for each dropped channel, following the existing providerSupported notification pattern from the web-search flow. Preserve the current filtering and enabled-channel behavior while ensuring every unsupported reviewed channel is reported.src/lib/onboard/machine/handlers/provider-inference.ts (1)
410-425: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDerive both helpers from one accepted-intent predicate.
needsLegacyConfigurationConfirmationreadssession?.intentDraft?.phase !== "accepted", and the call site at Line 985 computes the same condition again asreviewedIntentAccepted. Two places now encode the accepted-intent rule. Add one predicate, for examplereviewedIntentAccepted(session), and use it in both helpers so a later phase rename touches one line.🤖 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.ts` around lines 410 - 425, The accepted-intent rule is duplicated between needsLegacyConfigurationConfirmation and the reviewedIntentAccepted value at the call site. Add a shared reviewedIntentAccepted(session) predicate and update both helpers/call-site logic to use it, preserving the existing non-interactive and summary behaviors while centralizing the phase check.src/lib/onboard/credential-navigation.ts (1)
11-14: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a phase-specific
neveroverload forassertDraftNavigationAllowed.
CredentialNavigationPolicy.onBackUnavailablecan usenever, but the change does not compile as written.assertDraftNavigationAllowedreturns for"collecting", so its declared return type isvoid. Add a throwing overload or helper for"materializing", use it in the policy callback, then changeonBackUnavailabletonever.🤖 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/credential-navigation.ts` around lines 11 - 14, Update the draft-navigation flow around assertDraftNavigationAllowed so the "materializing" phase has a throwing overload or helper with a never return type, while preserving void behavior for "collecting". Use that phase-specific throwing path in CredentialNavigationPolicy.onBackUnavailable, then change the callback’s return type to never.
🤖 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 `@docs/get-started/quickstart-langchain-deepagents-code.mdx`:
- Line 113: Update the Deep Agents Code wizard description to remove the web
search choice from the list of collected onboarding inputs, keeping the
remaining items unchanged. Ensure the surrounding documentation consistently
reflects that this flow has no NemoClaw-managed web-search prompt.
In `@docs/reference/commands.mdx`:
- Around line 249-253: Update the choice-collection documentation paragraph to
state that entering “exit” or “quit” exits onboarding, matching the existing
prompt hint and collection behavior. Also replace “nonapplicable” with “not
applicable,” while preserving the existing back-navigation details.
In `@src/lib/onboard/intent-draft/boundary.ts`:
- Around line 28-39: Update crossOnboardIntentDraftBoundary to explicitly reject
or throw when shouldCollect is false but options.existingDraft is a non-accepted
draft, such as a "collecting" phase. Preserve the current accepted-draft
handling and collection flow, while preventing the invalid disabled-collection
path from returning continue with a null draft.
In `@src/lib/onboard/intent-draft/schema.ts`:
- Around line 150-170: Update validateOnboardIntentEndpointUrl to reject or
remove URL fragments before returning the normalized URL, ensuring values in
parsed.hash cannot be persisted in the endpoint intent. Preserve the existing
protocol, userinfo, and credential-query validation behavior.
In `@src/lib/onboard/intent-draft/seed.ts`:
- Around line 55-65: Update the resource seeding logic around opts.sandboxGpu
and the resources assignment to preserve whether --gpu or --no-gpu was
explicitly selected, rather than treating both as null. Normalize that CLI
choice before constructing answers.resources so the GPU value reflects the
explicit selection instead of defaulting to "auto"; add boundary tests covering
both flags.
In `@src/lib/onboard/intent-draft/ui.ts`:
- Around line 301-315: Update the edit submenu loop around the prompt and
validation message to call the existing writeNavigationHint helper, so users can
discover the supported back and exit commands. Also change the prompt text from
“Choose a choice to edit” to “Choose a group to edit,” while preserving the
existing numeric selection and navigation handling.
In `@src/lib/onboard/machine/handlers/sandbox-messaging.ts`:
- Around line 377-383: Update setupSelectedMessagingChannels in
src/lib/onboard/machine/handlers/sandbox-messaging.ts:377-383 to pass
selectionProvided, or an equivalent credentials-only signal, alongside
selectionCompleted so resumed reviewed-channel runs with saved credentials do
not re-prompt. Add coverage in
src/lib/onboard/messaging-channel-setup.test.ts:458-473 by pre-setting
TELEGRAM_BOT_TOKEN, invoking the selectionProvided path, and asserting prompt is
not called for that credential.
In `@src/lib/onboard/policy-selection-recorded-tier.test.ts`:
- Around line 121-138: Add a paired test for the same suggested-mode harness
setup that omits acceptTierSuggestions while retaining tierName, and assert
selectTierPresetsAndAccess is called and selectPolicyTier is not called; keep
the existing reviewed-tier case and assertions so the pair isolates
acceptTierSuggestions from the ambient policy mode.
In `@src/lib/onboard/setup-nim-provider-discovery.test.ts`:
- Around line 18-35: Update the test case around prepareProviderDiscovery to
have getNonInteractiveProvider return a provider value distinct from the "build"
fallback, while keeping the expected reviewed model and getNonInteractiveModel
assertion aligned with that provider. This ensures the test verifies the
requested provider is forwarded rather than merely matching the fallback.
In `@src/lib/onboard/setup-nim-provider-discovery.ts`:
- Around line 109-112: Update the requestedModel logic in
setup-nim-provider-discovery so getNonInteractiveModel is invoked only for
non-interactive onboarding; interactive runs must continue to the provider menu
even when NEMOCLAW_MODEL is invalid. Add coverage for the interactive
invalid-model and provider-environment combination.
In `@src/lib/state/onboard-session.ts`:
- Line 777: Update normalizeSession around parseOnboardIntentDraft so a non-null
persisted intentDraft that fails parsing rejects the session instead of being
converted to null. Follow the existing rejection pattern used for the Station
Express fields, while preserving valid and genuinely null values; add resume
coverage verifying malformed intentDraft data is rejected after materialization.
In `@test/onboard-intent-draft-pty.test.ts`:
- Around line 27-36: Handle errors emitted by child.stdin in the
sendRepliesForVisiblePrompts flow, including EPIPE when the child exits before a
final stdout chunk is processed. Attach an stdin error handler that prevents the
unhandled stream error and allows the existing timeout diagnostic to be reported
instead; keep the child-level error handling unchanged.
---
Outside diff comments:
In `@src/lib/onboard/messaging-channel-setup.ts`:
- Around line 184-195: Update the channel selection initialization in the setup
flow around statusForChannel so the non-interactive path with selectionProvided
true preserves the accepted intentDraft channel IDs instead of deriving channels
from configured inputs; retain filtering to availableChannels as appropriate,
and add a test covering selectionProvided true with the accepted draft IDs.
---
Nitpick comments:
In `@src/lib/onboard.ts`:
- Around line 3937-3949: The NEMOCLAW_ONBOARD_INTENT_ACCEPTED check must have
one shared owner. In src/lib/onboard.ts lines 3937-3949, export a predicate from
the intent-draft module and update hasAcceptedOnboardIntent to call it; in
src/lib/onboard/web-search-flow.ts lines 460-478, replace the direct env
comparison with that same predicate, passing the injected env so tests remain
isolated.
In `@src/lib/onboard/credential-navigation.ts`:
- Around line 11-14: Update the draft-navigation flow around
assertDraftNavigationAllowed so the "materializing" phase has a throwing
overload or helper with a never return type, while preserving void behavior for
"collecting". Use that phase-specific throwing path in
CredentialNavigationPolicy.onBackUnavailable, then change the callback’s return
type to never.
In `@src/lib/onboard/intent-draft/controller.test.ts`:
- Around line 178-191: Extend both tests around assertDraftNavigationAllowed and
assertDraftRevisionAllowed to cover the "accepted" phase, asserting it throws
the same post-Apply errors as "materializing". Preserve the existing collecting
and materializing assertions while ensuring resumed accepted drafts cannot
navigate Back or revise an accepted choice.
In `@src/lib/onboard/intent-draft/controller.ts`:
- Around line 122-129: Document the idempotence and reference-identity contract
on the prepareReview option used by the controller loop: callers must return the
same draft object when no changes are needed, and only return a new object when
preparation changes it. Update the option’s type or adjacent doc comment, using
prepareDraftForReview as the established behavior, without changing the loop
logic.
In `@src/lib/onboard/intent-draft/ui.test.ts`:
- Around line 13-17: Update the test dependency factory’s prompt fake in
makeDeps to record each prompt text alongside its consumed reply, and ensure
queued replies are identified by the prompt they are intended for rather than
relying on unlabeled positional entries. Add assertions after every relevant
test flow that the reply queue is empty, while preserving the existing
missing-reply failure behavior and using the recorded prompt/reply pairs to make
each test’s navigation path explicit.
- Line 90: Relax layout-sensitive assertions in the intent-draft UI tests,
including the checks around deps.lines and the related assertions at the
referenced locations, by matching trimmed text or stable label substrings
instead of exact indentation. Preserve exact-string assertions only where
spacing is part of the prompt contract, such as the CPU default prompt.
In `@src/lib/onboard/intent-draft/ui.ts`:
- Around line 451-462: Extract the duplicated token parsing logic from
promptMessaging and promptManagedTools into a shared parseMultiSelect helper
near the existing choice utilities. The helper should split on whitespace or
commas, resolve tokens with findChoice, deduplicate selected values, and return
either the selected values or the first invalid token; update both prompts to
use it while preserving their existing labels and answer wrappers.
- Around line 608-614: Make profileValue non-optional in the resource prompt by
narrowing or validating prior?.profile before constructing the resource answer,
while preserving the back-entry path that starts at the "gpu" stage. Apply the
same invariant to the corresponding prompt logic around the second resource flow
at lines 677-685, so any emitted OnboardResourceIntent always has a defined
profile.
In `@src/lib/onboard/machine/handlers/provider-inference.ts`:
- Around line 410-425: The accepted-intent rule is duplicated between
needsLegacyConfigurationConfirmation and the reviewedIntentAccepted value at the
call site. Add a shared reviewedIntentAccepted(session) predicate and update
both helpers/call-site logic to use it, preserving the existing non-interactive
and summary behaviors while centralizing the phase check.
In `@src/lib/onboard/machine/handlers/sandbox-messaging.test.ts`:
- Around line 266-271: Update the accepted-draft fixtures in
src/lib/onboard/machine/handlers/sandbox-messaging.test.ts#L266-L271 and
src/lib/onboard/machine/handlers/provider-inference.test.ts#L1435-L1442 to pass
intentDraft through createSession, removing direct post-construction assignment
so parseOnboardIntentDraft is exercised. Preserve the intended accepted-draft
shapes, and in provider-inference.test.ts use a complete answer set if parsing
rejects an empty answers object.
In `@src/lib/onboard/messaging-channel-setup.test.ts`:
- Around line 458-473: Add a companion test near “uses reviewed channel IDs
while requesting their credentials after Apply (`#6005`)” that sets
TELEGRAM_BOT_TOKEN before calling setupMessagingChannels with selectionProvided:
true. Assert the Telegram channel remains selected and prompt is not called for
the token, preserving the existing credential without invoking saveCredential
again.
In `@src/lib/onboard/messaging-channel-setup.ts`:
- Around line 183-189: Update the channel filtering logic in the setup flow
around the enabled Set to identify reviewed channel IDs absent from
availableChannels and emit a note for each dropped channel, following the
existing providerSupported notification pattern from the web-search flow.
Preserve the current filtering and enabled-channel behavior while ensuring every
unsupported reviewed channel is reported.
In `@src/lib/onboard/policy-selection.ts`:
- Around line 402-407: Extract the duplicated applied-preset preservation logic
into a helper that accepts chosen, appliedForPreservation, and suppressedNames
and returns the names retained after excluding already chosen or suppressed
entries. Replace both preservation loops with calls to this helper, preserving
the existing behavior and allowing the non-interactive note to use the returned
list.
In `@src/lib/state/onboard-session/intent-draft.test.ts`:
- Around line 46-64: Add tests alongside the existing normalizeSession coverage
for both materialization boundaries: verify an intent draft with phase
"accepted" survives the save/load round trip unchanged, and verify a draft with
an unsupported version normalizes to null and is not persisted. Reuse the
existing session creation and normalization helpers in the test.
🪄 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: 3dfc15dc-7dd3-4336-bf94-0820bc102794
📒 Files selected for processing (41)
ci/env-var-doc-allowlist.jsondocs/get-started/quickstart-hermes.mdxdocs/get-started/quickstart-langchain-deepagents-code.mdxdocs/get-started/quickstart.mdxdocs/reference/commands.mdxsrc/lib/onboard.tssrc/lib/onboard/credential-navigation.test.tssrc/lib/onboard/credential-navigation.tssrc/lib/onboard/intent-draft/boundary.test.tssrc/lib/onboard/intent-draft/boundary.tssrc/lib/onboard/intent-draft/controller.test.tssrc/lib/onboard/intent-draft/controller.tssrc/lib/onboard/intent-draft/index.tssrc/lib/onboard/intent-draft/runtime.test.tssrc/lib/onboard/intent-draft/runtime.tssrc/lib/onboard/intent-draft/schema.test.tssrc/lib/onboard/intent-draft/schema.tssrc/lib/onboard/intent-draft/seed.test.tssrc/lib/onboard/intent-draft/seed.tssrc/lib/onboard/intent-draft/ui.test.tssrc/lib/onboard/intent-draft/ui.tssrc/lib/onboard/machine/handlers/policies.test.tssrc/lib/onboard/machine/handlers/policies.tssrc/lib/onboard/machine/handlers/provider-inference.test.tssrc/lib/onboard/machine/handlers/provider-inference.tssrc/lib/onboard/machine/handlers/sandbox-messaging.test.tssrc/lib/onboard/machine/handlers/sandbox-messaging.tssrc/lib/onboard/messaging-channel-setup.test.tssrc/lib/onboard/messaging-channel-setup.tssrc/lib/onboard/policy-selection-recorded-tier.test.tssrc/lib/onboard/policy-selection.tssrc/lib/onboard/setup-nim-flow.test.tssrc/lib/onboard/setup-nim-flow.tssrc/lib/onboard/setup-nim-provider-discovery.test.tssrc/lib/onboard/setup-nim-provider-discovery.tssrc/lib/onboard/web-search-flow.test.tssrc/lib/onboard/web-search-flow.tssrc/lib/state/onboard-session.tssrc/lib/state/onboard-session/intent-draft.test.tstest/fixtures/onboard-intent-draft-pty-driver.tstest/onboard-intent-draft-pty.test.ts
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
Sensitive-path security review refreshVerdictPASS on exact head FindingsNo findings. Detailed analysis
Files reviewed
|
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/lib/onboard/intent-draft/ollama-model-selection.ts (1)
97-103: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the injected abort instead of
process.exit(1).
OllamaModelSelectorDepsdeclaresabortNonInteractive: (message: string) => neverfor non-interactive termination. Lines 143 and 160 use it. This branch callsconsole.errorandprocess.exit(1)directly. That gives two mechanisms for one concept, and it terminates the process during tests, so the branch cannot be asserted.Route this branch through
deps.abortNonInteractiveas well.♻️ Proposed refactor
} else if (deps.isNonInteractive()) { - console.error( - ` Ollama model '${selectedModel}' (${sizeLabel}) is not installed and ` + - "non-interactive mode cannot prompt for confirmation. " + - "Re-run with --yes / -y (or NEMOCLAW_YES=1) to authorise the download.", - ); - process.exit(1); + deps.abortNonInteractive( + `Ollama model '${selectedModel}' (${sizeLabel}) is not installed and ` + + "non-interactive mode cannot prompt for confirmation. " + + "Re-run with --yes / -y (or NEMOCLAW_YES=1) to authorise the download.", + ); } else {🤖 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/intent-draft/ollama-model-selection.ts` around lines 97 - 103, Update the non-interactive branch in the Ollama model selection flow to call deps.abortNonInteractive with the existing download-authorization message instead of calling process.exit(1). Preserve the current error context and ensure this branch uses the same injected termination mechanism as the paths near the other abortNonInteractive calls.src/lib/onboard/intent-draft/deps.ts (2)
158-164: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueAlign
compatibility.webSearchwithcompatibility.messaging.
compatibility.messagingreusesmessagingChoices, so a persisted channel stays compatible only when it appears in the offered choices.compatibility.webSearchinstead re-callsagentSupportsWebSearchProviderdirectly and casts an arbitrary string withprovider as DraftWebSearchProvider. A persisted provider outsideoptions.webSearchProviderscan therefore pass compatibility while no matching choice exists in the UI.Reuse
webSearchChoicesfor the compatibility predicate. This removes the duplicated call, drops the unchecked cast, and makes both predicates use one rule.♻️ Proposed refactor
return { prompt: options.prompt, log: (message = "") => console.log(message),Extract the web-search choice builder next to
messagingChoices, then reuse it:+ const webSearchChoices = (agentName: string) => + options.webSearchProviders + .filter((provider) => + agentSupportsWebSearchProvider( + draftAgent(options, agentName), + provider, + fromDockerfile, + options.rootDir, + ), + ) + .map((provider) => ({ value: provider, label: options.webSearchLabelFor(provider) }));- webSearch: (agentName, provider) => - agentSupportsWebSearchProvider( - draftAgent(options, agentName), - provider as DraftWebSearchProvider, - fromDockerfile, - options.rootDir, - ), + webSearch: (agentName, provider) => + webSearchChoices(agentName).some((choice) => choice.value === provider),🤖 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/intent-draft/deps.ts` around lines 158 - 164, Update the compatibility.webSearch predicate to reuse webSearchChoices, matching compatibility.messaging’s choice-based compatibility rule. Move or expose the webSearchChoices builder alongside messagingChoices as needed, remove the direct agentSupportsWebSearchProvider call and DraftWebSearchProvider cast, and ensure persisted providers are compatible only when represented in the offered choices.
95-110: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider caching GPU detection and host probing across draft visits.
inferenceChoicescallsdetectGpu()on every invocation.discoverInferenceIntentChoicesthen callsdetectInferenceProviderHostStatewithprobeOllama: trueandprobeVllm: true(seesrc/lib/onboard/setup-nim-flow.tslines 253-307). The draft flow adds Back and Review navigation, so the user can reach the inference step several times in one session. Each visit repeats GPU detection and provider probing on the prompt path.If fresh probing per visit is not a requirement, cache the detection result for the draft session. If fresh probing is intended, this is acceptable as-is.
🤖 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/intent-draft/deps.ts` around lines 95 - 110, Cache the GPU detection and inference-provider host probing within the draft session so repeated inferenceChoices invocations reuse the existing results. Update the inferenceChoices flow and its supporting discovery call, using session-scoped memoization while preserving the current choice mapping and probe behavior; avoid caching only if fresh detection is explicitly required.
🤖 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/intent-draft/ollama-model-selection.ts`:
- Around line 66-81: The locked-model loop can repeat indefinitely after
non-probe validation failures or declined downloads because those branches
continue with the same lockedModel. In the validation-failure and
download-declined branches, return { outcome: "back-to-selection" } when
validation.retry !== "selection" or the download is declined, and add tests
covering both outcomes while preserving the existing probe-failure retry
behavior.
---
Nitpick comments:
In `@src/lib/onboard/intent-draft/deps.ts`:
- Around line 158-164: Update the compatibility.webSearch predicate to reuse
webSearchChoices, matching compatibility.messaging’s choice-based compatibility
rule. Move or expose the webSearchChoices builder alongside messagingChoices as
needed, remove the direct agentSupportsWebSearchProvider call and
DraftWebSearchProvider cast, and ensure persisted providers are compatible only
when represented in the offered choices.
- Around line 95-110: Cache the GPU detection and inference-provider host
probing within the draft session so repeated inferenceChoices invocations reuse
the existing results. Update the inferenceChoices flow and its supporting
discovery call, using session-scoped memoization while preserving the current
choice mapping and probe behavior; avoid caching only if fresh detection is
explicitly required.
In `@src/lib/onboard/intent-draft/ollama-model-selection.ts`:
- Around line 97-103: Update the non-interactive branch in the Ollama model
selection flow to call deps.abortNonInteractive with the existing
download-authorization message instead of calling process.exit(1). Preserve the
current error context and ensure this branch uses the same injected termination
mechanism as the paths near the other abortNonInteractive calls.
🪄 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: b569ba39-4286-4645-8bf5-6dd4cc9da126
📒 Files selected for processing (6)
ci/source-architecture-budget.jsonsrc/lib/onboard.tssrc/lib/onboard/intent-draft/deps.tssrc/lib/onboard/intent-draft/index.tssrc/lib/onboard/intent-draft/ollama-model-selection.tssrc/lib/onboard/intent-draft/runtime.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/lib/onboard/intent-draft/index.ts
- src/lib/onboard/intent-draft/runtime.ts
- src/lib/onboard.ts
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/lib/onboard/intent-draft/seed.test.ts (1)
9-24: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winProve CLI precedence with conflicting environment values.
This test does not distinguish CLI precedence from environment precedence. Add conflicting
NEMOCLAW_AGENTandNEMOCLAW_POLICY_TIERvalues. Keep the expected CLI values.Proposed test update
- it("preserves explicit CLI and environment choices for review", () => { + it("uses CLI choices over environment choices for review", () => { const draft = seedOnboardIntentDraft( { agent: "hermes", sandboxGpu: "enable", policyTier: "restricted", }, "hermes-demo", { + NEMOCLAW_AGENT: "openclaw", NEMOCLAW_PROVIDER: "hermesProvider", NEMOCLAW_MODEL: "claude", NEMOCLAW_HERMES_AUTH_METHOD: "oauth", NEMOCLAW_WEB_SEARCH_PROVIDER: "tavily", NEMOCLAW_RESOURCE_PROFILE: "large", + NEMOCLAW_POLICY_TIER: "balanced", }, );As per path instructions, review tests for behavioral confidence rather than implementation lock-in.
🤖 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/intent-draft/seed.test.ts` around lines 9 - 24, Update the test case “preserves explicit CLI and environment choices for review” to add environment values for NEMOCLAW_AGENT and NEMOCLAW_POLICY_TIER that conflict with the CLI agent and policyTier inputs. Keep the expected assertions focused on the existing CLI values so the test verifies CLI precedence while preserving the current behavior checks.Source: Path instructions
src/lib/onboard.ts (1)
4006-4061: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftComplete intent-draft cutover coverage.
runOnboardstill callsselectOnboardAgentandsetupNimafter the boundary. Add public-entrypoint tests for fresh, resume, retry, rebuild, and exit flows that prove accepted values prevent superseded prompts and selections.🤖 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.ts` around lines 4006 - 4061, Complete the intent-draft migration in runOnboard by removing or bypassing post-boundary calls to selectOnboardAgent and setupNim when accepted draft values already provide those selections. Add public-entrypoint coverage for fresh, resume, retry, rebuild, and exit flows, verifying accepted values prevent the superseded prompts and selections while exit preserves the saved-draft return behavior.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.
Inline comments:
In `@src/lib/onboard/intent-draft/boundary.test.ts`:
- Around line 95-103: Update the rejection test for
crossOnboardIntentDraftBoundary to retain references to the collect and accept
mocks, then assert both were not called after the promise rejects. Keep the
existing rejection message assertion and verify the Apply boundary rejects
before invoking either operation.
---
Outside diff comments:
In `@src/lib/onboard.ts`:
- Around line 4006-4061: Complete the intent-draft migration in runOnboard by
removing or bypassing post-boundary calls to selectOnboardAgent and setupNim
when accepted draft values already provide those selections. Add
public-entrypoint coverage for fresh, resume, retry, rebuild, and exit flows,
verifying accepted values prevent the superseded prompts and selections while
exit preserves the saved-draft return behavior.
In `@src/lib/onboard/intent-draft/seed.test.ts`:
- Around line 9-24: Update the test case “preserves explicit CLI and environment
choices for review” to add environment values for NEMOCLAW_AGENT and
NEMOCLAW_POLICY_TIER that conflict with the CLI agent and policyTier inputs.
Keep the expected assertions focused on the existing CLI values so the test
verifies CLI precedence while preserving the current behavior checks.
🪄 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: 3db04f2c-6c0f-4f90-9235-495fa17ee362
📒 Files selected for processing (23)
docs/get-started/quickstart-langchain-deepagents-code.mdxdocs/reference/commands.mdxsrc/lib/onboard.tssrc/lib/onboard/intent-draft/boundary.test.tssrc/lib/onboard/intent-draft/boundary.tssrc/lib/onboard/intent-draft/deps.tssrc/lib/onboard/intent-draft/ollama-model-selection.test.tssrc/lib/onboard/intent-draft/ollama-model-selection.tssrc/lib/onboard/intent-draft/schema.test.tssrc/lib/onboard/intent-draft/schema.tssrc/lib/onboard/intent-draft/seed.test.tssrc/lib/onboard/intent-draft/seed.tssrc/lib/onboard/intent-draft/ui.test.tssrc/lib/onboard/intent-draft/ui.tssrc/lib/onboard/messaging-channel-setup.test.tssrc/lib/onboard/policy-selection-recorded-tier.test.tssrc/lib/onboard/setup-nim-flow.tssrc/lib/onboard/setup-nim-provider-discovery.test.tssrc/lib/onboard/setup-nim-provider-discovery.tssrc/lib/state/onboard-session.tssrc/lib/state/onboard-session/intent-draft.test.tstest/fixtures/onboard-intent-draft-pty-driver.tstest/onboard-intent-draft-pty.test.ts
🚧 Files skipped from review as they are similar to previous changes (15)
- src/lib/onboard/messaging-channel-setup.test.ts
- src/lib/onboard/setup-nim-provider-discovery.test.ts
- src/lib/state/onboard-session.ts
- docs/get-started/quickstart-langchain-deepagents-code.mdx
- src/lib/onboard/intent-draft/schema.test.ts
- test/fixtures/onboard-intent-draft-pty-driver.ts
- src/lib/onboard/setup-nim-flow.ts
- docs/reference/commands.mdx
- src/lib/onboard/intent-draft/deps.ts
- test/onboard-intent-draft-pty.test.ts
- src/lib/onboard/intent-draft/ollama-model-selection.ts
- src/lib/onboard/intent-draft/seed.ts
- src/lib/onboard/intent-draft/boundary.ts
- src/lib/onboard/intent-draft/ui.ts
- src/lib/onboard/intent-draft/schema.ts
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
|
CodeRabbit final-pass follow-up on
Validation: focused 68/68, changed 10/10, CLI typecheck, test-size, and test-conditional checks passed. Exact-head documentation writer review is PASS ( |
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
|
Addressed exact-head advisor blocker Accepted drafts are now revalidated against the current agent, inference provider/auth, web-search, messaging, managed-tool, resource-profile, sandbox-name, model-safety, and policy menus immediately before the materialization callback. If any recorded choice has become unavailable, onboarding throws the documented fail-closed diagnostic before projecting accepted inputs or starting provider, gateway, Docker, policy, or sandbox work. Regressions cover a removed provider on an accepted resume and prove the materialization callback is not invoked after validation rejects. Validation: focused 81/81, |
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
|
Addressed PRA-1 in |
|
Exact-head CI status for |
|
Exact-head queue follow-up for The PR body’s security and documentation receipts are still bound to |
cv
left a comment
cv
left a comment
There was a problem hiding this comment.
Reviewed commit 84fb9aee2. The feature is tied to accepted issue #6005, keeps raw credentials out of the persisted draft, rejects endpoint userinfo/query/fragment input, and keeps materialization behind an accepted review boundary. The commits after the recorded security and documentation reviews are base-refresh merges; I found no new authored behavior or unresolved major review finding. The test inventory covers controller transitions, invalidation, resume revalidation, policy/provider/message handoff, schema, seed, and the PTY boundary.
Approval is blocked because this commit has none of the six required merge-gate checks attached. Do not infer readiness from the earlier local and review evidence. The documentation-writer and sensitive-path receipts also identify commit c522b7f74, so they must be rerun for the current commit before approval.
4 participants
Summary
Onboarding now collects configuration in a reversible draft, shows one Review screen, and materializes credentials, policies, and sandbox state only after the user selects Apply configuration. Users can walk Back to Step 1, preserve compatible later answers, edit any choice directly from Review, and return immediately to Review when no dependent answer needs to be collected again.
Related Issue
Closes #6005.
Supersedes #7190 while preserving its
bdiscoverability intent through the complete pre-materialization workflow required by the issue.Changes
nemoclaw onboard, with typed answer, Back, Review edit, Apply, and Exit transitions. Issue [All Platforms][Onboard][GitHub Issue #6005] nemoclaw onboard wizard has no back-navigation — users cannot correct a previous step without Ctrl+C #6005 requires navigation before external effects; a direct key alias in the existing interleaved flow cannot safely undo created sandboxes or collected credentials. Controller, UI, boundary, runtime, schema, seed, session, and PTY tests protect this contract.--freshpaths.Type of Change
Quality Gates
c522b7f74with no findings: feat(onboard): add reversible configuration review #8171 (comment)Documentation Writer Review
docs-updatednpm run docspassed with 66 guarded routes, 0 errors, and 2 existing 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 — GitHub CI pending; the local Docker daemon is unavailable.npm run docsbuilds without warnings (doc changes only)Signed-off-by: Prekshi Vyas prekshiv@nvidia.com
Summary by CodeRabbit
New Features
Bug Fixes
Documentation