refactor(sse): resolve open-sse utils/translator type diagnostics for TS 7 by backryun · Pull Request #8483 · diegosouzapw/OmniRoute

backryun · 2026-07-24T22:23:30Z

Context

First slice of the split you asked for on #7697:

I'd suggest splitting this into a smaller, fully-green PR first (baseUrl removal + TS6 diagnostic fixes only) and pursuing the TS7 native-CLI adoption as a separate, isolated follow-up once it's green end-to-end.

No toolchain change here — no dependency, CI, or tsc invocation is touched. This is only the diagnostic half, and only in the low-risk modules: 12 diagnostics across 8 files in open-sse/utils and open-sse/translator. chatCore.ts and stream.ts — the runtime-logic files you flagged — are deliberately not in this PR; they get their own slice with their own validation.

(baseUrl removal is #8473, also standalone.)

Fixes, by root cause

Cause	Files	Fix
`Transformer` lib type omits `cancel`	`progressTracker`, `sseHeartbeat` (+ `stream.ts`'s existing handler)	Patch the interface in `open-sse/types.d.ts`
Boolean-literal discriminant under `strictNullChecks: false`	`earlyStreamKeepalive`	Retag with a string discriminant
Assignment resets `typeof` narrowing	`toolCallShim`, `openai-responses`	Read through a local
`unknown` → `string` parameter	`sanitizeToolResultId`	Coerce
`opts = {}` inferred as `{}`	`openaiHelper`	Add `FilterToOpenAIFormatOptions`
`Buffer<ArrayBuffer>` vs `Buffer<ArrayBufferLike>` (@types/node 26)	`cursorAgentProtobuf`	Bare `Buffer` on the locals

Two of these deserve a note.

transformer.cancel is load-bearing, not dead code. The WHATWG Streams standard defines transformer.cancel(reason) and Node implements it — verified on v24, cancelling the readable side invokes the handler — but lib.dom.d.ts still omits it from Transformer, so every new TransformStream({ ..., cancel() {} }) was TS2353. Those handlers clear the heartbeat/progress intervals when an SSE client disconnects. Deleting them to satisfy the checker would leak a timer per abandoned stream, so the type is patched instead. A test asserts the runtime contract so the premise can't rot.

The strictNullChecks: false interaction is worth knowing for the later slices. SettledHandler was { ok: true; response } | { ok: false; error }. Without strictNullChecks, a boolean-literal discriminant narrows the positive branch but not the negative one — which is why only the single .error read errored while the two .response reads in the same file were fine. A string discriminant narrows both. Expect this pattern again in the remaining slices.

Validation

Raw counts under open-sse/tsconfig.json are only comparable once the config error is out of the way (TS5101 aborts type-checking), so the base was run with ignoreDeprecations: "6.0" and the two full error sets diffed:

	diagnostics
base	335
this PR	321
new errors introduced	0

gate	result
`npm run typecheck:core`	clean
`npm run lint` (changed files)	0 problems
`npm run check:type-coverage`	pass — 92.17% → 94.17%
new `tests/unit/ts7-open-sse-type-fixes.test.ts`	11/11
every existing test file importing a touched module (114 files)	pass

One caveat, stated plainly: tests/unit/plan3-p0.test.ts reports 37 pass / 1 fail (getModelInfoCore returns explicit ambiguity metadata…, expects null, gets 'github'). It fails identically with and without this change — verified by running it on a worktree carrying none of these edits. It has no test-scoped DATA_DIR, so it reads the developer's real ~/.omniroute DB and resolves against whatever providers happen to be configured locally. Pre-existing and environment-dependent; not touched here.

Tests

The refactors themselves are covered by the existing keepalive/heartbeat suites (53 tests, all still green). The new file covers the three surfaces those don't:

transformer.cancel() fires on readable cancel, and a handler can clear an interval — the guard against someone "fixing" the type error by deleting the handler.
sanitizeToolResultId coercion, including the falsy-id contract that keeps orphan tool_result blocks skipped.
Read shim limit clamping at both bounds — the narrowing rewrite must stay behavior-identical.

Remaining slices

Roughly, lowest risk first: open-sse/executors (58) → open-sse/services + src/lib/guardrails (41) → src/sse + peripheral handlers → chatCore.ts / stream.ts standalone. Happy to reorder if you'd rather see a different grouping.

… TS 7 First slice of the TypeScript 7 migration split requested on diegosouzapw#7697: resolve the type diagnostics under `open-sse/tsconfig.json` in the lowest-risk modules, with no toolchain change. 12 diagnostics across 8 files, all outside the hot path — `chatCore.ts` and `stream.ts` are deliberately left for a later, standalone slice. Fixes, by cause: * `Transformer.cancel` (progressTracker, sseHeartbeat, and stream.ts's existing handler) — the WHATWG Streams standard defines `transformer.cancel(reason)` and Node implements it (verified on v24: cancelling the readable side invokes it), but `lib.dom.d.ts` still omits it from `Transformer`, so every such handler was TS2353. These handlers clear the heartbeat/progress intervals when an SSE client disconnects, so deleting them to satisfy the checker would leak a timer per abandoned stream. The interface is patched in `open-sse/types.d.ts` instead. * `earlyStreamKeepalive` — `SettledHandler` was discriminated by `ok: true | false`. This workspace compiles with `strictNullChecks: false`, where a boolean-literal discriminant narrows the positive branch but not the negative one, so reading `.error` off the rejected arm did not type-check (the two `.response` reads elsewhere in the file did, which is why only one site errored). Retagged with a string discriminant, which narrows both branches under the same settings. * `toolCallShim` / `openai-responses` — assigning back to a property declared `unknown` resets the `typeof` narrowing, so the following comparison no longer saw a number/array. Both now read through a local. The `Read` limit clamp is behavior-identical: its two branches are mutually exclusive at READ_MAX_LIMIT 2000. * `sanitizeToolResultId` — takes `unknown` but forwards to a `string` parameter; a non-string id previously reached `.replace()` and threw. Coerced instead. * `openaiHelper` — `opts = {}` inferred `{}`; typed as `FilterToOpenAIFormatOptions`. * `cursorAgentProtobuf` — `Buffer.alloc(0)` infers `Buffer<ArrayBuffer>` under @types/node 26 while the decoded field is `Buffer<ArrayBufferLike>`; the locals now use bare `Buffer`, matching `requestMetadata` a few lines above. Validation: 335 -> 321 diagnostics with zero new errors (full tsc error-set diff against the base config). typecheck:core clean, lint clean, check:type-coverage 92.17% -> 94.17%. All 114 existing test files that import a touched module pass; `plan3-p0.test.ts` fails identically with and without this change (it reads the developer's real ~/.omniroute DB instead of a test-scoped DATA_DIR). The new test covers the three behavioral surfaces rather than the refactors the existing keepalive/heartbeat suites already hold: that `transformer.cancel()` really fires and can clear an interval, the id coercion, and the limit-clamp bounds.

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

Thanks for this — really appreciate the discipline of keeping this to the diagnostic-only slice with zero toolchain changes, exactly as scoped in #7697.

I reproduced your numbers independently in a clean worktree: diffed the full tsc error set between this branch and the release base (with ignoreDeprecations: "6.0" to get past TS5101), and every diagnostic you list as fixed is confirmed fixed, including the Transformer.cancel gap resolving across all 4 real call sites (stream.ts, responsesTransformer.ts, progressTracker.ts, sseHeartbeat.ts) even though only types.d.ts changed. Zero new errors introduced. Ran your new test file (11/11 pass) plus every existing suite that imports a touched module (150 tests, 0 failures) and typecheck:core/lint clean on the changed files.

This is ready to merge as-is on our end. Looking forward to the executors slice next.

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

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

Eight diagnostics across three executors, all the same root cause already recorded in diegosouzapw#8483: this workspace compiles with `strictNullChecks: false`, where a boolean-literal discriminant narrows the positive branch but not the negative one. Reading a failure-only field after `!result.ok` therefore leaves the full union. auggie.ts 2 .error on AuggieModelResolution muse-spark-web 4 .error on GraphqlResult notion-web 2 .retryable / .errorResult on the runOnce union diegosouzapw#8483 retagged its union with a string discriminant. That is the better shape when the union is module-private and small, but it does not fit here: `resolveAuggieModel` is exported and its tests deep-equal the literal `{ ok: true, model }` object, so retagging would churn public API and assertions to fix a checker limitation. Each union instead gets an explicit type predicate, which narrows correctly under these compiler settings while leaving the shape, every call site, and the tests untouched. notion-web's inline union is named `NotionAttempt` first so it has something to `Extract` from. Validation: full tsc error-set diff against the base config — 335 -> 327, zero new errors (line-number-agnostic). `typecheck:core` clean; the 6 existing test files importing a touched executor pass. Coverage: each predicate is a one-liner whose control flow inverts on a stray `!`, and all three failure branches already have behavioral guards — `auggie-executor.test.ts` (400 + /Unknown Auggie model/), `muse-spark-web-continuation.test.ts` ("Warmup failed: …"), and `executor-notion-web.test.ts` (nested temporarily-unavailable → retried). The two assertions added here pin the arm that the predicate unlocks on the one union that is exported and directly reachable.

* refactor(sse): narrow three result unions via type predicates Eight diagnostics across three executors, all the same root cause already recorded in #8483: this workspace compiles with `strictNullChecks: false`, where a boolean-literal discriminant narrows the positive branch but not the negative one. Reading a failure-only field after `!result.ok` therefore leaves the full union. auggie.ts 2 .error on AuggieModelResolution muse-spark-web 4 .error on GraphqlResult notion-web 2 .retryable / .errorResult on the runOnce union #8483 retagged its union with a string discriminant. That is the better shape when the union is module-private and small, but it does not fit here: `resolveAuggieModel` is exported and its tests deep-equal the literal `{ ok: true, model }` object, so retagging would churn public API and assertions to fix a checker limitation. Each union instead gets an explicit type predicate, which narrows correctly under these compiler settings while leaving the shape, every call site, and the tests untouched. notion-web's inline union is named `NotionAttempt` first so it has something to `Extract` from. Validation: full tsc error-set diff against the base config — 335 -> 327, zero new errors (line-number-agnostic). `typecheck:core` clean; the 6 existing test files importing a touched executor pass. Coverage: each predicate is a one-liner whose control flow inverts on a stray `!`, and all three failure branches already have behavioral guards — `auggie-executor.test.ts` (400 + /Unknown Auggie model/), `muse-spark-web-continuation.test.ts` ("Warmup failed: …"), and `executor-notion-web.test.ts` (nested temporarily-unavailable → retried). The two assertions added here pin the arm that the predicate unlocks on the one union that is exported and directly reachable. * chore(quality): rebaseline muse-spark-web.ts for #8499 own growth The new isGraphqlFailure() type-predicate helper (TS7 strictNullChecks:false narrowing fix) grows the frozen file 1396->1405 (+9), irreducible per the justification recorded in config/quality/file-size-baseline.json. Co-authored-by: diegosouzapw <diegosouza.pw@gmail.com> --------- Co-authored-by: ikelvingo <im.kelvinwong@gmail.com> Co-authored-by: diegosouzapw <diegosouza.pw@gmail.com>

All 6 diagnostics in adobeFireflyClient.ts are one cause — the `strictNullChecks: false` limitation this campaign has hit repeatedly (diegosouzapw#8483, diegosouzapw#8499, diegosouzapw#8531): a boolean-literal discriminant narrows the positive branch but leaves the negative one as the full union, so the `else` after `if (authed.ok)` could not see `status` or `error`. imsCheckToken(): { ok: true; token; data } | { ok: false; status; error } -> { state: "ok"; … } | { state: "failed"; … } Retagged rather than fixed with a predicate: `imsCheckToken` is module-private (no export, no test reference), which is the rule recorded on diegosouzapw#8499 — predicates exist to avoid churning an exported shape, and there is none here. 208 -> 202, zero new, on a line-number-agnostic diff of the full tsc error set. Kept the union inline in the return position instead of extracting a named type: adobeFireflyClient.ts is frozen at 2317 lines with no headroom, and extracting it (plus a doc comment) pushed the file to 2325 and failed check:file-size. The diff is 7 lines changed, 0 added; the reasoning lives here. No test added, and no gap to fill. "cookie exchange rejects guest IMS tokens" already drives both arms in one flow: `guest_allowed=false` returns HTTP 400, taking the failed arm and reading `status`/`error` (its assertion depends on the "All session cookies are empty" text), then `guest_allowed=true` returns 200 and takes the ok arm. 50/50 across the three Adobe Firefly suites; the same suites pass on the parent commit, confirming the retag changes nothing observable.

… TS 7 (diegosouzapw#8483) First slice of the TypeScript 7 migration split requested on diegosouzapw#7697: resolve the type diagnostics under `open-sse/tsconfig.json` in the lowest-risk modules, with no toolchain change. 12 diagnostics across 8 files, all outside the hot path — `chatCore.ts` and `stream.ts` are deliberately left for a later, standalone slice. Fixes, by cause: * `Transformer.cancel` (progressTracker, sseHeartbeat, and stream.ts's existing handler) — the WHATWG Streams standard defines `transformer.cancel(reason)` and Node implements it (verified on v24: cancelling the readable side invokes it), but `lib.dom.d.ts` still omits it from `Transformer`, so every such handler was TS2353. These handlers clear the heartbeat/progress intervals when an SSE client disconnects, so deleting them to satisfy the checker would leak a timer per abandoned stream. The interface is patched in `open-sse/types.d.ts` instead. * `earlyStreamKeepalive` — `SettledHandler` was discriminated by `ok: true | false`. This workspace compiles with `strictNullChecks: false`, where a boolean-literal discriminant narrows the positive branch but not the negative one, so reading `.error` off the rejected arm did not type-check (the two `.response` reads elsewhere in the file did, which is why only one site errored). Retagged with a string discriminant, which narrows both branches under the same settings. * `toolCallShim` / `openai-responses` — assigning back to a property declared `unknown` resets the `typeof` narrowing, so the following comparison no longer saw a number/array. Both now read through a local. The `Read` limit clamp is behavior-identical: its two branches are mutually exclusive at READ_MAX_LIMIT 2000. * `sanitizeToolResultId` — takes `unknown` but forwards to a `string` parameter; a non-string id previously reached `.replace()` and threw. Coerced instead. * `openaiHelper` — `opts = {}` inferred `{}`; typed as `FilterToOpenAIFormatOptions`. * `cursorAgentProtobuf` — `Buffer.alloc(0)` infers `Buffer<ArrayBuffer>` under @types/node 26 while the decoded field is `Buffer<ArrayBufferLike>`; the locals now use bare `Buffer`, matching `requestMetadata` a few lines above. Validation: 335 -> 321 diagnostics with zero new errors (full tsc error-set diff against the base config). typecheck:core clean, lint clean, check:type-coverage 92.17% -> 94.17%. All 114 existing test files that import a touched module pass; `plan3-p0.test.ts` fails identically with and without this change (it reads the developer's real ~/.omniroute DB instead of a test-scoped DATA_DIR). The new test covers the three behavioral surfaces rather than the refactors the existing keepalive/heartbeat suites already hold: that `transformer.cancel()` really fires and can clear an interval, the id coercion, and the limit-clamp bounds.

…ouzapw#8499) * refactor(sse): narrow three result unions via type predicates Eight diagnostics across three executors, all the same root cause already recorded in diegosouzapw#8483: this workspace compiles with `strictNullChecks: false`, where a boolean-literal discriminant narrows the positive branch but not the negative one. Reading a failure-only field after `!result.ok` therefore leaves the full union. auggie.ts 2 .error on AuggieModelResolution muse-spark-web 4 .error on GraphqlResult notion-web 2 .retryable / .errorResult on the runOnce union diegosouzapw#8483 retagged its union with a string discriminant. That is the better shape when the union is module-private and small, but it does not fit here: `resolveAuggieModel` is exported and its tests deep-equal the literal `{ ok: true, model }` object, so retagging would churn public API and assertions to fix a checker limitation. Each union instead gets an explicit type predicate, which narrows correctly under these compiler settings while leaving the shape, every call site, and the tests untouched. notion-web's inline union is named `NotionAttempt` first so it has something to `Extract` from. Validation: full tsc error-set diff against the base config — 335 -> 327, zero new errors (line-number-agnostic). `typecheck:core` clean; the 6 existing test files importing a touched executor pass. Coverage: each predicate is a one-liner whose control flow inverts on a stray `!`, and all three failure branches already have behavioral guards — `auggie-executor.test.ts` (400 + /Unknown Auggie model/), `muse-spark-web-continuation.test.ts` ("Warmup failed: …"), and `executor-notion-web.test.ts` (nested temporarily-unavailable → retried). The two assertions added here pin the arm that the predicate unlocks on the one union that is exported and directly reachable. * chore(quality): rebaseline muse-spark-web.ts for diegosouzapw#8499 own growth The new isGraphqlFailure() type-predicate helper (TS7 strictNullChecks:false narrowing fix) grows the frozen file 1396->1405 (+9), irreducible per the justification recorded in config/quality/file-size-baseline.json. Co-authored-by: diegosouzapw <diegosouza.pw@gmail.com> --------- Co-authored-by: ikelvingo <im.kelvinwong@gmail.com> Co-authored-by: diegosouzapw <diegosouza.pw@gmail.com>

… TS 7 (diegosouzapw#8483) First slice of the TypeScript 7 migration split requested on diegosouzapw#7697: resolve the type diagnostics under `open-sse/tsconfig.json` in the lowest-risk modules, with no toolchain change. 12 diagnostics across 8 files, all outside the hot path — `chatCore.ts` and `stream.ts` are deliberately left for a later, standalone slice. Fixes, by cause: * `Transformer.cancel` (progressTracker, sseHeartbeat, and stream.ts's existing handler) — the WHATWG Streams standard defines `transformer.cancel(reason)` and Node implements it (verified on v24: cancelling the readable side invokes it), but `lib.dom.d.ts` still omits it from `Transformer`, so every such handler was TS2353. These handlers clear the heartbeat/progress intervals when an SSE client disconnects, so deleting them to satisfy the checker would leak a timer per abandoned stream. The interface is patched in `open-sse/types.d.ts` instead. * `earlyStreamKeepalive` — `SettledHandler` was discriminated by `ok: true | false`. This workspace compiles with `strictNullChecks: false`, where a boolean-literal discriminant narrows the positive branch but not the negative one, so reading `.error` off the rejected arm did not type-check (the two `.response` reads elsewhere in the file did, which is why only one site errored). Retagged with a string discriminant, which narrows both branches under the same settings. * `toolCallShim` / `openai-responses` — assigning back to a property declared `unknown` resets the `typeof` narrowing, so the following comparison no longer saw a number/array. Both now read through a local. The `Read` limit clamp is behavior-identical: its two branches are mutually exclusive at READ_MAX_LIMIT 2000. * `sanitizeToolResultId` — takes `unknown` but forwards to a `string` parameter; a non-string id previously reached `.replace()` and threw. Coerced instead. * `openaiHelper` — `opts = {}` inferred `{}`; typed as `FilterToOpenAIFormatOptions`. * `cursorAgentProtobuf` — `Buffer.alloc(0)` infers `Buffer<ArrayBuffer>` under @types/node 26 while the decoded field is `Buffer<ArrayBufferLike>`; the locals now use bare `Buffer`, matching `requestMetadata` a few lines above. Validation: 335 -> 321 diagnostics with zero new errors (full tsc error-set diff against the base config). typecheck:core clean, lint clean, check:type-coverage 92.17% -> 94.17%. All 114 existing test files that import a touched module pass; `plan3-p0.test.ts` fails identically with and without this change (it reads the developer's real ~/.omniroute DB instead of a test-scoped DATA_DIR). The new test covers the three behavioral surfaces rather than the refactors the existing keepalive/heartbeat suites already hold: that `transformer.cancel()` really fires and can clear an interval, the id coercion, and the limit-clamp bounds.

…ouzapw#8499) * refactor(sse): narrow three result unions via type predicates Eight diagnostics across three executors, all the same root cause already recorded in diegosouzapw#8483: this workspace compiles with `strictNullChecks: false`, where a boolean-literal discriminant narrows the positive branch but not the negative one. Reading a failure-only field after `!result.ok` therefore leaves the full union. auggie.ts 2 .error on AuggieModelResolution muse-spark-web 4 .error on GraphqlResult notion-web 2 .retryable / .errorResult on the runOnce union diegosouzapw#8483 retagged its union with a string discriminant. That is the better shape when the union is module-private and small, but it does not fit here: `resolveAuggieModel` is exported and its tests deep-equal the literal `{ ok: true, model }` object, so retagging would churn public API and assertions to fix a checker limitation. Each union instead gets an explicit type predicate, which narrows correctly under these compiler settings while leaving the shape, every call site, and the tests untouched. notion-web's inline union is named `NotionAttempt` first so it has something to `Extract` from. Validation: full tsc error-set diff against the base config — 335 -> 327, zero new errors (line-number-agnostic). `typecheck:core` clean; the 6 existing test files importing a touched executor pass. Coverage: each predicate is a one-liner whose control flow inverts on a stray `!`, and all three failure branches already have behavioral guards — `auggie-executor.test.ts` (400 + /Unknown Auggie model/), `muse-spark-web-continuation.test.ts` ("Warmup failed: …"), and `executor-notion-web.test.ts` (nested temporarily-unavailable → retried). The two assertions added here pin the arm that the predicate unlocks on the one union that is exported and directly reachable. * chore(quality): rebaseline muse-spark-web.ts for diegosouzapw#8499 own growth The new isGraphqlFailure() type-predicate helper (TS7 strictNullChecks:false narrowing fix) grows the frozen file 1396->1405 (+9), irreducible per the justification recorded in config/quality/file-size-baseline.json. Co-authored-by: diegosouzapw <diegosouza.pw@gmail.com> --------- Co-authored-by: ikelvingo <im.kelvinwong@gmail.com> Co-authored-by: diegosouzapw <diegosouza.pw@gmail.com>

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

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): narrow three result unions via type predicates #8499

Merged

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

backryun mentioned this pull request Jul 25, 2026

refactor(sse): retag the designer-web result unions with string discriminants #8531

Merged

backryun deleted the chore/ts7-types-open-sse-utils branch July 25, 2026 07:48

This was referenced Jul 26, 2026

refactor(sse): retag the Adobe Firefly IMS token-check union #8638

Merged

refactor(sse): narrow three media-generation result unions #8645

Merged

diegosouzapw mentioned this pull request Jul 28, 2026

Release v3.8.49 #7076

Merged

Navigation Menu

Uh oh!

refactor(sse): resolve open-sse utils/translator type diagnostics for TS 7 - #8483

refactor(sse): resolve open-sse utils/translator type diagnostics for TS 7#8483
diegosouzapw merged 1 commit into
diegosouzapw:release/v3.8.49from
backryun:chore/ts7-types-open-sse-utils

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

Context

Fixes, by root cause

Validation

Tests

Remaining slices

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