Skip to content

Rework onboarding around one default harness - #3574

Open
morgmart wants to merge 6 commits into
mainfrom
morgan/onboarding-agent-config
Open

Rework onboarding around one default harness#3574
morgmart wants to merge 6 commits into
mainfrom
morgan/onboarding-agent-config

Conversation

@morgmart

Copy link
Copy Markdown
Contributor

Summary

  • turns agent onboarding into a two-step flow: choose one default harness, then set up only that harness
  • keeps chooser selection local until Finish, with Skip for now and Back preserving a user's choice
  • reuses the existing install, sign-in, provider/model, baked-default, runtime-file, and readiness behavior for the selected harness
  • keeps provider/model setup visible until completion, hides unsupported Goose effort, and preserves runtime-specific defaults and auth behavior
  • adds focused coverage for install/sign-in failures, catalog retries, baked defaults, same-harness persistence, and sensitive error handling

Product behavior

  1. Choose your default harness shows Claude Code, Codex, Goose, and Buzz as simple single-choice cards. Clicking one advances immediately.
  2. Set up the selected harness shows only the work that choice needs:
    • install the local CLI/adapter when missing
    • sign in for Claude Code or Codex
    • choose provider/model credentials for Goose or Buzz
    • skip setup entirely when runtime-file or baked build defaults already satisfy it
  3. Finish is enabled only when the selected harness is ready. preferred_runtime is persisted only on Finish.

Testing

  • Biome check
  • TypeScript check
  • desktop file-size guard
  • Agent Config Core tests: 5 passed
  • onboarding Playwright E2E: 20 passed
  • pre-push hooks: desktop check/test, Rust tests, Tauri tests, and Tauri clippy passed

@morgmart

Copy link
Copy Markdown
Contributor Author

🤖 ## Screenshots

Choose a default harness

The chooser is a pure local selection. Clicking a card advances immediately; setup is deferred to the selected harness.

Choose a default harness

Install a missing harness

The setup page explains what is missing, runs installation locally, and keeps Finish disabled until the harness is ready.

Install a missing harness

Configure provider and model

Provider-backed harnesses show only the required first-run defaults. Labels stay accessible while the compact onboarding treatment uses field placeholders.

Configure provider and model

Signed-off-by: morgmart <98432065+morgmart@users.noreply.github.com>
@morgmart
morgmart force-pushed the morgan/onboarding-agent-config branch from b477195 to 917a7ae Compare July 29, 2026 16:09
morgmart added 2 commits July 29, 2026 09:36
Signed-off-by: morgmart <98432065+morgmart@users.noreply.github.com>
Signed-off-by: morgmart <98432065+morgmart@users.noreply.github.com>
@morgmart
morgmart marked this pull request as ready for review July 29, 2026 18:01
@morgmart
morgmart requested a review from a team as a code owner July 29, 2026 18:01

@jedwards27 jedwards27 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:bot: Jude’s code review agent

Reviewed base bbd20fae75ecc3bd7a83cc12a65379fac22a2b79 through head 0027b461bf817a69416973fe80ae5e4d74097c0a.

Blocking finding

[P1] Fail closed when the existing global agent config cannot be readdesktop/src/features/onboarding/ui/DefaultConfigStep.tsx:561-585,622-624,650-658

loadDefaults() uses Promise.allSettled, but a rejected getGlobalAgentConfig() is silently ignored. The component therefore leaves config at EMPTY_GLOBAL_CONFIG and clears isConfigLoading. For an already-ready Claude/Codex selection, canFinish becomes true. Finish then persists the empty fallback plus the selected runtime, erasing unread provider/model values and credential environment variables from the user's existing config.

This is not hypothetical: a temporary sequenced Playwright probe seeded non-empty config, allowed the app's initial read to succeed, rejected the config-page read, and asserted that Finish remained disabled. It failed causally with Expected disabled; Received enabled. The probe was removed afterward and the checkout returned clean.

Please track config-read failure as an explicit blocking/error state, offer retry/back, and do not enable Finish or call setGlobalAgentConfig until the existing config has loaded successfully. Add permanent E2E coverage proving a rejected read cannot Finish or mutate persisted config, then that retry succeeds and preserves all existing fields except the intended runtime change.

Validation

At exact clean head 0027b461bf817a69416973fe80ae5e4d74097c0a:

  • Desktop check and typecheck passed (two pre-existing informational useTemplate diagnostics outside the PR diff).
  • Full Desktop JS suite passed: 3,769/3,769.
  • Focused onboarding Playwright suite passed: 20/20 after a clean rerun. An earlier local Python server exit caused four ERR_CONNECTION_REFUSED infrastructure failures after 16 passes; the rerun cleared them.
  • All applicable GitHub checks are green.
  • Native GUI/lifecycle validation was not run; focused Playwright exercised the changed UI.
  • No relay/schema/identity/release changes were found in the 16-file three-dot diff.
  • GitHub currently reports the PR as conflicting/dirty. Conflict resolution will create a new head and require fresh delta review.

The happy path is well covered. The read boundary currently fails open and can destroy saved credentials, so this head is not safe to merge.

Signed-off-by: morgmart <98432065+morgmart@users.noreply.github.com>
…nt-config

Signed-off-by: morgmart <98432065+morgmart@users.noreply.github.com>
Signed-off-by: morgmart <98432065+morgmart@users.noreply.github.com>

@jedwards27 jedwards27 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:bot: Jude’s code review agent — changes requested at 8b85a2b315b686b4abf6a43e8a0d1e570004a72c.

The config-read failure is now fenced on initial load, but this head still has two material blockers.

[P1] Retry re-enables Finish before the replacement config read succeeds

desktop/src/features/onboarding/ui/DefaultConfigStep.tsx:94-100 sets isLoading=true but also clears configLoadError before getGlobalAgentConfig() settles. The completion gate at :220-235 checks !configLoadError but not loading, and handleComplete at :350-367 does not defensively re-check persistenceState.canComplete.

After Back preserves a valid dirty draft, a failed read followed by Try again therefore makes Finish actionable during the pending retry. The stale commit can overwrite newer/external config before disk state is known.

A causal Playwright probe delayed the successful retry read by 2s and asserted Finish was disabled immediately after Try again. It failed with Expected disabled; Received enabled. Please include loading in the completion fence, guard handleComplete with the current completion state, and add a deferred-read regression proving no completion/write occurs until retry succeeds.

[P1] The merge replaced the PR’s single-harness journey with the old multi-harness setup

The PR contract says: choose one default harness, advance immediately, then configure only that harness. Instead, desktop/src/features/onboarding/ui/SetupStep.tsx:619-642 renders Set up your agent harnesses and a RuntimeCard for every visible runtime. At :694-729 it collects every ready runtime and advances only through a generic Next button; MachineOnboardingFlow.tsx:539-550 receives that entire set. The intended chooser is now dead code: repo-wide search under desktop/src and desktop/tests finds HarnessChoiceCard only in its own definition.

This is user-visible and opposite to the PR title/body: newcomers again face installation/sign-in choices for all harnesses rather than one selected default. The changed screenshot journey catches it: the focused onboarding/docked-CTA run fails because Choose your default harness is absent (onboarding-docked-cta-screenshots.spec.ts:129-131; 3 passed, 1 failed). The rewritten defaults spec passing does not validate the promised journey.

Please restore the single-choice setup behavior, scope the following page to only that runtime, and restore a journey test proving card click immediately advances, other harness controls are absent, Back preserves selection, and no config is written before Finish.

Validation at this exact head

  • Live PR head and local clean HEAD: 8b85a2b315b686b4abf6a43e8a0d1e570004a72c
  • git diff --check f88cda9eb886500ec7d205e1d265ac6f654aa433...HEAD: pass
  • pnpm --dir desktop check: pass with four diagnostics in unchanged files
  • pnpm --dir desktop typecheck: pass
  • Full Desktop JS suite: 5,099/5,099 pass
  • Focused config rejection/preserve/retry regression: pass; mutation-removing the fail-closed branch makes it fail
  • Focused defaults spec: 26/26 pass
  • Onboarding/docked-CTA impact run: 3/4; fails on missing chooser heading
  • Deferred retry mutation probe: fails because Finish is enabled while read is pending
  • GitHub checks at this head are non-green: Desktop Smoke E2E (2), Desktop Smoke E2E (3), Desktop E2E Integration (1/2), and aggregate Desktop E2E Integration fail
  • Scope is 11 Desktop frontend/E2E files; no relay, schema, identity, or release changes found
  • Native GUI was not launched on the shared machine; Playwright exercised the renderer journey

The initial read-error repair is substantial, but a timed persistence hole and removal of the feature’s primary journey both remain merge-blocking.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants