mirror of
https://github.com/Ark0N/Codeman.git
synced 2026-09-30 12:39:42 +02:00
fix(cli-registry): address round-2 review on #458 — count-based allowlist, RUN_MODE_LAUNCH drift guard
Three of Ark0N's four "will take at merge" items, applied instead since they were straightforward to do properly: 1. test/frontend-cli-no-id-branching.test.ts's ALLOWED_BRANCHES keyed on <file>::<expression> (fixed last round) closed the line-shift problem but opened a new one: every stock id was already allowlisted for session-ui.js in the `mode === '<id>'` form, so a BRAND NEW branch reusing that exact expression anywhere in the file passed unnoticed. Reproduced live (`if (this.mode === 'codex')` injected into runOpenCode()) — stayed green under the old version. Each allowlist entry now carries the exact count of approved call sites, and a new test asserts actual-vs-declared count for every key; a mismatch in either direction is real (higher = new unreviewed branch riding in on an existing approval, lower = a reviewed site was removed and the entry is now stale). Reproduced again against the fix: same injection now fails with an exact diagnostic (expected 2, found 3). 2. Added test/run-mode-launch-table-drift.test.ts. RUN_MODE_LAUNCH restates four things stock.ts already owns (label, install command, supportsCustomModel, the external-mode key set), and they agree today with nothing enforcing it. supportsCustomModel is the dangerous one: the Run-menu picker's rows come from the server-injected window.__codemanCustomModelClis (built from capabilities.customModelInjection.kind), so a CLI gaining a real injection recipe later would be OFFERED in the picker while _runCliMode silently 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 and compares RUN_MODE_LAUNCH against STOCK_CLIS on all four axes. 3. Inlined the "Open Question 7 in PR-B2.md" references in the allowlist reasons — PR-B2.md is a local planning doc, never part of the committed tree, so the reference was dead on arrival for anyone reading the repo. Points at the PR #458 review thread instead. 4. Added a sentence to docs/cli-registry.md naming the new frontend guard alongside the backend one it mirrors. Full gate: 406 files / 7721 tests / 0 failures, typecheck/lint/format/ check:frontend-syntax all 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
2df9355367
commit
d9e6ebb20a
@@ -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 `<file>::<expression>` 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 === '<id>'`, `mode !== '<id>'`, `case '<id>':`, and `['<id>', …].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 `<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).
|
||||
|
||||
@@ -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
|
||||
* `<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).
|
||||
* 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 `<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.
|
||||
*
|
||||
* 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<string, string> = {
|
||||
"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<string, { count: number; reason: string }> = {
|
||||
"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<string, number> {
|
||||
const counts = new Map<string, number>();
|
||||
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([]);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -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<string, unknown>) => Record<string, unknown> | null;
|
||||
}
|
||||
|
||||
function loadRunModeLaunch(): Record<string, RunModeLaunchEntry> {
|
||||
const dom = new JSDOM('<!doctype html><body></body>', { 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<string, RunModeLaunchEntry> }).__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);
|
||||
}
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user