diff --git a/.coderabbit.yaml b/.coderabbit.yaml index cf61abd910..620c466331 100644 --- a/.coderabbit.yaml +++ b/.coderabbit.yaml @@ -79,6 +79,9 @@ reviews: Require regression coverage at the lowest valid harness with behavior-focused assertions, including relevant negative, error, false/unset, and boundary cases. Check cleanup and deterministic async behavior and prefer shared typed test helpers. + For guards and validators, require tests for both the accept path and the reject path + through every public entry point; a reject-only matrix cannot catch a guard that rejects + good input. Visible webview changes require a durable Playwright component snapshot; behavior-only changes do not. Reject weak assertions on values that could take multiple forms: .toBeDefined() or diff --git a/AGENTS.md b/AGENTS.md index 17249b8688..b1a04cabd2 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -44,6 +44,9 @@ Prefer the narrowest test layer that proves the behavior. This follows standard - Use `apps/vscode-e2e` only when the behavior depends on the real VS Code extension host, VS Code workspace APIs, extension activation, webview/extension messaging, file watcher behavior, or a complete user workflow. - Keep e2e tests focused on high-value smoke coverage across boundaries. Avoid placing detailed protocol, parsing, storage, retry, or edge-case assertions in e2e when they can be covered reliably at a lower layer. - When fixing a regression, add the regression test at the lowest layer that would have failed for the bug. Add an e2e test only if lower-level tests cannot represent the failure mode. +- For a guard or validator, test the accept path and the reject path through every public entry point. A test that only rejects bad input does not prove the guard still accepts good input. +- Use `it.each` for repeated cases and share the setup in a helper. Do not copy a test body per input. +- A `Stryker disable` comment states the reason in one sentence. ESLint `max-len` limits it to 300 characters. ## Task Lifecycle Changes diff --git a/src/api/providers/__tests__/gemini.spec.ts b/src/api/providers/__tests__/gemini.spec.ts index 0920042586..77d31eb9af 100644 --- a/src/api/providers/__tests__/gemini.spec.ts +++ b/src/api/providers/__tests__/gemini.spec.ts @@ -12,6 +12,8 @@ vitest.mock("@roo-code/telemetry", () => ({ import { Anthropic } from "@anthropic-ai/sdk" +import type { GenerateContentResponse } from "@google/genai" + import { type ModelInfo, geminiDefaultModelId, ApiProviderError } from "@roo-code/types" import { t } from "i18next" @@ -21,6 +23,11 @@ import { asyncStreamFrom, collectStream } from "../../../test-utils/stream" const GEMINI_MODEL_NAME = geminiDefaultModelId +// @google/genai's GenerateContentResponse exposes `text` via a getter backed by +// `candidates`, so the stub only carries the field the provider reads; the double +// cast is the least-friction way to satisfy the class type in mocks. +const stubGenerateContentResponse = (text: string) => ({ text }) as unknown as GenerateContentResponse + describe("GeminiHandler", () => { let handler: GeminiHandler let mockGenerateContentStream: ReturnType @@ -430,6 +437,575 @@ describe("GeminiHandler", () => { }) }) + describe("googleGeminiBaseUrl security (CWE-319)", () => { + it("should reject a non-HTTPS non-loopback googleGeminiBaseUrl in completePrompt", async () => { + const insecureHandler = new GeminiHandler({ + apiKey: "test-key", + + apiModelId: GEMINI_MODEL_NAME, + + geminiApiKey: "test-key", + + googleGeminiBaseUrl: "http://gemini.example.test", + }) + + insecureHandler["client"] = handler["client"] + + vi.mocked(handler["client"].models.generateContent).mockResolvedValue( + stubGenerateContentResponse("Response"), + ) + + await expect(insecureHandler.completePrompt("Test prompt")).rejects.toThrow( + t("common:errors.gemini.generate_complete_prompt", { + error: "Google Gemini base URL must use HTTPS (or a loopback HTTP endpoint for local test proxies)", + }), + ) + + expect(handler["client"].models.generateContent).not.toHaveBeenCalled() + }) + + it("should allow a loopback HTTP googleGeminiBaseUrl in completePrompt", async () => { + const loopbackHandler = new GeminiHandler({ + apiKey: "test-key", + + apiModelId: GEMINI_MODEL_NAME, + + geminiApiKey: "test-key", + + googleGeminiBaseUrl: "http://127.0.0.1:8080", + }) + + loopbackHandler["client"] = handler["client"] + + vi.mocked(handler["client"].models.generateContent).mockResolvedValue( + stubGenerateContentResponse("Response"), + ) + + const result = await loopbackHandler.completePrompt("Test prompt") + + expect(result).toBe("Response") + + expect(handler["client"].models.generateContent).toHaveBeenCalledWith( + expect.objectContaining({ + config: expect.objectContaining({ + httpOptions: { + baseUrl: "http://127.0.0.1:8080", + }, + }), + }), + ) + }) + + it("should reject a non-HTTPS non-loopback googleGeminiBaseUrl in createMessage", async () => { + const messages: Anthropic.Messages.MessageParam[] = [ + { + role: "user", + + content: "Hello", + }, + ] + + const stub = vi.fn().mockReturnValue((async function* () {})()) + + const insecureHandler = new GeminiHandler({ + apiKey: "test-key", + + apiModelId: GEMINI_MODEL_NAME, + + geminiApiKey: "test-key", + + googleGeminiBaseUrl: "http://insecure.example.com", + }) + + insecureHandler["client"] = handler["client"] + + handler["client"].models.generateContentStream = stub + + const stream = insecureHandler.createMessage("You are a helpful assistant", messages) + + const error = await collectStream(stream).catch((e: unknown) => e) + + expect(error).toBeInstanceOf(ApiProviderError) + + expect((error as ApiProviderError).message).toBe( + "Google Gemini base URL must use HTTPS (or a loopback HTTP endpoint for local test proxies)", + ) + + expect((error as ApiProviderError).provider).toBe("Gemini") + + expect((error as ApiProviderError).operation).toBe("createMessage") + + expect(stub).not.toHaveBeenCalled() + }) + + it("should allow a loopback HTTP googleGeminiBaseUrl in createMessage", async () => { + const messages: Anthropic.Messages.MessageParam[] = [ + { + role: "user", + + content: "Hello", + }, + ] + + const stub = vi.fn().mockReturnValue((async function* () {})()) + + const loopbackHandler = new GeminiHandler({ + apiKey: "test-key", + + apiModelId: GEMINI_MODEL_NAME, + + geminiApiKey: "test-key", + + googleGeminiBaseUrl: "http://127.0.0.1:8080", + }) + + loopbackHandler["client"] = handler["client"] + + handler["client"].models.generateContentStream = stub + + await collectStream(loopbackHandler.createMessage("You are a helpful assistant", messages)) + + const config = stub.mock.calls[0][0].config + + expect(config.httpOptions).toEqual({ baseUrl: "http://127.0.0.1:8080" }) + }) + + it("should allow an http://localhost googleGeminiBaseUrl in createMessage", async () => { + const messages: Anthropic.Messages.MessageParam[] = [ + { + role: "user", + + content: "Hello", + }, + ] + + const stub = vi.fn().mockReturnValue((async function* () {})()) + + const localhostHandler = new GeminiHandler({ + apiKey: "test-key", + + apiModelId: GEMINI_MODEL_NAME, + + geminiApiKey: "test-key", + + googleGeminiBaseUrl: "http://localhost:8080", + }) + + localhostHandler["client"] = handler["client"] + + handler["client"].models.generateContentStream = stub + + await collectStream(localhostHandler.createMessage("You are a helpful assistant", messages)) + + expect(stub.mock.calls[0][0].config.httpOptions).toEqual({ baseUrl: "http://localhost:8080" }) + }) + + it("should allow an http://[::1] googleGeminiBaseUrl (IPv6 loopback host)", async () => { + const messages: Anthropic.Messages.MessageParam[] = [ + { + role: "user", + + content: "Hello", + }, + ] + + const stub = vi.fn().mockReturnValue((async function* () {})()) + + const ipv6Handler = new GeminiHandler({ + apiKey: "test-key", + + apiModelId: GEMINI_MODEL_NAME, + + geminiApiKey: "test-key", + + googleGeminiBaseUrl: "http://[::1]:8080", + }) + + ipv6Handler["client"] = handler["client"] + + handler["client"].models.generateContentStream = stub + + await collectStream(ipv6Handler.createMessage("You are a helpful assistant", messages)) + + expect(stub.mock.calls[0][0].config.httpOptions).toEqual({ baseUrl: "http://[::1]:8080" }) + }) + + it("should reject hostnames that only resemble the 127. range", async () => { + // "127a.b" is not a four-part 127.0.0.0/8 literal, so it must not + + // pass the loopback check. A prefix-style "starts with 127." test + + // would have matched it and wrongly allowed cleartext. (Hosts such + + // as a127.0.0.1 are rejected by new URL() outright — its mixed + + // digit-led/letter-led label rule — so they never reach the check.) + + const messages: Anthropic.Messages.MessageParam[] = [ + { + role: "user", + + content: "Hello", + }, + ] + + const stub = vi.fn().mockReturnValue((async function* () {})()) + + const restrictedHandler = new GeminiHandler({ + apiKey: "test-key", + + apiModelId: GEMINI_MODEL_NAME, + + geminiApiKey: "test-key", + + googleGeminiBaseUrl: "http://127a.b:8080", + }) + + restrictedHandler["client"] = handler["client"] + + handler["client"].models.generateContentStream = stub + + const error = await collectStream( + restrictedHandler.createMessage("You are a helpful assistant", messages), + ).catch((e: unknown) => e) + + expect(error).toBeInstanceOf(ApiProviderError) + + expect((error as ApiProviderError).message).toBe( + "Google Gemini base URL must use HTTPS (or a loopback HTTP endpoint for local test proxies)", + ) + + expect((error as ApiProviderError).provider).toBe("Gemini") + + expect(stub).not.toHaveBeenCalled() + }) + + it("should reject a valid hostname containing the 127. substring that is not loopback", async () => { + // "foo127.bar" is accepted by new URL() (a syntactically valid + + // hostname) and contains the "127." substring, yet is not a + + // loopback address: its first label is "foo127", not the literal + + // 127 octet, so the four-part 127.0.0.0/8 check rejects it. + + const messages: Anthropic.Messages.MessageParam[] = [ + { + role: "user", + + content: "Hello", + }, + ] + + const stub = vi.fn().mockReturnValue((async function* () {})()) + + const restrictedHandler = new GeminiHandler({ + apiKey: "test-key", + + apiModelId: GEMINI_MODEL_NAME, + + geminiApiKey: "test-key", + + googleGeminiBaseUrl: "http://foo127.bar:8080", + }) + + restrictedHandler["client"] = handler["client"] + + handler["client"].models.generateContentStream = stub + + const error = await collectStream( + restrictedHandler.createMessage("You are a helpful assistant", messages), + ).catch((e: unknown) => e) + + expect(error).toBeInstanceOf(ApiProviderError) + + expect((error as ApiProviderError).message).toBe( + "Google Gemini base URL must use HTTPS (or a loopback HTTP endpoint for local test proxies)", + ) + + expect((error as ApiProviderError).provider).toBe("Gemini") + + expect(stub).not.toHaveBeenCalled() + }) + + const INSECURE_BASE_URL_CASES = [ + // "127.example.test" is a syntactically valid hostname (a digit-led + + // first label is legal DNS) that new URL() accepts, but it is a + + // public domain — not a literal 127.0.0.0/8 address — so cleartext + + // must be rejected. A "starts with 127." prefix check would wrongly + + // allow it and leak the API key in cleartext. + + ["a public hostname whose first label is 127", "http://127.example.test:8080"], + + // "127.0.0.a" is accepted by new URL() (a non-digit last label + + // skips IPv4 validation), but the last label is not a decimal + + // octet, so the host is not loopback and cleartext must be + + // rejected. + + ["a four-part host with a non-numeric last octet", "http://127.0.0.a:8080"], + + // "127.0.a.b" is accepted by new URL(), but its third label is + + // not a decimal octet, so the host is not loopback and cleartext + + // must be rejected even though the first and fourth labels are + + // numeric. + + ["a four-part host with a non-numeric middle octet", "http://127.0.a.b:8080"], + + // "127.0.0.1.a" is a hostname (not an IPv4 literal) that new + + // URL() accepts: its five labels mean it must not pass the + + // four-part 127.0.0.0/8 check, so cleartext must be rejected. + + ["a five-part host that starts with a loopback address", "http://127.0.0.1.a:8080"], + + // "10.0.0.1" is a valid IPv4 literal that new URL() accepts, but its + + // first octet is not 127, so it is not loopback and cleartext must + + // be rejected. This pins the 127 label comparison itself. + + ["a four-part IPv4 host outside the 127.0.0.0/8 range", "http://10.0.0.1:8080"], + + // The parsed hostname is evil.example.test: "127.0.0.1@" is userinfo. + ["a loopback address used only as userinfo", "http://127.0.0.1@evil.example.test:8080"], + + // new URL() normalizes this to [::ffff:7f00:1], which is not [::1]. + ["an IPv4-mapped IPv6 loopback address", "http://[::ffff:127.0.0.1]:8080"], + ] as const + + it.each(INSECURE_BASE_URL_CASES)("should reject %s in completePrompt", async (_name, googleGeminiBaseUrl) => { + const restrictedHandler = new GeminiHandler({ + apiKey: "test-key", + apiModelId: GEMINI_MODEL_NAME, + geminiApiKey: "test-key", + googleGeminiBaseUrl, + }) + restrictedHandler["client"] = handler["client"] + + await expect(restrictedHandler.completePrompt("Test prompt")).rejects.toThrow( + t("common:errors.gemini.generate_complete_prompt", { + error: "Google Gemini base URL must use HTTPS (or a loopback HTTP endpoint for local test proxies)", + }), + ) + + expect(handler["client"].models.generateContent).not.toHaveBeenCalled() + }) + + it("should allow an HTTPS googleGeminiBaseUrl in createMessage", async () => { + const stub = vi.fn().mockReturnValue((async function* () {})()) + const httpsHandler = new GeminiHandler({ + apiKey: "test-key", + apiModelId: GEMINI_MODEL_NAME, + geminiApiKey: "test-key", + googleGeminiBaseUrl: "https://gemini.example.test", + }) + httpsHandler["client"] = handler["client"] + handler["client"].models.generateContentStream = stub + + await collectStream( + httpsHandler.createMessage("You are a helpful assistant", [{ role: "user", content: "Hello" }]), + ) + + expect(stub.mock.calls[0][0].config.httpOptions).toEqual({ baseUrl: "https://gemini.example.test" }) + }) + + it("should allow an HTTPS googleGeminiBaseUrl in completePrompt", async () => { + const httpsHandler = new GeminiHandler({ + apiKey: "test-key", + apiModelId: GEMINI_MODEL_NAME, + geminiApiKey: "test-key", + googleGeminiBaseUrl: "https://gemini.example.test", + }) + httpsHandler["client"] = handler["client"] + vi.mocked(handler["client"].models.generateContent).mockResolvedValue( + stubGenerateContentResponse("Response"), + ) + + await expect(httpsHandler.completePrompt("Test prompt")).resolves.toBe("Response") + + expect(handler["client"].models.generateContent).toHaveBeenCalledWith( + expect.objectContaining({ + config: expect.objectContaining({ httpOptions: { baseUrl: "https://gemini.example.test" } }), + }), + ) + }) + + it.each(INSECURE_BASE_URL_CASES)("should reject %s", async (_name, googleGeminiBaseUrl) => { + const messages: Anthropic.Messages.MessageParam[] = [ + { + role: "user", + + content: "Hello", + }, + ] + + const stub = vi.fn().mockReturnValue((async function* () {})()) + + const restrictedHandler = new GeminiHandler({ + apiKey: "test-key", + + apiModelId: GEMINI_MODEL_NAME, + + geminiApiKey: "test-key", + + googleGeminiBaseUrl, + }) + + restrictedHandler["client"] = handler["client"] + + handler["client"].models.generateContentStream = stub + + const error = await collectStream( + restrictedHandler.createMessage("You are a helpful assistant", messages), + ).catch((e: unknown) => e) + + expect(error).toBeInstanceOf(ApiProviderError) + + expect((error as ApiProviderError).message).toBe( + "Google Gemini base URL must use HTTPS (or a loopback HTTP endpoint for local test proxies)", + ) + + expect((error as ApiProviderError).provider).toBe("Gemini") + + expect(stub).not.toHaveBeenCalled() + }) + + it("should allow the full 127.0.0.0/8 range including 127.255.255.255", async () => { + // The octet boundary is inclusive: 255 is the largest valid decimal + + // octet and must be accepted as loopback. A < instead of <= (or a + + // lowered boundary) would wrongly reject valid loopback endpoints. + + const messages: Anthropic.Messages.MessageParam[] = [ + { + role: "user", + + content: "Hello", + }, + ] + + const stub = vi.fn().mockReturnValue((async function* () {})()) + + const loopbackHandler = new GeminiHandler({ + apiKey: "test-key", + + apiModelId: GEMINI_MODEL_NAME, + + geminiApiKey: "test-key", + + googleGeminiBaseUrl: "http://127.255.255.255:8080", + }) + + loopbackHandler["client"] = handler["client"] + + handler["client"].models.generateContentStream = stub + + await collectStream(loopbackHandler.createMessage("You are a helpful assistant", messages)) + + const config = stub.mock.calls[0][0].config + + expect(config.httpOptions).toEqual({ baseUrl: "http://127.255.255.255:8080" }) + }) + + it("should reject a non-HTTP loopback scheme such as ftp://localhost", async () => { + // The protocol operand (left of the && in the loopback exception) must + + // still be enforced: a non-HTTP scheme aimed at a loopback host is not + + // a local test proxy and must be rejected like any other non-HTTPS URL. + + const messages: Anthropic.Messages.MessageParam[] = [ + { + role: "user", + + content: "Hello", + }, + ] + + const stub = vi.fn().mockReturnValue((async function* () {})()) + + const ftpHandler = new GeminiHandler({ + apiKey: "test-key", + + apiModelId: GEMINI_MODEL_NAME, + + geminiApiKey: "test-key", + + googleGeminiBaseUrl: "ftp://localhost:8080", + }) + + ftpHandler["client"] = handler["client"] + + handler["client"].models.generateContentStream = stub + + const error = await collectStream(ftpHandler.createMessage("You are a helpful assistant", messages)).catch( + (e: unknown) => e, + ) + + expect(error).toBeInstanceOf(ApiProviderError) + + expect((error as ApiProviderError).message).toBe( + "Google Gemini base URL must use HTTPS (or a loopback HTTP endpoint for local test proxies)", + ) + + expect((error as ApiProviderError).provider).toBe("Gemini") + + expect(stub).not.toHaveBeenCalled() + }) + + it("should reject an invalid googleGeminiBaseUrl in createMessage", async () => { + const messages: Anthropic.Messages.MessageParam[] = [ + { + role: "user", + + content: "Hello", + }, + ] + + const stub = vi.fn().mockReturnValue((async function* () {})()) + + const invalidHandler = new GeminiHandler({ + apiKey: "test-key", + + apiModelId: GEMINI_MODEL_NAME, + + geminiApiKey: "test-key", + + googleGeminiBaseUrl: "not a valid url", + }) + + invalidHandler["client"] = handler["client"] + + handler["client"].models.generateContentStream = stub + + const error = await collectStream( + invalidHandler.createMessage("You are a helpful assistant", messages), + ).catch((e: unknown) => e) + + expect(error).toBeInstanceOf(ApiProviderError) + + expect((error as ApiProviderError).message).toBe("Invalid Google Gemini base URL (not a valid URL)") + + expect((error as ApiProviderError).provider).toBe("Gemini") + + expect((error as ApiProviderError).operation).toBe("createMessage") + + expect(stub).not.toHaveBeenCalled() + }) + }) + describe("getModel", () => { it("should return correct model info", () => { const modelInfo = handler.getModel() diff --git a/src/api/providers/gemini.ts b/src/api/providers/gemini.ts index 7674521d13..da1f9186e3 100644 --- a/src/api/providers/gemini.ts +++ b/src/api/providers/gemini.ts @@ -172,6 +172,59 @@ function sanitizeSchemaForGemini( return result } +// googleGeminiBaseUrl is user-editable and can reach non-HTTPS values (settings, +// imported profiles). The @google/genai client keeps API-key authentication for +// custom endpoints, so reject cleartext base URLs before any request — with a +// narrow loopback exception for local test proxies. +// A decimal octet is a 1-3 digit string; a non-numeric part also fails the +// range check, because Number() of a non-numeric string is NaN, which is not +// <= 255. +function isOctet(octet: string): boolean { + // Stryker disable next-line Regex,LogicalOperator,ConditionalExpression: equivalent mutants, new URL() rejects malformed or out-of-range octets before this check + return /^\d{1,3}$/.test(octet) && Number(octet) <= 255 +} + +// Only literal IPv4 loopback (127.0.0.0/8) qualifies: public hostnames may +// start with a "127." label (e.g. 127.example.test), which a prefix test would +// misclassify as loopback and allow cleartext. assertSecureGeminiBaseUrl has +// already parsed the URL and only reaches this for the HTTP exception, so the +// parsed hostname is passed directly; `new URL` keeps the brackets in IPv6 +// hostnames, so `[::1]` is the loopback host form to compare against. +function isLoopbackHostname(hostname: string): boolean { + if (hostname === "localhost" || hostname === "[::1]") { + return true + } + const parts = hostname.split(".") + // Stryker disable next-line ConditionalExpression: equivalent mutants, new URL() rejects non-numeric or out-of-range middle octets before this check + return parts.length === 4 && parts[0] === "127" && isOctet(parts[1]) && isOctet(parts[2]) && isOctet(parts[3]) +} + +// Throws an ApiProviderError when baseUrl is not HTTPS (loopback HTTP is the +// narrow exception, for local test proxies). The provider/model/operation +// arguments keep the structured error context consistent with the request-path +// ApiProviderError instances in this file. +function assertSecureGeminiBaseUrl(baseUrl: string, modelId: string, operation: string): void { + let parsed: URL + try { + parsed = new URL(baseUrl) + } catch { + throw new ApiProviderError("Invalid Google Gemini base URL (not a valid URL)", "Gemini", modelId, operation) + } + if (parsed.protocol === "https:") { + return + } + if (parsed.protocol === "http:" && isLoopbackHostname(parsed.hostname)) { + // Loopback endpoints (localhost/127.x/::1) are allowed for local test proxies. + return + } + throw new ApiProviderError( + "Google Gemini base URL must use HTTPS (or a loopback HTTP endpoint for local test proxies)", + "Gemini", + modelId, + operation, + ) +} + export class GeminiHandler extends BaseProvider implements SingleCompletionHandler { protected options: ApiHandlerOptions @@ -296,6 +349,12 @@ export class GeminiHandler extends BaseProvider implements SingleCompletionHandl ? (this.options.modelTemperature ?? info.defaultTemperature ?? 1) : info.defaultTemperature + // Reject cleartext (non-loopback) base URLs before building the request so the + // API key is never sent over an insecure endpoint. + if (this.options.googleGeminiBaseUrl) { + assertSecureGeminiBaseUrl(this.options.googleGeminiBaseUrl, model, "createMessage") + } + const config: GenerateContentConfig = { systemInstruction, httpOptions: this.options.googleGeminiBaseUrl ? { baseUrl: this.options.googleGeminiBaseUrl } : undefined, @@ -590,6 +649,13 @@ export class GeminiHandler extends BaseProvider implements SingleCompletionHandl ? (this.options.modelTemperature ?? info.defaultTemperature ?? 1) : info.defaultTemperature + // Reject cleartext (non-loopback) base URLs before building the request so the + // API key is never sent over an insecure endpoint. + if (this.options.googleGeminiBaseUrl) { + // Stryker disable next-line StringLiteral: completePrompt's catch reads only .message from the thrown error before building its own telemetry error, so this operation argument is unobservable + assertSecureGeminiBaseUrl(this.options.googleGeminiBaseUrl, model, "completePrompt") + } + const promptConfig: GenerateContentConfig = { httpOptions: this.options.googleGeminiBaseUrl ? { baseUrl: this.options.googleGeminiBaseUrl } diff --git a/src/api/providers/openai-codex.ts b/src/api/providers/openai-codex.ts index 8f49ad1337..68a6ff95a5 100644 --- a/src/api/providers/openai-codex.ts +++ b/src/api/providers/openai-codex.ts @@ -273,7 +273,7 @@ export class OpenAiCodexHandler extends BaseProvider implements SingleCompletion } catch (error) { // An abort landing while the lookup is pending must surface as the shared abort // contract, not as an authentication failure. - // Stryker disable next-line ConditionalExpression: the false mutant differs from the original only if an abort lands between the OAuth race's settle and this catch — a microtask window (the abort event is a microtask) no test can schedule deterministically, because the test's own microtasks queue after the settle that precedes the window; the observable contract (a genuine failure with no cancellation propagates as-is, an abort surfaces as the shared contract) is pinned by the adjacent regressions and the true mutant is killed by the non-abort-failure regression. + // Stryker disable next-line ConditionalExpression: the false mutant needs an abort inside a microtask window no test can schedule; the non-abort-failure regression kills the true mutant if (isRequestAborted(error, abortSignal)) { throw createAbortError(this.providerName) } @@ -1457,7 +1457,7 @@ export class OpenAiCodexHandler extends BaseProvider implements SingleCompletion // A buffered chunk can still be pulled in the window between the abort and the // inner generator's own stop, so break here: post-abort output must never be // joined into the completion. - // Stryker disable next-line ConditionalExpression: the false mutant differs from the original only if an abort lands between the transport's last pre-yield check and this consumer's resumption — the yield-hop microtask window no test can schedule deterministically (the abort event is a microtask; the test's own microtasks queue after the chunk's yield); the guard is retained for that production window, the abort regressions pin its pre-abort contract, and the true mutant is killed by the multi-chunk happy paths. + // Stryker disable next-line ConditionalExpression: the false mutant needs an abort inside the yield-hop microtask window no test can schedule; the multi-chunk happy paths kill the true mutant if (requestSignal?.aborted) { break } diff --git a/src/core/task/Task.ts b/src/core/task/Task.ts index b84a449c1d..d8a06faf6c 100644 --- a/src/core/task/Task.ts +++ b/src/core/task/Task.ts @@ -4173,7 +4173,7 @@ export class Task extends EventEmitter implements TaskLike { // under the canonical name slip past a name comparison. The first call wins. const idAlreadyUsed = this.assistantMessageContent.some( (entry) => - // Stryker disable next-line ConditionalExpression: only tool_use and mcp_tool_use entries ever carry an id — the text block pushed in the text case has no id field and the start events reaching this guard always carry the string id locked in by processRawChunk — so the type check cannot change the outcome of entry.id === event.id for any entry this pipeline produces; the false replacement is additionally killed by the rejection tests. + // Stryker disable next-line ConditionalExpression: equivalent mutant, only tool_use and mcp_tool_use entries carry an id, so the type check cannot change entry.id === event.id (entry.type === "tool_use" || entry.type === "mcp_tool_use") && (entry as { id?: string }).id === event.id, ) @@ -4226,7 +4226,7 @@ export class Task extends EventEmitter implements TaskLike { // keep the deprecated single-entry-per-ID lookup. let streamingKey: string | undefined - // Stryker disable next-line ConditionalExpression: event.name is always a string on this path — the only delta events reaching this handler are emitted by processRawChunk with the tracked start name (never undefined), so the legacy nameless branch below is unreachable defensive code and the conditional always takes the named side. + // Stryker disable next-line ConditionalExpression: equivalent mutant, event.name is always a string here, so the legacy nameless branch is unreachable if (event.name !== undefined) { streamingKey = NativeToolCallParser.makeStreamingKey(event.id, event.name) } else { @@ -4243,7 +4243,7 @@ export class Task extends EventEmitter implements TaskLike { } const partialToolUse = - // Stryker disable next-line ConditionalExpression: streamingKey can never be undefined here — makeStreamingKey always returns a string and the legacy lookup branch above is unreachable (see the directive on the event.name check) — so the null side is unreachable defensive code. + // Stryker disable next-line ConditionalExpression: equivalent mutant, streamingKey is always a string here (makeStreamingKey), so the null side is unreachable streamingKey === undefined ? null : NativeToolCallParser.processStreamingChunk( @@ -4252,7 +4252,7 @@ export class Task extends EventEmitter implements TaskLike { nativeToolCallParserScope, ) - // Stryker disable next-line ConditionalExpression, EqualityOperator: streamingKey is always a defined string on this path (makeStreamingKey never returns undefined; the legacy branch is unreachable per the directives above), and processStreamingChunk returns a partial ToolUse for a known key, so both defensive conditions reduce to the partialToolUse check and cannot change the observed behavior. + // Stryker disable next-line ConditionalExpression, EqualityOperator: equivalent mutants, streamingKey is always defined here, so both conditions reduce to the partialToolUse check if (partialToolUse && streamingKey !== undefined) { // Reuse the compound key built above: it is exactly the key the start event // registered under, so the delta lookup needs no second encoding. @@ -4643,7 +4643,7 @@ export class Task extends EventEmitter implements TaskLike { if (event.type === "tool_call_end") { // Create compound key for deduplication (same pattern as streaming handler). // End events carry the tool name; fall back to the id for events that don't. - // Stryker disable next-line LogicalOperator: event.name is always a string on this path — finalizeRawChunks emits end events with the tracked start name (a string, set before the tracker can start), so the ?? event.id fallback guards a nameless end event this pipeline cannot produce. + // Stryker disable next-line LogicalOperator: equivalent mutant, finalizeRawChunks emits end events with a string name, so the ?? event.id fallback is unreachable let eventName = event.name ?? event.id let dedupKey = NativeToolCallParser.makeStreamingKey(event.id, eventName) @@ -4660,7 +4660,7 @@ export class Task extends EventEmitter implements TaskLike { // name (provider quirk), the compound-key lookup above misses and both the // parser entry and the dedup tracking would stay stale. Resolve the tracked // entry by id and target the real compound key for finalization and cleanup. - // Stryker disable next-line ConditionalExpression, LogicalOperator, EqualityOperator: the both-miss case is unreachable via the raw-chunk pipeline — end events carry the locked start name so the compound key resolves the registered entry, and a same-ID rejected-start end is always processed after the accepted start's end has already removed that entry; this defensive path protects against a cross-name end event the pipeline cannot produce. + // Stryker disable next-line ConditionalExpression, LogicalOperator, EqualityOperator: equivalent mutants, the both-miss case is unreachable via the raw-chunk pipeline if (finalToolUse === null && toolUseIndex === undefined) { const resolved = NativeToolCallParser.getStreamingToolCallById( event.id, diff --git a/src/eslint.config.mjs b/src/eslint.config.mjs index 5cbaf1b795..820e4e17c8 100644 --- a/src/eslint.config.mjs +++ b/src/eslint.config.mjs @@ -22,6 +22,13 @@ export default [ "@typescript-eslint/ban-ts-comment": "off", }, }, + { + // A Stryker directive gives a short reason. Put longer rationale in the test or a code comment. + // ignorePattern skips every line that does not contain "Stryker", so only directives are measured. + rules: { + "max-len": ["error", { code: 300, ignorePattern: "^(?!.*Stryker).*" }], + }, + }, { files: ["core/assistant-message/presentAssistantMessage.ts", "core/webview/webviewMessageHandler.ts"], rules: { diff --git a/src/utils/pathUtils.ts b/src/utils/pathUtils.ts index c433d4737d..4eff5791ac 100644 --- a/src/utils/pathUtils.ts +++ b/src/utils/pathUtils.ts @@ -85,7 +85,6 @@ async function realPathOfExistingAncestor(filePath: string): Promise