refactor(sse): declare the executor execute() result contract by backryun · Pull Request #8489 · diegosouzapw/OmniRoute

backryun · 2026-07-24T22:45:19Z

Part of #8484. Type-only; no toolchain change.

What was wrong

normalizeExecutorResult() accepts Response | { response, url, headers, transformedBody } and wraps the bare arm — that union is the real contract, it is written into the normalizer's parameter type, and chatcore-upstream-timeouts.test.ts already asserts both shapes work.

BaseExecutor.execute has no explicit return type, so TypeScript inferred it from the method's single return statement — the object shape alone. Every override returning a bare Response was therefore rejected:

where	count	error
`duckduckgo-web.ts` — 14 valid returns of `errorResponse()` / `processResponse()`	14	`TS2739`
`felo-web.ts`, `gitlab.ts` — declare `Promise<Response>`	2	`TS2416`

duckduckgo-web.ts compounded it by pinning its own execute() signature to just the object shape while returning Response from those 14 paths.

What this does

Fixes the declaration, not the call sites. base.ts exports ExecutorExecuteResult — the same union the normalizer already accepts — and annotates BaseExecutor.execute with it. duckduckgo-web.ts then drops its over-narrow annotation, matching BaseExecutor and the ~38 other executors that leave the return type inferred (only ~6 declare one, and two of those are the TS2416s above).

Two subclasses read .response straight off super.execute() and now narrow first:

github.ts — its existing !result.response guard already meant "bare Response, nothing to materialize". That intent is now expressed as result instanceof Response. Same branch taken for every input shape (bare / object / nullish).
pollinations.ts — reads the status through both arms for its session-pool bookkeeping.

Wrapping DuckDuckGo's 14 returns would have been the wrong fix. Those values are already correct, and normalizeExecutorResult() produces exactly { response, url: "", headers: {}, transformedBody: null } for them — wrapping by hand would duplicate the normalizer and churn 14 call sites to satisfy a declaration that was itself wrong.

Validation

Full tsc error-set diff against the base config (base run with ignoreDeprecations: "6.0" so compilation gets past TS5101):

	diagnostics
base	335
this PR	319
new errors	0

Two duckduckgo-web.ts TS2345s appear in a naive line-based diff — they are the same two pre-existing errors renumbered by the comments this PR adds. A line-number-agnostic diff of the two error sets is empty. They are unrelated root causes (an argument type and a Buffer/BodyInit mismatch) and are left for a later slice rather than padding this one.

gate	result
`npm run typecheck:core`	clean
`npm run check:type-coverage`	pass — 92.17% → 94.17%
new `tests/unit/ts7-executor-result-contract.test.ts`	3/3
existing test files importing a touched executor (50)	49 pass

Two pre-existing conditions on the base branch, both verified on a worktree carrying none of these changes, neither touched here:

tests/unit/plan3-p0.test.ts — 37 pass / 1 fail. Reads the developer's real ~/.omniroute DB instead of a test-scoped DATA_DIR, so it resolves against whatever providers happen to be configured locally.
npm run lint — 4 errors in tests/unit/claude-to-openai-think-close-5123.test.ts. Its eslint-suppressions.json entry allows count: 2 but the file now holds more anys. Baseline drift, worth its own tiny PR. Lint is clean for every file this PR touches (base.ts carries no suppression entry and reports 0).

Tests

The normalizer half is already covered by chatcore-upstream-timeouts.test.ts. The new file pins the half this PR changed — that GithubExecutor.execute passes a bare Response through untouched, still materializes the capture-object arm, and tolerates a nullish result — so a later "simplification" can't quietly drop the bare-Response arm now that the type permits it.

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

diegosouzapw · 2026-07-25T03:31:47Z

Thanks for another clean slice of the TS7 migration — this is exactly the right fix. Declaring ExecutorExecuteResult on BaseExecutor.execute() as the same union normalizeExecutorResult() already accepted (rather than letting TS infer only the object shape from the single literal return) is the correct root-cause fix, and leaving DuckDuckGo's 14 valid returns untouched instead of wrapping them was the right call.

Verified locally: the new ts7-executor-result-contract.test.ts passes (3/3), typecheck:core is clean, and the 115 existing tests across executor-github, executor-pollinations, duckduckgo-web-executor, executor-base-utils, executor-default-base, and executor-github-prefill-sanitize all still pass. Confirmed BaseExecutor.execute() only has the one object-shaped return today, so this is genuinely type-only with no behavior change.

One small optional idea for a later slice: normalizeExecutorResult() in upstreamTimeouts.ts still redeclares the same union inline — could import your new ExecutorExecuteResult there instead, purely for DRY. Not blocking.

Merging this as-is — nice work.

diegosouzapw · 2026-07-25T05:54:08Z

Merged into release/v3.8.49 — thanks @backryun! Validated via a 26-PR local merge-train + per-file discriminator confirming zero regressions vs the pure release tip. 🙏

`BaseExecutor.buildHeaders(credentials, stream?, clientHeaders?, model?, health?)` was shadowed in three executors by same-named helpers with unrelated signatures: hailuo-web private buildHeaders(token: string, yy: string) lmarena protected buildHeaders(_model: string, credentials: unknown, _body: unknown) qwen-web private buildHeaders(token: string, cookieHeader: string, chatId?: string) Name collisions, not overrides — each reported TS2416. They are renamed to `buildStreamHeaders` / `buildRequestHeaders` / `buildApiHeaders`; the two lmarena test files that called the helper directly are updated with them. Worth stating precisely, because the shadow sat on a live dispatch path without being a live bug: `BaseExecutor.countTokens()` calls `this.buildHeaders(credentials, false)`, and all three inherit `countTokens()`. It is unreachable today only because `buildCountTokensUrl()` returns null unless `config.format === "claude"` and the URL carries `/messages` — hailuo-web and qwen-web set no format, lmarena sets `"openai"` — so `countTokens()` returns at the guard above. Latent, not live; one `format` change away from passing a credentials object where a token string is expected. Two more, surfaced by clearing the above: * `lmarena` declared `buildUrl` and `transformRequest` `protected` while both are public on BaseExecutor (TS2415 — a subclass may widen visibility, never narrow it). Both were masked behind the buildHeaders TS2416 and appeared one at a time as it cleared. Runtime is unaffected; JavaScript has no member visibility. * `GithubExecutor.refreshCredentials` had no declared return type, so TypeScript inferred the union of its four literal returns. `GheCopilotExecutor` legitimately overrides it with a wider `providerSpecificData` (it also records the enterprise proxy URL) and no `expiresIn`, which is not assignable to that inferred union. Declared as `RefreshedCopilotCredentials | null` — same shape of fix as diegosouzapw#8489, on a different method. Validation: full tsc error-set diff against the base config — 335 -> 331, zero new errors (line-number-agnostic). `typecheck:core` clean; the 15 existing test files importing a touched executor pass, including lmarena's 44 across the two updated files. `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 that the inherited method is no longer shadowed — verified to fail on the base, where all three prototypes still carry their own `buildHeaders` — and that the `countTokens()` early return which kept it harmless still holds.

…rs (#8498) `BaseExecutor.buildHeaders(credentials, stream?, clientHeaders?, model?, health?)` was shadowed in three executors by same-named helpers with unrelated signatures: hailuo-web private buildHeaders(token: string, yy: string) lmarena protected buildHeaders(_model: string, credentials: unknown, _body: unknown) qwen-web private buildHeaders(token: string, cookieHeader: string, chatId?: string) Name collisions, not overrides — each reported TS2416. They are renamed to `buildStreamHeaders` / `buildRequestHeaders` / `buildApiHeaders`; the two lmarena test files that called the helper directly are updated with them. Worth stating precisely, because the shadow sat on a live dispatch path without being a live bug: `BaseExecutor.countTokens()` calls `this.buildHeaders(credentials, false)`, and all three inherit `countTokens()`. It is unreachable today only because `buildCountTokensUrl()` returns null unless `config.format === "claude"` and the URL carries `/messages` — hailuo-web and qwen-web set no format, lmarena sets `"openai"` — so `countTokens()` returns at the guard above. Latent, not live; one `format` change away from passing a credentials object where a token string is expected. Two more, surfaced by clearing the above: * `lmarena` declared `buildUrl` and `transformRequest` `protected` while both are public on BaseExecutor (TS2415 — a subclass may widen visibility, never narrow it). Both were masked behind the buildHeaders TS2416 and appeared one at a time as it cleared. Runtime is unaffected; JavaScript has no member visibility. * `GithubExecutor.refreshCredentials` had no declared return type, so TypeScript inferred the union of its four literal returns. `GheCopilotExecutor` legitimately overrides it with a wider `providerSpecificData` (it also records the enterprise proxy URL) and no `expiresIn`, which is not assignable to that inferred union. Declared as `RefreshedCopilotCredentials | null` — same shape of fix as #8489, on a different method. Validation: full tsc error-set diff against the base config — 335 -> 331, zero new errors (line-number-agnostic). `typecheck:core` clean; the 15 existing test files importing a touched executor pass, including lmarena's 44 across the two updated files. `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 that the inherited method is no longer shadowed — verified to fail on the base, where all three prototypes still carry their own `buildHeaders` — and that the `countTokens()` early return which kept it harmless still holds.

…ouzapw#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.

…rs (diegosouzapw#8498) `BaseExecutor.buildHeaders(credentials, stream?, clientHeaders?, model?, health?)` was shadowed in three executors by same-named helpers with unrelated signatures: hailuo-web private buildHeaders(token: string, yy: string) lmarena protected buildHeaders(_model: string, credentials: unknown, _body: unknown) qwen-web private buildHeaders(token: string, cookieHeader: string, chatId?: string) Name collisions, not overrides — each reported TS2416. They are renamed to `buildStreamHeaders` / `buildRequestHeaders` / `buildApiHeaders`; the two lmarena test files that called the helper directly are updated with them. Worth stating precisely, because the shadow sat on a live dispatch path without being a live bug: `BaseExecutor.countTokens()` calls `this.buildHeaders(credentials, false)`, and all three inherit `countTokens()`. It is unreachable today only because `buildCountTokensUrl()` returns null unless `config.format === "claude"` and the URL carries `/messages` — hailuo-web and qwen-web set no format, lmarena sets `"openai"` — so `countTokens()` returns at the guard above. Latent, not live; one `format` change away from passing a credentials object where a token string is expected. Two more, surfaced by clearing the above: * `lmarena` declared `buildUrl` and `transformRequest` `protected` while both are public on BaseExecutor (TS2415 — a subclass may widen visibility, never narrow it). Both were masked behind the buildHeaders TS2416 and appeared one at a time as it cleared. Runtime is unaffected; JavaScript has no member visibility. * `GithubExecutor.refreshCredentials` had no declared return type, so TypeScript inferred the union of its four literal returns. `GheCopilotExecutor` legitimately overrides it with a wider `providerSpecificData` (it also records the enterprise proxy URL) and no `expiresIn`, which is not assignable to that inferred union. Declared as `RefreshedCopilotCredentials | null` — same shape of fix as diegosouzapw#8489, on a different method. Validation: full tsc error-set diff against the base config — 335 -> 331, zero new errors (line-number-agnostic). `typecheck:core` clean; the 15 existing test files importing a touched executor pass, including lmarena's 44 across the two updated files. `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 that the inherited method is no longer shadowed — verified to fail on the base, where all three prototypes still carry their own `buildHeaders` — and that the `countTokens()` early return which kept it harmless still holds.

…ouzapw#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.

…rs (diegosouzapw#8498) `BaseExecutor.buildHeaders(credentials, stream?, clientHeaders?, model?, health?)` was shadowed in three executors by same-named helpers with unrelated signatures: hailuo-web private buildHeaders(token: string, yy: string) lmarena protected buildHeaders(_model: string, credentials: unknown, _body: unknown) qwen-web private buildHeaders(token: string, cookieHeader: string, chatId?: string) Name collisions, not overrides — each reported TS2416. They are renamed to `buildStreamHeaders` / `buildRequestHeaders` / `buildApiHeaders`; the two lmarena test files that called the helper directly are updated with them. Worth stating precisely, because the shadow sat on a live dispatch path without being a live bug: `BaseExecutor.countTokens()` calls `this.buildHeaders(credentials, false)`, and all three inherit `countTokens()`. It is unreachable today only because `buildCountTokensUrl()` returns null unless `config.format === "claude"` and the URL carries `/messages` — hailuo-web and qwen-web set no format, lmarena sets `"openai"` — so `countTokens()` returns at the guard above. Latent, not live; one `format` change away from passing a credentials object where a token string is expected. Two more, surfaced by clearing the above: * `lmarena` declared `buildUrl` and `transformRequest` `protected` while both are public on BaseExecutor (TS2415 — a subclass may widen visibility, never narrow it). Both were masked behind the buildHeaders TS2416 and appeared one at a time as it cleared. Runtime is unaffected; JavaScript has no member visibility. * `GithubExecutor.refreshCredentials` had no declared return type, so TypeScript inferred the union of its four literal returns. `GheCopilotExecutor` legitimately overrides it with a wider `providerSpecificData` (it also records the enterprise proxy URL) and no `expiresIn`, which is not assignable to that inferred union. Declared as `RefreshedCopilotCredentials | null` — same shape of fix as diegosouzapw#8489, on a different method. Validation: full tsc error-set diff against the base config — 335 -> 331, zero new errors (line-number-agnostic). `typecheck:core` clean; the 15 existing test files importing a touched executor pass, including lmarena's 44 across the two updated files. `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 that the inherited method is no longer shadowed — verified to fail on the base, where all three prototypes still carry their own `buildHeaders` — and that the `countTokens()` early return which kept it harmless still holds.

backryun requested a review from diegosouzapw as a code owner July 24, 2026 22:45

This was referenced Jul 24, 2026

feat(dependencies): TypeScript 7 migration — tracking #8484

Closed

test(sse): repair two base-red gates on release/v3.8.49 #8490

Merged

refactor(sse): stop three executors shadowing BaseExecutor.buildHeaders #8498

Merged

diegosouzapw merged commit 5a20314 into diegosouzapw:release/v3.8.49 Jul 25, 2026
5 checks passed

backryun deleted the chore/ts7-types-duckduckgo-web branch July 25, 2026 07:48

diegosouzapw mentioned this pull request Jul 28, 2026

Release v3.8.49 #7076

Merged

Navigation Menu

Uh oh!

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

refactor(sse): declare the executor execute() result contract#8489
diegosouzapw merged 1 commit into
diegosouzapw:release/v3.8.49from
backryun:chore/ts7-types-duckduckgo-web

backryun commented Jul 24, 2026

Uh oh!

diegosouzapw commented Jul 25, 2026

Uh oh!

Uh oh!

diegosouzapw commented Jul 25, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

backryun commented Jul 24, 2026

What was wrong

What this does

Validation

Tests

Uh oh!

diegosouzapw commented Jul 25, 2026

Uh oh!

Uh oh!

diegosouzapw commented Jul 25, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants