refactor(sse): declare the executor execute() result contract (#8489)

`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<Response>`

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

@@ -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<string, string>;
+      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<ExecutorExecuteResult> {
     const {
       model,
       body,

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<string, string>;
-    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<string, unknown>;

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

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) {

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<T>(stub: ExecuteFn, run: () => Promise<T>): Promise<T> {
+  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");
+});