From 319420d1968ecf8369dc3e2c2d85794fd2e0556c Mon Sep 17 00:00:00 2001 From: "J. Smith" Date: Sat, 12 Sep 2026 23:31:22 -0500 Subject: [PATCH] fix(web): persist the default CLI at any installed count, not only a sole one The sole-install persistence added in config-form.tsx fixed the one-CLI case but left the multi-CLI case in exactly the state its own comment warns about: const only = list.filter((c) => c.installed); if (only.length !== 1) return list.find((c) => c.installed)?.id || ""; if (!readSavedCliId()) persistCliId(only[0].id); With two or more CLIs installed the early return highlights the first one and persists nothing. Every AI surface reads `career-ops:config` directly and sends it as `cliId`, so on a machine with claude + codex (or, here, claude, codex, gemini and opencode) Config shows a selected CLI over empty localStorage and every AI feature silently does nothing -- /api/explore/ai answers {"error":"query and cliId required"} and the console shows "no CLI configured". Nothing prompts a Save, because nothing looks unset. Reproduced on a clean browser profile against 1.32.0: localStorage is empty after visiting /config. Whatever Config renders as selected is what it must persist, so the pick is now pickDefaultInstalled() -- first installed, any count -- while pickSoleInstalled keeps its narrower meaning for resolveCliId(). Both live in src/lib/cli-pick.mjs, and saved-cli.ts re-exports them. That is what lets tests/lib/saved-cli-pick.test.mjs import the REAL functions: the suite previously held a hand-copied mirror of pickSoleInstalled, which by construction cannot fail when the real function changes. Reverting pickDefaultInstalled to the sole-install behaviour now turns 2 tests red. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Yafkyw2o71V3kyMu4QRbqB --- web/src/components/config-form.tsx | 17 +++++--- web/src/lib/cli-pick.mjs | 22 +++++++++++ web/src/lib/saved-cli.ts | 9 ++--- web/tests/lib/saved-cli-pick.test.mjs | 56 ++++++++++++++++++++++++--- 4 files changed, 87 insertions(+), 17 deletions(-) create mode 100644 web/src/lib/cli-pick.mjs diff --git a/web/src/components/config-form.tsx b/web/src/components/config-form.tsx index c3b12ccb7f..31aac3dc49 100644 --- a/web/src/components/config-form.tsx +++ b/web/src/components/config-form.tsx @@ -13,6 +13,7 @@ import { import { cn } from "@/lib/cn"; import { CadenceSettings } from "@/components/followups/cadence-settings"; import { persistCliId, readSavedCliId } from "@/lib/saved-cli"; +import { pickDefaultInstalled } from "@/lib/cli-pick.mjs"; type Cli = { id: string; @@ -68,14 +69,18 @@ export function ConfigForm() { .then((d) => { const list: Cli[] = d.clis ?? []; setClis(list); - // Highlight + persist the only installed CLI when Config was never saved. - // Highlight-only used to look configured while jobs still read empty localStorage. + // Highlight + persist the default installed CLI when Config was never + // saved. Highlight-only looks configured while jobs still read empty + // localStorage -- so whatever is rendered as selected must be written. + // This applies at ANY installed count: restricting it to a SOLE install + // left every multi-CLI machine (claude + codex, say) in exactly the + // broken state the highlight was meant to avoid. setCliId((prev) => { if (prev) return prev; - const only = list.filter((c) => c.installed); - if (only.length !== 1) return list.find((c) => c.installed)?.id || ""; - if (!readSavedCliId()) persistCliId(only[0].id); - return only[0].id; + const auto = pickDefaultInstalled(list); + if (!auto) return ""; + if (!readSavedCliId()) persistCliId(auto); + return auto; }); }) .catch(() => setClis([])); diff --git a/web/src/lib/cli-pick.mjs b/web/src/lib/cli-pick.mjs new file mode 100644 index 0000000000..40175763e5 --- /dev/null +++ b/web/src/lib/cli-pick.mjs @@ -0,0 +1,22 @@ +// Plain .mjs so tests/lib/saved-cli-pick.test.mjs can import the REAL functions +// under `node --test` (no TypeScript loader). saved-cli.ts re-exports these, so +// there is one implementation rather than a mirrored copy in the suite. + +/** The single installed CLI, or null when there is not exactly one. */ +export function pickSoleInstalled(clis) { + const installed = (clis || []).filter((c) => c && c.installed); + return installed.length === 1 ? installed[0].id : null; +} + +/** + * The CLI the Config page should start on: the first installed one, whatever + * the count. + * + * Config renders this as the active choice, so it must also be the value that + * gets persisted. Persisting only the SOLE installed CLI left anyone with two + * or more looking at a selected CLI over empty localStorage -- and every AI + * surface reads that key, so they all silently did nothing. + */ +export function pickDefaultInstalled(clis) { + return (clis || []).find((c) => c && c.installed)?.id || null; +} diff --git a/web/src/lib/saved-cli.ts b/web/src/lib/saved-cli.ts index fb4d01bf78..acc386b2ba 100644 --- a/web/src/lib/saved-cli.ts +++ b/web/src/lib/saved-cli.ts @@ -1,3 +1,5 @@ +import { pickSoleInstalled } from "./cli-pick.mjs"; + export const CONFIG_KEY = "career-ops:config"; export function readSavedCliId(): string | null { @@ -23,12 +25,7 @@ export function persistCliId(cliId: string) { } } -export function pickSoleInstalled( - clis: { id: string; installed?: boolean }[] | undefined, -): string | null { - const installed = (clis || []).filter((c) => c.installed); - return installed.length === 1 ? installed[0].id : null; -} +export { pickDefaultInstalled, pickSoleInstalled } from "./cli-pick.mjs"; /** Saved Config cliId, or the only installed CLI (and persist that pick). */ export async function resolveCliId(): Promise { diff --git a/web/tests/lib/saved-cli-pick.test.mjs b/web/tests/lib/saved-cli-pick.test.mjs index 90662dc14e..5791c585b4 100644 --- a/web/tests/lib/saved-cli-pick.test.mjs +++ b/web/tests/lib/saved-cli-pick.test.mjs @@ -1,11 +1,11 @@ import assert from "node:assert/strict"; import { test } from "node:test"; -// Mirror of pickSoleInstalled in src/lib/saved-cli.ts (TS; this suite is .mjs). -function pickSoleInstalled(clis) { - const installed = (clis || []).filter((c) => c.installed); - return installed.length === 1 ? installed[0].id : null; -} +// Imports the REAL implementation from src/lib/cli-pick.mjs. This suite used to +// hold a hand-copied mirror of pickSoleInstalled, which cannot fail when the +// real function changes -- exactly the drift that let the multi-CLI gap below +// go unnoticed. +import { pickDefaultInstalled, pickSoleInstalled } from "../../src/lib/cli-pick.mjs"; test("sole installed CLI is the default", () => { assert.equal( @@ -27,3 +27,49 @@ test("zero or two installed CLIs stay unset", () => { null, ); }); + +test("pickSoleInstalled tolerates undefined and sparse entries", () => { + assert.equal(pickSoleInstalled(undefined), null); + assert.equal(pickSoleInstalled([null, { id: "codex", installed: true }]), "codex"); +}); + +// --- pickDefaultInstalled: what Config renders, and therefore must persist --- + +test("first installed CLI is the default at ANY installed count", () => { + assert.equal( + pickDefaultInstalled([ + { id: "claude", installed: true }, + { id: "codex", installed: true }, + { id: "gemini", installed: true }, + ]), + "claude", + ); +}); + +test("the default skips CLIs that are not installed", () => { + assert.equal( + pickDefaultInstalled([ + { id: "claude", installed: false }, + { id: "codex", installed: true }, + ]), + "codex", + ); +}); + +test("no installed CLI yields null, never an empty-string id", () => { + assert.equal(pickDefaultInstalled([{ id: "claude", installed: false }]), null); + assert.equal(pickDefaultInstalled([]), null); + assert.equal(pickDefaultInstalled(undefined), null); +}); + +// The regression: a multi-CLI machine used to fall through the SOLE-install +// guard, so Config highlighted a CLI it never wrote. Every AI surface reads +// that key, so all of them silently did nothing. +test("multi-CLI machines get a persistable default, unlike pickSoleInstalled", () => { + const clis = [ + { id: "claude", installed: true }, + { id: "codex", installed: true }, + ]; + assert.equal(pickSoleInstalled(clis), null, "sole-pick correctly declines"); + assert.equal(pickDefaultInstalled(clis), "claude", "but a default must still exist to persist"); +});