From d861b76b5fc2b00dc4e82f7ae6dab0f6e3c32d0e Mon Sep 17 00:00:00 2001 From: Xiangzhe Date: Sun, 23 Aug 2026 21:52:17 -0300 Subject: [PATCH] fix(tests): drain compression/kiro/memory base-red mini-cluster from 2026-08-24 merges Three stale unit tests went red across PRs #11301/#11303/#11304 because their mocks/fixtures predated intentional contract changes merged the same day: - compression-cli-rest-fallback-6571.test.ts: #10960 moved the CLI's MCP transport from the never-mounted /api/mcp/tools/call to the real Streamable HTTP endpoint /api/mcp/stream. The REST-fallback trigger in this test's fetch mock still targeted the retired path, so mcpCallTool() threw on an unmocked fetch instead of exercising the fallback. Updated the mock to intercept /api/mcp/stream. - memory-system-first-6135.test.ts: #11290/#11303 added a Claude-family reroute to the leading-system-message placement (Opus 5 rejects a system message spliced right after a plain-text assistant turn). The regression test used "anthropic" as its NON-Claude-specific example, which is now classified as Claude-family and no longer exercises the plain cache-safe splice path it targets. Switched the fixture provider to "openai". - kiro-auto-import-name-dedup-3615.test.ts: #10815/#11287 hardened findKiroConnectionByIdentity to require an account-level identifier (email/clientId) alongside a matching profileArn before trusting the match, since distinct Builder ID accounts can share a CodeWhisperer profile ARN. Extended findKiroConnectionByProfileArn (test-only helper) with an optional account-identity param, and added a companion test documenting the new ARN-alone-is-untrusted safety behavior. All three are genuine stale-test drift, not production bugs: the code changes were deliberate fixes from other reviewed PRs; only the tests' fixtures needed to catch up. --- src/app/api/oauth/kiro/auto-import/route.ts | 13 ++++++-- ...compression-cli-rest-fallback-6571.test.ts | 12 ++++++-- .../kiro-auto-import-name-dedup-3615.test.ts | 30 ++++++++++++++++--- tests/unit/memory-system-first-6135.test.ts | 8 ++++- 4 files changed, 53 insertions(+), 10 deletions(-) diff --git a/src/app/api/oauth/kiro/auto-import/route.ts b/src/app/api/oauth/kiro/auto-import/route.ts index c0862f21c5..2b3ead4fd8 100755 --- a/src/app/api/oauth/kiro/auto-import/route.ts +++ b/src/app/api/oauth/kiro/auto-import/route.ts @@ -439,13 +439,22 @@ type ProviderConnectionLike = { * whose stored `providerSpecificData.profileArn` matches the given ARN. * Returns null when profileArn is undefined/null or no match is found. * + * #10815 hardened `findKiroConnectionByIdentity` to require an account-level + * identifier (email or clientId) alongside a matching profileArn before + * trusting the match — distinct Builder ID accounts (Google/GitHub social + * login) can share the same CodeWhisperer profile ARN, and matching on ARN + * alone let a second social login silently overwrite the first connection. + * `email`/`clientId` here let a caller supply that account identifier; the + * real `saveAndRespond()` call sites already do (see below). + * * Exported for unit tests (#3615). */ export function findKiroConnectionByProfileArn( connections: ProviderConnectionLike[], - profileArn: string | undefined + profileArn: string | undefined, + accountIdentity?: { email?: string | null; clientId?: string | null } ): ProviderConnectionLike | null { - return findKiroConnectionByIdentity(connections, { profileArn }); + return findKiroConnectionByIdentity(connections, { profileArn, ...accountIdentity }); } // ── Save to OmniRoute DB ────────────────────────────────────────────────────── diff --git a/tests/unit/compression-cli-rest-fallback-6571.test.ts b/tests/unit/compression-cli-rest-fallback-6571.test.ts index 2816cce87a..a22d0d102a 100644 --- a/tests/unit/compression-cli-rest-fallback-6571.test.ts +++ b/tests/unit/compression-cli-rest-fallback-6571.test.ts @@ -2,9 +2,15 @@ import test from "node:test"; import assert from "node:assert/strict"; // Repro for #6571 — REST-fallback path of `omniroute compression` (hit only when -// /api/mcp/tools/call is not mounted, i.e. mcpCall()'s 404/501 branch) uses the +// the MCP surface is not mounted, i.e. mcpCall()'s 404/501 branch) uses the // nonexistent `engine` field instead of the canonical `defaultMode` field, and // the table renderer prints "[object Object]" for nested object cells. +// +// #10960 moved the MCP transport from the never-mounted `/api/mcp/tools/call` +// to the real Streamable HTTP endpoint `/api/mcp/stream` (mcpClient.mjs -> +// callMcpEndpoint()). The REST-fallback trigger in these mocks must match +// that endpoint, not the retired one, or mcpCallTool() throws on an +// unmocked fetch instead of exercising the fallback path this test targets. type MockResponse = Pick; @@ -45,7 +51,7 @@ test("restCompressionStatus (via runCompressionStatus REST fallback) should surf const origFetch = globalThis.fetch; globalThis.fetch = (async (url: string | URL | Request) => { const u = String(url); - if (u.includes("/api/mcp/tools/call")) return makeResp({ error: "not mounted" }, 404); + if (u.includes("/api/mcp/stream")) return makeResp({ error: "not mounted" }, 404); if (u.includes("/api/settings/compression")) { // Canonical server payload — NOTE: field is `defaultMode`, there is no `engine` key. // src/lib/db/compression.ts COMPRESSION_MODES / GET route just returns getCompressionSettings(). @@ -85,7 +91,7 @@ test("restSetEngine (via runCompressionEngineSet REST fallback) should PUT `defa const putBodies: Record[] = []; globalThis.fetch = (async (url: string | URL | Request, init?: RequestInit) => { const u = String(url); - if (u.includes("/api/mcp/tools/call")) return makeResp({ error: "not mounted" }, 404); + if (u.includes("/api/mcp/stream")) return makeResp({ error: "not mounted" }, 404); if (u.includes("/api/settings/compression") && init?.method === "PUT") { const body = init?.body ? JSON.parse(String(init.body)) : {}; putBodies.push(body); diff --git a/tests/unit/kiro-auto-import-name-dedup-3615.test.ts b/tests/unit/kiro-auto-import-name-dedup-3615.test.ts index f354514acc..748312a316 100644 --- a/tests/unit/kiro-auto-import-name-dedup-3615.test.ts +++ b/tests/unit/kiro-auto-import-name-dedup-3615.test.ts @@ -113,12 +113,18 @@ test("derived name is never empty or null", () => { const FAKE_PROFILE_ARN = "arn:aws:iam::123456789012:user/sso-user"; +const FAKE_CLIENT_ID = "client-abc"; + const fakeConnectionWithArn = { id: "conn-abc", provider: "kiro", authType: "oauth", email: null, - providerSpecificData: { profileArn: FAKE_PROFILE_ARN, region: "us-east-1" }, + providerSpecificData: { + profileArn: FAKE_PROFILE_ARN, + region: "us-east-1", + clientId: FAKE_CLIENT_ID, + }, }; const fakeConnectionNoArn = { @@ -129,13 +135,29 @@ const fakeConnectionNoArn = { providerSpecificData: { region: "us-east-1" }, }; -test("findKiroConnectionByProfileArn returns the matching connection", async () => { - // The function should scan existing kiro connections and match by profileArn. +test("findKiroConnectionByProfileArn returns the matching connection when an account identifier agrees", async () => { + // #10815 — matching on profileArn alone is unsafe (distinct Builder ID + // accounts can share a profile ARN), so the caller must also supply an + // account-level identifier (email or clientId) that does not contradict + // the stored connection, exactly like saveAndRespond()'s real call sites do. + const result = await findKiroConnectionByProfileArn( + [fakeConnectionWithArn, fakeConnectionNoArn], + FAKE_PROFILE_ARN, + { clientId: FAKE_CLIENT_ID } + ); + assert.deepEqual(result, fakeConnectionWithArn); +}); + +test("findKiroConnectionByProfileArn returns null for a profileArn-only match with no account identifier (#10815)", async () => { + // Guards the #10815 fix: two different Builder ID accounts (Google/GitHub + // social login) can share the same CodeWhisperer profile ARN, so trusting + // an ARN match without any account identifier would let a second social + // login silently overwrite the first connection. const result = await findKiroConnectionByProfileArn( [fakeConnectionWithArn, fakeConnectionNoArn], FAKE_PROFILE_ARN ); - assert.deepEqual(result, fakeConnectionWithArn); + assert.equal(result, null); }); test("findKiroConnectionByProfileArn returns null when no match exists", async () => { diff --git a/tests/unit/memory-system-first-6135.test.ts b/tests/unit/memory-system-first-6135.test.ts index 339f83792c..60a99eb315 100644 --- a/tests/unit/memory-system-first-6135.test.ts +++ b/tests/unit/memory-system-first-6135.test.ts @@ -111,7 +111,13 @@ describe("injectMemory system-must-be-first (#6135)", () => { it("regression: a NON-flagged provider keeps the existing cache-safe placement", () => { const req = multiTurn(); - const out = injectMemory(req, [mem("dark mode")], "anthropic", { cacheSafe: true }); + // #11290/#11303 added a Claude-family-specific reroute to injectSystemFirst() + // for the mid-array splice (a system message right after a plain-text + // assistant turn is rejected by Claude Opus 5), so "anthropic" no longer + // exercises the plain cache-safe splice path this test targets. Use a + // provider outside both the strict-system-first set AND the Claude family + // to keep testing the original (still-current) cache-safe behavior. + const out = injectMemory(req, [mem("dark mode")], "openai", { cacheSafe: true }); // Existing behavior: memory inserted just before the last user message (index 3). assert.equal(out.messages[3].role, "system"); assert.ok(out.messages[3].content.includes("Memory context"));