fix(custom-model): address Ark0N's PR review — client-side probe timeout, defer "last used" past confirmation, docs, zh-CN

Four things from the maintainer's review on PR #459, all fixed:

1. Bound _getCustomModelCurrentlyLoaded's probe client-side (~800ms via
   Promise.race, on top of — never instead of — the route's own 5s
   server-side timeout). Without it, an asleep/firewalled endpoint behind
   a saved model list left the picker completely invisible for up to 5s
   after the Run menu had already closed, with no spinner or toast.
   `timeoutMs` is an optional param (default 800, real callers never pass
   it) so a test can drive it in milliseconds, same pattern as
   `_watchLlamaSwapLoading`'s own `pollIntervalMs` — this code runs in a
   JSDOM window's own realm, whose setTimeout vi.useFakeTimers() cannot
   patch.

2. "Last used" is now written only once a launch actually applies, never
   on the mere click. It moved out of runCustomModelEntry (unconditional)
   and into each path's own success point: _quickStartWithCustomModelConfirm
   after the final post succeeds, and _runCustomModelEntryViaRestart right
   after the apply's success check. A context-window-warning decline means
   this exact model cannot work with this CLI at all, so the old
   unconditional write would promote, next time the picker opened, the one
   model guaranteed to fail again.

3. Documented the promotion/tag precedence and the new
   codeman:customModelLastUsed:<mode>:<endpointId> localStorage key in both
   CLAUDE.md's Custom Model Endpoint Profiles section and
   docs/custom-model-endpoints.md's Run-menu picker section.

4. Added zh-CN entries for "Currently loaded" and "Last used" in i18n.js,
   next to this modal's existing "Choose a model"/"Custom Endpoints" pair.

New tests: the client-side timeout (endpoint that never answers, one that
answers within the bound, and a rejected-after-timeout probe settling
quietly), and "last used" recording on success vs. NOT recording on either
confirmation's decline, for both the restart and one-shot paths.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01N6eadpRyqpA9PD3i139cSD
This commit is contained in:
Devvyn
2026-09-20 21:19:42 +08:00
co-authored by Claude Sonnet 5
parent 73607663fd
commit 88e5b7b200
6 changed files with 142 additions and 25 deletions
+1 -1
View File
File diff suppressed because one or more lines are too long
+22
View File
@@ -183,6 +183,28 @@ launch, with the endpoint's `defaultModelId` marked but not auto-chosen —
the point of asking is letting one launch deliberately differ from the the point of asking is letting one launch deliberately differ from the
saved default, not just confirming it. saved default, not just confirming it.
The modal promotes exactly one row to the top of the list rather than
always showing raw discovery order, so the zero-wait choice is the one
under your thumb:
- **"Currently loaded"** — a model from this host's own list that
llama-swap reports `ready` right now, queried via
`GET /api/model-endpoints/:id/running-status`. Bounded client-side to
~800ms (`Promise.race`), on top of the route's own 5s server-side
timeout, so an endpoint that is asleep or firewalled cannot leave the
modal invisible for the full 5s after the Run menu has already closed.
- **"Last used"** — shown only when nothing is currently loaded: the model
actually launched last for this exact (harness, endpoint) pair, read
from the per-device `codeman:customModelLastUsed:<mode>:<endpointId>`
localStorage key. Written by `runCustomModelEntry` /
`_quickStartWithCustomModelConfirm` only once the model is actually
applied, never on the mere click — declining the context-window warning
means this exact model cannot work with this CLI at all, so promoting it
next time would be actively wrong, not just premature.
Neither tag reorders anything past that one promoted row, and both defer
to "Default" when neither applies.
**How the launch itself applies the endpoint depends on the harness.** For **How the launch itself applies the endpoint depends on the harness.** For
opencode, Codex, Gemini, Pi, Grok, DeepSeek and OMP (`runCustomModelEntry` → opencode, Codex, Gemini, Pi, Grok, DeepSeek and OMP (`runCustomModelEntry` →
`_runCustomModelEntryOneShot`), the endpoint/model is folded into the SAME `_runCustomModelEntryOneShot`), the endpoint/model is folded into the SAME
+2
View File
@@ -312,6 +312,8 @@
'运行菜单选择器会为此端点应用该模型。请先发现可用模型。', '运行菜单选择器会为此端点应用该模型。请先发现可用模型。',
'Custom Endpoints': '自定义端点', 'Custom Endpoints': '自定义端点',
'Choose a model': '选择模型', 'Choose a model': '选择模型',
'Currently loaded': '当前已加载',
'Last used': '上次使用',
'That endpoint no longer exists': '该端点已不存在', 'That endpoint no longer exists': '该端点已不存在',
'No models discovered for this endpoint yet': '此端点尚未发现任何模型', 'No models discovered for this endpoint yet': '此端点尚未发现任何模型',
'Subagent Options': '子智能体选项', 'Subagent Options': '子智能体选项',
+41 -11
View File
@@ -666,16 +666,31 @@ Object.assign(CodemanApp.prototype, {
* `state === 'ready'` check. Returns null for a plain (non-llama-swap) server, an * `state === 'ready'` check. Returns null for a plain (non-llama-swap) server, an
* unreachable endpoint, or a loaded model this host no longer lists as discovered — * unreachable endpoint, or a loaded model this host no longer lists as discovered —
* never throws, since a failed probe should just skip promotion, not break the picker. * never throws, since a failed probe should just skip promotion, not break the picker.
*
* ⚠️ Client-side bounded to ~800ms via Promise.race, on top of (never instead of) the
* route's own 5s server-side timeout (`RUNNING_TIMEOUT_MS`, custom-model-routes.ts) —
* a saved endpoint keeps its discovered models cached, so "the box behind this endpoint
* is asleep or firewalled" is a normal way to reach this path, not an exotic one, and
* the modal must not sit invisible (Run menu already closed, nothing else on screen)
* for the full 5s a slow/dead endpoint can take. The losing side of the race is left to
* resolve on its own — `.catch(() => null)` only stops an unhandled-rejection warning
* when it eventually fails, it never cancels the in-flight fetch.
*
* `timeoutMs` exists to let a test drive this in milliseconds instead of the real
* 800 — same reasoning as `_watchLlamaSwapLoading`'s own `pollIntervalMs`: this code
* runs inside a JSDOM window's own realm, whose `setTimeout` is not the one
* `vi.useFakeTimers()` patches, so a param is the only way to test the timeout without
* actually waiting on it. Real callers never pass it.
*/ */
async _getCustomModelCurrentlyLoaded(host) { async _getCustomModelCurrentlyLoaded(host, timeoutMs = 800) {
try { const probe = this._apiJson(`/api/model-endpoints/${encodeURIComponent(host.id)}/running-status`).catch(
const status = await this._apiJson(`/api/model-endpoints/${encodeURIComponent(host.id)}/running-status`); () => null
);
const timeout = new Promise((resolve) => setTimeout(() => resolve(null), timeoutMs));
const status = await Promise.race([probe, timeout]);
if (!status?.isLlamaSwap) return null; if (!status?.isLlamaSwap) return null;
const ready = (status.running || []).find((r) => r.state === 'ready' && (host.models || []).includes(r.model)); const ready = (status.running || []).find((r) => r.state === 'ready' && (host.models || []).includes(r.model));
return ready?.model || null; return ready?.model || null;
} catch {
return null;
}
}, },
/** /**
@@ -844,10 +859,12 @@ Object.assign(CodemanApp.prototype, {
* (Codex, confirmed live) than on claude's own `--resume`-based restart. * (Codex, confirmed live) than on claude's own `--resume`-based restart.
*/ */
async runCustomModelEntry(mode, endpointId, modelId) { async runCustomModelEntry(mode, endpointId, modelId) {
// Recorded on the attempt, not gated on success below — the picker's "Last used" // "Last used" is recorded by each path itself, ONLY once the model is actually
// promotion is a convenience hint, not a launch-history log, so it should reflect // applied — never here, unconditionally, on the mere attempt. A context-window
// what the user picked even if this particular launch goes on to fail. // warning or a swap-conflict question can still say no after this call, and the
this._setCustomModelLastUsed(mode, endpointId, modelId); // context-warning case is the one that bites: declining it means this exact
// model cannot work with this CLI at all, so promoting it as "Last used" next
// time the picker opens would be actively wrong, not just premature.
if (mode === 'claude') { if (mode === 'claude') {
return this._runCustomModelEntryViaRestart(mode, endpointId, modelId); return this._runCustomModelEntryViaRestart(mode, endpointId, modelId);
} }
@@ -940,7 +957,15 @@ Object.assign(CodemanApp.prototype, {
answered = { ...answered, confirmedSwap: true }; answered = { ...answered, confirmedSwap: true };
data = await post({ ...bodyObj, customModel: { ...bodyObj.customModel, ...answered } }); data = await post({ ...bodyObj, customModel: { ...bodyObj.customModel, ...answered } });
} }
this._lastCustomModelLaunchResult = data?.success !== false ? data?.data : undefined; const launched = data?.success !== false;
this._lastCustomModelLaunchResult = launched ? data?.data : undefined;
// Only once actually launched, and only for a call that carried a custom-model pick
// at all — `_launchQuickStartInstances` runs every quick-start body (custom-model or
// not) through this same function, so a plain launch must not fall through here with
// an undefined endpointId/modelId that quietly no-ops the (mode, endpointId) key.
if (launched && bodyObj.customModel) {
this._setCustomModelLastUsed(bodyObj.mode, bodyObj.customModel.endpointId, bodyObj.customModel.modelId);
}
return data; return data;
}, },
@@ -1068,6 +1093,11 @@ Object.assign(CodemanApp.prototype, {
return; return;
} }
// The apply has actually succeeded and both questions (if asked) are answered
// yes — only now is this a real "last used" for the picker's next open, not
// before either confirmation had a chance to decline it.
this._setCustomModelLastUsed(mode, endpointId, modelId);
// The apply above already succeeded — the session IS pointed at the endpoint — but // The apply above already succeeded — the session IS pointed at the endpoint — but
// llama-swap itself may still be unloading the old model and loading this one, which // llama-swap itself may still be unloading the old model and loading this one, which
// can take well over a minute. Without this, a prompt sent during that window either // can take well over a minute. Without this, a prompt sent during that window either
+13 -3
View File
@@ -142,7 +142,7 @@ describe('_quickStartWithCustomModelConfirm', () => {
})) as unknown as typeof fetch; })) as unknown as typeof fetch;
} }
it('returns the response directly when no confirmation is needed', async () => { it('returns the response directly when no confirmation is needed, and records "last used"', async () => {
const { win, app } = bootApp(); const { win, app } = bootApp();
withFetch(win, (body) => ({ success: true, data: { sessionId: 's1', modelSwapInProgress: false, body } })); withFetch(win, (body) => ({ success: true, data: { sessionId: 's1', modelSwapInProgress: false, body } }));
const data = await app._quickStartWithCustomModelConfirm({ const data = await app._quickStartWithCustomModelConfirm({
@@ -152,9 +152,17 @@ describe('_quickStartWithCustomModelConfirm', () => {
expect(data.success).toBe(true); expect(data.success).toBe(true);
expect(data.data.sessionId).toBe('s1'); expect(data.data.sessionId).toBe('s1');
expect(app._lastCustomModelLaunchResult).toEqual(data.data); expect(app._lastCustomModelLaunchResult).toEqual(data.data);
expect(win.localStorage.getItem('codeman:customModelLastUsed:codex:e')).toBe('m');
}); });
it('confirming re-sends with confirmedSwap and returns the second response', async () => { it('a plain launch with no customModel at all never touches the "last used" key (undefined endpointId/modelId would otherwise silently no-op it)', async () => {
const { win, app } = bootApp();
withFetch(win, () => ({ success: true, data: { sessionId: 's1' } }));
await app._quickStartWithCustomModelConfirm({ mode: 'codex' });
expect(win.localStorage.getItem('codeman:customModelLastUsed:codex:undefined')).toBeNull();
});
it('confirming re-sends with confirmedSwap, returns the second response, and only THEN records "last used"', async () => {
const { win, app } = bootApp(); const { win, app } = bootApp();
app._confirmModelSwap = async () => true; app._confirmModelSwap = async () => true;
let calls = 0; let calls = 0;
@@ -183,9 +191,10 @@ describe('_quickStartWithCustomModelConfirm', () => {
expect(calls).toBe(2); expect(calls).toBe(2);
expect(data.data.sessionId).toBe('s1'); expect(data.data.sessionId).toBe('s1');
expect(app._lastCustomModelLaunchResult.modelSwapInProgress).toBe(true); expect(app._lastCustomModelLaunchResult.modelSwapInProgress).toBe(true);
expect(win.localStorage.getItem('codeman:customModelLastUsed:codex:e')).toBe('m');
}); });
it('cancelling never re-sends, and reports a cancellation error', async () => { it('cancelling never re-sends, reports a cancellation error, and must NEVER record "last used" for a launch that never happened', async () => {
const { win, app } = bootApp(); const { win, app } = bootApp();
app._confirmModelSwap = async () => false; app._confirmModelSwap = async () => false;
let calls = 0; let calls = 0;
@@ -208,6 +217,7 @@ describe('_quickStartWithCustomModelConfirm', () => {
expect(data.success).toBe(false); expect(data.success).toBe(false);
expect(data.error).toMatch(/cancelled/i); expect(data.error).toMatch(/cancelled/i);
expect(app._lastCustomModelLaunchResult).toBeUndefined(); expect(app._lastCustomModelLaunchResult).toBeUndefined();
expect(win.localStorage.getItem('codeman:customModelLastUsed:codex:e')).toBeNull();
}); });
}); });
+60 -7
View File
@@ -357,10 +357,19 @@ describe('Custom Model Endpoint Profiles: the "which model" picker', () => {
expect(buttons[0].textContent).toContain('qwen3'); expect(buttons[0].textContent).toContain('qwen3');
}); });
it('remembers the launched model as "last used" for this (harness, endpoint) pair', async () => { it('remembers the launched model as "last used" only once the apply actually succeeds, not on the mere attempt', async () => {
const { win, app } = bootApp({}); const { win, app } = bootApp({});
app.run = async () => {}; app.activeSessionId = 'old-session';
app._api = async () => ({ ok: true, json: async () => ({ success: true, data: {} }) }); // A real new session, and a real successful apply with no questions asked — the
// restart path only reaches its _setCustomModelLastUsed call past both.
app.run = async () => {
app.activeSessionId = 'new-session';
};
app._api = async () => ({
ok: true,
status: 200,
json: async () => ({ success: true, data: { customModel: { endpointId: 'llama-box' }, restarted: true } }),
});
await app.runCustomModelEntry('claude', 'llama-box', 'qwen3'); await app.runCustomModelEntry('claude', 'llama-box', 'qwen3');
@@ -464,6 +473,48 @@ describe('Custom Model Endpoint Profiles: the "which model" picker', () => {
}); });
}); });
describe('Custom Model Endpoint Profiles: _getCustomModelCurrentlyLoaded is client-side bounded', () => {
// `timeoutMs` driven in milliseconds rather than the real 800 — same reasoning as
// `_watchLlamaSwapLoading`'s own `pollIntervalMs` a few describe blocks down: this
// code runs inside the JSDOM window's own realm, whose setTimeout vi.useFakeTimers()
// does not patch, so this is the only way to test the bound without actually waiting
// on it (or, worse, hanging on a promise that deliberately never resolves).
it('never lets an endpoint that never answers keep the picker waiting past the client-side bound', async () => {
const { app } = bootApp({
hosts: [{ id: 'llama-box', label: 'llama.cpp', baseUrl: 'http://x', models: ['qwen3'] }],
});
// A `running-status` probe that simply never resolves — the exact shape of an
// endpoint that is asleep or firewalled, distinct from one that answers an error.
app._apiJson = () => new Promise(() => {});
const result = await app._getCustomModelCurrentlyLoaded({ id: 'llama-box', models: ['qwen3'] }, 5);
expect(result).toBeNull();
});
it('an endpoint that answers well within the bound is unaffected by it', async () => {
const { app } = bootApp({});
app._apiJson = async () => ({ isLlamaSwap: true, running: [{ model: 'qwen3', state: 'ready' }] });
const result = await app._getCustomModelCurrentlyLoaded({ id: 'llama-box', models: ['qwen3'] }, 5);
expect(result).toBe('qwen3');
});
it('a rejected probe settles quietly to null rather than leaving an unhandled rejection once the timeout has already won the race', async () => {
const { app } = bootApp({});
app._apiJson = () => new Promise((_resolve, reject) => setTimeout(() => reject(new Error('boom')), 10));
const result = await app._getCustomModelCurrentlyLoaded({ id: 'llama-box', models: ['qwen3'] }, 2);
expect(result).toBeNull();
// Give the loser of the race a turn to actually reject and hit its own .catch —
// an unswallowed rejection here would surface as an "Unhandled Errors" failure
// for the whole test file, not a failed assertion in this test.
await new Promise((resolve) => setTimeout(resolve, 20));
});
});
describe('Custom Model Endpoint Profiles: applying a picked entry', () => { 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 () => { it('does not apply the endpoint to a session that was already open when the launch fails', async () => {
const { app } = bootApp({}); const { app } = bootApp({});
@@ -1156,8 +1207,8 @@ describe("Custom Model Endpoint Profiles: requiresContextWarning (this CLI's own
return { win, app, applyBodies }; return { win, app, applyBodies };
} }
it('confirming the in-app context-warning modal re-sends the apply with confirmed:true', async () => { it('confirming the in-app context-warning modal re-sends the apply with confirmed:true, and only THEN records "last used"', async () => {
const { app, applyBodies } = launchHarness([ const { win, app, applyBodies } = launchHarness([
{ requiresContextWarning: true, modelId: 'qwen3', contextLength: 16384, minSafeContextTokens: 40000 }, { requiresContextWarning: true, modelId: 'qwen3', contextLength: 16384, minSafeContextTokens: 40000 },
{ customModel: { endpointId: 'llama-box' }, restarted: true, modelSwapInProgress: false }, { customModel: { endpointId: 'llama-box' }, restarted: true, modelSwapInProgress: false },
]); ]);
@@ -1174,10 +1225,11 @@ describe("Custom Model Endpoint Profiles: requiresContextWarning (this CLI's own
{ endpointId: 'llama-box', modelId: 'qwen3' }, { endpointId: 'llama-box', modelId: 'qwen3' },
{ endpointId: 'llama-box', modelId: 'qwen3', confirmedContext: true }, { endpointId: 'llama-box', modelId: 'qwen3', confirmedContext: true },
]); ]);
expect(win.localStorage.getItem('codeman:customModelLastUsed:claude:llama-box')).toBe('qwen3');
}); });
it('declining the in-app context-warning modal keeps the native backend and never re-sends the apply', async () => { it('declining the in-app context-warning modal keeps the native backend, never re-sends the apply, and must NEVER record this model as "last used" — it cannot work with this CLI at all', async () => {
const { app, applyBodies } = launchHarness([ const { win, app, applyBodies } = launchHarness([
{ requiresContextWarning: true, modelId: 'qwen3', contextLength: 16384, minSafeContextTokens: 40000 }, { requiresContextWarning: true, modelId: 'qwen3', contextLength: 16384, minSafeContextTokens: 40000 },
]); ]);
app._confirmContextWarning = async () => false; app._confirmContextWarning = async () => false;
@@ -1190,6 +1242,7 @@ describe("Custom Model Endpoint Profiles: requiresContextWarning (this CLI's own
expect(applyBodies).toHaveLength(1); // no second (confirmed) call expect(applyBodies).toHaveLength(1); // no second (confirmed) call
expect(toastMessage).toMatch(/context window too small/i); expect(toastMessage).toMatch(/context window too small/i);
expect(win.localStorage.getItem('codeman:customModelLastUsed:claude:llama-box')).toBeNull();
}); });
}); });