From dbc9f6081861edeef0eabfc212b859efe03135d0 Mon Sep 17 00:00:00 2001 From: Diego Rodrigues de Sa e Souza Date: Sat, 18 Jul 2026 22:22:51 -0300 Subject: [PATCH] fix(base-red): align least-used combo tests with executionKey usage keying (#7015) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit sortTargetsByUsage (open-sse/services/combo/targetSorters.ts, since #7015/#7059) keys usage lookups by the per-target executionKey (combo-name + step-id), not by the bare model string, so accounts sharing a modelStr don't collapse into a single usage bucket. Three tests called recordComboRequest() directly without a `target`, so the recorded usage landed under a modelStr fallback key that never matches the real executionKey computed at combo-resolution time — every target read back as 0 usage and the original combo order won, failing the "prefers the least-used model" assertions. Production is unaffected: every real combo.ts call site already passes `target: toRecordedTarget(target)`. Fixed by priming usage through real handleComboChat calls (which route recordComboRequest through the actual resolved target) instead of calling recordComboRequest() directly with an unlinked target. --- tests/unit/combo-routing-engine.test.ts | 92 +++++++++++++++++-------- tests/unit/combo-strategies.test.ts | 15 ++-- 2 files changed, 73 insertions(+), 34 deletions(-) diff --git a/tests/unit/combo-routing-engine.test.ts b/tests/unit/combo-routing-engine.test.ts index 7ea5271345..f4d5c323e8 100644 --- a/tests/unit/combo-routing-engine.test.ts +++ b/tests/unit/combo-routing-engine.test.ts @@ -667,31 +667,48 @@ test("handleComboChat p2c selects the better of two random choices by metrics", }); test("handleComboChat least-used strategy prefers the model with fewer recorded requests", async () => { - recordComboRequest("least-used-combo", "model-a", { - success: true, - latencyMs: 100, + // Least-used sorts by the per-target executionKey (combo-name + step-id), + // not by the bare model string (#7015/#7059 — sortTargetsByUsage keys + // byTarget[executionKey] so accounts sharing a modelStr don't collapse into + // one shared bucket). Calling recordComboRequest() directly without a + // `target` falls back to keying by modelStr, which never matches the real + // executionKey computed at combo-resolution time. Drive usage through real + // handleComboChat calls instead, so recordComboRequest is invoked with the + // actual resolved target (matching production behavior). + const comboDef = { + name: "least-used-combo", strategy: "least-used", - }); - recordComboRequest("least-used-combo", "model-a", { - success: true, - latencyMs: 100, - strategy: "least-used", - }); - recordComboRequest("least-used-combo", "model-b", { - success: true, - latencyMs: 100, - strategy: "least-used", - }); + models: ["model-a", "model-b", "model-c"], + }; + const primingCalls: any[] = []; + const runPrimingCall = () => + handleComboChat({ + body: {}, + combo: comboDef, + handleSingleModel: async (_body: any, modelStr: any) => { + primingCalls.push(modelStr); + return okResponse(); + }, + isModelAvailable: async () => true, + log: createLog(), + settings: null, + relayOptions: null as any, + allCombos: null, + }); + + // Tied at 0 usage, order is preserved: 1st priming call picks model-a, + // 2nd priming call (model-a now at 1) picks model-b (tied with model-c at + // 0, but model-b sorts first). That leaves model-a and model-b each used + // once, model-c untouched. + await runPrimingCall(); + await runPrimingCall(); + assert.deepEqual(primingCalls, ["model-a", "model-b"]); const calls: any[] = []; await handleComboChat({ body: {}, - combo: { - name: "least-used-combo", - strategy: "least-used", - models: ["model-a", "model-b", "model-c"], - }, + combo: comboDef, handleSingleModel: async (_body: any, modelStr: any) => { calls.push(modelStr); return okResponse(); @@ -2022,20 +2039,37 @@ test("handleComboChat eval-driven routing can match bare model eval target ids", }); test("handleComboChat normalizes legacy strategy names at runtime", async () => { - const usageCalls: any[] = []; - recordComboRequest("legacy-usage-combo", "model-a", { - success: true, - latencyMs: 100, - strategy: "least-used", + // See the least-used test above: recordComboRequest() must be driven + // through a real handleComboChat call so it records against the actual + // resolved executionKey, not a bare modelStr fallback that never matches + // sortTargetsByUsage's lookup (#7015/#7059). + const comboDef = { + name: "legacy-usage-combo", + strategy: "usage", + models: ["model-a", "model-b"], + }; + const primingCalls: any[] = []; + await handleComboChat({ + body: {}, + combo: comboDef, + handleSingleModel: async (_body: any, modelStr: any) => { + primingCalls.push(modelStr); + return okResponse(); + }, + isModelAvailable: async () => true, + log: createLog(), + settings: null, + relayOptions: null as any, + allCombos: null, }); + // Tied at 0 usage, order preserved: model-a (first in the list) wins. + assert.deepEqual(primingCalls, ["model-a"]); + + const usageCalls: any[] = []; await handleComboChat({ body: {}, - combo: { - name: "legacy-usage-combo", - strategy: "usage", - models: ["model-a", "model-b"], - }, + combo: comboDef, handleSingleModel: async (_body: any, modelStr: any) => { usageCalls.push(modelStr); return okResponse(); diff --git a/tests/unit/combo-strategies.test.ts b/tests/unit/combo-strategies.test.ts index dc19036722..127a20ab80 100644 --- a/tests/unit/combo-strategies.test.ts +++ b/tests/unit/combo-strategies.test.ts @@ -213,11 +213,16 @@ test("least-used strategy prefers the model with fewer recorded combo requests", models: [busyModel, idleModel], }); - recordComboRequest(name, busyModel, { - success: true, - latencyMs: 10, - strategy: "least-used", - }); + // Prime usage through a real handleComboChat call rather than calling + // recordComboRequest() directly: least-used sorts by the per-target + // executionKey (combo-name + step-id), not by the bare model string + // (#7015/#7059 — sortTargetsByUsage keys byTarget[executionKey] so accounts + // sharing a modelStr don't collapse into one bucket). Recording without a + // `target` falls back to keying by modelStr, which never matches the real + // executionKey and made this assertion flaky against the intended fix. + // With no prior usage, least-used ties at 0 and keeps combo order, so this + // priming call always lands on busyModel (first in the models array). + assert.equal(await selectedModelFor(combo, reqBodyTextArray), busyModel); assert.equal(await selectedModelFor(combo, reqBodyTextArray), idleModel); });