mirror of
https://github.com/Ark0N/Codeman.git
synced 2026-09-30 12:39:42 +02:00
fix(cli-registry): address PR B2 review — fix two test guards, drop unused catalogue
Two required fixes from Ark0N's review of #458: 1. test/frontend-cli-no-id-branching.test.ts's ALLOWED_BRANCHES keyed on <file>::<line>::<expression>. A single inserted line anywhere above an entry shifted every subsequent line number, so all 21 entries went stale simultaneously and the same 21 branches were reported as "new" — on a file six other open PRs also touch. Dropped the line number from the key (<file>::<expression>, matching the backend guard's own design), which collapses 21 line-keyed entries to 11 or-collapse where the same expression recurs at multiple call sites in the same file. 2. test/run-mode-ui.test.ts's terminal-ownership guard scanned method bodies via `^ {2}async (run[A-Za-z]*)\(\) \{$`, which matched the 8 one-line run<Mode>() wrappers PR B2 introduced but not _runCliMode(mode), where the real logic (and the actual risk the guard exists to catch) now lives. Fixed the regex to `^ {2}async (_?run[A-Za-z]*)\(\w*\) \{$` and added _runCliMode to the sanity list. Same-class fix in test/opencode-resize.test.ts, which had the identical blind spot via runOpenCode.toString(). Both reproduced live before fixing (inserted the same comment line; added this.terminal.clear() to _runCliMode) to confirm the bug, then confirmed the fix catches it and the suite stays green otherwise. Also resolves Open Question 2 by dropping window.__codemanCliCatalog entirely: nothing consumed it, and a registry DECLARED_FOR_LATER field costs nothing until read while an unconsumed script tag on every page render is a different trade. Reverts Phase 1 cleanly — server.ts's injection, shortBadge back in types.ts's DECLARED_FOR_LATER list and the pinned guard test, and the three associated render-index-html.test.ts / server-index-title.test.ts assertions. Full gate: 405 files / 7717 tests / 0 failures (net unchanged), typecheck/ lint/format clean. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GuHtuPiHXdykq9T6rKQJ9n
This commit is contained in:
Devvyn
co-authored by
Claude Sonnet 5
Claude Sonnet 5
parent
cd64b0a3f7
commit
2df9355367
@@ -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',
|
||||
|
||||
@@ -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
|
||||
* `<file>::<line>::<the matched expression>`. 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.
|
||||
* `<file>::<the matched expression>` — deliberately NO line number. An
|
||||
* earlier version keyed on `<file>::<line>::<expression>`, 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<string, string> = {
|
||||
// --- 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([]);
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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 </script>, 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 () => {
|
||||
|
||||
@@ -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<Mode>() 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<string, string>();
|
||||
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',
|
||||
|
||||
@@ -96,9 +96,9 @@ describe('WebServer index.html <title> 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</title>')[0];
|
||||
const afterTitle = rawTemplate.split('<title>Codeman</title>')[1];
|
||||
expect(html.startsWith(beforeTitle)).toBe(true);
|
||||
|
||||
Reference in New Issue
Block a user