diff --git a/docs/cli-registry.md b/docs/cli-registry.md index e684b611..c1a9d871 100644 --- a/docs/cli-registry.md +++ b/docs/cli-registry.md @@ -98,6 +98,8 @@ DeepSeek is worth reading before assuming an entry looks like its siblings — t `test/cli-registry-no-id-branching.test.ts` fails the build if a CLI id comparison appears outside the stock catalog. It builds its id list from the live catalog, blanks comment lines before scanning (comments legitimately quote the pattern to explain why a branch was removed, and blanking rather than dropping is what keeps reported line numbers pointing at the real file), and keeps an allowlist in which **every entry carries its reason**. +`test/frontend-cli-no-id-branching.test.ts` is the same guard for the two frontend files the CLI registry's Run-menu consolidation touches, `session-ui.js` and `mobile-overview.js` — deliberately not the rest of `src/web/public/`, whose per-CLI rules stay out of scope for now (see "Fields declared for later" below). Its allowlist keys on `::` with no line number, since a single unrelated edit to a contended file would otherwise shift every subsequent line and make every entry go stale at once, and each entry additionally carries the exact number of approved call sites — a bare key would let a brand-new branch reusing an already-approved expression land unreviewed. + It matches four shapes, not one: `mode === ''`, `mode !== ''`, `case '':`, and `['', …].includes(mode)`. The first version matched `===` only, and that gap was not academic — the refactor it guards converted the `===` sites and left the negated ones, so 36 `!==` branches survived it, including a seven-mode chain auto-enabling Ralph under a comment asking the next person to keep it in step with a predicate by hand while the sibling code path already read the capability. A guard that sees half the shapes reports a count measured over the half it happens to catch. The allowlist is not a formality. If a branch is about what a CLI can DO it belongs in `CliCapabilities`; the entries that remain are things that are not CLI-behaviour branches at all — chiefly the legacy per-mode `Config` objects on `POST /api/sessions`, which are a fact about the public HTTP API rather than about any CLI, plus a few documented cases where `mode === 'claude'` is genuinely the right question (Read My Mind reads Claude's _own_ transcript, so a capability there would be actively wrong). diff --git a/test/frontend-cli-no-id-branching.test.ts b/test/frontend-cli-no-id-branching.test.ts index 0c52b955..50b4deaa 100644 --- a/test/frontend-cli-no-id-branching.test.ts +++ b/test/frontend-cli-no-id-branching.test.ts @@ -24,53 +24,70 @@ const PUBLIC = fileURLToPath(new URL('../src/web/public/', import.meta.url)); const SCANNED_FILES = ['session-ui.js', 'mobile-overview.js']; /** - * Every currently-surviving branch, each with the reason it is not a - * CLI-behaviour branch a `CliCapabilities` field should express, keyed - * `::` — deliberately NO line number. An - * earlier version keyed on `::::`, and inserting one - * comment line at the top of `session-ui.js` shifted every subsequent line - * number, so all 21 entries went stale and the same 21 branches were then - * reported as "new". `session-ui.js` is one of the most contended files in - * the repo, so a guard that goes red on any unrelated edit to it sends the - * next person after the wrong problem. Several entries below cover more than - * one physical call site sharing the same expression in the same file — - * that collapsing is the point, not a loss of precision (the backend guard - * this mirrors made the identical choice, for the identical reason). + * Every currently-surviving branch, each with the COUNT of physical call + * sites carrying it and the reason none of them is a `CliCapabilities` + * field, keyed `::` — deliberately NO line + * number. An earlier version keyed on `::::`, and + * inserting one comment line at the top of `session-ui.js` shifted every + * subsequent line number, so all 21 entries went stale and the same 21 + * branches were then reported as "new". `session-ui.js` is one of the most + * contended files in the repo, so a guard that goes red on any unrelated + * edit to it sends the next person after the wrong problem. + * + * The `count` is what closes the gap dropping the line number opened: a key + * alone says "this expression is approved somewhere in this file", so a + * BRAND NEW `mode === 'codex'` site anywhere in `session-ui.js` would reuse + * the same key as the two approved ones and pass silently. The count makes + * that a mismatch — one more occurrence than declared — and the "counts + * match" test below catches it, while a genuinely new expression (a CLI id + * with no ALLOWED_BRANCHES entry at all) is still caught by the separate + * "no unapproved id branches" test either way. */ -const ALLOWED_BRANCHES: Record = { - "session-ui.js::mode === 'shell'": - 'run() dispatch: shell needs no CLI probe at all, and it keeps its own row in the ' + - 'button-label ternary (pinned exact text, see Open Question 7 in PR-B2.md)', +const ALLOWED_BRANCHES: Record = { + "session-ui.js::mode === 'shell'": { + count: 2, + reason: + 'run() dispatch (shell needs no CLI probe at all) and the button-label ternary (pinned exact ' + + "text — test/run-mode-ui.test.ts asserts e.g. 'Run OMP', which diverges from CliEntry.shortBadge " + + "for at least omp ('OM' vs the displayed 'OMP'), so a catalogue-driven rewrite would silently " + + 'change user-visible text and break that pinned test; the maintainer confirmed leaving this ' + + 'hardcoded, see the PR #458 review thread)', + }, - "session-ui.js::mode === 'claude'": - 'four claude-specific call sites, not one branch: run() dispatch (claude has its own ' + - 'remote/docker branching and parallel-create path, unlike every RUN_MODE_LAUNCH entry), ' + - 'runCustomModelEntry() (restart-vs-one-shot launch mechanism, not a preference — see ' + - "CLAUDE.md's Custom Model Endpoint Profiles section), the Respawn/Ralph section (claude-only " + - "by design, mirroring the backend capabilities.ralph gate), and the runMode setter's " + - 'validity check', + "session-ui.js::mode === 'claude'": { + count: 4, + reason: + 'four claude-specific call sites, not one branch: run() dispatch (claude has its own ' + + 'remote/docker branching and parallel-create path, unlike every RUN_MODE_LAUNCH entry), ' + + 'runCustomModelEntry() (restart-vs-one-shot launch mechanism, not a preference — see ' + + "CLAUDE.md's Custom Model Endpoint Profiles section), the Respawn/Ralph section (claude-only " + + "by design, mirroring the backend capabilities.ralph gate), and the runMode setter's " + + 'validity check', + }, // The 8 external CLIs share the same two call sites and the same reason at - // each: the button-label ternary (pinned exact text — test/run-mode-ui.test.ts - // asserts e.g. 'Run OMP', which diverges from CliEntry.shortBadge for at - // least omp ('OM' vs the displayed 'OMP'), so a catalogue-driven rewrite - // would silently change user-visible text and break that pinned test — see - // Open Question 7 in PR-B2.md), and the runMode property setter's validity - // allowlist (not a behaviour branch; left hardcoded in Phase 2 since its - // chain has no shell arm at all and no evidence of what callers rely on it). - "session-ui.js::mode === 'opencode'": 'button-label ternary + runMode setter validity check (see the header comment)', - "session-ui.js::mode === 'codex'": 'button-label ternary + runMode setter validity check (see the header comment)', - "session-ui.js::mode === 'gemini'": 'button-label ternary + runMode setter validity check (see the header comment)', - "session-ui.js::mode === 'antigravity'": - 'button-label ternary + runMode setter validity check (see the header comment)', - "session-ui.js::mode === 'pi'": 'button-label ternary + runMode setter validity check (see the header comment)', - "session-ui.js::mode === 'grok'": 'button-label ternary + runMode setter validity check (see the header comment)', - "session-ui.js::mode === 'deepseek'": 'button-label ternary + runMode setter validity check (see the header comment)', - "session-ui.js::mode === 'omp'": 'button-label ternary + runMode setter validity check (see the header comment)', + // each: the button-label ternary (see the shell entry above for why it + // stays hardcoded) and the runMode property setter's validity allowlist + // (not a behaviour branch; left hardcoded in Phase 2 since its chain has + // no shell arm at all and no evidence of what callers rely on it). + "session-ui.js::mode === 'opencode'": { count: 2, reason: 'button-label ternary + runMode setter validity check' }, + "session-ui.js::mode === 'codex'": { count: 2, reason: 'button-label ternary + runMode setter validity check' }, + "session-ui.js::mode === 'gemini'": { count: 2, reason: 'button-label ternary + runMode setter validity check' }, + "session-ui.js::mode === 'antigravity'": { + count: 2, + reason: 'button-label ternary + runMode setter validity check', + }, + "session-ui.js::mode === 'pi'": { count: 2, reason: 'button-label ternary + runMode setter validity check' }, + "session-ui.js::mode === 'grok'": { count: 2, reason: 'button-label ternary + runMode setter validity check' }, + "session-ui.js::mode === 'deepseek'": { count: 2, reason: 'button-label ternary + runMode setter validity check' }, + "session-ui.js::mode === 'omp'": { count: 2, reason: 'button-label ternary + runMode setter validity check' }, // mobile-overview.js: shell is exempt from the isCliAvailable() gate the // same way the toolbar's #runModeMenu exempts it (shell needs no CLI). - "mobile-overview.js::mode !== 'shell'": 'shell needs no CLI, so it is exempt from the availability gate', + "mobile-overview.js::mode !== 'shell'": { + count: 1, + reason: 'shell needs no CLI, so it is exempt from the availability gate', + }, }; /** Every stock CLI id, derived rather than restated so a new entry is covered automatically. */ @@ -119,6 +136,12 @@ function scan(): Finding[] { const findings = scan(); +function actualCounts(): Map { + const counts = new Map(); + for (const f of findings) counts.set(f.key, (counts.get(f.key) ?? 0) + 1); + return counts; +} + describe('no NEW CLI-id branching in session-ui.js / mobile-overview.js (PR B2)', () => { it('scans both files (sanity)', () => { // If this drops to zero the scanner or the file list drifted and every @@ -164,13 +187,33 @@ describe('no NEW CLI-id branching in session-ui.js / mobile-overview.js (PR B2)' ).toEqual([]); }); - it('has no stale allowlist entries', () => { - // An allowlisted branch that no longer exists anywhere in either file is a - // lie about the codebase, and the next person to reintroduce that exact - // expression would sail straight through under a pre-approved reason that - // no longer describes anything real. - const present = new Set(findings.map((f) => f.key)); - const stale = Object.keys(ALLOWED_BRANCHES).filter((key) => !present.has(key)); - expect(stale, `ALLOWED_BRANCHES entries no longer present — delete them:\n ${stale.join('\n ')}`).toEqual([]); + it('every allowlisted branch occurs exactly its declared number of times', () => { + // This is what closes the gap the line-number removal opened (see the + // ALLOWED_BRANCHES header comment): a key alone cannot tell "the two + // approved sites" from "the two approved sites plus a brand new third + // one reusing the same expression" — the count can. A mismatch in + // either direction is real: higher means an unreviewed NEW branch + // landed reusing an approved expression, lower means one of the + // reviewed call sites was removed and the entry is now a stale lie + // about the codebase (the count going to 0 is the old "stale entry" + // case, now folded into this same check rather than a separate one). + const actual = actualCounts(); + const mismatches: string[] = []; + for (const [key, { count: expected }] of Object.entries(ALLOWED_BRANCHES)) { + const got = actual.get(key) ?? 0; + if (got !== expected) { + mismatches.push(` ${key} expected ${expected}, found ${got}`); + } + } + expect( + mismatches, + mismatches.length === 0 + ? '' + : `ALLOWED_BRANCHES count mismatch(es):\n${mismatches.join('\n')}\n\n` + + 'A count LOWER than declared means a reviewed call site was removed — update or delete ' + + 'the entry. A count HIGHER than declared means a NEW branch landed reusing an already-' + + 'approved expression — review it and bump the count (or fix the branch) explicitly, ' + + 'rather than let it ride in on an existing approval.' + ).toEqual([]); }); }); diff --git a/test/run-mode-launch-table-drift.test.ts b/test/run-mode-launch-table-drift.test.ts new file mode 100644 index 00000000..ba42c18e --- /dev/null +++ b/test/run-mode-launch-table-drift.test.ts @@ -0,0 +1,91 @@ +/** + * @fileoverview `RUN_MODE_LAUNCH` (session-ui.js, PR B2) restates four things + * `stock.ts` already owns: label, an install command, whether the CLI + * supports a custom-model launch, and the external-mode key set itself. + * They agree today, but nothing enforced it — the dangerous drift is + * `supportsCustomModel`: the Run menu's "CLI (endpoint)" rows come from the + * server-injected `window.__codemanCustomModelClis` (built from + * `capabilities.customModelInjection.kind !== 'unsupported'`), so a CLI that + * gains a real injection recipe later would be OFFERED in that menu while + * `_runCliMode` still drops the `customModel` field for it — the session + * launches on the vendor's cloud while the UI claims the local endpoint. + * + * Drives the REAL session-ui.js via JSDOM (`runScripts: 'dangerously'`, same + * approach as test/custom-model-one-shot-launch.test.ts), extracting the + * module-level `RUN_MODE_LAUNCH` const by appending one assignment line to + * the SAME source string before the one `eval()` call — it is not attached + * to `window` on its own (top-level `const` lives in the script's own + * lexical scope, not the global object), and a SEPARATE later `eval()` call + * cannot see an earlier call's top-level bindings either (measured: each + * `window.eval()` invocation gets its own top-level lexical environment in + * jsdom), so the assignment has to ride in the same evaluated string. + * + * Port: none. + */ +import { readFileSync } from 'node:fs'; +import { JSDOM } from 'jsdom'; +import { describe, expect, it } from 'vitest'; +import { STOCK_CLIS } from '../src/config/cli-registry/stock.js'; + +const SESSION_UI_JS = readFileSync(new URL('../src/web/public/session-ui.js', import.meta.url), 'utf-8'); + +interface RunModeLaunchEntry { + label: string; + installHint: string; + supportsCustomModel: boolean; + buildConfig: (globalSettings: Record) => Record | null; +} + +function loadRunModeLaunch(): Record { + const dom = new JSDOM('', { url: 'http://localhost/', runScripts: 'dangerously' }); + const win = dom.window as unknown as Window & typeof globalThis & { CodemanApp: new () => unknown }; + (win as unknown as { eval: (s: string) => void }).eval('window.CodemanApp = function CodemanApp() {};'); + // The assignment MUST be part of the same evaluated string as + // SESSION_UI_JS — RUN_MODE_LAUNCH is a bare top-level `const`, so it only + // exists in the lexical scope of THIS eval call. + (win as unknown as { eval: (s: string) => void }).eval( + `${SESSION_UI_JS}\nwindow.__TEST_RUN_MODE_LAUNCH = RUN_MODE_LAUNCH;` + ); + return (win as unknown as { __TEST_RUN_MODE_LAUNCH: Record }).__TEST_RUN_MODE_LAUNCH; +} + +describe('RUN_MODE_LAUNCH (session-ui.js) stays in step with stock.ts', () => { + const runModeLaunch = loadRunModeLaunch(); + const byId = new Map(STOCK_CLIS.map((e) => [e.id as string, e])); + + it('covers exactly the non-claude, non-shell stock CLIs — no more, no fewer', () => { + const expectedIds = STOCK_CLIS.map((e) => e.id as string) + .filter((id) => id !== 'claude' && id !== 'shell') + .sort(); + expect(Object.keys(runModeLaunch).sort()).toEqual(expectedIds); + }); + + it('label matches CliEntry.label for every entry', () => { + for (const [id, entry] of Object.entries(runModeLaunch)) { + const stockEntry = byId.get(id); + expect(stockEntry, `no stock entry for ${id}`).toBeTruthy(); + expect(entry.label, `${id} label drifted from stock.ts`).toBe(stockEntry!.label); + } + }); + + it('installHint embeds the real linux install command', () => { + for (const [id, entry] of Object.entries(runModeLaunch)) { + const command = byId.get(id)!.discovery.install.command?.linux; + if (!command) continue; // shell-less entries (none today) carry no command to check + expect(entry.installHint, `${id} installHint no longer matches stock.ts's linux install command`).toContain( + command + ); + } + }); + + it('supportsCustomModel matches capabilities.customModelInjection.kind !== "unsupported"', () => { + // This is the one that fails SILENTLY if it drifts (see file header): + // window.__codemanCustomModelClis (server.ts) is built from this same + // stock.ts field, so a mismatch here means the Run-menu picker and the + // actual launch body disagree about which CLIs are custom-model-capable. + for (const [id, entry] of Object.entries(runModeLaunch)) { + const supported = byId.get(id)!.capabilities.customModelInjection.kind !== 'unsupported'; + expect(entry.supportsCustomModel, `${id}.supportsCustomModel drifted from stock.ts's capability`).toBe(supported); + } + }); +});