diff --git a/src/config/cli-registry/types.ts b/src/config/cli-registry/types.ts index 5022360c..871a3762 100644 --- a/src/config/cli-registry/types.ts +++ b/src/config/cli-registry/types.ts @@ -629,15 +629,12 @@ export interface CliOverlays { /** * ⚠️ DECLARED-FOR-LATER: fields no code reads yet. * - * `accent`, `overlays.credStore`, `capabilities.echo`, `capabilities.wheelForward`, + * `shortBadge`, `accent`, `overlays.credStore`, `capabilities.echo`, `capabilities.wheelForward`, * `capabilities.keyboardAccessory` and `capabilities.maxFrameBytes` all describe FRONTEND - * behaviour, and most of the frontend is deliberately untouched by the change that introduced - * this registry — `app.js`, `terminal-ui.js`, `styles.css` and friends keep their own + * behaviour, and the frontend is deliberately untouched by the change that introduced this + * registry — `app.js`, `terminal-ui.js`, `styles.css` and friends keep their own * hand-authored per-CLI rules, and moving them is its own piece of work with its own way of - * being verified (a mobile/browser suite the CI gate cannot see). `shortBadge` graduated out of - * this list (PR B2): it is read server-side into `window.__codemanCliCatalog` - * (`src/web/server.ts`), the general run-menu catalogue injected for the frontend to consume — - * see `docs/cli-registry.md`. + * being verified (a mobile/browser suite the CI gate cannot see). * * They are declared now because each entry should describe its CLI completely, and because * transcribing them while the hand-written source is still on screen is when the values are diff --git a/src/web/public/session-ui.js b/src/web/public/session-ui.js index 9385a710..cdac0a5e 100644 --- a/src/web/public/session-ui.js +++ b/src/web/public/session-ui.js @@ -20,15 +20,14 @@ * near-identical bodies guaranteed to drift, exactly what the CLI registry's * own no-id-branching rule exists to prevent server-side. * - * Deliberately a LOCAL table rather than reading `window.__codemanCliCatalog` - * (server.ts, PR B2): that catalogue carries only menu-facing metadata - * (id/label/shortBadge/order/kind), and several unit tests exercise these - * run*() methods inside a bare `vm.createContext()` sandbox with no `window` - * global at all (see test/run-mode-ui.test.ts) — referencing `window` there - * unguarded would throw, not degrade. `buildConfig` returns the CLI's - * top-level legacy config field for a LOCAL launch, or `null` for a CLI that - * sends none (pi: no bypass flag exists, so there is nothing to send — see - * runPi's own history below for why that must stay true). + * Deliberately a LOCAL table rather than a server-injected catalogue: several + * unit tests exercise these run*() methods inside a bare `vm.createContext()` + * sandbox with no `window` global at all (see test/run-mode-ui.test.ts) — + * referencing `window` there unguarded would throw, not degrade. `buildConfig` + * returns the CLI's top-level legacy config field for a LOCAL launch, or + * `null` for a CLI that sends none (pi: no bypass flag exists, so there is + * nothing to send — see runPi's own history below for why that must stay + * true). */ const RUN_MODE_LAUNCH = { opencode: { @@ -112,9 +111,9 @@ const RUN_MODE_LAUNCH = { /** * External (non-Claude, non-Shell) CLI run modes — the keys of RUN_MODE_LAUNCH - * above, kept as its own Set so `_isAltCliMode()` doesn't recompute an array - * every call. Single source for what used to be two hand-copied 8-way - * `session.mode === '' || ...` chains inside one function + * above, kept as its own Set (`EXTERNAL_CLI_MODES.has(mode)`) rather than an + * array recomputed per call. Single source for what used to be two hand-copied + * 8-way `session.mode === '' || ...` chains inside one function * (`openSessionOptions`), guaranteed to drift from each other the moment a * ninth CLI landed in one and not the other. */ diff --git a/src/web/server.ts b/src/web/server.ts index 967efd06..f2f9b419 100644 --- a/src/web/server.ts +++ b/src/web/server.ts @@ -1674,24 +1674,6 @@ export class WebServer extends EventEmitter { '', `\n` ); - // The general-purpose run-menu catalogue (PR B2, docs/cli-registry.md): every - // ENABLED CliEntry's menu-facing fields, unfiltered by capability — unlike - // __codemanCustomModelClis above, which is narrowed to one picker's needs. - // Field list mirrors what scripts/generate-cli-catalog.mts exports to - // config/clis.stock.json (id/label/shortBadge/order/kind); launch/env/ - // capabilities/overlays are spawn-time concerns the server alone interprets - // and must never leak here, same rule as that generated artifact. - const cliCatalog = enabledClis().map((entry) => ({ - id: entry.id, - label: entry.label, - shortBadge: entry.shortBadge, - order: entry.order, - kind: entry.kind, - })); - // `label`/`shortBadge` are user-clis.json-settable strings, so this needs the - // same -breakout guard as __codemanCustomModelClis above. - const cliCatalogJson = escapeScriptJson(JSON.stringify(cliCatalog)); - html = html.replace('', `\n`); } if (!soloSessionId && process.env.CODEMAN_GESTURE === '1') { html = html.replace('', `\n`); diff --git a/test/cli-registry-no-id-branching.test.ts b/test/cli-registry-no-id-branching.test.ts index 0c8cd1f1..8d1bc4de 100644 --- a/test/cli-registry-no-id-branching.test.ts +++ b/test/cli-registry-no-id-branching.test.ts @@ -314,6 +314,7 @@ describe('declared-for-later fields', () => { * list — and wiring one up should make its line here fail, which is the good direction. */ const DECLARED_FOR_LATER = [ + 'shortBadge', 'accent', 'capabilities.echo', 'capabilities.wheelForward', diff --git a/test/frontend-cli-no-id-branching.test.ts b/test/frontend-cli-no-id-branching.test.ts index c36ba2a7..0c52b955 100644 --- a/test/frontend-cli-no-id-branching.test.ts +++ b/test/frontend-cli-no-id-branching.test.ts @@ -26,61 +26,51 @@ 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 - * `::::`. Found by running the scanner - * below against the post-B2 state of both files (2026-09-19) and reviewing - * each hit in context — none of these are leftover oversights, each is a - * verified, deliberate exception. + * `::` — 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). */ const ALLOWED_BRANCHES: Record = { - // --- run(): claude/shell get their own dispatch path, everything else goes --- - // --- through the shared _runCliMode() — see RUN_MODE_LAUNCH's header comment --- - "session-ui.js::507::mode === 'shell'": 'run() dispatch: shell needs no CLI probe at all', - "session-ui.js::510::mode === 'claude'": - 'run() dispatch: claude has its own remote/docker branching and parallel-create path ' + - '(runClaude), unlike every RUN_MODE_LAUNCH entry', + "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)', - // --- runCustomModelEntry(): claude restarts its CLI process in place; every --- - // --- other custom-model-eligible CLI applies the pick one-shot, before the --- - // --- session exists at all. Documented in CLAUDE.md's Custom Model Endpoint --- - // --- Profiles section: "Two launch paths, chosen by mechanism, not preference." --- - "session-ui.js::862::mode === 'claude'": 'restart-vs-one-shot custom-model launch mechanism, not a preference', + "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', - // --- Button-label ternary (Open Question 7, DEPLOYMENT_PLAN.md): pinned by --- - // --- test/run-mode-ui.test.ts's exact-text assertions (e.g. 'Run OMP'), which --- - // --- diverge from CliEntry.shortBadge for at least one CLI (omp: 'OM' vs the --- - // --- displayed 'OMP') — a catalogue-driven rewrite would silently change --- - // --- user-visible text and break that pinned test. --- - "session-ui.js::1527::mode === 'opencode'": 'button-label ternary, pinned exact text (see Open Question 7)', - "session-ui.js::1527::mode === 'codex'": 'button-label ternary, pinned exact text (see Open Question 7)', - "session-ui.js::1527::mode === 'gemini'": 'button-label ternary, pinned exact text (see Open Question 7)', - "session-ui.js::1527::mode === 'antigravity'": 'button-label ternary, pinned exact text (see Open Question 7)', - "session-ui.js::1527::mode === 'pi'": 'button-label ternary, pinned exact text (see Open Question 7)', - "session-ui.js::1527::mode === 'grok'": 'button-label ternary, pinned exact text (see Open Question 7)', - "session-ui.js::1527::mode === 'deepseek'": 'button-label ternary, pinned exact text (see Open Question 7)', - "session-ui.js::1527::mode === 'omp'": 'button-label ternary, pinned exact text (see Open Question 7)', - "session-ui.js::1527::mode === 'shell'": 'button-label ternary, pinned exact text (see Open Question 7)', + // 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)', - // --- Respawn/Ralph section: claude-only by product design, mirroring the --- - // --- backend's own capabilities.ralph gate (isExternalCliMode() already --- - // --- excludes Ralph tracking for every non-claude mode server-side). --- - "session-ui.js::2200::mode === 'claude'": 'Respawn/Ralph section is claude-only by design', - - // --- runMode property setter: a validity allowlist, not a behaviour branch. --- - // --- Left untouched in Phase 2 (uncertain 'shell' asymmetry — this chain has --- - // --- no shell arm at all — and no evidence of what callers rely on it). --- - "session-ui.js::4431::mode === 'opencode'": 'runMode setter validity check, not behaviour', - "session-ui.js::4431::mode === 'codex'": 'runMode setter validity check, not behaviour', - "session-ui.js::4431::mode === 'gemini'": 'runMode setter validity check, not behaviour', - "session-ui.js::4431::mode === 'antigravity'": 'runMode setter validity check, not behaviour', - "session-ui.js::4431::mode === 'pi'": 'runMode setter validity check, not behaviour', - "session-ui.js::4431::mode === 'grok'": 'runMode setter validity check, not behaviour', - "session-ui.js::4431::mode === 'deepseek'": 'runMode setter validity check, not behaviour', - "session-ui.js::4431::mode === 'omp'": 'runMode setter validity check, not behaviour', - "session-ui.js::4431::mode === 'claude'": 'runMode setter validity check, not behaviour', - - // --- 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::574::mode !== 'shell'": 'shell needs no CLI, so it is exempt from the availability gate', + // 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', }; /** Every stock CLI id, derived rather than restated so a new entry is covered automatically. */ @@ -120,7 +110,7 @@ function scan(): Finding[] { BRANCH_PATTERN.lastIndex = 0; // shared /g regex — see utils/regex-patterns.ts for (const match of line.matchAll(BRANCH_PATTERN)) { const expression = match[0].replace(/\s+/g, ' ').replace(/^(?:id|agentType)/, 'mode'); - findings.push({ file, expression, line: i + 1, key: `${file}::${i + 1}::${expression}` }); + findings.push({ file, expression, line: i + 1, key: `${file}::${expression}` }); } }); } @@ -167,17 +157,18 @@ describe('no NEW CLI-id branching in session-ui.js / mobile-overview.js (PR B2)' ? '' : `Found ${offenders.length} new CLI-id branch(es) in session-ui.js/mobile-overview.js:\n${detail}\n\n` + 'Two ways out, in order of preference:\n' + - ' 1. Derive the difference from window.__codemanCliCatalog (server.ts) or a shared\n' + - ' module-level constant, the way _runCliMode()/EXTERNAL_CLI_MODES do.\n' + + ' 1. Derive the difference from a shared module-level constant, the way\n' + + ' _runCliMode()/RUN_MODE_LAUNCH/EXTERNAL_CLI_MODES do.\n' + ' 2. If it is a genuine mechanism difference (not a CLI-behaviour branch), add it to\n' + ' ALLOWED_BRANCHES in this file WITH the reason.' ).toEqual([]); }); it('has no stale allowlist entries', () => { - // An allowlisted branch that no longer exists at that line is a lie about the - // codebase, and the next person to reintroduce that exact branch elsewhere - // would sail straight through under the old line number. + // 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([]); diff --git a/test/opencode-resize.test.ts b/test/opencode-resize.test.ts index 8704d683..5d75bc89 100644 --- a/test/opencode-resize.test.ts +++ b/test/opencode-resize.test.ts @@ -55,18 +55,22 @@ describe('OpenCode session initial resize', () => { await context?.close(); }); - it('selectSession is not bypassed when runOpenCode sets activeSessionId', async () => { - // This test verifies at the code level that runOpenCode does NOT - // pre-set activeSessionId before calling selectSession. - // If it did, selectSession would early-return and skip sendResize. + it('selectSession is not bypassed when the shared launcher sets activeSessionId', async () => { + // This test verifies at the code level that the OpenCode launch path does + // NOT pre-set activeSessionId before calling selectSession. If it did, + // selectSession would early-return and skip sendResize. + // + // PR B2 consolidated runOpenCode() (and 7 siblings) into one shared + // _runCliMode(mode) — runOpenCode is now a one-line wrapper + // (`return this._runCliMode('opencode')`), so inspecting ITS source would + // never see the real launch logic and this check would pass vacuously + // regardless of what _runCliMode actually does. Inspect _runCliMode itself. ({ context, page } = await freshPage()); await navigateAndWait(page); - // Read the runOpenCode source from the live app and verify - // it doesn't assign activeSessionId before selectSession const hasPreAssignment = await page.evaluate(() => { - const app = (window as unknown as { app: { runOpenCode: { toString: () => string } } }).app; - const source = app.runOpenCode.toString(); + const app = (window as unknown as { app: { _runCliMode: { toString: () => string } } }).app; + const source = app._runCliMode.toString(); // Check: the source should NOT have activeSessionId = ... before selectSession // Find positions of both patterns diff --git a/test/render-index-html.test.ts b/test/render-index-html.test.ts index 32634483..bc3de380 100644 --- a/test/render-index-html.test.ts +++ b/test/render-index-html.test.ts @@ -209,38 +209,6 @@ describe('WebServer.renderIndexHtml', () => { } }); - it('exposes the general run-menu CLI catalogue with the menu-facing fields only', async () => { - // Unlike __codemanCustomModelClis above (narrowed to one picker's needs), this - // one carries every ENABLED CliEntry, agent and shell alike, with no capability - // filter — but still excludes launch/env/capabilities/overlays, the same rule - // scripts/generate-cli-catalog.mts follows for config/clis.stock.json. - const { server } = makeServer({}); - const html = await render(server); - expect(html).toContain('window.__codemanCliCatalog='); - const catalog = JSON.parse(html.match(/window\.__codemanCliCatalog=(\[.*?\]);/)![1]) as Array<{ - id: string; - label: string; - shortBadge: string; - order: number; - kind: string; - }>; - const ids = catalog.map((c) => c.id); - expect(ids).toContain('claude'); - expect(ids).toContain('shell'); - for (const cli of catalog) { - expect(typeof cli.id).toBe('string'); - expect(typeof cli.label).toBe('string'); - expect(typeof cli.shortBadge).toBe('string'); - expect(typeof cli.order).toBe('number'); - expect(['agent', 'shell']).toContain(cli.kind); - expect(cli).not.toHaveProperty('launch'); - expect(cli).not.toHaveProperty('env'); - expect(cli).not.toHaveProperty('capabilities'); - expect(cli).not.toHaveProperty('overlays'); - expect(cli).not.toHaveProperty('discovery'); - } - }); - it('escapeScriptJson neutralizes a literal , and still round-trips as a JS literal', () => { // CliEntry.label is a plain string a user's own clis.json can set (up to 60 // chars), unlike __codemanCliAvailable's booleans-only payload, so this is @@ -286,7 +254,6 @@ describe('WebServer.renderIndexHtml', () => { const html = await render(server, 'sess-123'); expect(html).not.toContain('__codemanCliAvailable'); expect(html).not.toContain('__codemanCustomModelClis'); - expect(html).not.toContain('__codemanCliCatalog'); }); it('does not expose gesture at all when CODEMAN_GESTURE is unset', async () => { diff --git a/test/run-mode-ui.test.ts b/test/run-mode-ui.test.ts index 79d6080e..49e20cae 100644 --- a/test/run-mode-ui.test.ts +++ b/test/run-mode-ui.test.ts @@ -166,8 +166,18 @@ describe('Run launch synchronization', () => { // Methods live in one Object.assign(prototype, {...}) block at a fixed // 2-space indent, so `\n },` reliably closes the one we are inside. + // + // `(\w*)` in the param list, not `()`, and the leading `_?`: PR B2 + // consolidated the eight run() bodies into one shared + // `_runCliMode(mode)`, and the ORIGINAL `\(\)`-only pattern matched every + // one-line wrapper (`async runOpenCode() { return this._runCliMode(...) }`) + // but not `_runCliMode` itself, where the real terminal-ownership logic + // now lives — so this guard could see 8 clean one-liners and stay green + // while the actual bug shipped unseen for all eight external CLIs at + // once. Confirmed live: adding `this.terminal.clear()` to `_runCliMode` + // left this test 32/32 green under the old pattern. const bodies = new Map(); - const header = /^ {2}async (run[A-Za-z]*)\(\) \{$/gm; + const header = /^ {2}async (_?run[A-Za-z]*)\(\w*\) \{$/gm; for (let m = header.exec(src); m; m = header.exec(src)) { const start = m.index + m[0].length; const end = src.indexOf('\n },', start); @@ -181,6 +191,7 @@ describe('Run launch synchronization', () => { expect.arrayContaining([ 'runClaude', 'runShell', + '_runCliMode', 'runOpenCode', 'runCodex', 'runGemini', diff --git a/test/server-index-title.test.ts b/test/server-index-title.test.ts index d36e5606..e2c7b979 100644 --- a/test/server-index-title.test.ts +++ b/test/server-index-title.test.ts @@ -96,9 +96,9 @@ describe('WebServer index.html templating (#82)', () => { it('only substitutes the <title> tag — the rest of the template is identical (modulo asset cache-busting)', async () => { // renderIndexHtml also appends ?v=<mtime> cache-bust params to same-origin - // .js/.css refs, and injects the CLI-availability flags, the custom-model - // Run-menu picker's CLI list, and the general run-menu CLI catalogue (PR B2) - // before </head>; strip all so the title remains the only other change. + // .js/.css refs, and injects the CLI-availability flags plus the custom-model + // Run-menu picker's CLI list before </head>; strip all so the title remains + // the only other change. // // The flag strips are what keep this test environment-independent. The // CLI-availability one used to pass here by luck: that script was injected @@ -109,8 +109,7 @@ describe('WebServer index.html <title> templating (#82)', () => { const html = (await render('laptop')) .replace(/(\.(?:js|css))\?v=[^"]*/g, '$1') .replace(/<script>window\.__codemanCliAvailable=\{.*?\};<\/script>\n/, '') - .replace(/<script>window\.__codemanCustomModelClis=\[.*?\];<\/script>\n/, '') - .replace(/<script>window\.__codemanCliCatalog=\[.*?\];<\/script>\n/, ''); + .replace(/<script>window\.__codemanCustomModelClis=\[.*?\];<\/script>\n/, ''); const beforeTitle = rawTemplate.split('<title>Codeman')[0]; const afterTitle = rawTemplate.split('Codeman')[1]; expect(html.startsWith(beforeTitle)).toBe(true);