diff --git a/docs/custom-model-endpoints.md b/docs/custom-model-endpoints.md index 64d24b41..b4aa4d52 100644 --- a/docs/custom-model-endpoints.md +++ b/docs/custom-model-endpoints.md @@ -444,6 +444,14 @@ id, model and injected key NAMES are persisted, the values are re-derived from the endpoint store on recovery, and the pane keeps running against the endpoint in between because tmux retains its environment. +⚠️ Clearing removes injected keys **by name**, and `CLAUDE_CONFIG_DIR` is one +of the names claude's selection injects — so a session that ALSO had +`CLAUDE_CONFIG_DIR` set through the generic `envOverrides` field (the +per-client-account case) loses that override on clear too, and silently +falls back to the server's default Claude account. If you route a session +to a specific account this way, re-apply the override after clearing a +custom-model selection from it. + **New sessions always default back to the harness's native backend.** A custom-endpoint selection is a per-session choice, never a sticky global default — starting a fresh session doesn't inherit whatever the last one was diff --git a/src/web/public/panels-ui.js b/src/web/public/panels-ui.js index 7a0ccca9..cd4711c6 100644 --- a/src/web/public/panels-ui.js +++ b/src/web/public/panels-ui.js @@ -5583,12 +5583,21 @@ Object.assign(CodemanApp.prototype, { el.id = 'customModelCenterStatus'; document.body.appendChild(el); } + // A pending hide from a PREVIOUS dismiss() (e.g. switchingToast.dismiss() right + // before this same-origin call reopens the banner within its 200ms fade) must + // never fire against the node this call is about to show — clear it before + // reusing the shared DOM node, or the old timer hides the fresh banner ~200ms in. + if (el._hideTimer) { + clearTimeout(el._hideTimer); + el._hideTimer = null; + } el.className = `center-status-banner center-status-${type}`; el.innerHTML = ''; const dismiss = () => { el.classList.remove('show'); - setTimeout(() => { + el._hideTimer = setTimeout(() => { el.hidden = true; + el._hideTimer = null; }, 200); }; if (type !== 'error') { diff --git a/src/web/routes/custom-model-routes.ts b/src/web/routes/custom-model-routes.ts index b12cee2b..2e9ee1cb 100644 --- a/src/web/routes/custom-model-routes.ts +++ b/src/web/routes/custom-model-routes.ts @@ -447,7 +447,14 @@ async function pumpLlamaSwapLogTail( } catch { // connection dropped / aborted / endpoint unreachable — a future access starts fresh } finally { - llamaSwapLogTails.delete(host.id); + // Delete by IDENTITY, not just by key: an aborted pump can finish after a NEWER + // entry was already created for the same endpoint id (e.g. abort-then-immediately- + // re-request), and deleting unconditionally would remove that newer entry and orphan + // its connection — nothing would ever prune it, since pruneIdleLlamaSwapLogTails only + // walks entries still present in the map. + if (llamaSwapLogTails.get(host.id) === entry) { + llamaSwapLogTails.delete(host.id); + } } } diff --git a/src/web/routes/session-routes.ts b/src/web/routes/session-routes.ts index 62b07ff7..b1d2648f 100644 --- a/src/web/routes/session-routes.ts +++ b/src/web/routes/session-routes.ts @@ -1254,15 +1254,21 @@ export function registerSessionRoutes( // that is currently using it — never just because a swap is needed at all. `confirmed` // (set by the caller after showing that warning once) skips asking again. if (swapNeeded && !body.confirmed) { - const affectedSessions = [...ctx.sessions.values()] - .filter( - (s) => - s.id !== session.id && - s.customModel?.endpointId === endpoint.id && - s.customModel?.modelId === currentlyLoaded - ) - .map((s) => ({ id: s.id, name: s.name })); - if (affectedSessions.length > 0) { + const conflicting = [...ctx.sessions.values()].filter( + (s) => + s.id !== session.id && s.customModel?.endpointId === endpoint.id && s.customModel?.modelId === currentlyLoaded + ); + if (conflicting.length > 0) { + // Applying a custom model is ungated for any session owner, so in multi-user + // mode a non-admin pointing their own session at a shared endpoint must not + // learn another user's session names in the confirm dialog — with + // autoNameSessions on, those names are that user's own prompts. The swap is + // still blocked pending confirmation regardless of ownership (a foreign + // session is just as real a disruption); only which ones get NAMED is scoped. + const requestUser = getAuthUser(req); + const affectedSessions = conflicting + .filter((s) => canAccessOwned(requestUser, s.owner)) + .map((s) => ({ id: s.id, name: s.name })); return { requiresConfirmation: true, currentlyLoadedModel: currentlyLoaded, affectedSessions }; } } @@ -3634,10 +3640,17 @@ export function registerSessionRoutes( const cmTargetReady = cmSwapStatus.running.some((r) => r.model === customModel.modelId && r.state === 'ready'); qsCustomModelSwapInProgress = cmSwapStatus.isLlamaSwap && !cmTargetReady; if (cmSwapNeeded && !customModel.confirmed) { - const cmAffectedSessions = [...ctx.sessions.values()] - .filter((s) => s.customModel?.endpointId === cmEndpoint.id && s.customModel?.modelId === cmCurrentlyLoaded) - .map((s) => ({ id: s.id, name: s.name })); - if (cmAffectedSessions.length > 0) { + const cmConflicting = [...ctx.sessions.values()].filter( + (s) => s.customModel?.endpointId === cmEndpoint.id && s.customModel?.modelId === cmCurrentlyLoaded + ); + if (cmConflicting.length > 0) { + // Same reasoning as the dedicated /custom-model route above: the swap is + // still blocked pending confirmation regardless of ownership, but a + // non-admin caller only learns the names of sessions they can access. + const cmRequestUser = getAuthUser(req); + const cmAffectedSessions = cmConflicting + .filter((s) => canAccessOwned(cmRequestUser, s.owner)) + .map((s) => ({ id: s.id, name: s.name })); return { requiresConfirmation: true, currentlyLoadedModel: cmCurrentlyLoaded, diff --git a/src/web/server.ts b/src/web/server.ts index 54de4f59..59577ef0 100644 --- a/src/web/server.ts +++ b/src/web/server.ts @@ -1233,7 +1233,13 @@ export class WebServer extends EventEmitter { return undefined; } try { - return applyCustomModelInjection(entry, endpoint, saved.modelId, session.id)?.envOverrides; + return applyCustomModelInjection( + entry, + endpoint, + saved.modelId, + session.id, + endpoint.modelContextLengths?.[saved.modelId] + )?.envOverrides; } catch (err) { console.warn('[WebServer] Failed to rebuild custom-model env on recovery:', err); return undefined; diff --git a/test/custom-model-run-menu-ui.test.ts b/test/custom-model-run-menu-ui.test.ts index 66842ae9..80d0f067 100644 --- a/test/custom-model-run-menu-ui.test.ts +++ b/test/custom-model-run-menu-ui.test.ts @@ -16,7 +16,7 @@ */ import { readFileSync } from 'node:fs'; import { JSDOM } from 'jsdom'; -import { describe, expect, it } from 'vitest'; +import { describe, expect, it, vi } 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'); @@ -970,6 +970,29 @@ describe('Custom Model Endpoint Profiles: _showCenterStatus Cancel button (real expect(win.document.querySelector('.center-status-cancel')).toBeNull(); }); + it('reopening within the 200ms fade cancels the previous dismiss(), so the fresh banner is not hidden out from under it', () => { + // The real bug: dismiss() schedules el.hidden = true 200ms later with nothing + // to cancel it. _runCustomModelEntryViaRestart calls switchingToast.dismiss() + // then awaits one same-origin request (5-30ms locally) before reopening the + // banner for the model-load wait — well inside that 200ms window — so the + // stale timer fired against the shared DOM node and hid the fresh banner. + vi.useFakeTimers(); + try { + const { win, app } = bootAppWithRealCenterStatus(); + const first = app._showCenterStatus('Claude started — switching to llama-swap…'); + first.dismiss(); + vi.advanceTimersByTime(20); + app._showCenterStatus('Loading qwen3 on llama-box…'); + vi.advanceTimersByTime(280); + + const el = win.document.getElementById('customModelCenterStatus') as HTMLElement; + expect(el.hidden).toBe(false); + expect(el.textContent).toContain('Loading qwen3 on llama-box…'); + } finally { + vi.useRealTimers(); + } + }); + it('re-asserts [hidden] over the flex display, so dismiss() actually hides it', () => { // .center-status-banner is display:flex, which defeats the `hidden` attribute — // dismiss()'s only visibility lever — unless this rule exists: without it the card diff --git a/test/routes/session-custom-model.test.ts b/test/routes/session-custom-model.test.ts index 95023b7f..1a918e78 100644 --- a/test/routes/session-custom-model.test.ts +++ b/test/routes/session-custom-model.test.ts @@ -33,9 +33,9 @@ const CLAUDE_ENDPOINT: CustomModelHost = { apiKey: 'k', }; -async function setup() { +async function setup(ctxOptions?: Parameters[1]) { await writeCustomModelHosts(getDataDir(), [CLAUDE_ENDPOINT]); - return createRouteTestHarness(registerSessionRoutes); + return createRouteTestHarness(registerSessionRoutes, ctxOptions); } describe('POST /api/sessions/:id/custom-model', () => { @@ -294,6 +294,68 @@ describe('POST /api/sessions/:id/custom-model', () => { expect(session.restartCli).not.toHaveBeenCalled(); }); + describe('multi-user: the confirm dialog must not name a session the caller cannot access', () => { + const saved: Record = {}; + + beforeEach(() => { + saved.CODEMAN_MULTIUSER = process.env.CODEMAN_MULTIUSER; + process.env.CODEMAN_MULTIUSER = '1'; + }); + + afterEach(() => { + if (saved.CODEMAN_MULTIUSER === undefined) delete process.env.CODEMAN_MULTIUSER; + else process.env.CODEMAN_MULTIUSER = saved.CODEMAN_MULTIUSER; + }); + + it("still blocks the swap pending confirmation, but omits a foreign owner's session from affectedSessions", async () => { + const { app, ctx } = await setup({ authUser: { username: 'bob', role: 'user' } }); + const session = ctx.sessions.get('test-session-1')!; + session.mode = 'claude'; + (session as unknown as { owner?: string }).owner = 'bob'; + const other = createMockSession('other-session'); + other.name = 'w2-otherbox'; + other.customModel = { endpointId: 'ep1', modelId: 'llama3' }; + (other as unknown as { owner?: string }).owner = 'alice'; + ctx.sessions.set('other-session', other); + mockRunning([{ model: 'llama3', state: 'ready' }]); + + const res = await app.inject({ + method: 'POST', + url: '/api/sessions/test-session-1/custom-model', + payload: { endpointId: 'ep1', modelId: 'qwen3' }, + }); + + const body = res.json(); + // Still asks — a foreign session is just as real a disruption as an owned one. + expect(body.requiresConfirmation).toBe(true); + expect(body.currentlyLoadedModel).toBe('llama3'); + // But bob never learns alice's session id or name. + expect(body.affectedSessions).toEqual([]); + expect(session.setCustomModel).not.toHaveBeenCalled(); + }); + + it('names the affected session when the caller DOES own it', async () => { + const { app, ctx } = await setup({ authUser: { username: 'bob', role: 'user' } }); + const session = ctx.sessions.get('test-session-1')!; + session.mode = 'claude'; + (session as unknown as { owner?: string }).owner = 'bob'; + const other = createMockSession('other-session'); + other.name = 'w2-otherbox'; + other.customModel = { endpointId: 'ep1', modelId: 'llama3' }; + (other as unknown as { owner?: string }).owner = 'bob'; + ctx.sessions.set('other-session', other); + mockRunning([{ model: 'llama3', state: 'ready' }]); + + const res = await app.inject({ + method: 'POST', + url: '/api/sessions/test-session-1/custom-model', + payload: { endpointId: 'ep1', modelId: 'qwen3' }, + }); + + expect(res.json().affectedSessions).toEqual([{ id: 'other-session', name: 'w2-otherbox' }]); + }); + }); + it('applies once confirmed, skipping the conflict check the second time', async () => { const { app, ctx } = await setup(); const session = ctx.sessions.get('test-session-1')!;