From 9e07bdd3f69e5bdd1f7dc327f9d6621c1c8a1cdf Mon Sep 17 00:00:00 2001 From: up <1457368987@qq.com> Date: Fri, 7 Aug 2026 12:56:26 +0800 Subject: [PATCH 1/2] fix(tui): derive required setup fields from schema - Background: required-fields routing duplicated LLM provider rules instead of using the TUI schema. - Changes: add boolean/predicate required metadata, mark provider fields in the schema, validate active visible fields generically, and support persisted secret-location references safely. - Tests: cover conditional and hidden requirements, empty values, keyless Ollama, secret pointers, matching legacy provider blocks, and entry routing. - Verification: targeted Vitest (34 tests), npm run lint, npm run build, git diff --check, and independent review passed. Full npm test reached 8,361 passing tests; 18 unrelated environment/timing failures remained in browser, REPL, health, and packaged-binary suites. Fixes #257 --- src/cli/tui/schema/llm.ts | 7 +- src/cli/tui/schema/types.ts | 1 + src/cli/tui/state/required-fields.ts | 75 ++++++++++------ .../cli/tui/entry-required-fields.test.ts | 24 +++++ tests/unit/cli/tui/schema/llm.test.ts | 4 + .../cli/tui/state/required-fields.test.ts | 87 ++++++++++++++++++- 6 files changed, 171 insertions(+), 27 deletions(-) diff --git a/src/cli/tui/schema/llm.ts b/src/cli/tui/schema/llm.ts index 38bfe29d4..3de47b7c9 100644 --- a/src/cli/tui/schema/llm.ts +++ b/src/cli/tui/schema/llm.ts @@ -1,5 +1,8 @@ import type { CategoryDef } from './types.js'; +const usesApiKey = (ctx: Parameters>[0]): boolean => + (ctx.pending.llmProvider ?? ctx.current.llmProvider) !== 'ollama'; + export const llmCategory: CategoryDef = { id: 'llm', label: 'LLM Provider', @@ -21,6 +24,7 @@ export const llmCategory: CategoryDef = { }, ], default: 'anthropic', + required: true, }, { key: 'WIGOLO_LLM_API_KEY', @@ -29,10 +33,11 @@ export const llmCategory: CategoryDef = { kind: 'masked', secret: true, propagateToAgents: true, + required: usesApiKey, help: 'Stored in OS keychain when available; never written to config.json.', // Ollama is keyless — hide the API-key field when it's the chosen provider // so the wizard never prompts for a credential the local server ignores. - visible: (ctx) => (ctx.pending.llmProvider ?? ctx.current.llmProvider) !== 'ollama', + visible: usesApiKey, }, ], }; diff --git a/src/cli/tui/schema/types.ts b/src/cli/tui/schema/types.ts index ab048bfd2..5d33722d4 100644 --- a/src/cli/tui/schema/types.ts +++ b/src/cli/tui/schema/types.ts @@ -31,6 +31,7 @@ export interface FieldDef { max?: number; secret?: true; propagateToAgents?: boolean; + required?: boolean | ((ctx: Ctx) => boolean); visible?: (ctx: Ctx) => boolean; validate?: (v: unknown) => string | null; futureNote?: string; diff --git a/src/cli/tui/state/required-fields.ts b/src/cli/tui/state/required-fields.ts index b120bf00d..f781fa89f 100644 --- a/src/cli/tui/state/required-fields.ts +++ b/src/cli/tui/state/required-fields.ts @@ -1,28 +1,53 @@ -/** - * hasRequiredFields — pure predicate for entry routing. - * - * Returns true when the persisted config is complete enough to skip the - * first-run wizard: - * - a non-empty `llmProvider`, AND - * - for keyed cloud providers, a non-empty `llmApiKey`. - * - * The `ollama` provider is KEYLESS (it runs against a local LLM server), so it - * is complete with NO api key — requiring one would re-route a near-zero- - * friction ollama user back into the wizard. Cloud providers still require a - * key; absence of either field sends the user into setup. - * - * TODO: derive from schema once FieldDef carries a `required` flag — - * then this can auto-collect all required fields from CATALOG instead of - * hardcoding the paths above. - */ import type { PersistedConfig } from '../../../persisted-config.js'; +import { CATALOG } from '../schema/catalog.js'; +import type { CategoryDef, Ctx, FieldDef } from '../schema/types.js'; -export function hasRequiredFields(config: PersistedConfig): boolean { - const { settings } = config; - const provider = settings.llmProvider; - if (typeof provider !== 'string' || provider.length === 0) return false; - // Keyless local LLM server — no api key needed to count as configured. - if (provider === 'ollama') return true; - const key = settings.llmApiKey; - return typeof key === 'string' && key.length > 0; +function isActiveRequiredField(field: FieldDef, ctx: Ctx): boolean { + if (field.visible && !field.visible(ctx)) return false; + return typeof field.required === 'function' ? field.required(ctx) : field.required === true; +} + +function hasCompleteValue(field: FieldDef, value: unknown): boolean { + if (value === undefined || value === null) return false; + + if (field.kind === 'multiselect') { + return Array.isArray(value) && value.length > 0; + } + if (field.kind === 'number') { + return typeof value === 'number' && Number.isFinite(value); + } + if (field.kind === 'toggle') { + return typeof value === 'boolean'; + } + if (typeof value === 'string') { + return value.trim().length > 0; + } + return false; +} + +function hasSecretReference(config: PersistedConfig, field: FieldDef): boolean { + if (field.secret !== true) return false; + return hasCompleteValue(field, config.settings[`${field.settingsPath}KeyLocation`]); +} + +/** Return whether persisted settings satisfy every active schema requirement. */ +export function hasRequiredFields( + config: PersistedConfig, + catalog: ReadonlyArray = CATALOG, +): boolean { + const ctx: Ctx = { current: config.settings, pending: {} }; + const requiredFields = catalog.flatMap((category) => category.fields) + .filter((field) => isActiveRequiredField(field, ctx)); + return requiredFields.every((field) => { + if (hasCompleteValue(field, config.settings[field.settingsPath])) return true; + if (hasSecretReference(config, field)) return true; + + // The legacy provider block predates per-field secret-location pointers. + // Scope it to the LLM key and require the selected provider to match. + return field.secret === true + && field.settingsPath === 'llmApiKey' + && config.provider?.name === config.settings.llmProvider + && typeof config.provider?.keyLocation === 'string' + && config.provider.keyLocation.length > 0; + }); } diff --git a/tests/unit/cli/tui/entry-required-fields.test.ts b/tests/unit/cli/tui/entry-required-fields.test.ts index 69e790c29..805d356c5 100644 --- a/tests/unit/cli/tui/entry-required-fields.test.ts +++ b/tests/unit/cli/tui/entry-required-fields.test.ts @@ -23,6 +23,12 @@ function writeCfg(file: string, settings: Record): string { return p; } +function writePersistedCfg(file: string, config: Record): string { + const p = join(tmpDir, file); + writeFileSync(p, JSON.stringify({ version: 1, ...config }), { mode: 0o600 }); + return p; +} + beforeEach(() => { tmpDir = mkdtempSync(join(tmpdir(), 'wigolo-entry-rf-')); }); @@ -40,6 +46,24 @@ describe('resolveEntry auto-routing with required-fields check', () => { expect(r.firstRun).toBe(false); }); + it('config with a secret-location reference → home without persisting the key', async () => { + const p = writeCfg('key-reference.json', { + llmProvider: 'anthropic', + llmApiKeyKeyLocation: 'keychain', + }); + const r = await resolveEntry({ mode: 'auto', configPath: p, isTTY: true }); + expect(r.mode).toBe('home'); + }); + + it('legacy provider block with a matching provider → home', async () => { + const p = writePersistedCfg('legacy-provider.json', { + settings: { llmProvider: 'openai' }, + provider: { name: 'openai', keyLocation: 'file' }, + }); + const r = await resolveEntry({ mode: 'auto', configPath: p, isTTY: true }); + expect(r.mode).toBe('home'); + }); + it('config exists but missing llmProvider → wizard', async () => { const p = writeCfg('no-provider.json', { llmApiKey: 'sk-xxx' }); const r = await resolveEntry({ mode: 'auto', configPath: p, isTTY: true }); diff --git a/tests/unit/cli/tui/schema/llm.test.ts b/tests/unit/cli/tui/schema/llm.test.ts index 6999f53ac..a09c704d0 100644 --- a/tests/unit/cli/tui/schema/llm.test.ts +++ b/tests/unit/cli/tui/schema/llm.test.ts @@ -25,6 +25,7 @@ describe('llmCategory', () => { ]); expect(provider?.options?.map((o) => o.value)).not.toContain('custom'); expect(provider?.default).toBe('anthropic'); + expect(provider?.required).toBe(true); }); it('exposes ollama as a keyless local-LLM choice (no api-key field shown when selected)', () => { @@ -52,6 +53,9 @@ describe('llmCategory', () => { expect(key?.secret).toBe(true); expect(key?.propagateToAgents).toBe(true); expect(key?.key).toBe('WIGOLO_LLM_API_KEY'); + expect(typeof key?.required).toBe('function'); + expect(key?.required?.({ current: { llmProvider: 'anthropic' }, pending: {} })).toBe(true); + expect(key?.required?.({ current: { llmProvider: 'ollama' }, pending: {} })).toBe(false); // Help text must mention the keychain so users understand where secrets land. expect(key?.help).toMatch(/keychain/i); }); diff --git a/tests/unit/cli/tui/state/required-fields.test.ts b/tests/unit/cli/tui/state/required-fields.test.ts index 28f1e9b62..95cc7b165 100644 --- a/tests/unit/cli/tui/state/required-fields.test.ts +++ b/tests/unit/cli/tui/state/required-fields.test.ts @@ -13,11 +13,27 @@ import { describe, it, expect } from 'vitest'; import { hasRequiredFields } from '../../../../../src/cli/tui/state/required-fields.js'; import type { PersistedConfig } from '../../../../../src/persisted-config.js'; +import type { CategoryDef, FieldDef } from '../../../../../src/cli/tui/schema/types.js'; function cfg(settings: Record): PersistedConfig { return { version: 1, settings }; } +function catalogWith(...fields: FieldDef[]): ReadonlyArray { + return [{ id: 'advanced', label: 'Test', description: 'Test fields', fields }]; +} + +function requiredField(overrides: Partial = {}): FieldDef { + return { + key: 'TEST_VALUE', + settingsPath: 'testValue', + label: 'Test value', + kind: 'text', + required: true, + ...overrides, + }; +} + describe('hasRequiredFields', () => { it('empty config → false', () => { expect(hasRequiredFields(cfg({}))).toBe(false); @@ -32,7 +48,41 @@ describe('hasRequiredFields', () => { }); it('provider + key both set → true', () => { - expect(hasRequiredFields(cfg({ llmProvider: 'anthropic', llmApiKey: 'sk-xxx' }))).toBe(true); + expect(hasRequiredFields(cfg({ llmProvider: 'anthropic', llmApiKey: 'test-key' }))).toBe(true); + }); + + it('recognizes a persisted secret-location reference without reading the key', () => { + expect(hasRequiredFields(cfg({ + llmProvider: 'anthropic', + llmApiKeyKeyLocation: 'keychain', + }))).toBe(true); + expect(hasRequiredFields({ + version: 1, + settings: { llmProvider: 'openai' }, + provider: { name: 'openai', keyLocation: 'file' }, + })).toBe(true); + }); + + it('rejects a legacy provider reference for a different selected provider', () => { + expect(hasRequiredFields({ + version: 1, + settings: { llmProvider: 'openai' }, + provider: { name: 'anthropic', keyLocation: 'keychain' }, + })).toBe(false); + }); + + it('does not use the legacy LLM provider block for unrelated secret fields', () => { + const catalog = catalogWith(requiredField({ + settingsPath: 'otherSecret', + kind: 'masked', + secret: true, + })); + + expect(hasRequiredFields({ + version: 1, + settings: {}, + provider: { name: 'anthropic', keyLocation: 'keychain' }, + }, catalog)).toBe(false); }); it('provider is empty string → false', () => { @@ -71,4 +121,39 @@ describe('hasRequiredFields', () => { it('key is non-string (object) → false', () => { expect(hasRequiredFields(cfg({ llmProvider: 'anthropic', llmApiKey: {} }))).toBe(false); }); + + it('derives required paths from the supplied schema instead of provider names', () => { + const catalog = catalogWith(requiredField({ settingsPath: 'customRequired' })); + + expect(hasRequiredFields(cfg({}), catalog)).toBe(false); + expect(hasRequiredFields(cfg({ customRequired: 'configured' }), catalog)).toBe(true); + }); + + it('evaluates conditional required rules against persisted settings', () => { + const catalog = catalogWith(requiredField({ + required: (ctx) => ctx.current.mode === 'cloud', + })); + + expect(hasRequiredFields(cfg({ mode: 'local' }), catalog)).toBe(true); + expect(hasRequiredFields(cfg({ mode: 'cloud' }), catalog)).toBe(false); + expect(hasRequiredFields(cfg({ mode: 'cloud', testValue: 'configured' }), catalog)).toBe(true); + }); + + it('ignores required fields that are not visible in the active schema context', () => { + const catalog = catalogWith(requiredField({ visible: () => false })); + expect(hasRequiredFields(cfg({}), catalog)).toBe(true); + }); + + it('treats empty strings and lists as missing while accepting zero and false', () => { + const textCatalog = catalogWith(requiredField()); + const listCatalog = catalogWith(requiredField({ kind: 'multiselect' })); + const numberCatalog = catalogWith(requiredField({ kind: 'number' })); + const toggleCatalog = catalogWith(requiredField({ kind: 'toggle' })); + + expect(hasRequiredFields(cfg({ testValue: ' ' }), textCatalog)).toBe(false); + expect(hasRequiredFields(cfg({ testValue: [] }), listCatalog)).toBe(false); + expect(hasRequiredFields(cfg({ testValue: ['one'] }), listCatalog)).toBe(true); + expect(hasRequiredFields(cfg({ testValue: 0 }), numberCatalog)).toBe(true); + expect(hasRequiredFields(cfg({ testValue: false }), toggleCatalog)).toBe(true); + }); }); From cdf9d81afe46d4e9046424bc925f7d19619c5460 Mon Sep 17 00:00:00 2001 From: up <1457368987@qq.com> Date: Fri, 7 Aug 2026 13:09:04 +0800 Subject: [PATCH 2/2] fix(tui): reject blank legacy key locations - Background: CodeRabbit found that whitespace-only legacy key locations could satisfy setup completion. - Changes: validate legacy provider key-location references through the same field-aware completion helper and add a regression test. - Verification: targeted Vitest (35 tests), npm run lint, and git diff --check passed. --- src/cli/tui/state/required-fields.ts | 3 +-- tests/unit/cli/tui/state/required-fields.test.ts | 8 ++++++++ 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/src/cli/tui/state/required-fields.ts b/src/cli/tui/state/required-fields.ts index f781fa89f..c3a7a47df 100644 --- a/src/cli/tui/state/required-fields.ts +++ b/src/cli/tui/state/required-fields.ts @@ -47,7 +47,6 @@ export function hasRequiredFields( return field.secret === true && field.settingsPath === 'llmApiKey' && config.provider?.name === config.settings.llmProvider - && typeof config.provider?.keyLocation === 'string' - && config.provider.keyLocation.length > 0; + && hasCompleteValue(field, config.provider?.keyLocation); }); } diff --git a/tests/unit/cli/tui/state/required-fields.test.ts b/tests/unit/cli/tui/state/required-fields.test.ts index 95cc7b165..78b824f16 100644 --- a/tests/unit/cli/tui/state/required-fields.test.ts +++ b/tests/unit/cli/tui/state/required-fields.test.ts @@ -71,6 +71,14 @@ describe('hasRequiredFields', () => { })).toBe(false); }); + it('rejects a blank legacy provider key-location reference', () => { + expect(hasRequiredFields({ + version: 1, + settings: { llmProvider: 'anthropic' }, + provider: { name: 'anthropic', keyLocation: ' ' as 'keychain' }, + })).toBe(false); + }); + it('does not use the legacy LLM provider block for unrelated secret fields', () => { const catalog = catalogWith(requiredField({ settingsPath: 'otherSecret',