From 5a2031478224da7aa775b700f5b08eb3c626102a Mon Sep 17 00:00:00 2001 From: backryun Date: Sat, 25 Jul 2026 14:53:19 +0900 Subject: [PATCH] refactor(sse): declare the executor execute() result contract (#8489) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `normalizeExecutorResult()` has always accepted `Response | { response, url, headers, transformedBody }` — the bare arm is what the web/scraping executors return from their error and passthrough paths, and `chatcore-upstream-timeouts.test.ts` already covers that both shapes are handled. But `BaseExecutor.execute` has no explicit return type, so TypeScript inferred it from the method's single `return` — the object shape alone. Every override returning a bare `Response` was therefore reported as incompatible: * 14 × TS2739 in `duckduckgo-web.ts`, whose `execute()` additionally pinned its own signature to just the object shape while returning `errorResponse()` / `processResponse()` (both `Response`) from 14 valid paths * TS2416 in `felo-web.ts` and `gitlab.ts`, which declare `Promise` Fix the declaration rather than the call sites: export `ExecutorExecuteResult` from `base.ts` — the same union `normalizeExecutorResult()` accepts — and annotate `BaseExecutor.execute` with it. `duckduckgo-web.ts` then drops its over-narrow annotation, matching BaseExecutor and the ~38 other executors that let the return type be inferred. Two subclasses read `.response` straight off `super.execute()` and now narrow first: * `github.ts` — the existing `!result.response` guard already meant "bare Response, nothing to materialize"; it is now expressed as `result instanceof Response`, which is the same branch for every input (bare / object / nullish) * `pollinations.ts` — reads the status through both arms for its pool bookkeeping Wrapping DuckDuckGo's 14 returns would have been the wrong fix: the values are already correct, and `normalizeExecutorResult()` produces exactly `{ response, url: "", headers: {}, transformedBody: null }` for them. Validation: full tsc error-set diff against the base config — 335 -> 319, **zero new errors** (line-number-agnostic diff is empty; the two `duckduckgo-web.ts` TS2345s that appear to move are the same two pre-existing errors renumbered by added comments, and are left for a later slice). `typecheck:core` clean, `check:type-coverage` 92.17% -> 94.17%, and 49 of the 50 existing test files importing a touched executor pass — `plan3-p0.test.ts` fails identically with and without this change (it reads the developer's real ~/.omniroute DB rather than a test-scoped DATA_DIR). The new test pins the runtime behavior of the narrowing so a later simplification cannot quietly drop the bare-Response arm. --- open-sse/executors/base.ts | 21 ++++- open-sse/executors/duckduckgo-web.ts | 13 +-- open-sse/executors/github.ts | 5 +- open-sse/executors/pollinations.ts | 4 +- .../unit/ts7-executor-result-contract.test.ts | 89 +++++++++++++++++++ 5 files changed, 123 insertions(+), 9 deletions(-) create mode 100644 tests/unit/ts7-executor-result-contract.test.ts diff --git a/open-sse/executors/base.ts b/open-sse/executors/base.ts index 4ad99520db..5680e45ac8 100644 --- a/open-sse/executors/base.ts +++ b/open-sse/executors/base.ts @@ -254,6 +254,25 @@ export function stripVersionedToolModelPrefix(tools: unknown): void { * Implements the Strategy pattern: subclasses override specific methods * (buildUrl, buildHeaders, transformRequest, etc.) for each provider. */ +/** + * What an executor's `execute()` may resolve to. + * + * Both arms are real: the web/scraping executors return a bare `Response` from their + * error and passthrough paths, while the HTTP executors return the richer capture + * object used for upstream request logging. `normalizeExecutorResult()` accepts + * exactly this union and wraps the bare form, so the contract is the union — not the + * object shape that `BaseExecutor.execute` happens to infer from its single return. + */ +export type ExecutorExecuteResult = + | Response + | { + response: Response; + url?: string; + headers?: Record; + transformedBody?: unknown; + transport?: string; + }; + export class BaseExecutor { provider: string; config: ProviderConfig; @@ -582,7 +601,7 @@ export class BaseExecutor { } } - async execute(input: ExecuteInput) { + async execute(input: ExecuteInput): Promise { const { model, body, diff --git a/open-sse/executors/duckduckgo-web.ts b/open-sse/executors/duckduckgo-web.ts index a7b1a0ec0d..bcf23669a0 100644 --- a/open-sse/executors/duckduckgo-web.ts +++ b/open-sse/executors/duckduckgo-web.ts @@ -429,12 +429,13 @@ export class DuckDuckGoWebExecutor extends BaseExecutor { } } - async execute(input: ExecuteInput): Promise<{ - response: Response; - url: string; - headers: Record; - transformedBody: unknown; - }> { + // No explicit return type, matching BaseExecutor and the other ~38 executors: this + // method legitimately returns either a bare `Response` (error paths, processResponse) + // or the richer `{ response, url, headers, transformedBody }` capture object. + // `normalizeExecutorResult()` accepts exactly that union and wraps the bare form, so + // pinning the signature to only the object shape was wrong — it reported 14 valid + // `return` statements as errors. + async execute(input: ExecuteInput) { const { model, body, stream, signal, upstreamExtraHeaders } = input; const upstreamModel = normalizeDuckDuckGoModel(model); const bodyObj = (body || {}) as Record; diff --git a/open-sse/executors/github.ts b/open-sse/executors/github.ts index bbb6bb3641..3045a30aa7 100644 --- a/open-sse/executors/github.ts +++ b/open-sse/executors/github.ts @@ -250,7 +250,10 @@ export class GithubExecutor extends BaseExecutor { async execute(input: ExecuteInput) { const result = await super.execute(input); - if (!result || !result.response) return result; + // BaseExecutor.execute() is typed as the union it contracts for; the bare-Response + // arm has nothing to materialize, which is what the existing `!result.response` + // guard already meant. + if (result instanceof Response || !result?.response) return result; if (!input.stream) { // wreq-js clone/text semantics consume the original response body. Materialize diff --git a/open-sse/executors/pollinations.ts b/open-sse/executors/pollinations.ts index b14144ef64..9619ee9ebf 100644 --- a/open-sse/executors/pollinations.ts +++ b/open-sse/executors/pollinations.ts @@ -79,7 +79,9 @@ export class PollinationsExecutor extends BaseExecutor { const result = await super.execute(input); if (session && pool) { - const status = result.response.status; + // execute() contracts for `Response | { response, ... }`; both arms carry the + // status this pool bookkeeping needs. + const status = (result instanceof Response ? result : result.response).status; if (status === 429) { pool.reportCooldown(session); } else if (status >= 500) { diff --git a/tests/unit/ts7-executor-result-contract.test.ts b/tests/unit/ts7-executor-result-contract.test.ts new file mode 100644 index 0000000000..0c38eccfde --- /dev/null +++ b/tests/unit/ts7-executor-result-contract.test.ts @@ -0,0 +1,89 @@ +/** + * Guards the executor `execute()` result contract. + * + * `normalizeExecutorResult()` has always accepted `Response | { response, ... }` — the + * bare arm is what the web/scraping executors return from their error and passthrough + * paths (see `chatcore-upstream-timeouts.test.ts`, which covers the normalizer itself). + * `BaseExecutor.execute` nonetheless *inferred* only the object shape from its single + * return statement, so every override returning a bare `Response` was reported as + * incompatible (TS2416) and DuckDuckGo's 14 valid `return`s as TS2739. + * + * Declaring `ExecutorExecuteResult` on the base fixed that, and required the two + * subclasses that consume `super.execute()` to narrow before reading `.response`. + * These tests pin the runtime behavior of that narrowing so a future "simplification" + * cannot quietly drop the bare-Response arm. + */ +import test from "node:test"; +import assert from "node:assert/strict"; + +import { BaseExecutor } from "../../open-sse/executors/base.ts"; +import { GithubExecutor } from "../../open-sse/executors/github.ts"; + +type ExecuteFn = typeof BaseExecutor.prototype.execute; + +/** Swap BaseExecutor.execute for the duration of one call, then restore it. */ +async function withBaseExecuteStub(stub: ExecuteFn, run: () => Promise): Promise { + const original = BaseExecutor.prototype.execute; + BaseExecutor.prototype.execute = stub; + try { + return await run(); + } finally { + BaseExecutor.prototype.execute = original; + } +} + +const INPUT = { + model: "gpt-4o", + body: { messages: [{ role: "user", content: "hi" }] }, + stream: false, + credentials: {}, + signal: new AbortController().signal, +}; + +test("GithubExecutor.execute passes a bare Response through untouched", async () => { + const bare = new Response("upstream body", { status: 503 }); + + const result = await withBaseExecuteStub( + (async () => bare) as ExecuteFn, + () => new GithubExecutor().execute(INPUT) + ); + + assert.equal( + result, + bare, + "the bare-Response arm has no capture object to materialize and must be returned as-is" + ); +}); + +test("GithubExecutor.execute still materializes the capture-object arm", async () => { + const captured = { + response: new Response("hello", { status: 200, statusText: "OK" }), + url: "https://api.githubcopilot.com/chat/completions", + headers: { "x-req": "1" }, + transformedBody: { a: 1 }, + }; + + const result = await withBaseExecuteStub( + (async () => captured) as ExecuteFn, + () => new GithubExecutor().execute(INPUT) + ); + + assert.ok(!(result instanceof Response), "the object arm must stay an object"); + const obj = result as typeof captured; + + // The body is re-wrapped into a native Response so downstream reads work after + // wreq-js clone/text semantics have consumed the original. + assert.equal(obj.response.status, 200); + assert.equal(await obj.response.text(), "hello"); + assert.equal(obj.url, captured.url, "the capture fields must survive materialization"); + assert.deepEqual(obj.transformedBody, { a: 1 }); +}); + +test("GithubExecutor.execute tolerates a nullish result without throwing", async () => { + const result = await withBaseExecuteStub( + (async () => undefined) as unknown as ExecuteFn, + () => new GithubExecutor().execute(INPUT) + ); + + assert.equal(result, undefined, "a nullish base result must short-circuit, not throw"); +});