From 0113d89ab870a40a81690faffaf69e42313a5509 Mon Sep 17 00:00:00 2001 From: Jarvis Date: Thu, 25 Jun 2026 13:22:59 -0500 Subject: [PATCH] fix(wizard): avoid rerunning completed setup steps --- docs/scratchpads/B4-wizard-step-dedup.md | 36 +++++++++ .../integration/unified-wizard.test.ts | 76 +++++++++++++++++++ .../mosaic/src/stages/gateway-config.spec.ts | 39 ++++++++++ packages/mosaic/src/stages/gateway-config.ts | 3 + packages/mosaic/src/stages/quick-start.ts | 4 +- packages/mosaic/src/wizard.ts | 30 ++++++-- 6 files changed, 182 insertions(+), 6 deletions(-) create mode 100644 docs/scratchpads/B4-wizard-step-dedup.md diff --git a/docs/scratchpads/B4-wizard-step-dedup.md b/docs/scratchpads/B4-wizard-step-dedup.md new file mode 100644 index 0000000..cc8d4ce --- /dev/null +++ b/docs/scratchpads/B4-wizard-step-dedup.md @@ -0,0 +1,36 @@ +# B4 — Wizard step deduplication + +## Problem + +Greenfield wizard testing showed completed wizard steps could be executed again after the menu marked them `[done]`. In practice this made the Providers/API-key flow and Skills flow appear twice in one wizard run. + +There was a second related API-key duplication path: when the Providers step was completed with no key, `gatewayConfigStage` still prompted for `ANTHROPIC_API_KEY` during Finish because it only skipped the gateway API-key prompt when `providerKey` was non-empty. + +## Diagnosis + +- `runMenuLoop` labeled completed sections with `[done]`, but still dispatched the selected step again if the user selected that row. +- Quick Start ran Providers and Skills but did not mark those sections complete in `completedSections`. +- `runFinishPath`/`quickStartPath` defaulted `providerType` to `none` for gateway config, which made it impossible for `gatewayConfigStage` to distinguish: + - provider step completed and user intentionally skipped the key, vs. + - provider step was never run. + +## Fix + +- Added a shared menu section key helper and a completed-step guard in `runMenuLoop`. +- Completed menu steps now log a skip message instead of re-running their stage. +- Quick Start marks Providers and Skills complete after running them. +- Finish/Quick Start now pass `state.providerType` as-is to gateway config instead of defaulting to `none`. +- `gatewayConfigStage` treats `providerType: 'none'` as an explicit completed provider setup with no key and skips the second gateway API-key prompt. + +## Verification + +- Added unified wizard regression coverage asserting repeated Providers/Skills menu selections only execute each stage once. +- Added gateway config coverage asserting `providerType: 'none'` does not prompt for a gateway API key and writes no API key env var. +- Targeted: `pnpm --filter @mosaicstack/mosaic test -- unified-wizard gateway-config` +- `pnpm format:check` +- `pnpm typecheck` +- `pnpm lint` +- `pnpm build` +- `pnpm test` +- Codex code review: approve. +- Codex security review: no findings. diff --git a/packages/mosaic/__tests__/integration/unified-wizard.test.ts b/packages/mosaic/__tests__/integration/unified-wizard.test.ts index 5becc83..764f5b2 100644 --- a/packages/mosaic/__tests__/integration/unified-wizard.test.ts +++ b/packages/mosaic/__tests__/integration/unified-wizard.test.ts @@ -11,9 +11,37 @@ import { join } from 'node:path'; import { tmpdir } from 'node:os'; import { HeadlessPrompter } from '../../src/prompter/headless-prompter.js'; import { createConfigService } from '../../src/config/config-service.js'; +import type { SelectOption } from '../../src/prompter/interface.js'; +import type { MenuSection, WizardState } from '../../src/types.js'; const gatewayConfigMock = vi.fn(); const gatewayBootstrapMock = vi.fn(); +const providerSetupMock = vi.fn(); +const skillsSelectMock = vi.fn(); + +class SequencedMenuPrompter extends HeadlessPrompter { + constructor( + answers: Record, + private readonly menuChoices: string[], + ) { + super(answers); + } + + override async select(opts: { + message: string; + options: SelectOption[]; + initialValue?: T; + }): Promise { + if (opts.message === 'What would you like to configure?') { + const next = this.menuChoices.shift(); + if (!next) throw new Error('No queued menu choice left'); + const match = opts.options.find((o) => String(o.value) === next); + if (!match) throw new Error(`Queued menu choice not available: ${next}`); + return match.value; + } + return super.select(opts); + } +} vi.mock('../../src/stages/gateway-config.js', () => ({ gatewayConfigStage: (...args: unknown[]) => gatewayConfigMock(...args), @@ -23,6 +51,14 @@ vi.mock('../../src/stages/gateway-bootstrap.js', () => ({ gatewayBootstrapStage: (...args: unknown[]) => gatewayBootstrapMock(...args), })); +vi.mock('../../src/stages/provider-setup.js', () => ({ + providerSetupStage: (...args: unknown[]) => providerSetupMock(...args), +})); + +vi.mock('../../src/stages/skills-select.js', () => ({ + skillsSelectStage: (...args: unknown[]) => skillsSelectMock(...args), +})); + // Import AFTER the mocks so runWizard picks up the mocked stage modules. import { runWizard } from '../../src/wizard.js'; @@ -44,6 +80,16 @@ describe('Unified wizard (runWizard with default skipGateway)', () => { } gatewayConfigMock.mockReset(); gatewayBootstrapMock.mockReset(); + providerSetupMock.mockReset(); + skillsSelectMock.mockReset(); + providerSetupMock.mockImplementation(async (_p: HeadlessPrompter, state: WizardState) => { + state.providerType = 'none'; + state.completedSections?.add('providers' satisfies MenuSection); + }); + skillsSelectMock.mockImplementation(async (_p: HeadlessPrompter, state: WizardState) => { + state.selectedSkills = []; + state.completedSections?.add('skills' satisfies MenuSection); + }); // Pretend we're on an interactive TTY so the wizard's headless-abort // branch does not call `process.exit(1)` during these tests. Object.defineProperty(process.stdin, 'isTTY', { value: true, configurable: true }); @@ -184,4 +230,34 @@ describe('Unified wizard (runWizard with default skipGateway)', () => { expect(gatewayConfigMock).not.toHaveBeenCalled(); expect(gatewayBootstrapMock).not.toHaveBeenCalled(); }); + + it('does not re-run completed provider or skills menu steps', async () => { + const prompter = new SequencedMenuPrompter( + { + 'What name should agents use?': 'TestBot', + 'Communication style': 'direct', + 'Your name': 'Tester', + 'Your pronouns': 'They/Them', + 'Your timezone': 'UTC', + }, + ['providers', 'providers', 'skills', 'skills', 'finish'], + ); + + await runWizard({ + mosaicHome: tmpDir, + sourceDir: tmpDir, + prompter, + configService: createConfigService(tmpDir, tmpDir), + skipGateway: true, + }); + + expect(providerSetupMock).toHaveBeenCalledTimes(1); + expect(skillsSelectMock).toHaveBeenCalledTimes(1); + expect(prompter.getLogs()).toEqual( + expect.arrayContaining([ + expect.stringContaining('Providers [done] is already complete; skipping.'), + expect.stringContaining('Skills [done] is already complete; skipping.'), + ]), + ); + }); }); diff --git a/packages/mosaic/src/stages/gateway-config.spec.ts b/packages/mosaic/src/stages/gateway-config.spec.ts index fe71e48..52554ac 100644 --- a/packages/mosaic/src/stages/gateway-config.spec.ts +++ b/packages/mosaic/src/stages/gateway-config.spec.ts @@ -167,6 +167,45 @@ describe('gatewayConfigStage', () => { expect(state.gateway?.regeneratedConfig).toBe(true); }); + it('does not ask for a gateway API key when provider setup was completed with no key', async () => { + delete process.env['MOSAIC_ASSUME_YES']; + const originalIsTTY = process.stdin.isTTY; + Object.defineProperty(process.stdin, 'isTTY', { value: true, configurable: true }); + + try { + const textFn = vi.fn(async (opts: { message: string; initialValue?: string }) => { + if (opts.message === 'Gateway port') return opts.initialValue ?? '14242'; + if (opts.message === 'Web UI hostname (for browser access)') return 'localhost'; + if (opts.message.includes('API_KEY')) { + throw new Error('gateway API key prompt should be skipped'); + } + return ''; + }); + const p = buildPrompter({ text: textFn, select: vi.fn().mockResolvedValue('local') }); + const state = makeState('/home/user/.config/mosaic'); + + const result = await gatewayConfigStage(p, state, { + host: 'localhost', + defaultPort: 14242, + skipInstall: true, + providerType: 'none', + }); + + expect(result.ready).toBe(true); + expect(textFn).not.toHaveBeenCalledWith( + expect.objectContaining({ message: expect.stringContaining('API_KEY') }), + ); + const envContents = readFileSync(daemonState.envFile, 'utf-8'); + expect(envContents).not.toContain('ANTHROPIC_API_KEY='); + expect(envContents).not.toContain('OPENAI_API_KEY='); + } finally { + Object.defineProperty(process.stdin, 'isTTY', { + value: originalIsTTY, + configurable: true, + }); + } + }); + it('short-circuits when gateway is already fully installed and user declines rerun', async () => { // Pre-populate both files + running daemon + meta with token const fs = require('node:fs'); diff --git a/packages/mosaic/src/stages/gateway-config.ts b/packages/mosaic/src/stages/gateway-config.ts index fdfd8ba..b6abde3 100644 --- a/packages/mosaic/src/stages/gateway-config.ts +++ b/packages/mosaic/src/stages/gateway-config.ts @@ -506,6 +506,9 @@ async function collectAndWriteConfig( if (opts.providerKey) { anthropicKey = opts.providerKey; p.log(`Using API key from provider setup (${opts.providerType ?? 'unknown'}).`); + } else if (opts.providerType === 'none') { + anthropicKey = ''; + p.log('No API key provided during provider setup; skipping gateway API key prompt.'); } else { anthropicKey = await p.text({ message: 'ANTHROPIC_API_KEY (optional, press Enter to skip)', diff --git a/packages/mosaic/src/stages/quick-start.ts b/packages/mosaic/src/stages/quick-start.ts index f132cc4..aa2d63e 100644 --- a/packages/mosaic/src/stages/quick-start.ts +++ b/packages/mosaic/src/stages/quick-start.ts @@ -37,6 +37,7 @@ export async function quickStartPath( // 1. Provider setup (first question) await providerSetupStage(prompter, state); + state.completedSections?.add('providers'); // Apply sensible defaults for everything else state.soul.agentName ??= 'Mosaic'; @@ -57,6 +58,7 @@ export async function quickStartPath( // Skills (recommended set, no user input in quick mode) await skillsSelectStage(prompter, state); + state.completedSections?.add('skills'); // Finalize writes configs/assets/skills, but defer the success summary until // after the gateway health/bootstrap gates complete. @@ -75,7 +77,7 @@ export async function quickStartPath( portOverride: options.gatewayPortOverride, skipInstall: options.skipGatewayNpmInstall, providerKey: state.providerKey, - providerType: state.providerType ?? 'none', + providerType: state.providerType, }); if (!configResult.ready || !configResult.host || !configResult.port) { diff --git a/packages/mosaic/src/wizard.ts b/packages/mosaic/src/wizard.ts index 757aeee..571b724 100644 --- a/packages/mosaic/src/wizard.ts +++ b/packages/mosaic/src/wizard.ts @@ -126,6 +126,11 @@ type MenuChoice = | 'advanced' | 'finish'; +function menuSectionKey(section: MenuChoice): MenuSection | null { + if (section === 'quick-start' || section === 'finish') return null; + return section === 'gateway-config' ? 'gateway' : section; +} + function menuLabel(section: MenuChoice, completed: Set): string { const labels: Record = { 'quick-start': 'Quick Start', @@ -137,14 +142,24 @@ function menuLabel(section: MenuChoice, completed: Set): string { finish: 'Finish & Apply', }; const base = labels[section]; - const sectionKey: MenuSection = - section === 'gateway-config' ? 'gateway' : (section as MenuSection); - if (completed.has(sectionKey)) { + const sectionKey = menuSectionKey(section); + if (sectionKey && completed.has(sectionKey)) { return `${base} [done]`; } return base; } +function skipCompletedMenuChoice( + prompter: WizardPrompter, + completed: Set, + choice: MenuChoice, +): boolean { + const sectionKey = menuSectionKey(choice); + if (!sectionKey || !completed.has(sectionKey)) return false; + prompter.log(`${menuLabel(choice, completed)} is already complete; skipping.`); + return true; +} + async function runMenuLoop( prompter: WizardPrompter, state: WizardState, @@ -201,21 +216,25 @@ async function runMenuLoop( return; // Quick start is a complete flow — exit menu case 'providers': + if (skipCompletedMenuChoice(prompter, completed, choice)) break; await providerSetupStage(prompter, state); completed.add('providers'); break; case 'identity': + if (skipCompletedMenuChoice(prompter, completed, choice)) break; await agentIntentStage(prompter, state); completed.add('identity'); break; case 'skills': + if (skipCompletedMenuChoice(prompter, completed, choice)) break; await skillsSelectStage(prompter, state); completed.add('skills'); break; case 'gateway-config': + if (skipCompletedMenuChoice(prompter, completed, choice)) break; // Gateway config is handled during Finish — mark as "configured" // after user reviews settings. await runGatewaySubMenu(prompter, state, options); @@ -223,6 +242,7 @@ async function runMenuLoop( break; case 'advanced': + if (skipCompletedMenuChoice(prompter, completed, choice)) break; await runAdvancedSubMenu(prompter, state); completed.add('advanced'); break; @@ -325,7 +345,7 @@ async function runFinishPath( portOverride: options.gatewayPortOverride, skipInstall: options.skipGatewayNpmInstall, providerKey: state.providerKey, - providerType: state.providerType ?? 'none', + providerType: state.providerType, }); if (configResult.ready && configResult.host && configResult.port) { @@ -396,7 +416,7 @@ async function runHeadlessPath( portOverride: options.gatewayPortOverride, skipInstall: options.skipGatewayNpmInstall, providerKey: state.providerKey, - providerType: state.providerType ?? 'none', + providerType: state.providerType, }); if (!configResult.ready || !configResult.host || !configResult.port) {