fix(custom-model): act on the draft review — unparseable onclick, unwrapped envelope, wrong-session apply, missing lock, no tests

Addresses every blocker, both majors, and all but one minor from the
maintainer's review of the draft PR.

Blockers:

1. Every generated inline onclick was unparseable. JSON.stringify's own
   double quotes terminated the double-quoted HTML attribute at the first
   one, leaving btn.onclick null on every picker entry and every Discover/
   Edit/Delete button. Fixed with escapeHtml(JSON.stringify(...)) per
   argument, the same idiom deleteCase's onclick already uses four lines
   away in session-ui.js. This also closes the live-HTML-injection route
   through modelId (server-controlled, from the endpoint's own /v1/models
   reply): with quoting intact, a `>` inside it can no longer terminate the
   <button> tag early.
2. GET /api/model-endpoints wraps its body in the {success,data} envelope
   like every other /api route (server.ts's preSerialization hook applies
   to arrays too), so Array.isArray(hosts) was always false in production
   and the picker/settings panel silently saw nothing. Both call sites now
   go through _apiJson(), which already exists for exactly this.
3. A failed or declined run*() (missing CLI, isBusy, a caught exception)
   returns normally without ever changing activeSessionId, so the apply
   step used to silently re-point and restart whatever session the user was
   already looking at. runCustomModelEntry() now snapshots activeSessionId
   before the launch and requires it to have actually changed.

Majors:

4. Routes the launch through run() itself via a temporary _runMode swap
   (never persisted — setRunMode() would sync it to the server) instead of
   a parallel hardcoded dispatch table, so a custom-model launch now holds
   the same _runInFlight lock every other Run click gets. This also
   resolves the "hardcoded runners map contradicts the PR's own design"
   minor: dispatch is run()'s own, so a CLI whose customModelInjection
   recipe lands later needs no update here.
5. New test/custom-model-run-menu-ui.test.ts drives the real session-ui.js
   against a JSDOM window (runScripts:"dangerously" — this JSDOM only ever
   parses markup this module generated itself) for exactly the DOM-level
   facts the review said needed no Playwright and no tmux: a generated
   button's onclick genuinely compiles and fires, a dangerous modelId never
   produces a live element, the envelope unwrap works, the session-changed
   guard holds, run() actually gets called (proving the in-flight lock
   engages), and _runMode is restored afterward. Confirmed against the
   pre-fix code first (reproduces btn.onclick === null exactly) so this
   isn't a vacuous pass. Plus new tests in custom-model-routes.test.ts and
   render-index-html.test.ts for the other fixes below.

Minors:

- Generated entries now filter through isCliAvailable(), matching
  _refreshRunModeAvailability's own gating of the stock entries.
- The CRUD panel is now gated on customModelEndpointsEnabled
  (applyCustomModelEndpointsVisibility(), wired to the toggle's onchange
  and to settings-modal open) instead of always rendering; the endpoint GET
  no longer fires unconditionally either.
- API keys are never handed back to the browser on GET, POST or PUT —
  redactApiKey() replaces the field with a computed apiKeySet: boolean, and
  a PUT with no apiKey now keeps the stored one server-side
  (applyStoredApiKey()) instead of the client resending a value it was
  never given. New tests cover both directions (kept vs. replaced) by
  observing the actual auth header a subsequent discovery request sends.
- "+ Add endpoint" hides for a non-admin in multi-user mode
  (_applyCustomModelAdminGate(), also wired to admin-ui.js's codeman:me
  event, since the real role can resolve after settings were first opened)
  — endpoint writes were already admin-only server-side, but the button
  used to render for everyone and eat a 403.
- design doc (custom-model-endpoints-plan.md §4) now says up front that its
  toolbar-button design was superseded by the Run-menu picker.
- docs/api-reference.md gained a Custom Model Endpoints section (every
  route, the apiKeySet/defaultModelId contract, the restart mechanics).
- Wiki page now covers un-pointing a session (curl/delete, no UI yet) and
  that the picker is desktop-only for now.
- .set-inline-form uses --control-bg instead of a hardcoded black alpha
  (CLAUDE.md already records that exact literal turning the settings
  preview into a grey slab on light skins), .run-mode-custom-models gets
  the same gap: 2px .run-mode-menu's own flex gap only applies one level
  up, and the index.html comment naming the wrong function is fixed.
- __codemanCustomModelClis's JSON is now escaped against a literal
  </script> (CliEntry.label is user-clis.json-settable, unlike
  __codemanCliAvailable's booleans-only payload) via a new exported
  escapeScriptJson(), pure and unit-tested without needing a WebServer.
- Added defaultModelId + the new /v1/model-endpoints routes to
  docs/api-reference.md; left the "no zh-CN for the new Models-section
  group" minor unaddressed only insofar as the wider Models section (task
  routing, thinking effort, etc.) has never had zh-CN coverage either —
  everything this PR itself introduces (labels, hints, button text, the
  Run-menu's "Custom Endpoints" header) IS translated in i18n.js.

Regression caught while fixing #4: the admin-gate's codeman:me listener is
a module-level document.addEventListener() call, which threw in
run-mode-ui.test.ts's minimal vm-context fake document and failed all 10
of that file's tests. Fixed with optional chaining before it ever reached
the branch this commit lands on; full targeted suite (route tests,
structural guards, every settings-ui.js-loading frontend test) reverified
green afterward.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RqZeHrRS6DYcGcGX2p9EwG
This commit is contained in:
Devvyn
2026-09-16 07:01:23 +08:00
co-authored by Claude Sonnet 5
parent fed6582d3e
commit 60e1bd52f7
13 changed files with 687 additions and 121 deletions
+243
View File
@@ -0,0 +1,243 @@
/**
* @fileoverview Frontend tests for the Custom Model Endpoint Profiles Run-menu
* picker (docs/custom-model-endpoints-plan.md): the generated entries in
* session-ui.js's `_refreshCustomModelRunOptions()` / `runCustomModelEntry()`.
*
* These are DOM-level facts that need no Playwright and no tmux — `runScripts:
* "dangerously"` is used deliberately (this JSDOM only ever parses markup this
* module itself generated, never live user input) so that a broken inline
* `onclick` attribute shows up as a genuinely uncallable handler, the same way
* it would in a real browser, rather than merely as a string this test parses
* by eye. `test/admin-ui.test.ts` and `test/home-sessions.test.ts` are the
* precedent for driving a real frontend module against a JSDOM window rather
* than a live server.
*
* Port: none.
*/
import { readFileSync } from 'node:fs';
import { JSDOM } from 'jsdom';
import { describe, expect, it } from 'vitest';
const CONSTANTS_JS = readFileSync(new URL('../src/web/public/constants.js', import.meta.url), 'utf-8');
const SESSION_UI_JS = readFileSync(new URL('../src/web/public/session-ui.js', import.meta.url), 'utf-8');
function resp(body: unknown, ok = true) {
return { ok, json: async () => body };
}
/**
* Boots a minimal CodemanApp instance with constants.js + session-ui.js
* evaluated against a real JSDOM window, so escapeHtml and the picker's own
* innerHTML-building code run exactly as they do in the browser.
*/
function bootApp(
options: {
customModelClis?: Array<{ id: string; label: string }>;
hosts?: unknown;
cliAvailable?: (id: string) => boolean;
activeCase?: { location?: string } | null;
settingsEnabled?: boolean;
} = {}
) {
const dom = new JSDOM(
`<!doctype html><body>
<select id="quickStartCase"><option value="testcase" selected>testcase</option></select>
<input id="tabCount" value="1">
<button id="runBtn"></button>
<div id="runModeMenu">
<div id="runModeCustomModelSep" style="display:none"></div>
<div id="runModeCustomModelHeader" style="display:none"></div>
<div id="runModeCustomModels"></div>
</div>
</body>`,
{ url: 'http://localhost/', runScripts: 'dangerously' }
);
const win = dom.window as unknown as Window &
typeof globalThis & {
CodemanApp: new () => any;
__codemanCustomModelClis?: Array<{ id: string; label: string }>;
};
(win as unknown as { eval: (s: string) => void }).eval('window.CodemanApp = function CodemanApp() {};');
(win as unknown as { eval: (s: string) => void }).eval(CONSTANTS_JS);
(win as unknown as { eval: (s: string) => void }).eval(SESSION_UI_JS);
win.__codemanCustomModelClis = options.customModelClis ?? [{ id: 'claude', label: 'Claude Code' }];
const app = new win.CodemanApp();
app.cases = options.activeCase ? [{ name: 'testcase', ...options.activeCase }] : [{ name: 'testcase' }];
app.loadAppSettingsFromStorage = () => ({ customModelEndpointsEnabled: options.settingsEnabled ?? true });
app.isCliAvailable = options.cliAvailable ?? (() => true);
app.showToast = () => {};
// _apiJson unwraps the {success,data} envelope for real against a live
// server; here it stands in for that, driven from a fixed `hosts` fixture
// so these tests exercise the picker's OWN code, not the envelope helper.
app._apiJson = async (path: string) => {
if (path === '/api/model-endpoints') return options.hosts ?? [];
return null;
};
return { dom, win, app };
}
describe('Custom Model Endpoint Profiles: Run-menu picker generation', () => {
it('generates a real, clickable button per (capable CLI, endpoint) pair', async () => {
const { win, app } = bootApp({
hosts: [{ id: 'llama-box', label: 'llama.cpp', baseUrl: 'http://localhost:8080', models: ['qwen3'] }],
});
const menu = win.document.getElementById('runModeMenu')!;
await app._refreshCustomModelRunOptions(menu);
const container = win.document.getElementById('runModeCustomModels')!;
const buttons = container.querySelectorAll('button');
expect(buttons.length).toBe(1);
const btn = buttons[0] as unknown as HTMLButtonElement & { onclick: unknown };
// The real bug: JSON.stringify's own double quotes terminate the
// double-quoted onclick attribute at the first one, so btn.onclick comes
// back null and the parsed attribute is garbage. With escapeHtml wrapping
// each stringified argument, jsdom (which compiles inline handlers under
// runScripts:"dangerously" exactly like a real browser) parses it as a
// real, callable function.
expect(typeof btn.onclick).toBe('function');
win.app = app;
expect(() => btn.onclick!(new (win as any).Event('click'))).not.toThrow();
});
it('escapes a model id containing HTML-significant characters instead of letting it break out of the tag', async () => {
// modelId comes from the endpoint's OWN /v1/models reply, which this box
// does not control — a live-HTML-injection vector if it ever reaches the
// markup unescaped, distinct from (and on top of) the quoting bug above.
const dangerousModel = '"><img src=x onerror=alert(1)>';
const { win, app } = bootApp({
hosts: [{ id: 'llama-box', label: 'llama.cpp', baseUrl: 'http://localhost:8080', models: [dangerousModel] }],
});
const menu = win.document.getElementById('runModeMenu')!;
await app._refreshCustomModelRunOptions(menu);
const container = win.document.getElementById('runModeCustomModels')!;
// The injected markup must never have produced a live <img> element: if it
// did, the attacker-controlled tag closed the button early and escaped
// into sibling markup instead of staying inert string data.
expect(container.querySelector('img')).toBeNull();
expect(container.querySelectorAll('button').length).toBe(1);
});
it('is hidden when the feature setting is off, even with capable CLIs and endpoints present', async () => {
const { win, app } = bootApp({
settingsEnabled: false,
hosts: [{ id: 'llama-box', label: 'llama.cpp', baseUrl: 'http://localhost:8080', models: ['qwen3'] }],
});
const menu = win.document.getElementById('runModeMenu')!;
await app._refreshCustomModelRunOptions(menu);
expect(win.document.getElementById('runModeCustomModels')!.innerHTML).toBe('');
expect((win.document.getElementById('runModeCustomModelSep') as HTMLElement).style.display).toBe('none');
});
it('is hidden for a remote or Docker active case, since the apply route refuses both', async () => {
for (const location of ['remote', 'docker']) {
const { win, app } = bootApp({
activeCase: { location },
hosts: [{ id: 'llama-box', label: 'llama.cpp', baseUrl: 'http://localhost:8080', models: ['qwen3'] }],
});
const menu = win.document.getElementById('runModeMenu')!;
await app._refreshCustomModelRunOptions(menu);
expect(win.document.getElementById('runModeCustomModels')!.innerHTML, location).toBe('');
}
});
it('skips an endpoint with no discovered model and no default, rather than generating a dead entry', async () => {
const { win, app } = bootApp({
hosts: [{ id: 'undiscovered', label: 'Not discovered yet', baseUrl: 'http://localhost:8080', models: [] }],
});
const menu = win.document.getElementById('runModeMenu')!;
await app._refreshCustomModelRunOptions(menu);
expect(win.document.getElementById('runModeCustomModels')!.innerHTML).toBe('');
});
it('omits a CLI the host does not have installed, matching the stock entries’ own gating', async () => {
const { win, app } = bootApp({
customModelClis: [
{ id: 'claude', label: 'Claude Code' },
{ id: 'codex', label: 'Codex' },
],
hosts: [{ id: 'llama-box', label: 'llama.cpp', baseUrl: 'http://localhost:8080', models: ['qwen3'] }],
cliAvailable: (id: string) => id === 'claude',
});
const menu = win.document.getElementById('runModeMenu')!;
await app._refreshCustomModelRunOptions(menu);
const container = win.document.getElementById('runModeCustomModels')!;
expect(container.querySelectorAll('button').length).toBe(1);
expect(container.textContent).toContain('Claude Code');
expect(container.textContent).not.toContain('Codex');
});
});
describe('Custom Model Endpoint Profiles: applying a picked entry', () => {
it('does not apply the endpoint to a session that was already open when the launch fails', async () => {
const { app } = bootApp({});
app.activeSessionId = 'already-open-session';
// Simulate every run*() function's own documented behaviour: a declined or
// failed launch handles its own error and returns normally without ever
// changing activeSessionId — it does NOT throw and does NOT leave it null.
app.run = async () => {};
app._runInFlight = false;
let applyCalled = false;
const realApiJson = app._apiJson.bind(app);
app._apiJson = async (path: string, opts?: unknown) => {
if (path.includes('/custom-model')) applyCalled = true;
return realApiJson(path, opts as never);
};
await app.runCustomModelEntry('claude', 'llama-box', 'qwen3');
expect(applyCalled).toBe(false);
expect(app.activeSessionId).toBe('already-open-session');
});
it('applies the endpoint once run() actually produces a NEW active session', async () => {
const { app } = bootApp({});
app.activeSessionId = 'old-session';
app.run = async () => {
app.activeSessionId = 'new-session';
};
const calls: Array<{ path: string; body: unknown }> = [];
app._apiJson = async (path: string, opts?: { body?: unknown }) => {
calls.push({ path, body: opts?.body });
return { customModel: { endpointId: 'llama-box' }, restarted: true };
};
await app.runCustomModelEntry('claude', 'llama-box', 'qwen3');
expect(calls).toHaveLength(1);
expect(calls[0].path).toBe('/api/sessions/new-session/custom-model');
expect(calls[0].body).toEqual({ endpointId: 'llama-box', modelId: 'qwen3' });
});
it('routes through run() itself, so the Run in-flight lock actually engages', async () => {
// CLAUDE.md, Run launch synchronization: the lock exists so a double click
// cannot create duplicate sessions. A hardcoded dispatch table bypassing
// run() would never set _runInFlight, which is what this pins.
const { app } = bootApp({});
let sawInFlight = false;
app.run = async function (this: typeof app) {
if (this._runInFlight) return;
this._runInFlight = true;
sawInFlight = true;
this._runInFlight = false;
};
await app.runCustomModelEntry('claude', 'llama-box', 'qwen3');
expect(sawInFlight).toBe(true);
});
it('restores the previous _runMode after a one-off custom-model launch, never persisting it', async () => {
const { app } = bootApp({});
app._runMode = 'opencode';
let modeDuringRun: string | undefined;
app.run = async function (this: typeof app) {
modeDuringRun = this._runMode;
};
await app.runCustomModelEntry('claude', 'llama-box', 'qwen3');
expect(modeDuringRun).toBe('claude');
expect(app._runMode).toBe('opencode');
});
});
+14 -1
View File
@@ -11,7 +11,7 @@
* Port: N/A (no server start).
*/
import { describe, it, expect, afterEach, vi } from 'vitest';
import { WebServer } from '../src/web/server.js';
import { WebServer, escapeScriptJson } from '../src/web/server.js';
import { isClaudeAvailable } from '../src/utils/claude-cli-resolver.js';
import { isOpenCodeAvailable } from '../src/utils/opencode-cli-resolver.js';
import { isCodexAvailable } from '../src/utils/codex-cli-resolver.js';
@@ -209,6 +209,19 @@ describe('WebServer.renderIndexHtml', () => {
}
});
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
// the one injection that needs it. Exported so this tests the pure
// function directly rather than needing a real WebServer (which needs tmux).
const dangerous = JSON.stringify([{ id: 'x', label: '</script><script>alert(1)</script>' }]);
const escaped = escapeScriptJson(dangerous);
expect(escaped).not.toContain('</script');
// Proves it decodes back to the real value the way a browser's own JS
// parser would, not just "the output contains no </script>".
expect(eval(escaped)[0].label).toBe('</script><script>alert(1)</script>');
});
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
// wrong by only injecting when something resolves.
+94
View File
@@ -295,3 +295,97 @@ describe('defaultModelId — the Run-menu picker’s per-endpoint default', () =
expect(stored?.defaultModelId).toBe('qwen3');
});
});
describe('apiKey is never handed back to the browser', () => {
afterEach(() => {
fetchMock.mockReset();
});
it('POST, GET and PUT responses all carry apiKeySet instead of the real key', async () => {
const { app } = await setup();
const create = await app.inject({
method: 'POST',
url: '/api/model-endpoints',
payload: { id: 'ep-secret', label: 'A', baseUrl: 'http://localhost:8080', apiKey: 'super-secret' },
});
expect(create.json().data.host.apiKey).toBeUndefined();
expect(create.json().data.host.apiKeySet).toBe(true);
const list = await app.inject({ method: 'GET', url: '/api/model-endpoints' });
const listed = (list.json() as Array<{ id: string; apiKey?: string; apiKeySet?: boolean }>).find(
(h) => h.id === 'ep-secret'
);
expect(listed?.apiKey).toBeUndefined();
expect(listed?.apiKeySet).toBe(true);
expect(JSON.stringify(list.json())).not.toContain('super-secret');
const update = await app.inject({
method: 'PUT',
url: '/api/model-endpoints/ep-secret',
payload: { label: 'Renamed', baseUrl: 'http://localhost:8080' },
});
expect(update.json().data.host.apiKey).toBeUndefined();
expect(update.json().data.host.apiKeySet).toBe(true);
expect(JSON.stringify(update.json())).not.toContain('super-secret');
});
it('a host with no key set at all reports apiKeySet: false', async () => {
const { app } = await setup();
const create = await app.inject({
method: 'POST',
url: '/api/model-endpoints',
payload: { id: 'ep-nokey', label: 'A', baseUrl: 'http://localhost:8080' },
});
expect(create.json().data.host.apiKeySet).toBe(false);
});
it('PUT with no apiKey keeps the stored one, rather than clearing it', async () => {
const { app } = await setup();
await app.inject({
method: 'POST',
url: '/api/model-endpoints',
payload: { id: 'ep-keep-key', label: 'A', baseUrl: 'http://localhost:8080', apiKey: 'original-key' },
});
// Edit without touching the API key field — the real bug this guards: a
// browser round-trip that only ever sees apiKeySet, never the real value,
// must not accidentally send an empty string and wipe a working credential.
const update = await app.inject({
method: 'PUT',
url: '/api/model-endpoints/ep-keep-key',
payload: { label: 'Renamed', baseUrl: 'http://localhost:8080' },
});
expect(update.json().data.host.apiKeySet).toBe(true);
// Prove it by observing the auth header discovery actually sends.
fetchMock.mockImplementation(async (_url: URL, init?: RequestInit) => {
const headers = init?.headers as Record<string, string>;
expect(headers.Authorization).toBe('Bearer original-key');
return new Response(JSON.stringify({ data: [] }), { status: 200 });
});
const discover = await app.inject({ method: 'POST', url: '/api/model-endpoints/ep-keep-key/discover-models' });
expect(discover.json().success).toBe(true);
expect(fetchMock).toHaveBeenCalledTimes(1);
});
it('PUT with a new apiKey replaces the stored one', async () => {
const { app } = await setup();
await app.inject({
method: 'POST',
url: '/api/model-endpoints',
payload: { id: 'ep-replace-key', label: 'A', baseUrl: 'http://localhost:8080', apiKey: 'old-key' },
});
await app.inject({
method: 'PUT',
url: '/api/model-endpoints/ep-replace-key',
payload: { label: 'A', baseUrl: 'http://localhost:8080', apiKey: 'new-key' },
});
fetchMock.mockImplementation(async (_url: URL, init?: RequestInit) => {
const headers = init?.headers as Record<string, string>;
expect(headers.Authorization).toBe('Bearer new-key');
return new Response(JSON.stringify({ data: [] }), { status: 200 });
});
await app.inject({ method: 'POST', url: '/api/model-endpoints/ep-replace-key/discover-models' });
expect(fetchMock).toHaveBeenCalledTimes(1);
});
});