mirror of
https://github.com/Ark0N/Codeman.git
synced 2026-10-07 16:09:43 +02:00
fix(custom-model): address third pre-merge review (Ark0N)
Blocker 1: the loading banner hides itself ~200ms after it reopens. - _showCenterStatus reuses one shared DOM node; dismiss() scheduled el.hidden = true 200ms later with nothing to cancel it. On the Claude path, switchingToast.dismiss() is followed by one same- origin request (5-30ms locally) before _watchLlamaSwapLoading opens the new banner -- well inside that window -- so the stale timer fired against the shared node and hid the fresh banner, leaving the whole model-load wait with no progress text, no log line and no reachable Cancel button. - Fixed by parking the pending timeout on the element and clearing it at the top of _showCenterStatus. Added a regression test that reproduces the exact repro (open, dismiss, reopen 20ms later, advance past 200ms) alongside the existing Cancel-button DOM tests; confirmed it fails without the fix and passes with it. Blocker 2: the swap-conflict warning named other users' sessions. - Both affectedSessions scans (POST .../custom-model and quick-start) walked the whole session map with no ownership filter, so in multi- user mode a non-admin pointing their own session at a shared endpoint learned another user's session name and id -- which with autoNameSessions on is that user's own prompt. - The swap is still blocked pending confirmation regardless of ownership (a foreign session is just as real a disruption); only which ones get NAMED back to the caller is scoped, via the already-imported canAccessOwned. Added a two-owner test to test/routes/session-custom-model.test.ts covering both the foreign-owner (blocked, not named) and same-owner (named) cases. Smaller ride-along fixes: - server.ts boot recovery now passes contextLength into applyCustomModelInjection, so CLAUDE_CODE_MAX_CONTEXT_TOKENS is correctly rebuilt into _envOverrides after a restart instead of surviving only because tmux retains the old setenv. - pumpLlamaSwapLogTail's finally now deletes by IDENTITY, not just by key, so an aborted pump finishing after a newer entry was created for the same endpoint can no longer delete that newer entry and orphan its connection. - docs/custom-model-endpoints.md now notes that clearing a custom model removes injected keys by name, including CLAUDE_CONFIG_DIR -- so a session that also had CLAUDE_CONFIG_DIR set via envOverrides (the per-client-account case) silently falls back to the default account on clear. Left for later, as flagged in the review itself: the quick-start case-scaffolding/cancel ordering (real behavioural reordering across a large handler, too risky to make without a live re-test), and retiring runCustomModelEntry's mode === 'claude' branch behind a launchStrategy registry field (explicitly deferred by the reviewer to "the next one"). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ea59JhUmHBm1gRCsiYF33R
This commit is contained in:
co-authored by
Claude Sonnet 5
parent
9982a1325f
commit
afb6754453
@@ -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
|
from the endpoint store on recovery, and the pane keeps running against the
|
||||||
endpoint in between because tmux retains its environment.
|
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
|
**New sessions always default back to the harness's native backend.** A
|
||||||
custom-endpoint selection is a per-session choice, never a sticky global
|
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
|
default — starting a fresh session doesn't inherit whatever the last one was
|
||||||
|
|||||||
@@ -5583,12 +5583,21 @@ Object.assign(CodemanApp.prototype, {
|
|||||||
el.id = 'customModelCenterStatus';
|
el.id = 'customModelCenterStatus';
|
||||||
document.body.appendChild(el);
|
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.className = `center-status-banner center-status-${type}`;
|
||||||
el.innerHTML = '';
|
el.innerHTML = '';
|
||||||
const dismiss = () => {
|
const dismiss = () => {
|
||||||
el.classList.remove('show');
|
el.classList.remove('show');
|
||||||
setTimeout(() => {
|
el._hideTimer = setTimeout(() => {
|
||||||
el.hidden = true;
|
el.hidden = true;
|
||||||
|
el._hideTimer = null;
|
||||||
}, 200);
|
}, 200);
|
||||||
};
|
};
|
||||||
if (type !== 'error') {
|
if (type !== 'error') {
|
||||||
|
|||||||
@@ -447,7 +447,14 @@ async function pumpLlamaSwapLogTail(
|
|||||||
} catch {
|
} catch {
|
||||||
// connection dropped / aborted / endpoint unreachable — a future access starts fresh
|
// connection dropped / aborted / endpoint unreachable — a future access starts fresh
|
||||||
} finally {
|
} 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);
|
||||||
|
}
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -1254,15 +1254,21 @@ export function registerSessionRoutes(
|
|||||||
// that is currently using it — never just because a swap is needed at all. `confirmed`
|
// 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.
|
// (set by the caller after showing that warning once) skips asking again.
|
||||||
if (swapNeeded && !body.confirmed) {
|
if (swapNeeded && !body.confirmed) {
|
||||||
const affectedSessions = [...ctx.sessions.values()]
|
const conflicting = [...ctx.sessions.values()].filter(
|
||||||
.filter(
|
(s) =>
|
||||||
(s) =>
|
s.id !== session.id && s.customModel?.endpointId === endpoint.id && s.customModel?.modelId === currentlyLoaded
|
||||||
s.id !== session.id &&
|
);
|
||||||
s.customModel?.endpointId === endpoint.id &&
|
if (conflicting.length > 0) {
|
||||||
s.customModel?.modelId === currentlyLoaded
|
// 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
|
||||||
.map((s) => ({ id: s.id, name: s.name }));
|
// learn another user's session names in the confirm dialog — with
|
||||||
if (affectedSessions.length > 0) {
|
// 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 };
|
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');
|
const cmTargetReady = cmSwapStatus.running.some((r) => r.model === customModel.modelId && r.state === 'ready');
|
||||||
qsCustomModelSwapInProgress = cmSwapStatus.isLlamaSwap && !cmTargetReady;
|
qsCustomModelSwapInProgress = cmSwapStatus.isLlamaSwap && !cmTargetReady;
|
||||||
if (cmSwapNeeded && !customModel.confirmed) {
|
if (cmSwapNeeded && !customModel.confirmed) {
|
||||||
const cmAffectedSessions = [...ctx.sessions.values()]
|
const cmConflicting = [...ctx.sessions.values()].filter(
|
||||||
.filter((s) => s.customModel?.endpointId === cmEndpoint.id && s.customModel?.modelId === cmCurrentlyLoaded)
|
(s) => s.customModel?.endpointId === cmEndpoint.id && s.customModel?.modelId === cmCurrentlyLoaded
|
||||||
.map((s) => ({ id: s.id, name: s.name }));
|
);
|
||||||
if (cmAffectedSessions.length > 0) {
|
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 {
|
return {
|
||||||
requiresConfirmation: true,
|
requiresConfirmation: true,
|
||||||
currentlyLoadedModel: cmCurrentlyLoaded,
|
currentlyLoadedModel: cmCurrentlyLoaded,
|
||||||
|
|||||||
+7
-1
@@ -1233,7 +1233,13 @@ export class WebServer extends EventEmitter {
|
|||||||
return undefined;
|
return undefined;
|
||||||
}
|
}
|
||||||
try {
|
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) {
|
} catch (err) {
|
||||||
console.warn('[WebServer] Failed to rebuild custom-model env on recovery:', err);
|
console.warn('[WebServer] Failed to rebuild custom-model env on recovery:', err);
|
||||||
return undefined;
|
return undefined;
|
||||||
|
|||||||
@@ -16,7 +16,7 @@
|
|||||||
*/
|
*/
|
||||||
import { readFileSync } from 'node:fs';
|
import { readFileSync } from 'node:fs';
|
||||||
import { JSDOM } from 'jsdom';
|
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 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');
|
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();
|
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', () => {
|
it('re-asserts [hidden] over the flex display, so dismiss() actually hides it', () => {
|
||||||
// .center-status-banner is display:flex, which defeats the `hidden` attribute —
|
// .center-status-banner is display:flex, which defeats the `hidden` attribute —
|
||||||
// dismiss()'s only visibility lever — unless this rule exists: without it the card
|
// dismiss()'s only visibility lever — unless this rule exists: without it the card
|
||||||
|
|||||||
@@ -33,9 +33,9 @@ const CLAUDE_ENDPOINT: CustomModelHost = {
|
|||||||
apiKey: 'k',
|
apiKey: 'k',
|
||||||
};
|
};
|
||||||
|
|
||||||
async function setup() {
|
async function setup(ctxOptions?: Parameters<typeof createRouteTestHarness>[1]) {
|
||||||
await writeCustomModelHosts(getDataDir(), [CLAUDE_ENDPOINT]);
|
await writeCustomModelHosts(getDataDir(), [CLAUDE_ENDPOINT]);
|
||||||
return createRouteTestHarness(registerSessionRoutes);
|
return createRouteTestHarness(registerSessionRoutes, ctxOptions);
|
||||||
}
|
}
|
||||||
|
|
||||||
describe('POST /api/sessions/:id/custom-model', () => {
|
describe('POST /api/sessions/:id/custom-model', () => {
|
||||||
@@ -294,6 +294,68 @@ describe('POST /api/sessions/:id/custom-model', () => {
|
|||||||
expect(session.restartCli).not.toHaveBeenCalled();
|
expect(session.restartCli).not.toHaveBeenCalled();
|
||||||
});
|
});
|
||||||
|
|
||||||
|
describe('multi-user: the confirm dialog must not name a session the caller cannot access', () => {
|
||||||
|
const saved: Record<string, string | undefined> = {};
|
||||||
|
|
||||||
|
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 () => {
|
it('applies once confirmed, skipping the conflict check the second time', async () => {
|
||||||
const { app, ctx } = await setup();
|
const { app, ctx } = await setup();
|
||||||
const session = ctx.sessions.get('test-session-1')!;
|
const session = ctx.sessions.get('test-session-1')!;
|
||||||
|
|||||||
Reference in New Issue
Block a user