mirror of
https://github.com/Ark0N/Codeman.git
synced 2026-10-08 16:39:42 +02:00
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
392 lines
15 KiB
TypeScript
392 lines
15 KiB
TypeScript
/**
|
||
* @fileoverview Route tests for Custom Model Endpoint Profiles CRUD + discovery.
|
||
*
|
||
* Discovery is mocked at `webviewFetch()` (webview-egress.ts), NOT at the global
|
||
* `fetch`: the route deliberately goes through the guarded undici dispatcher whose
|
||
* lookup hook refuses a name that resolves into a link-local / cloud-metadata range,
|
||
* so a global-fetch stub that still satisfied these tests would mean the guard had
|
||
* been bypassed.
|
||
* Port: N/A (app.inject, no real port needed)
|
||
*/
|
||
import { describe, it, expect, vi, afterEach } from 'vitest';
|
||
import { registerCustomModelRoutes } from '../../src/web/routes/custom-model-routes.js';
|
||
import { webviewFetch } from '../../src/web/webview-egress.js';
|
||
import { createRouteTestHarness } from './_route-test-utils.js';
|
||
|
||
vi.mock('../../src/web/webview-egress.js', async () => {
|
||
const actual = await vi.importActual<typeof import('../../src/web/webview-egress.js')>(
|
||
'../../src/web/webview-egress.js'
|
||
);
|
||
return { ...actual, webviewFetch: vi.fn() };
|
||
});
|
||
|
||
const fetchMock = vi.mocked(webviewFetch);
|
||
|
||
async function setup() {
|
||
return createRouteTestHarness(registerCustomModelRoutes);
|
||
}
|
||
|
||
describe('custom model endpoint CRUD', () => {
|
||
afterEach(() => {
|
||
fetchMock.mockReset();
|
||
});
|
||
|
||
it('starts empty', async () => {
|
||
const { app } = await setup();
|
||
const res = await app.inject({ method: 'GET', url: '/api/model-endpoints' });
|
||
expect(res.json()).toEqual([]);
|
||
});
|
||
|
||
it('creates, lists, updates, and deletes an endpoint', async () => {
|
||
const { app } = await setup();
|
||
|
||
const create = await app.inject({
|
||
method: 'POST',
|
||
url: '/api/model-endpoints',
|
||
payload: { id: 'ep1', label: 'llama.cpp box', baseUrl: 'http://192.168.1.50:8080' },
|
||
});
|
||
expect(create.statusCode).toBe(200);
|
||
expect(create.json().data.host.id).toBe('ep1');
|
||
|
||
const list = await app.inject({ method: 'GET', url: '/api/model-endpoints' });
|
||
expect(list.json()).toHaveLength(1);
|
||
|
||
const update = await app.inject({
|
||
method: 'PUT',
|
||
url: '/api/model-endpoints/ep1',
|
||
payload: { label: 'Renamed', baseUrl: 'http://192.168.1.50:8080' },
|
||
});
|
||
expect(update.statusCode).toBe(200);
|
||
expect(update.json().data.host.label).toBe('Renamed');
|
||
|
||
const del = await app.inject({ method: 'DELETE', url: '/api/model-endpoints/ep1' });
|
||
expect(del.statusCode).toBe(200);
|
||
|
||
const listAfter = await app.inject({ method: 'GET', url: '/api/model-endpoints' });
|
||
expect(listAfter.json()).toEqual([]);
|
||
});
|
||
|
||
it('rejects a duplicate id on create', async () => {
|
||
const { app } = await setup();
|
||
const payload = { id: 'dup', label: 'A', baseUrl: 'http://localhost:8080' };
|
||
await app.inject({ method: 'POST', url: '/api/model-endpoints', payload });
|
||
const second = await app.inject({ method: 'POST', url: '/api/model-endpoints', payload });
|
||
expect(second.json().success).toBe(false);
|
||
expect(second.json().errorCode).toBe('ALREADY_EXISTS');
|
||
});
|
||
|
||
it('404s updating/deleting an id that does not exist', async () => {
|
||
const { app } = await setup();
|
||
const update = await app.inject({
|
||
method: 'PUT',
|
||
url: '/api/model-endpoints/ghost',
|
||
payload: { label: 'A', baseUrl: 'http://localhost:8080' },
|
||
});
|
||
expect(update.json().errorCode).toBe('NOT_FOUND');
|
||
});
|
||
|
||
it('rejects a link-local/cloud-metadata base URL', async () => {
|
||
const { app } = await setup();
|
||
const res = await app.inject({
|
||
method: 'POST',
|
||
url: '/api/model-endpoints',
|
||
payload: { id: 'meta', label: 'A', baseUrl: 'http://169.254.169.254/' },
|
||
});
|
||
expect(res.json().success).toBe(false);
|
||
expect(res.json().errorCode).toBe('INVALID_INPUT');
|
||
});
|
||
|
||
it('discovers models via GET /v1/models and stores the result', async () => {
|
||
const { app } = await setup();
|
||
await app.inject({
|
||
method: 'POST',
|
||
url: '/api/model-endpoints',
|
||
payload: { id: 'ep1', label: 'A', baseUrl: 'http://localhost:8080', apiKey: 'k' },
|
||
});
|
||
|
||
fetchMock.mockImplementation(async (url: URL, init?: RequestInit) => {
|
||
expect(url.href).toBe('http://localhost:8080/v1/models');
|
||
const headers = init?.headers as Record<string, string>;
|
||
// Exactly ONE auth header — never both (a real server hung when sent both).
|
||
expect(headers.Authorization).toBe('Bearer k');
|
||
expect(headers['api-key']).toBeUndefined();
|
||
return new Response(JSON.stringify({ data: [{ id: 'qwen3' }, { id: 'llama3' }] }), { status: 200 });
|
||
});
|
||
|
||
const res = await app.inject({ method: 'POST', url: '/api/model-endpoints/ep1/discover-models' });
|
||
expect(res.json().data.models).toEqual(['qwen3', 'llama3']);
|
||
expect(fetchMock).toHaveBeenCalledTimes(1);
|
||
|
||
// Data dir is shared across this WHOLE test file (one temp HOME per file, not per
|
||
// test — test/setup.ts), so find by id rather than assuming index 0.
|
||
const list = await app.inject({ method: 'GET', url: '/api/model-endpoints' });
|
||
const stored = (list.json() as Array<{ id: string }>).find((h) => h.id === 'ep1');
|
||
expect(stored?.models).toEqual(['qwen3', 'llama3']);
|
||
expect(stored?.lastDiscoveredAt).toBeTruthy();
|
||
});
|
||
|
||
it('discovers models with authStyle "api-key" using only that header, never Authorization', async () => {
|
||
const { app } = await setup();
|
||
await app.inject({
|
||
method: 'POST',
|
||
url: '/api/model-endpoints',
|
||
payload: { id: 'ep-azure', label: 'A', baseUrl: 'http://localhost:8080', apiKey: 'k', authStyle: 'api-key' },
|
||
});
|
||
|
||
fetchMock.mockImplementation(async (_url: URL, init?: RequestInit) => {
|
||
const headers = init?.headers as Record<string, string>;
|
||
expect(headers['api-key']).toBe('k');
|
||
expect(headers.Authorization).toBeUndefined();
|
||
return new Response(JSON.stringify({ data: [] }), { status: 200 });
|
||
});
|
||
|
||
await app.inject({ method: 'POST', url: '/api/model-endpoints/ep-azure/discover-models' });
|
||
expect(fetchMock).toHaveBeenCalledTimes(1);
|
||
});
|
||
|
||
it('reports a clear error when the endpoint is unreachable', async () => {
|
||
const { app } = await setup();
|
||
await app.inject({
|
||
method: 'POST',
|
||
url: '/api/model-endpoints',
|
||
payload: { id: 'ep-err', label: 'A', baseUrl: 'http://localhost:8080' },
|
||
});
|
||
// undici's shape: a bare `fetch failed` with the real reason one level down.
|
||
fetchMock.mockRejectedValue(
|
||
new TypeError('fetch failed', { cause: new Error('connect ECONNREFUSED 127.0.0.1:8080') })
|
||
);
|
||
|
||
const res = await app.inject({ method: 'POST', url: '/api/model-endpoints/ep-err/discover-models' });
|
||
expect(res.json().success).toBe(false);
|
||
expect(res.json().errorCode).toBe('OPERATION_FAILED');
|
||
expect(res.json().error).toContain('ECONNREFUSED');
|
||
});
|
||
|
||
it('names the egress refusal when the endpoint resolves into a blocked range', async () => {
|
||
const { app } = await setup();
|
||
await app.inject({
|
||
method: 'POST',
|
||
url: '/api/model-endpoints',
|
||
payload: { id: 'ep-meta', label: 'A', baseUrl: 'http://models.example:8080' },
|
||
});
|
||
const { WebviewEgressBlockedError } = await vi.importActual<typeof import('../../src/web/webview-egress.js')>(
|
||
'../../src/web/webview-egress.js'
|
||
);
|
||
fetchMock.mockRejectedValue(
|
||
new TypeError('fetch failed', { cause: new WebviewEgressBlockedError('resolves to 169.254.169.254') })
|
||
);
|
||
|
||
const res = await app.inject({ method: 'POST', url: '/api/model-endpoints/ep-meta/discover-models' });
|
||
expect(res.json().success).toBe(false);
|
||
expect(res.json().error).toMatch(/refused.*169\.254\.169\.254/);
|
||
});
|
||
|
||
it('refuses a baseUrl with embedded credentials or a non-http scheme at save time', async () => {
|
||
const { app } = await setup();
|
||
for (const baseUrl of ['http://user:pw@host:8080', 'ftp://host/models', 'http://169.254.169.254']) {
|
||
const res = await app.inject({
|
||
method: 'POST',
|
||
url: '/api/model-endpoints',
|
||
payload: { id: 'bad', label: 'A', baseUrl },
|
||
});
|
||
expect(res.json().success, baseUrl).toBe(false);
|
||
expect(res.json().errorCode, baseUrl).toBe('INVALID_INPUT');
|
||
}
|
||
});
|
||
});
|
||
|
||
describe('defaultModelId — the Run-menu picker’s per-endpoint default', () => {
|
||
afterEach(() => {
|
||
fetchMock.mockReset();
|
||
});
|
||
|
||
it('rejects a defaultModelId that is not one of the endpoint’s discovered models, on both create and update', async () => {
|
||
const { app } = await setup();
|
||
const create = await app.inject({
|
||
method: 'POST',
|
||
url: '/api/model-endpoints',
|
||
payload: {
|
||
id: 'ep-default-reject',
|
||
label: 'A',
|
||
baseUrl: 'http://localhost:8080',
|
||
models: ['qwen3'],
|
||
defaultModelId: 'ghost',
|
||
},
|
||
});
|
||
expect(create.json().success).toBe(false);
|
||
expect(create.json().errorCode).toBe('INVALID_INPUT');
|
||
|
||
await app.inject({
|
||
method: 'POST',
|
||
url: '/api/model-endpoints',
|
||
payload: { id: 'ep-default-reject', label: 'A', baseUrl: 'http://localhost:8080', models: ['qwen3'] },
|
||
});
|
||
const update = await app.inject({
|
||
method: 'PUT',
|
||
url: '/api/model-endpoints/ep-default-reject',
|
||
payload: { label: 'A', baseUrl: 'http://localhost:8080', models: ['qwen3'], defaultModelId: 'ghost' },
|
||
});
|
||
expect(update.json().success).toBe(false);
|
||
expect(update.json().errorCode).toBe('INVALID_INPUT');
|
||
});
|
||
|
||
it('accepts a defaultModelId that IS one of the discovered models', async () => {
|
||
const { app } = await setup();
|
||
const res = await app.inject({
|
||
method: 'POST',
|
||
url: '/api/model-endpoints',
|
||
payload: {
|
||
id: 'ep-default-accept',
|
||
label: 'A',
|
||
baseUrl: 'http://localhost:8080',
|
||
models: ['qwen3', 'llama3'],
|
||
defaultModelId: 'llama3',
|
||
},
|
||
});
|
||
expect(res.json().success).toBe(true);
|
||
expect(res.json().data.host.defaultModelId).toBe('llama3');
|
||
});
|
||
|
||
it('drops a stale default that no longer appears in a fresh discovery, rather than carrying it forward invalid', async () => {
|
||
const { app } = await setup();
|
||
await app.inject({
|
||
method: 'POST',
|
||
url: '/api/model-endpoints',
|
||
payload: {
|
||
id: 'ep-default-drop',
|
||
label: 'A',
|
||
baseUrl: 'http://localhost:8080',
|
||
models: ['qwen3'],
|
||
defaultModelId: 'qwen3',
|
||
},
|
||
});
|
||
fetchMock.mockResolvedValue(new Response(JSON.stringify({ data: [{ id: 'llama3' }] }), { status: 200 }));
|
||
await app.inject({ method: 'POST', url: '/api/model-endpoints/ep-default-drop/discover-models' });
|
||
|
||
const list = await app.inject({ method: 'GET', url: '/api/model-endpoints' });
|
||
const stored = (list.json() as Array<{ id: string; defaultModelId?: string }>).find(
|
||
(h) => h.id === 'ep-default-drop'
|
||
);
|
||
expect(stored?.defaultModelId).toBeUndefined();
|
||
});
|
||
|
||
it('keeps a default that IS still present after a fresh discovery', async () => {
|
||
const { app } = await setup();
|
||
await app.inject({
|
||
method: 'POST',
|
||
url: '/api/model-endpoints',
|
||
payload: {
|
||
id: 'ep-default-keep',
|
||
label: 'A',
|
||
baseUrl: 'http://localhost:8080',
|
||
models: ['qwen3'],
|
||
defaultModelId: 'qwen3',
|
||
},
|
||
});
|
||
fetchMock.mockResolvedValue(
|
||
new Response(JSON.stringify({ data: [{ id: 'qwen3' }, { id: 'llama3' }] }), { status: 200 })
|
||
);
|
||
await app.inject({ method: 'POST', url: '/api/model-endpoints/ep-default-keep/discover-models' });
|
||
|
||
const list = await app.inject({ method: 'GET', url: '/api/model-endpoints' });
|
||
const stored = (list.json() as Array<{ id: string; defaultModelId?: string }>).find(
|
||
(h) => h.id === 'ep-default-keep'
|
||
);
|
||
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);
|
||
});
|
||
});
|