Skip to content

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

Merged
diegosouzapw merged 1 commit into
cheaper-inference:release/v3.8.49from
backryun:chore/ts7-types-executor-overrides
Jul 26, 2026
Merged

diegosouzapw merged 1 commit into
cheaper-inference:release/v3.8.49from
backryun:chore/ts7-types-executor-overrides

Conversation

@backryun

Copy link
Copy Markdown
Contributor

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

The collisions

BaseExecutor.buildHeaders(credentials, stream?, clientHeaders?, model?, health?) was shadowed in three executors by same-named helpers whose signatures have nothing to do with it:

executor declared as signature
hailuo-web private (token: string, yy: string)
lmarena protected (_model: string, credentials: unknown, _body: unknown)
qwen-web private (token: string, cookieHeader: string, chatId?: string)

Name collisions, not overrides — each reported TS2416. Renamed to buildStreamHeaders / buildRequestHeaders / buildApiHeaders, with the two lmarena test files that call the helper directly updated alongside.

Latent, not live — stating it precisely. BaseExecutor.countTokens() calls this.buildHeaders(credentials, false) and all three inherit countTokens(), so the shadow sat on a real dispatch path. 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 and lmarena sets "openai", so countTokens() returns at the guard one line above. One format change away from passing a credentials object where a token string is expected — but not a bug today, and I'm not going to dress it up as one.

Two more, surfaced by clearing the first

TypeScript reports these one at a time, so each fix revealed the next:

  • lmarena narrowed inherited visibility — buildUrl and transformRequest were protected while both are public on BaseExecutor (TS2415: a subclass may widen visibility, never narrow it). Verified identical on the pristine base ref, so pre-existing and masked behind the buildHeaders error. 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 — not assignable to that inferred union. Declared as RefreshedCopilotCredentials | null. Same shape of fix as refactor(sse): declare the executor execute() result contract #8489, different method.

Validation

diagnostics
base 335
this PR 331
new errors 0 (line-number-agnostic diff of the two error sets)

The remaining two TS2416 (felo-web, gitlab — both execute) are #8489's; untouched here.

gate result
npm run typecheck:core clean
new tests/unit/ts7-executor-override-signatures.test.ts 10/10
15 existing test files importing a touched executor pass

Two known base-red conditions, neither caused nor touched here: plan3-p0.test.ts (reads the developer's real ~/.omniroute DB instead of a test-scoped DATA_DIR) and npm run lint (stale suppression count in claude-to-openai-think-close-5123.test.ts). Both are addressed in #8490.

Tests

The renames are behavior-preserving, so the guard targets what actually changed — that the inherited method is no longer shadowed:

assert.equal(executor.buildHeaders, BaseExecutor.prototype.buildHeaders)

Verified to fail on the base, where all three prototypes still carry their own buildHeaders. The suite also pins that each provider keeps its own helper under the new name, and that the countTokens() early return which kept the shadow harmless still holds — so if that guard ever changes, the inherited method is the one that runs.

Overlap note

This touches open-sse/executors/github.ts, as does #8489 — different methods (execute guard there, refreshCredentials signature here) in well-separated regions, so they should auto-merge. Flagging it in case you want a merge order.

@backryun
backryun requested a review from diegosouzapw as a code owner July 24, 2026 23:53
@backryun
backryun force-pushed the chore/ts7-types-executor-overrides branch 2 times, most recently from a4af19a to 250f04b Compare July 25, 2026 08:26
`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 cheaper-inference#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
backryun force-pushed the chore/ts7-types-executor-overrides branch from 250f04b to f0025d0 Compare July 25, 2026 21:08
@diegosouzapw
diegosouzapw merged commit 06326a3 into cheaper-inference:release/v3.8.49 Jul 26, 2026
5 checks passed
@diegosouzapw

Copy link
Copy Markdown
Collaborator

Thanks @backryun — merged into release/v3.8.49 via the local merge-train (validated as one combined tree: full test:unit + test:vitest 274/274 on the 32-core box, tip d4b9ce6016). Your commit keeps its authorship. 🚀

@backryun
backryun deleted the chore/ts7-types-executor-overrides branch July 26, 2026 07:02
@diegosouzapw diegosouzapw mentioned this pull request Jul 28, 2026
HouMinXi pushed a commit to HouMinXi/OmniRoute that referenced this pull request Aug 2, 2026
…rs (cheaper-inference#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 cheaper-inference#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.
muhamadgalihsaputra pushed a commit to niyatna/NiyatnaRoute that referenced this pull request Sep 27, 2026
…rs (cheaper-inference#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 cheaper-inference#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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants