fix(web): scope model list to provider and resolve composite catalog identity
ci/woodpecker/pr/ci Pipeline was successful
ci/woodpecker/pr/ci Pipeline was successful
Fix-in-lane for the two Task-4 review findings, red-first TDD, inside the
existing nine-file fence.
M-A: the composer now filters model <option>s to the intentionally selected
provider (harness.providerId). With no provider chosen the model select offers
only its placeholder, so a user can never pick a model that belongs to a
different provider.
M-B: the model <option> value is now the collision-safe composite
`${providerId}:${modelId}` (was the bare modelId), the controlled select value
mirrors that same identity so the exact catalog row highlights, and the change
handler resolves the composite back to the exact catalog row and persists that
row's own {harnessId, providerId, modelId}. selectModel(providerId, modelId) no
longer combines a bare model id with ambient provider state, so two providers
exposing the same modelId stay distinct.
New red-first tests prove: cross-provider models absent, identical modelIds
under two providers stay distinct and resolve to the intended tuple, a provider
change invalidates the old model and keeps send disabled until the new tuple
persists, and a model pick does not enable send until its exact PUT resolves.
The four existing anti-masking invariants and the 422 tuple-preservation test
are intact.
Co-Authored-By: Claude Opus 4.8 <[email protected]>
Claude-Session: https://claude.ai/code/session_01ESFAnh2t9HmLwng8oW95St
This commit is contained in:
@@ -58,7 +58,18 @@ export function Composer({
|
||||
}
|
||||
}
|
||||
|
||||
const models = harness.catalog?.models ?? [];
|
||||
// Scope the model options to the intentionally selected provider. With no
|
||||
// provider chosen (`providerId === ''`) nothing matches, so the model select
|
||||
// offers only the placeholder — never a cross-provider row.
|
||||
const models = (harness.catalog?.models ?? []).filter(
|
||||
(model) => model.providerId === harness.providerId,
|
||||
);
|
||||
// A collision-safe composite option identity covering the full provider+model
|
||||
// tuple. The controlled select mirrors the same identity so the exact catalog
|
||||
// row highlights (a bare modelId would collide across providers).
|
||||
const modelOptionValue = (model: { providerId: string; modelId: string }): string =>
|
||||
`${model.providerId}:${model.modelId}`;
|
||||
const selectedModelValue = harness.modelId ? `${harness.providerId}:${harness.modelId}` : '';
|
||||
|
||||
return (
|
||||
<form
|
||||
@@ -98,14 +109,19 @@ export function Composer({
|
||||
</select>
|
||||
<select
|
||||
aria-label="Model"
|
||||
value={harness.modelId}
|
||||
onChange={(event) => harness.selectModel(event.target.value)}
|
||||
value={selectedModelValue}
|
||||
onChange={(event) => {
|
||||
// Resolve the composite option identity back to the exact catalog
|
||||
// row and persist that row's own provider+model — never a bare id.
|
||||
const selected = models.find((model) => modelOptionValue(model) === event.target.value);
|
||||
if (selected) harness.selectModel(selected.providerId, selected.modelId);
|
||||
}}
|
||||
disabled={harness.catalogUnavailable || models.length === 0}
|
||||
className="rounded border px-2 py-1 text-xs"
|
||||
>
|
||||
<option value="">Select a model…</option>
|
||||
{models.map((model) => (
|
||||
<option key={`${model.providerId}:${model.modelId}`} value={model.modelId}>
|
||||
<option key={modelOptionValue(model)} value={modelOptionValue(model)}>
|
||||
{model.displayName}
|
||||
</option>
|
||||
))}
|
||||
|
||||
@@ -205,7 +205,7 @@ describe('useHarnessSelection', () => {
|
||||
value().selectProvider('openai');
|
||||
});
|
||||
await act(async () => {
|
||||
value().selectModel('gpt-5');
|
||||
value().selectModel('openai', 'gpt-5');
|
||||
});
|
||||
await flush();
|
||||
|
||||
@@ -327,7 +327,7 @@ describe('useHarnessSelection', () => {
|
||||
value().selectProvider('openai');
|
||||
});
|
||||
await act(async () => {
|
||||
value().selectModel('gpt-5');
|
||||
value().selectModel('openai', 'gpt-5');
|
||||
});
|
||||
await flush();
|
||||
|
||||
@@ -342,6 +342,87 @@ describe('useHarnessSelection', () => {
|
||||
expect(value().canSend).toBe(false);
|
||||
});
|
||||
|
||||
it('invalidates the model on a provider change and keeps send disabled until the new tuple persists', async () => {
|
||||
installFetch({
|
||||
harnesses: [{ id: 'pi', displayName: 'Pi', capabilities: [] }],
|
||||
catalog: { body: PI_CATALOG },
|
||||
selection: null,
|
||||
});
|
||||
await mount();
|
||||
await act(async () => {
|
||||
value().selectHarness('pi');
|
||||
});
|
||||
await flush();
|
||||
await act(async () => {
|
||||
value().selectProvider('openai');
|
||||
});
|
||||
await act(async () => {
|
||||
value().selectModel('openai', 'gpt-5');
|
||||
});
|
||||
await flush();
|
||||
// A valid provider-A tuple has persisted.
|
||||
expect(value().canSend).toBe(true);
|
||||
expect(value().persistedSelection).toEqual({
|
||||
harnessId: 'pi',
|
||||
providerId: 'openai',
|
||||
modelId: 'gpt-5',
|
||||
});
|
||||
|
||||
// Switching provider clears the model that no longer belongs to it.
|
||||
await act(async () => {
|
||||
value().selectProvider('anthropic');
|
||||
});
|
||||
expect(value().modelId).toBe('');
|
||||
expect(value().canSend).toBe(false);
|
||||
|
||||
// Send stays disabled until the new exact provider-B tuple persists.
|
||||
await act(async () => {
|
||||
value().selectModel('anthropic', 'claude');
|
||||
});
|
||||
await flush();
|
||||
expect(value().canSend).toBe(true);
|
||||
expect(value().persistedSelection).toEqual({
|
||||
harnessId: 'pi',
|
||||
providerId: 'anthropic',
|
||||
modelId: 'claude',
|
||||
});
|
||||
expect(value().projection).toEqual({ provider: 'anthropic', modelId: 'claude' });
|
||||
});
|
||||
|
||||
it('does not enable send on a model pick until the PUT for that exact new tuple resolves', async () => {
|
||||
installFetch({
|
||||
harnesses: [{ id: 'pi', displayName: 'Pi', capabilities: [] }],
|
||||
catalog: { body: PI_CATALOG },
|
||||
selection: { harnessId: 'pi', providerId: 'openai', modelId: 'gpt-5' },
|
||||
deferPut: true,
|
||||
});
|
||||
await mount();
|
||||
// The persisted, in-catalog tuple is sendable after mount (no PUT needed).
|
||||
expect(value().canSend).toBe(true);
|
||||
|
||||
await act(async () => {
|
||||
value().selectProvider('anthropic');
|
||||
});
|
||||
expect(value().modelId).toBe('');
|
||||
expect(value().canSend).toBe(false);
|
||||
|
||||
await act(async () => {
|
||||
value().selectModel('anthropic', 'claude');
|
||||
});
|
||||
await flush();
|
||||
// PUT for the new tuple is still in flight — send MUST stay disabled.
|
||||
expect(value().canSend).toBe(false);
|
||||
|
||||
await act(async () => {
|
||||
putDeferred?.resolve(
|
||||
json({ selection: { harnessId: 'pi', providerId: 'anthropic', modelId: 'claude' } }),
|
||||
);
|
||||
});
|
||||
await flush();
|
||||
expect(value().canSend).toBe(true);
|
||||
expect(value().projection).toEqual({ provider: 'anthropic', modelId: 'claude' });
|
||||
});
|
||||
|
||||
it('never requests any /api/providers* endpoint across the whole flow', async () => {
|
||||
const fetchMock = installFetch({
|
||||
harnesses: [{ id: 'pi', displayName: 'Pi', capabilities: [] }],
|
||||
@@ -353,7 +434,7 @@ describe('useHarnessSelection', () => {
|
||||
value().selectProvider('anthropic');
|
||||
});
|
||||
await act(async () => {
|
||||
value().selectModel('claude');
|
||||
value().selectModel('anthropic', 'claude');
|
||||
});
|
||||
await flush();
|
||||
|
||||
|
||||
@@ -38,7 +38,10 @@ export interface HarnessSelectionValue {
|
||||
persistError: HarnessPersistError | null;
|
||||
selectHarness: (harnessId: string) => void;
|
||||
selectProvider: (providerId: string) => void;
|
||||
selectModel: (modelId: string) => void;
|
||||
/** Persist the EXACT catalog row's `{providerId, modelId}` — the caller
|
||||
* resolves the composite option identity to the real entry and passes both
|
||||
* ids, so a bare model id is never combined with ambient provider state. */
|
||||
selectModel: (providerId: string, modelId: string) => void;
|
||||
/** The compatibility `{provider, modelId}` projection for the legacy socket
|
||||
* send path — derived ONLY from the validated persisted tuple, never from any
|
||||
* free-text or unpersisted draft. Empty when nothing is sendable. */
|
||||
@@ -148,12 +151,19 @@ export function useHarnessSelection(): HarnessSelectionValue {
|
||||
}, []);
|
||||
|
||||
const selectModel = useCallback(
|
||||
(id: string): void => {
|
||||
setModelId(id);
|
||||
(selectedProviderId: string, selectedModelId: string): void => {
|
||||
// Bind the model to the EXACT catalog row's provider — never to ambient
|
||||
// provider state — so two providers exposing the same modelId can never
|
||||
// collide or mis-resolve. Keep the displayed provider consistent with the
|
||||
// resolved row.
|
||||
setProviderId(selectedProviderId);
|
||||
setModelId(selectedModelId);
|
||||
setPersistError(null);
|
||||
// Persist the FULL tuple — harness + provider come from the current draft
|
||||
// (this callback closes over the latest ids), the model is the pick.
|
||||
const requested: HarnessSelection = { harnessId, providerId, modelId: id };
|
||||
const requested: HarnessSelection = {
|
||||
harnessId,
|
||||
providerId: selectedProviderId,
|
||||
modelId: selectedModelId,
|
||||
};
|
||||
const requestId = persistRequestRef.current + 1;
|
||||
persistRequestRef.current = requestId;
|
||||
void (async (): Promise<void> => {
|
||||
@@ -173,7 +183,7 @@ export function useHarnessSelection(): HarnessSelectionValue {
|
||||
}
|
||||
})();
|
||||
},
|
||||
[harnessId, providerId],
|
||||
[harnessId],
|
||||
);
|
||||
|
||||
const draft: HarnessSelection = { harnessId, providerId, modelId };
|
||||
|
||||
@@ -499,10 +499,13 @@ describe('ChatPage', () => {
|
||||
expect([...harnessSelect.options].map((o) => o.value)).toContain('pi');
|
||||
expect([...providerSelect.options].map((o) => o.value)).toContain('openai');
|
||||
expect([...providerSelect.options].map((o) => o.value)).toContain('anthropic');
|
||||
// The model options are catalog-derived, not hardcoded provider models.
|
||||
expect([...modelSelect.options].map((o) => o.value)).toEqual(
|
||||
expect.arrayContaining(['gpt-5', 'claude']),
|
||||
);
|
||||
// The model options are catalog-derived (not hardcoded) and scoped to the
|
||||
// selected provider (openai, from the persisted tuple) using a collision-safe
|
||||
// composite identity — the anthropic row is absent, not a bare 'claude'.
|
||||
const modelValues = [...modelSelect.options].map((o) => o.value);
|
||||
expect(modelValues).toContain('openai:gpt-5');
|
||||
expect(modelValues).not.toContain('anthropic:claude');
|
||||
expect(modelValues).not.toContain('claude');
|
||||
});
|
||||
|
||||
it('sends provider/model derived from the persisted catalog tuple (never free text) and emits abort from Stop', async () => {
|
||||
@@ -524,7 +527,9 @@ describe('ChatPage', () => {
|
||||
});
|
||||
const modelSelect = container.querySelector('select[aria-label="Model"]') as HTMLSelectElement;
|
||||
await act(async () => {
|
||||
selectValue(modelSelect, 'claude');
|
||||
// Composite provider+model option identity (provider was switched to
|
||||
// anthropic above); the bare 'claude' no longer identifies an option.
|
||||
selectValue(modelSelect, 'anthropic:claude');
|
||||
});
|
||||
await flushAsync();
|
||||
|
||||
@@ -765,6 +770,134 @@ describe('ChatPage', () => {
|
||||
expect(fake.emitted.filter((e) => e.event === 'message')).toHaveLength(1);
|
||||
});
|
||||
|
||||
it('scopes the model options to the intentionally selected provider (cross-provider models absent)', async () => {
|
||||
await remountWithFetch(harnessFetch(null));
|
||||
|
||||
const harnessSelect = container.querySelector(
|
||||
'select[aria-label="Harness"]',
|
||||
) as HTMLSelectElement;
|
||||
await act(async () => {
|
||||
selectValue(harnessSelect, 'pi');
|
||||
});
|
||||
await flushAsync();
|
||||
|
||||
const providerSelect = container.querySelector(
|
||||
'select[aria-label="Provider"]',
|
||||
) as HTMLSelectElement;
|
||||
await act(async () => {
|
||||
selectValue(providerSelect, 'openai');
|
||||
});
|
||||
|
||||
const modelSelect = container.querySelector('select[aria-label="Model"]') as HTMLSelectElement;
|
||||
const optionValues = [...modelSelect.options].map((o) => o.value).filter((v) => v !== '');
|
||||
// Only the selected provider's models are offered — provider B's model
|
||||
// (anthropic:claude) is absent, so a user cannot pick across providers.
|
||||
expect(optionValues).toEqual(['openai:gpt-5']);
|
||||
expect(optionValues).not.toContain('anthropic:claude');
|
||||
});
|
||||
|
||||
it('keeps identical modelIds under two providers distinct and resolves the pick to the exact tuple', async () => {
|
||||
const COLLIDING_CATALOG = {
|
||||
harnessId: 'pi',
|
||||
version: '2026-08-11',
|
||||
fingerprint: 'fp',
|
||||
models: [
|
||||
{
|
||||
harnessId: 'pi',
|
||||
providerId: 'alpha',
|
||||
modelId: 'gpt-x',
|
||||
displayName: 'Alpha GPT-X',
|
||||
reasoningCapability: true,
|
||||
inputTypes: ['text'],
|
||||
authState: 'ready',
|
||||
availability: 'available',
|
||||
},
|
||||
{
|
||||
harnessId: 'pi',
|
||||
providerId: 'beta',
|
||||
modelId: 'gpt-x',
|
||||
displayName: 'Beta GPT-X',
|
||||
reasoningCapability: true,
|
||||
inputTypes: ['text'],
|
||||
authState: 'ready',
|
||||
availability: 'available',
|
||||
},
|
||||
],
|
||||
};
|
||||
const collidingFetch = vi.fn(async (input: unknown, init?: RequestInit) => {
|
||||
const url = String(input);
|
||||
const method = String(init?.method ?? 'GET').toUpperCase();
|
||||
if (url === '/api/harnesses') {
|
||||
return jsonResponse([{ id: 'pi', displayName: 'Pi', capabilities: [] }]);
|
||||
}
|
||||
if (url.startsWith('/api/harnesses/') && url.endsWith('/catalog')) {
|
||||
return jsonResponse(COLLIDING_CATALOG);
|
||||
}
|
||||
if (url === '/api/chat/preferences/selection' && method === 'GET') {
|
||||
return jsonResponse({ selection: null });
|
||||
}
|
||||
if (url === '/api/chat/preferences/selection' && method === 'PUT') {
|
||||
return jsonResponse({ selection: JSON.parse(String(init?.body)) });
|
||||
}
|
||||
return new Response('not found', { status: 404 });
|
||||
}) as unknown as typeof fetch;
|
||||
|
||||
await remountWithFetch(collidingFetch);
|
||||
|
||||
const harnessSelect = container.querySelector(
|
||||
'select[aria-label="Harness"]',
|
||||
) as HTMLSelectElement;
|
||||
await act(async () => {
|
||||
selectValue(harnessSelect, 'pi');
|
||||
});
|
||||
await flushAsync();
|
||||
|
||||
const providerSelect = container.querySelector(
|
||||
'select[aria-label="Provider"]',
|
||||
) as HTMLSelectElement;
|
||||
await act(async () => {
|
||||
selectValue(providerSelect, 'alpha');
|
||||
});
|
||||
|
||||
const modelSelect = container.querySelector('select[aria-label="Model"]') as HTMLSelectElement;
|
||||
// The colliding modelId is provider-qualified in the option value, never a
|
||||
// bare id, so the two providers' 'gpt-x' rows are uniquely identifiable.
|
||||
const optionValues = [...modelSelect.options].map((o) => o.value).filter((v) => v !== '');
|
||||
expect(optionValues).toEqual(['alpha:gpt-x']);
|
||||
|
||||
await act(async () => {
|
||||
selectValue(modelSelect, 'alpha:gpt-x');
|
||||
});
|
||||
await flushAsync();
|
||||
|
||||
// The controlled select highlights the alpha row via the composite identity.
|
||||
expect(modelSelect.value).toBe('alpha:gpt-x');
|
||||
|
||||
const textarea = container.querySelector(
|
||||
'textarea[aria-label="Message"]',
|
||||
) as HTMLTextAreaElement;
|
||||
await act(async () => {
|
||||
setValue(textarea, 'ping');
|
||||
});
|
||||
await act(async () => {
|
||||
textarea.dispatchEvent(
|
||||
new KeyboardEvent('keydown', { key: 'Enter', bubbles: true, cancelable: true }),
|
||||
);
|
||||
});
|
||||
|
||||
// The persisted/sent tuple resolves to provider alpha — NOT beta — even
|
||||
// though the bare modelId 'gpt-x' exists under both providers.
|
||||
expect(fake.emitted).toContainEqual({
|
||||
event: 'message',
|
||||
payload: {
|
||||
conversationId: undefined,
|
||||
content: 'ping',
|
||||
provider: 'alpha',
|
||||
modelId: 'gpt-x',
|
||||
},
|
||||
});
|
||||
});
|
||||
|
||||
it('removes socket handlers and tears down the socket on unmount, with no network calls', async () => {
|
||||
expect(fake.listeners.size).toBeGreaterThan(0);
|
||||
|
||||
|
||||
Reference in New Issue
Block a user