fix(custom-model): split the two confirmation questions, and seed the API key the way claude reads it

Two findings from the review of 5fc391a4, both fixed here rather than sent back.

**The API-key trust seed never matched a real key.** `seedApiKeyTrustFile()` wrote the
key verbatim into `customApiKeyResponses.approved`, but Claude Code stores and compares
only the last 20 characters (`key.trim().slice(-20)`, applied on both the write and the
lookup). For any real key the seed missed, so claude stopped at the interactive
"Detected a custom API key in your environment" prompt, whose default is
"No (recommended)": the launch hangs, or silently refuses the key this feature just
injected and falls through to an OAuth login the isolated config dir does not have. It
survived review because a keyless llama.cpp/llama-swap endpoint uses DEFAULT_API_KEY
('local-dummy-key', 15 chars), where slice(-20) returns the whole string and the seed
matches by accident, and every test used a key shorter than that. Now truncated through
`truncateApiKeyForTrustFile()`, with a test using a 57-character key that also asserts
the full credential never reaches that second file.

**One `confirmed` flag answered two different questions.** The context-floor warning
("this model's window is below what this CLI needs") and the swap-conflict warning
("loading this unloads the model another session is using") shared it, and the context
check runs first, so a user clicking "launch anyway" past the context warning silently
consented to evicting someone else's model. They are about different people, so an
answer to one is not consent to the other. Both routes now read `confirmedContext` and
`confirmedSwap` independently; the legacy `confirmed` still means both, because it
shipped in this feature's HTTP-API-only cut and an existing caller must keep working.
The frontend answers each question with its own flag and accumulates them, on the
one-shot path, the restart path and the batch carry-forward alike.

Also from the same review: the swap-confirm dialog no longer renders " are currently
using ..." when multi-user scoping leaves the affected-session list empty (the swap is
blocked regardless of ownership; only the NAMES are scoped), and the per-endpoint
llama-swap log tails are closed in `WebServer.stop()` instead of only by the idle sweep
whose interval that same teardown disposes.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
Codeman maintainer
2026-09-19 12:32:50 +02:00
parent 3b55957d79
commit fe3bd0074c
9 changed files with 244 additions and 32 deletions
+28
View File
@@ -123,6 +123,34 @@ describe('applyCustomModelInjection: apiKeyTrustFile (pre-approves the injected
expect(written.customApiKeyResponses.rejected).toEqual([]);
});
// ⚠ Claude Code stores and looks up only the LAST 20 CHARACTERS of a key
// (`key.trim().slice(-20)`, applied on both write and read), so seeding the whole key
// never matches for a REAL one and the launch stops at the interactive "Detected a
// custom API key" prompt whose default is "No (recommended)". Every other test here
// uses a key shorter than 20 characters, where slice(-20) is the whole string and the
// bug is invisible, which is exactly how it survived review.
it('claude: seeds a REAL-length key in the truncated form the CLI actually matches on', () => {
const sessionId = 'sess-trust-long';
sessionsToClean.push(sessionId);
const longKey = 'sk-or-v1-0123456789abcdef0123456789abcdef0123456789abcdef';
expect(longKey.length).toBeGreaterThan(20);
const applied = applyCustomModelInjection(
entryOrThrow('claude'),
{ ...endpoint, apiKey: longKey },
'qwen3',
sessionId
);
const written = JSON.parse(readFileSync(join(applied!.configDir!, '.claude.json'), 'utf8')) as {
customApiKeyResponses: { approved: string[] };
};
expect(written.customApiKeyResponses.approved).toEqual(['cdef0123456789abcdef']);
expect(written.customApiKeyResponses.approved[0]).toHaveLength(20);
// and the full credential is not written into this second file at all
expect(readFileSync(join(applied!.configDir!, '.claude.json'), 'utf8')).not.toContain(longKey);
});
it('claude: falls back to the dummy key when the endpoint has none, and still seeds it', () => {
const sessionId = 'sess-trust-2';
sessionsToClean.push(sessionId);
+93
View File
@@ -463,6 +463,99 @@ describe('POST /api/sessions/:id/custom-model', () => {
expect(session.restartCli).toHaveBeenCalledTimes(1);
});
// The two questions are about DIFFERENT people: a context window below the floor is
// the caller's own problem, while unloading a model takes it away from someone else's
// session. They shared one `confirmed` flag until this release, and because the
// context check runs first, clicking "launch anyway" past the context warning silently
// answered the swap question too and evicted another session's model unasked.
describe('answering one question is not consent to the other', () => {
async function bothConditions() {
const { app, ctx } = await setup();
await writeCustomModelHosts(getDataDir(), [CLAUDE_ENDPOINT, SMALL_CTX_ENDPOINT]);
const session = ctx.sessions.get('test-session-1')!;
session.mode = 'claude';
// another session is actively on the model this endpoint currently has loaded
const other = createMockSession('other-session');
other.name = 'w2-otherbox';
other.customModel = { endpointId: 'ep-small', modelId: 'llama3' };
ctx.sessions.set('other-session', other);
mockRunning([{ model: 'llama3', state: 'ready' }]);
return { app, session };
}
it('still asks about the swap after the context warning was confirmed', async () => {
const { app, session } = await bothConditions();
const res = await app.inject({
method: 'POST',
url: '/api/sessions/test-session-1/custom-model',
payload: {
endpointId: 'ep-small',
modelId: 'qwen3.8-27b-ud-q4_k_xl',
confirmedContext: true,
},
});
const body = res.json();
expect(body.requiresContextWarning).toBeUndefined();
expect(body.requiresConfirmation).toBe(true);
expect(body.currentlyLoadedModel).toBe('llama3');
// and crucially nothing was applied: the other session keeps its model
expect(session.setCustomModel).not.toHaveBeenCalled();
expect(session.restartCli).not.toHaveBeenCalled();
});
it('applies once BOTH questions are answered', async () => {
const { app, session } = await bothConditions();
const res = await app.inject({
method: 'POST',
url: '/api/sessions/test-session-1/custom-model',
payload: {
endpointId: 'ep-small',
modelId: 'qwen3.8-27b-ud-q4_k_xl',
confirmedContext: true,
confirmedSwap: true,
},
});
const body = res.json();
expect(body.requiresContextWarning).toBeUndefined();
expect(body.requiresConfirmation).toBeUndefined();
expect(session.setCustomModel).toHaveBeenCalledTimes(1);
});
it('confirmedSwap alone does not silence the context warning either', async () => {
const { app, session } = await bothConditions();
const res = await app.inject({
method: 'POST',
url: '/api/sessions/test-session-1/custom-model',
payload: { endpointId: 'ep-small', modelId: 'qwen3.8-27b-ud-q4_k_xl', confirmedSwap: true },
});
expect(res.json().requiresContextWarning).toBe(true);
expect(session.setCustomModel).not.toHaveBeenCalled();
});
// `confirmed` shipped in the HTTP-API-only cut of this feature, so a caller written
// against that must keep working: it means both, exactly as it used to.
it('keeps the legacy blanket `confirmed` meaning both', async () => {
const { app, session } = await bothConditions();
const res = await app.inject({
method: 'POST',
url: '/api/sessions/test-session-1/custom-model',
payload: { endpointId: 'ep-small', modelId: 'qwen3.8-27b-ud-q4_k_xl', confirmed: true },
});
const body = res.json();
expect(body.requiresContextWarning).toBeUndefined();
expect(body.requiresConfirmation).toBeUndefined();
expect(session.setCustomModel).toHaveBeenCalledTimes(1);
});
});
it('does not warn when the discovered context is comfortably above the floor', async () => {
const { app, ctx } = await setup();
const roomyEndpoint: CustomModelHost = {