mirror of
https://github.com/Ark0N/Codeman.git
synced 2026-09-30 12:39:42 +02:00
fix(cli-registry): merge-time fixes for the run-menu consolidation (#458)
- test/opencode-resize.test.ts: retarget the launcher guard at the real code (this.selectSession(firstSessionId), any this.activeSessionId assignment) with an anti-vacuity check; the old strings existed nowhere, so it could never fail - session-ui.js: restore as comments the two invariants the merged bodies lost (deepseek leaves statusReporting unset, i.e. ON; no effort field for external CLIs, it is Claude-specific) - docs/cli-registry.md: move the frontend-guard paragraph below the two backend-guard paragraphs so they keep their antecedent, and note the widened comparison shape - test/frontend-cli-no-id-branching.test.ts: the comparison shape accepts any left-hand identifier (const m = this._runMode; m === 'codex' was invisible), normalized to `mode`; the two `m !== 'shell'` display filters are allowlisted and the remaining blind spots documented - test/run-mode-dispatch.test.ts: table-driven pin of run() dispatch (claude to runClaude, each RUN_MODE_LAUNCH id to _runCliMode(id), shell to runShell, unknown to runClaude, lock held and released) - CLAUDE.md: name the second CI-gated guard next to the backend one - server.ts: every </head> injection passes a replacer function; a clis.json label containing $' re-injected the rest of the document past escapeScriptJson (two render tests pin it, proven failing on the string form) - _isAltCliMode(): no reference anywhere in the tree, nothing to fix Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> (cherry picked from commit 1ea363ff808a62861559bc141e724b163cc1c56e)
This commit is contained in:
@@ -98,12 +98,12 @@ 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/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.
|
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).
|
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).
|
||||||
|
|
||||||
|
`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. Its comparison shape differs from the backend guard's in one respect: the left-hand side may be any identifier, not only one named `mode`, `id` or `agentType`, because the review of #458 found `const m = this._runMode; if (m === 'codex')` slipping past the named form while the scanned file already filters with `(m) => m !== 'shell'`.
|
||||||
|
|
||||||
## Two namespaces called `param`
|
## Two namespaces called `param`
|
||||||
|
|
||||||
`launch.params` keys, `env.configSetenv[].fromParam` and `capabilities.privilegedParams[].param` all name a **launch param**. The **legacy wire field** a param arrives as is a separate namespace, and `launch.legacyConfigAliases` is the only bridge between the two.
|
`launch.params` keys, `env.configSetenv[].fromParam` and `capabilities.privilegedParams[].param` all name a **launch param**. The **legacy wire field** a param arrives as is a separate namespace, and `launch.legacyConfigAliases` is the only bridge between the two.
|
||||||
|
|||||||
@@ -105,6 +105,11 @@ const RUN_MODE_LAUNCH = {
|
|||||||
// sibling Run button sends its bypass switch. The harness has no bypass
|
// sibling Run button sends its bypass switch. The harness has no bypass
|
||||||
// FLAG, so this rides the `DSH_PERMISSION_MODE` export instead, and the
|
// FLAG, so this rides the `DSH_PERMISSION_MODE` export instead, and the
|
||||||
// multi-user clamp forces it back down to `workspace-write` server-side.
|
// multi-user clamp forces it back down to `workspace-write` server-side.
|
||||||
|
//
|
||||||
|
// `statusReporting` is deliberately LEFT UNSET, i.e. ON: it is what upgrades
|
||||||
|
// this mode from output-stabilization guessing to definitive idle/blocked
|
||||||
|
// hook events (the harness reports to Codeman as its supervisor, see
|
||||||
|
// deepseek-status-shim.ts). Never send `statusReporting: false` from here.
|
||||||
buildConfig: () => ({ deepSeekConfig: { permissionMode: 'danger-full-access' } }),
|
buildConfig: () => ({ deepSeekConfig: { permissionMode: 'danger-full-access' } }),
|
||||||
},
|
},
|
||||||
};
|
};
|
||||||
@@ -2157,6 +2162,10 @@ Object.assign(CodemanApp.prototype, {
|
|||||||
|
|
||||||
const globalSettings = this.loadAppSettingsFromStorage();
|
const globalSettings = this.loadAppSettingsFromStorage();
|
||||||
const envOverrides = this.buildEnvOverrides(this.getCaseSettings(caseName), globalSettings);
|
const envOverrides = this.buildEnvOverrides(this.getCaseSettings(caseName), globalSettings);
|
||||||
|
// No `effort` field for ANY entry in RUN_MODE_LAUNCH: effort is
|
||||||
|
// Claude-specific (runClaude() alone sends it, and the backend turns it
|
||||||
|
// into `claude --settings`); none of these CLIs has an /effort. Each of
|
||||||
|
// the eight bodies this launcher replaced carried that rule as a comment.
|
||||||
const firstSessionId = await this._launchQuickStartInstances(
|
const firstSessionId = await this._launchQuickStartInstances(
|
||||||
caseName,
|
caseName,
|
||||||
tabCount,
|
tabCount,
|
||||||
|
|||||||
+16
-5
@@ -1581,9 +1581,17 @@ export class WebServer extends EventEmitter {
|
|||||||
// the /session/:id URL path; this global is a belt-and-suspenders fallback.
|
// the /session/:id URL path; this global is a belt-and-suspenders fallback.
|
||||||
// The id is gated to JSON + <-escaped so it can't break out of the inline
|
// The id is gated to JSON + <-escaped so it can't break out of the inline
|
||||||
// <script> (ids are UUIDs in practice, but defense-in-depth is cheap).
|
// <script> (ids are UUIDs in practice, but defense-in-depth is cheap).
|
||||||
|
//
|
||||||
|
// Every `</head>` injection below passes a replacer FUNCTION, never a
|
||||||
|
// replacement STRING: `String.replace` interprets `$&`, `$'`, `` $` `` and
|
||||||
|
// `$<n>` inside a string replacement, so a payload carrying `$'` would splice
|
||||||
|
// the rest of the document (the whole <body>) into the inline script, past
|
||||||
|
// any escaping applied to the payload itself. The custom-model list below
|
||||||
|
// carries a user-settable `label` (clis.json), which is the site that made
|
||||||
|
// this real; the others follow the same rule so the class of bug stays out.
|
||||||
if (soloSessionId) {
|
if (soloSessionId) {
|
||||||
const safeId = JSON.stringify(soloSessionId).replace(/</g, '\\u003c');
|
const safeId = JSON.stringify(soloSessionId).replace(/</g, '\\u003c');
|
||||||
html = html.replace('</head>', `<script>window.__CODEMAN_SOLO__=${safeId};</script>\n</head>`);
|
html = html.replace('</head>', () => `<script>window.__CODEMAN_SOLO__=${safeId};</script>\n</head>`);
|
||||||
}
|
}
|
||||||
// Gesture-control overlay (Phase 5): dashboard only (not solo popups, which
|
// Gesture-control overlay (Phase 5): dashboard only (not solo popups, which
|
||||||
// have no tab strip). `CODEMAN_GESTURE=1` makes the feature *available* on
|
// have no tab strip). `CODEMAN_GESTURE=1` makes the feature *available* on
|
||||||
@@ -1655,7 +1663,7 @@ export class WebServer extends EventEmitter {
|
|||||||
};
|
};
|
||||||
html = html.replace(
|
html = html.replace(
|
||||||
'</head>',
|
'</head>',
|
||||||
`<script>window.__codemanCliAvailable=${JSON.stringify(available)};</script>\n</head>`
|
() => `<script>window.__codemanCliAvailable=${JSON.stringify(available)};</script>\n</head>`
|
||||||
);
|
);
|
||||||
// Which run modes the Run-menu picker (docs/custom-model-endpoints-plan.md) may
|
// Which run modes the Run-menu picker (docs/custom-model-endpoints-plan.md) may
|
||||||
// generate an entry for: read generically off the registry's `capabilities`
|
// generate an entry for: read generically off the registry's `capabilities`
|
||||||
@@ -1672,16 +1680,19 @@ export class WebServer extends EventEmitter {
|
|||||||
const customModelClisJson = escapeScriptJson(JSON.stringify(customModelClis));
|
const customModelClisJson = escapeScriptJson(JSON.stringify(customModelClis));
|
||||||
html = html.replace(
|
html = html.replace(
|
||||||
'</head>',
|
'</head>',
|
||||||
`<script>window.__codemanCustomModelClis=${customModelClisJson};</script>\n</head>`
|
() => `<script>window.__codemanCustomModelClis=${customModelClisJson};</script>\n</head>`
|
||||||
);
|
);
|
||||||
}
|
}
|
||||||
if (!soloSessionId && process.env.CODEMAN_GESTURE === '1') {
|
if (!soloSessionId && process.env.CODEMAN_GESTURE === '1') {
|
||||||
html = html.replace('</head>', `<script>window.__codemanGestureAvailable=true;</script>\n</head>`);
|
html = html.replace('</head>', () => `<script>window.__codemanGestureAvailable=true;</script>\n</head>`);
|
||||||
if (settings.gestureControlEnabled === true) {
|
if (settings.gestureControlEnabled === true) {
|
||||||
const v = this.gestureBundleVersion();
|
const v = this.gestureBundleVersion();
|
||||||
// Relative src so the injected `<base href>` resolves it under the mount
|
// Relative src so the injected `<base href>` resolves it under the mount
|
||||||
// prefix (a root-absolute `/gesture/...` would escape a sub-path mount).
|
// prefix (a root-absolute `/gesture/...` would escape a sub-path mount).
|
||||||
html = html.replace('</head>', `<script type="module" src="gesture/gesture-codeman.js${v}"></script>\n</head>`);
|
html = html.replace(
|
||||||
|
'</head>',
|
||||||
|
() => `<script type="module" src="gesture/gesture-codeman.js${v}"></script>\n</head>`
|
||||||
|
);
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
return html;
|
return html;
|
||||||
|
|||||||
@@ -82,6 +82,17 @@ const ALLOWED_BRANCHES: Record<string, { count: number; reason: string }> = {
|
|||||||
"session-ui.js::mode === 'deepseek'": { 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' },
|
"session-ui.js::mode === 'omp'": { count: 2, reason: 'button-label ternary + runMode setter validity check' },
|
||||||
|
|
||||||
|
// The docker adopt-preflight status line and the docker link/adopt toast
|
||||||
|
// both list the agent CLIs probed INSIDE the container and leave `shell`
|
||||||
|
// out of that human-readable "found ..." summary (it is always present and
|
||||||
|
// is not an agent CLI). Written as `(m) => m !== 'shell'`, the naming the
|
||||||
|
// original named-variable pattern could not see; the widened pattern
|
||||||
|
// normalizes the `m` to `mode` (see BRANCH_PATTERN below).
|
||||||
|
"session-ui.js::mode !== 'shell'": {
|
||||||
|
count: 2,
|
||||||
|
reason: 'display filter: the "CLIs found inside the container" summaries omit shell, which is not an agent CLI',
|
||||||
|
},
|
||||||
|
|
||||||
// mobile-overview.js: shell is exempt from the isCliAvailable() gate the
|
// mobile-overview.js: shell is exempt from the isCliAvailable() gate the
|
||||||
// same way the toolbar's #runModeMenu exempts it (shell needs no CLI).
|
// same way the toolbar's #runModeMenu exempts it (shell needs no CLI).
|
||||||
"mobile-overview.js::mode !== 'shell'": {
|
"mobile-overview.js::mode !== 'shell'": {
|
||||||
@@ -94,11 +105,35 @@ const ALLOWED_BRANCHES: Record<string, { count: number; reason: string }> = {
|
|||||||
const IDS = STOCK_CLIS.map((e) => e.id as string);
|
const IDS = STOCK_CLIS.map((e) => e.id as string);
|
||||||
const ID_ALT = IDS.join('|');
|
const ID_ALT = IDS.join('|');
|
||||||
|
|
||||||
/** Same four shapes as the backend guard — see its own comment for why all four matter. */
|
/**
|
||||||
|
* The backend guard's four shapes (see its own comment for why all four
|
||||||
|
* matter), with ONE deliberate widening on the first.
|
||||||
|
*
|
||||||
|
* The backend pattern accepts a comparison only when its left-hand side is
|
||||||
|
* literally named `mode`, `id` or `agentType`, so both
|
||||||
|
* `const m = this._runMode; if (m === 'codex')` and
|
||||||
|
* `if (this._runMode !== 'gemini')` slip past it, and `session-ui.js` already
|
||||||
|
* uses exactly that naming (`(m) => m !== 'shell'`, twice). The review of
|
||||||
|
* PR #458 surfaced that blind spot, so here the left-hand side is ANY
|
||||||
|
* identifier (`[\w$]+`, the leaf of a member chain), normalized to `mode` in
|
||||||
|
* the allowlist key by `scan()` so a local rename never churns the entries.
|
||||||
|
* Measured over both scanned files before widening: every extra hit was a
|
||||||
|
* genuine mode comparison (the two `m !== 'shell'` filters, allowlisted
|
||||||
|
* above), so the widening added no false positive; a future one gets an
|
||||||
|
* allowlist entry with its reason like any other. The backend guard keeps
|
||||||
|
* its narrower form and is deliberately not changed here.
|
||||||
|
*
|
||||||
|
* Still unseen, and worth knowing: a Yoda comparison (`'codex' === mode`),
|
||||||
|
* and an id list held in a variable (`EXTERNAL.includes(mode)`), since the
|
||||||
|
* third shape needs the literal list inline.
|
||||||
|
*/
|
||||||
const BRANCH_PATTERN = new RegExp(
|
const BRANCH_PATTERN = new RegExp(
|
||||||
[
|
[
|
||||||
`\\b(?:mode|id|agentType)\\s*[!=]==\\s*'(?:${ID_ALT})'`,
|
// <identifier> === 'codex' / <identifier> !== 'codex' (any left-hand identifier, see above)
|
||||||
|
`\\b[\\w$]+\\s*[!=]==\\s*'(?:${ID_ALT})'`,
|
||||||
|
// case 'codex':
|
||||||
`\\bcase\\s+'(?:${ID_ALT})'\\s*:`,
|
`\\bcase\\s+'(?:${ID_ALT})'\\s*:`,
|
||||||
|
// ['codex', 'gemini'].includes(mode) — the id list IS the branch, wherever `mode` sits
|
||||||
`'(?:${ID_ALT})'\\s*(?:,\\s*'(?:${ID_ALT})'\\s*)*\\]\\s*\\.includes\\(`,
|
`'(?:${ID_ALT})'\\s*(?:,\\s*'(?:${ID_ALT})'\\s*)*\\]\\s*\\.includes\\(`,
|
||||||
].join('|'),
|
].join('|'),
|
||||||
'g'
|
'g'
|
||||||
@@ -126,7 +161,10 @@ function scan(): Finding[] {
|
|||||||
lines.forEach((line, i) => {
|
lines.forEach((line, i) => {
|
||||||
BRANCH_PATTERN.lastIndex = 0; // shared /g regex — see utils/regex-patterns.ts
|
BRANCH_PATTERN.lastIndex = 0; // shared /g regex — see utils/regex-patterns.ts
|
||||||
for (const match of line.matchAll(BRANCH_PATTERN)) {
|
for (const match of line.matchAll(BRANCH_PATTERN)) {
|
||||||
const expression = match[0].replace(/\s+/g, ' ').replace(/^(?:id|agentType)/, 'mode');
|
// Normalize the comparison shape's left-hand identifier (whatever the
|
||||||
|
// local is called: `id`, `agentType`, `m`, `_runMode`) to `mode`; the
|
||||||
|
// lookahead leaves the `case`/`.includes(` shapes untouched.
|
||||||
|
const expression = match[0].replace(/\s+/g, ' ').replace(/^[\w$]+(?=\s*[!=]==)/, 'mode');
|
||||||
findings.push({ file, expression, line: i + 1, key: `${file}::${expression}` });
|
findings.push({ file, expression, line: i + 1, key: `${file}::${expression}` });
|
||||||
}
|
}
|
||||||
});
|
});
|
||||||
@@ -162,6 +200,9 @@ describe('no NEW CLI-id branching in session-ui.js / mobile-overview.js (PR B2)'
|
|||||||
"if (mode !== 'shell' && mode !== 'deepseek') { doSomething(); }",
|
"if (mode !== 'shell' && mode !== 'deepseek') { doSomething(); }",
|
||||||
"switch (mode) { case 'gemini': return 1; }",
|
"switch (mode) { case 'gemini': return 1; }",
|
||||||
"if (['codex', 'gemini'].includes(mode)) { doSomething(); }",
|
"if (['codex', 'gemini'].includes(mode)) { doSomething(); }",
|
||||||
|
// The two forms the named-variable pattern was blind to (see BRANCH_PATTERN).
|
||||||
|
"const m = this._runMode; if (m === 'codex') { doSomething(); }",
|
||||||
|
"if (this._runMode !== 'gemini') { doSomething(); }",
|
||||||
];
|
];
|
||||||
for (const sample of samples) {
|
for (const sample of samples) {
|
||||||
BRANCH_PATTERN.lastIndex = 0;
|
BRANCH_PATTERN.lastIndex = 0;
|
||||||
|
|||||||
@@ -68,23 +68,33 @@ describe('OpenCode session initial resize', () => {
|
|||||||
({ context, page } = await freshPage());
|
({ context, page } = await freshPage());
|
||||||
await navigateAndWait(page);
|
await navigateAndWait(page);
|
||||||
|
|
||||||
const hasPreAssignment = await page.evaluate(() => {
|
const { selectIdx, assignIdx } = await page.evaluate(() => {
|
||||||
const app = (window as unknown as { app: { _runCliMode: { toString: () => string } } }).app;
|
const app = (window as unknown as { app: { _runCliMode: { toString: () => string } } }).app;
|
||||||
const source = app._runCliMode.toString();
|
const source = app._runCliMode.toString();
|
||||||
|
|
||||||
// Check: the source should NOT have activeSessionId = ... before selectSession
|
// The launcher hands the FIRST created session to selectSession
|
||||||
// Find positions of both patterns
|
// (`_launchQuickStartInstances()` returns `firstSessionId`). An earlier
|
||||||
const assignIdx = source.indexOf('this.activeSessionId = data.sessionId');
|
// version of this check looked for `this.selectSession(data.sessionId)`,
|
||||||
const selectIdx = source.indexOf('this.selectSession(data.sessionId)');
|
// a string that exists nowhere in session-ui.js, so both lookups came
|
||||||
|
// back -1 and the assertion could never fail. Hence the anti-vacuity
|
||||||
// If assign doesn't exist at all, that's the correct fix
|
// check below: the select call itself must be found.
|
||||||
if (assignIdx === -1) return false;
|
const selectIdx = source.indexOf('this.selectSession(firstSessionId)');
|
||||||
|
// ANY assignment to activeSessionId (whatever the right-hand side is
|
||||||
// If assign comes before select, that's the bug
|
// called), not `==`/`===` comparisons and not the comment that mentions
|
||||||
return assignIdx < selectIdx;
|
// pre-setting it without a `this.` prefix.
|
||||||
|
const assign = /this\.activeSessionId\s*=(?!=)/.exec(source);
|
||||||
|
return { selectIdx, assignIdx: assign ? assign.index : -1 };
|
||||||
});
|
});
|
||||||
|
|
||||||
expect(hasPreAssignment).toBe(false);
|
// Anti-vacuity: if the select call is renamed again, fail here rather
|
||||||
|
// than pass on two -1s.
|
||||||
|
expect(selectIdx).toBeGreaterThan(-1);
|
||||||
|
// Correct: no assignment at all. Bug: an assignment that lands BEFORE
|
||||||
|
// selectSession runs, which makes selectSession early-return.
|
||||||
|
expect(
|
||||||
|
assignIdx === -1 || assignIdx > selectIdx,
|
||||||
|
`activeSessionId is assigned at ${assignIdx}, before selectSession at ${selectIdx}`
|
||||||
|
).toBe(true);
|
||||||
});
|
});
|
||||||
|
|
||||||
it('sends resize to server after creating a session via quick-start', async () => {
|
it('sends resize to server after creating a session via quick-start', async () => {
|
||||||
|
|||||||
@@ -23,6 +23,8 @@ import { isDeepSeekAvailable, isDeepSeekRunnable } from '../src/utils/deepseek-c
|
|||||||
import { isOmpAvailable } from '../src/utils/omp-cli-resolver.js';
|
import { isOmpAvailable } from '../src/utils/omp-cli-resolver.js';
|
||||||
import { isCloudflaredAvailable } from '../src/utils/cloudflared-resolver.js';
|
import { isCloudflaredAvailable } from '../src/utils/cloudflared-resolver.js';
|
||||||
import { isGitAvailable } from '../src/git-clone.js';
|
import { isGitAvailable } from '../src/git-clone.js';
|
||||||
|
import { enabledClis } from '../src/config/cli-registry/registry.js';
|
||||||
|
import { STOCK_CLIS } from '../src/config/cli-registry/stock.js';
|
||||||
|
|
||||||
// renderIndexHtml probes the real PATH for every CLI, which would make the
|
// renderIndexHtml probes the real PATH for every CLI, which would make the
|
||||||
// assertions below depend on whatever happens to be installed on the machine
|
// assertions below depend on whatever happens to be installed on the machine
|
||||||
@@ -79,6 +81,14 @@ vi.mock('../src/utils/cloudflared-resolver.js', () => ({
|
|||||||
vi.mock('../src/git-clone.js', () => ({
|
vi.mock('../src/git-clone.js', () => ({
|
||||||
isGitAvailable: vi.fn(() => false),
|
isGitAvailable: vi.fn(() => false),
|
||||||
}));
|
}));
|
||||||
|
// The custom-model list carries `label`, a string a user's own clis.json can set.
|
||||||
|
// Wrap enabledClis so ONE test below can hand renderIndexHtml a label with `$'`
|
||||||
|
// in it while every other test still reads the real stock registry through the
|
||||||
|
// real implementation.
|
||||||
|
vi.mock('../src/config/cli-registry/registry.js', async (importOriginal) => {
|
||||||
|
const actual = await importOriginal<typeof import('../src/config/cli-registry/registry.js')>();
|
||||||
|
return { ...actual, enabledClis: vi.fn(actual.enabledClis) };
|
||||||
|
});
|
||||||
|
|
||||||
const TEMPLATE = [
|
const TEMPLATE = [
|
||||||
'<head>',
|
'<head>',
|
||||||
@@ -222,6 +232,35 @@ describe('WebServer.renderIndexHtml', () => {
|
|||||||
expect(eval(escaped)[0].label).toBe('</script><script>alert(1)</script>');
|
expect(eval(escaped)[0].label).toBe('</script><script>alert(1)</script>');
|
||||||
});
|
});
|
||||||
|
|
||||||
|
it("inserts a label containing $' verbatim instead of splicing the document into the script", async () => {
|
||||||
|
// `String.replace` with a STRING replacement interprets `$'` as "the text
|
||||||
|
// after the match", so a clis.json label carrying it used to re-inject the
|
||||||
|
// rest of the document (the whole <body>) into the inline script, past
|
||||||
|
// escapeScriptJson, which only neutralizes `<`. Every `</head>` injection
|
||||||
|
// passes a replacer FUNCTION instead, whose return value is inserted
|
||||||
|
// verbatim. The other `$` forms ride along so a partial escape cannot pass.
|
||||||
|
const claude = STOCK_CLIS.find((e) => e.id === 'claude')!;
|
||||||
|
const label = "Claude $' $& $` $1 $$";
|
||||||
|
const real = vi.mocked(enabledClis).getMockImplementation()!;
|
||||||
|
vi.mocked(enabledClis).mockImplementation(() => [{ ...claude, label }]);
|
||||||
|
try {
|
||||||
|
const { server } = makeServer({});
|
||||||
|
const html = await render(server);
|
||||||
|
expect(html.match(/<body>/g)).toHaveLength(1);
|
||||||
|
const clis = JSON.parse(html.match(/window\.__codemanCustomModelClis=(\[.*?\]);/)![1]);
|
||||||
|
expect(clis).toEqual([{ id: 'claude', label }]);
|
||||||
|
} finally {
|
||||||
|
vi.mocked(enabledClis).mockImplementation(real);
|
||||||
|
}
|
||||||
|
});
|
||||||
|
|
||||||
|
it("inserts a solo id containing $' verbatim, under the same replacer rule", async () => {
|
||||||
|
const { server } = makeServer({});
|
||||||
|
const html = await render(server, "sess$'x");
|
||||||
|
expect(html.match(/<body>/g)).toHaveLength(1);
|
||||||
|
expect(html).toContain(`window.__CODEMAN_SOLO__="sess$'x"`);
|
||||||
|
});
|
||||||
|
|
||||||
it('still emits the object when nothing at all is installed', async () => {
|
it('still emits the object when nothing at all is installed', async () => {
|
||||||
// The all-false case is the one that matters most and the easiest to get
|
// The all-false case is the one that matters most and the easiest to get
|
||||||
// wrong by only injecting when something resolves.
|
// wrong by only injecting when something resolves.
|
||||||
|
|||||||
@@ -0,0 +1,147 @@
|
|||||||
|
/**
|
||||||
|
* @fileoverview Table-driven pin for `run()`'s dispatch in session-ui.js
|
||||||
|
* (PR #458). Before the run-menu consolidation, `run()` was an eight-arm
|
||||||
|
* `if (mode === 'codex') return this.runCodex(); ...` chain and each arm was
|
||||||
|
* pinned only by the name it called; after it, every non-claude, non-shell
|
||||||
|
* mode reaches ONE shared launcher, `_runCliMode(mode)`, gated on
|
||||||
|
* `EXTERNAL_CLI_MODES` (the key set of `RUN_MODE_LAUNCH`). Nothing pinned that
|
||||||
|
* gate: a mode dropped from the table would fall through to `runClaude()` and
|
||||||
|
* start a Claude session under a Codex label with no error, while an unknown
|
||||||
|
* mode reaching `_runCliMode()` would throw on `entry.label` of an undefined
|
||||||
|
* entry.
|
||||||
|
*
|
||||||
|
* The external ids are read off `RUN_MODE_LAUNCH` itself (same JSDOM
|
||||||
|
* extraction as test/run-mode-launch-table-drift.test.ts, which separately
|
||||||
|
* pins that key set against stock.ts), so a ninth CLI is covered the day it
|
||||||
|
* lands in the table.
|
||||||
|
*
|
||||||
|
* Port: none.
|
||||||
|
*/
|
||||||
|
import { readFileSync } from 'node:fs';
|
||||||
|
import { JSDOM } from 'jsdom';
|
||||||
|
import { describe, expect, it, vi } from 'vitest';
|
||||||
|
|
||||||
|
const SESSION_UI_JS = readFileSync(new URL('../src/web/public/session-ui.js', import.meta.url), 'utf-8');
|
||||||
|
|
||||||
|
interface HarnessApp {
|
||||||
|
_runMode?: string;
|
||||||
|
_runInFlight?: boolean;
|
||||||
|
_runMinLockMs?: number;
|
||||||
|
run: () => Promise<unknown>;
|
||||||
|
runClaude: ReturnType<typeof vi.fn>;
|
||||||
|
runShell: ReturnType<typeof vi.fn>;
|
||||||
|
_runCliMode: ReturnType<typeof vi.fn>;
|
||||||
|
}
|
||||||
|
|
||||||
|
interface Harness {
|
||||||
|
app: HarnessApp;
|
||||||
|
runBtn: HTMLButtonElement;
|
||||||
|
externalIds: string[];
|
||||||
|
}
|
||||||
|
|
||||||
|
function loadHarness(): Harness {
|
||||||
|
const dom = new JSDOM('<!doctype html><body><button id="runBtn"></button></body>', {
|
||||||
|
url: 'http://localhost/',
|
||||||
|
runScripts: 'dangerously',
|
||||||
|
});
|
||||||
|
const win = dom.window as unknown as {
|
||||||
|
eval: (s: string) => void;
|
||||||
|
document: Document;
|
||||||
|
CodemanApp: new () => HarnessApp;
|
||||||
|
__TEST_RUN_MODE_LAUNCH: Record<string, unknown>;
|
||||||
|
};
|
||||||
|
win.eval('window.CodemanApp = function CodemanApp() {};');
|
||||||
|
// The assignment rides in the SAME evaluated string as the module:
|
||||||
|
// RUN_MODE_LAUNCH is a bare top-level `const`, visible only to this eval call
|
||||||
|
// (see test/run-mode-launch-table-drift.test.ts for the measurement).
|
||||||
|
win.eval(`${SESSION_UI_JS}\nwindow.__TEST_RUN_MODE_LAUNCH = RUN_MODE_LAUNCH;`);
|
||||||
|
const app = new win.CodemanApp();
|
||||||
|
app._runMinLockMs = 0; // run() otherwise holds its lock for >= 500ms per call
|
||||||
|
app.runClaude = vi.fn(async () => 'claude');
|
||||||
|
app.runShell = vi.fn(async () => 'shell');
|
||||||
|
app._runCliMode = vi.fn(async (mode: string) => `cli:${mode}`);
|
||||||
|
return {
|
||||||
|
app,
|
||||||
|
runBtn: win.document.getElementById('runBtn') as HTMLButtonElement,
|
||||||
|
externalIds: Object.keys(win.__TEST_RUN_MODE_LAUNCH),
|
||||||
|
};
|
||||||
|
}
|
||||||
|
|
||||||
|
describe('run() dispatch (session-ui.js)', () => {
|
||||||
|
const { externalIds } = loadHarness();
|
||||||
|
|
||||||
|
it('reads at least the eight external CLIs off RUN_MODE_LAUNCH (anti-vacuity)', () => {
|
||||||
|
expect(externalIds.length).toBeGreaterThanOrEqual(8);
|
||||||
|
expect(externalIds).not.toContain('claude');
|
||||||
|
expect(externalIds).not.toContain('shell');
|
||||||
|
});
|
||||||
|
|
||||||
|
it("'claude' reaches runClaude() and nothing else", async () => {
|
||||||
|
const { app } = loadHarness();
|
||||||
|
app._runMode = 'claude';
|
||||||
|
await expect(app.run()).resolves.toBe('claude');
|
||||||
|
expect(app.runClaude).toHaveBeenCalledTimes(1);
|
||||||
|
expect(app._runCliMode).not.toHaveBeenCalled();
|
||||||
|
expect(app.runShell).not.toHaveBeenCalled();
|
||||||
|
});
|
||||||
|
|
||||||
|
it("'shell' reaches runShell(), which needs no CLI probe at all", async () => {
|
||||||
|
const { app } = loadHarness();
|
||||||
|
app._runMode = 'shell';
|
||||||
|
await expect(app.run()).resolves.toBe('shell');
|
||||||
|
expect(app.runShell).toHaveBeenCalledTimes(1);
|
||||||
|
expect(app.runClaude).not.toHaveBeenCalled();
|
||||||
|
expect(app._runCliMode).not.toHaveBeenCalled();
|
||||||
|
});
|
||||||
|
|
||||||
|
for (const id of externalIds) {
|
||||||
|
it(`'${id}' reaches _runCliMode('${id}') and never runClaude()`, async () => {
|
||||||
|
const { app } = loadHarness();
|
||||||
|
app._runMode = id;
|
||||||
|
await expect(app.run()).resolves.toBe(`cli:${id}`);
|
||||||
|
expect(app._runCliMode).toHaveBeenCalledTimes(1);
|
||||||
|
expect(app._runCliMode).toHaveBeenCalledWith(id);
|
||||||
|
expect(app.runClaude).not.toHaveBeenCalled();
|
||||||
|
expect(app.runShell).not.toHaveBeenCalled();
|
||||||
|
});
|
||||||
|
}
|
||||||
|
|
||||||
|
it('an unknown mode lands on runClaude(), never on the shared launcher', async () => {
|
||||||
|
// `_runCliMode(mode)` reads `RUN_MODE_LAUNCH[mode].label` unguarded, so an
|
||||||
|
// unknown id reaching it would throw rather than launch anything.
|
||||||
|
for (const mode of ['nope', 'CLAUDE', 'code x']) {
|
||||||
|
const { app } = loadHarness();
|
||||||
|
app._runMode = mode;
|
||||||
|
await expect(app.run()).resolves.toBe('claude');
|
||||||
|
expect(app.runClaude).toHaveBeenCalledTimes(1);
|
||||||
|
expect(app._runCliMode).not.toHaveBeenCalled();
|
||||||
|
expect(app.runShell).not.toHaveBeenCalled();
|
||||||
|
}
|
||||||
|
});
|
||||||
|
|
||||||
|
it('an unset or empty _runMode defaults to claude', async () => {
|
||||||
|
for (const mode of [undefined, '']) {
|
||||||
|
const { app } = loadHarness();
|
||||||
|
app._runMode = mode;
|
||||||
|
await expect(app.run()).resolves.toBe('claude');
|
||||||
|
expect(app.runClaude).toHaveBeenCalledTimes(1);
|
||||||
|
expect(app._runCliMode).not.toHaveBeenCalled();
|
||||||
|
}
|
||||||
|
});
|
||||||
|
|
||||||
|
it('holds the launch lock for the whole launch and releases it afterwards', async () => {
|
||||||
|
const { app, runBtn } = loadHarness();
|
||||||
|
app._runMode = 'codex';
|
||||||
|
const pending = app.run();
|
||||||
|
expect(app._runInFlight).toBe(true);
|
||||||
|
expect(runBtn.disabled).toBe(true);
|
||||||
|
expect(runBtn.getAttribute('aria-busy')).toBe('true');
|
||||||
|
// A second click while the first launch is in flight is a no-op.
|
||||||
|
await expect(app.run()).resolves.toBeUndefined();
|
||||||
|
await pending;
|
||||||
|
expect(app._runCliMode).toHaveBeenCalledTimes(1);
|
||||||
|
expect(app._runInFlight).toBe(false);
|
||||||
|
expect(runBtn.disabled).toBe(false);
|
||||||
|
expect(runBtn.hasAttribute('aria-busy')).toBe(false);
|
||||||
|
});
|
||||||
|
});
|
||||||
Reference in New Issue
Block a user