Repository navigation
refactor(sse): stop isClaudeEventPayload claiming a narrowing it never performs - #8557
Conversation
|
Verified this independently: isClaudeEventPayload's JsonRecord and the function itself are both module-private to stream.ts (no export, no external type consumer), and there's no real two-armed discriminated union here to retag (unlike #8499's ok:true/false cases) — getClaudeEventType is a content check, not a shape check, so payload is JsonRecord was never a legitimate narrowing in the first place. The consumer contract at passthroughTailProcessor.ts:27 already types this callback as returning plain boolean, which matches your fix exactly. Reproduced the never-branch failure on the base tip (stream.ts:2273-2290) and confirmed both stream-numeric-ids.test.ts guard tests pass on your branch. typecheck:core and eslint are clean, and the 2 file-size violations are pre-existing/unrelated. Looks correct and ready to merge. One note for your own tracking: the 280->272 full-repo diagnostic delta I could not confirm within the review window, so I'm treating that number as your measurement rather than a verified one — it doesn't affect the verdict. |
…r performs
Eight TS2339s in stream.ts read properties off `never`. `never` here did not
mean unreachable code — it meant a type predicate was lying.
function isClaudeEventPayload(payload: unknown): payload is JsonRecord
...
const flushedParsed = bufferedPayload as JsonRecord;
const isClaude = isClaudeEventPayload(flushedParsed);
} else if (!isClaude) { // JsonRecord minus JsonRecord = never
The predicate answers "does this payload carry a Claude event type?" — a
question about contents, not type. A Claude event and a non-Claude one are both
JsonRecord, so `payload is JsonRecord` narrows nothing; applied to an argument
already typed JsonRecord it collapsed the negative branch to `never`, taking
`.id` and `.choices` with it.
Changed the return type to `boolean`. No call site depended on the narrowing:
both other callers pass the value to updateClaudeEmptyResponseLifecycle(), whose
parameter is `unknown`, and passthroughTailProcessor.ts already declares this
function as `(payload: unknown) => boolean` — so this aligns the definition with
how the codebase already consumes it.
280 -> 272, zero new, on a line-number-agnostic diff of the full tsc error set.
The diff is one line, and a type predicate has no runtime representation, so the
emitted JavaScript is unchanged.
No test added, and no gap to fill: the `never`-typed branch is real, reachable
and already covered from both sides — stream-numeric-ids.test.ts exercises
"normalizes numeric id in final chunk without trailing newline" (the flush path
with a non-Claude payload, i.e. the branch TS believed impossible) and "Claude
passthrough does not normalize numeric ids" (the other arm). 338/338 across the
48 SSE/passthrough suites.
No explanatory comment either: stream.ts sits exactly at its frozen file-size
cap (2887), so a single added line fails check:file-size. The reasoning lives
here and in the PR rather than costing a baseline bump on a frozen file.
befc704 to
9bda94d
Compare
11f79b6
into
cheaper-inference:release/v3.8.49
|
Thanks @backryun — merged into |
…r performs (cheaper-inference#8557) Eight TS2339s in stream.ts read properties off `never`. `never` here did not mean unreachable code — it meant a type predicate was lying. function isClaudeEventPayload(payload: unknown): payload is JsonRecord ... const flushedParsed = bufferedPayload as JsonRecord; const isClaude = isClaudeEventPayload(flushedParsed); } else if (!isClaude) { // JsonRecord minus JsonRecord = never The predicate answers "does this payload carry a Claude event type?" — a question about contents, not type. A Claude event and a non-Claude one are both JsonRecord, so `payload is JsonRecord` narrows nothing; applied to an argument already typed JsonRecord it collapsed the negative branch to `never`, taking `.id` and `.choices` with it. Changed the return type to `boolean`. No call site depended on the narrowing: both other callers pass the value to updateClaudeEmptyResponseLifecycle(), whose parameter is `unknown`, and passthroughTailProcessor.ts already declares this function as `(payload: unknown) => boolean` — so this aligns the definition with how the codebase already consumes it. 280 -> 272, zero new, on a line-number-agnostic diff of the full tsc error set. The diff is one line, and a type predicate has no runtime representation, so the emitted JavaScript is unchanged. No test added, and no gap to fill: the `never`-typed branch is real, reachable and already covered from both sides — stream-numeric-ids.test.ts exercises "normalizes numeric id in final chunk without trailing newline" (the flush path with a non-Claude payload, i.e. the branch TS believed impossible) and "Claude passthrough does not normalize numeric ids" (the other arm). 338/338 across the 48 SSE/passthrough suites. No explanatory comment either: stream.ts sits exactly at its frozen file-size cap (2887), so a single added line fails check:file-size. The reasoning lives here and in the PR rather than costing a baseline bump on a frozen file. Co-authored-by: backryun <busan011@ormbiz.co.kr>
…r performs (cheaper-inference#8557) Eight TS2339s in stream.ts read properties off `never`. `never` here did not mean unreachable code — it meant a type predicate was lying. function isClaudeEventPayload(payload: unknown): payload is JsonRecord ... const flushedParsed = bufferedPayload as JsonRecord; const isClaude = isClaudeEventPayload(flushedParsed); } else if (!isClaude) { // JsonRecord minus JsonRecord = never The predicate answers "does this payload carry a Claude event type?" — a question about contents, not type. A Claude event and a non-Claude one are both JsonRecord, so `payload is JsonRecord` narrows nothing; applied to an argument already typed JsonRecord it collapsed the negative branch to `never`, taking `.id` and `.choices` with it. Changed the return type to `boolean`. No call site depended on the narrowing: both other callers pass the value to updateClaudeEmptyResponseLifecycle(), whose parameter is `unknown`, and passthroughTailProcessor.ts already declares this function as `(payload: unknown) => boolean` — so this aligns the definition with how the codebase already consumes it. 280 -> 272, zero new, on a line-number-agnostic diff of the full tsc error set. The diff is one line, and a type predicate has no runtime representation, so the emitted JavaScript is unchanged. No test added, and no gap to fill: the `never`-typed branch is real, reachable and already covered from both sides — stream-numeric-ids.test.ts exercises "normalizes numeric id in final chunk without trailing newline" (the flush path with a non-Claude payload, i.e. the branch TS believed impossible) and "Claude passthrough does not normalize numeric ids" (the other arm). 338/338 across the 48 SSE/passthrough suites. No explanatory comment either: stream.ts sits exactly at its frozen file-size cap (2887), so a single added line fails check:file-size. The reasoning lives here and in the PR rather than costing a baseline bump on a frozen file. Co-authored-by: backryun <busan011@ormbiz.co.kr>
Part of #8484. One line changed. Clears the largest cluster in
open-sse/utils/stream.ts.neverdid not mean unreachable — it meant the predicate was lyingEight TS2339s read
.idand.choicesoffnever:neverin a reachable branch is usually a signal, and it was one here:The predicate answers "does this payload carry a Claude event type?" — a question about contents, not type. A Claude event and a non-Claude one are both
JsonRecord, sopayload is JsonRecordnarrows nothing. Applied to an argument already typedJsonRecord, the negative branch becameJsonRecordminusJsonRecord=never, and every property read in it failed.The branch is not dead code — it is the flush-path numeric-ID normalization, and it runs.
Why
booleanis the right fix, not a castNo call site depends on the narrowing:
stream.ts:1028updateClaudeEmptyResponseLifecycle(…, payload: unknown)stream.ts:2266stream.ts:2277stream.ts:2188passthroughTailProcessorAnd that last one settles it —
passthroughTailProcessor.ts:27already declares the dependency as:So the codebase already consumes this as a plain boolean. The change aligns the definition with the consumer that was right all along, rather than papering over the
neverat eight call sites.Verification
280 → 272, zero new errors, on a line-number-agnostic diff of the full
tscerror set.npm run typecheck:core— cleaneslint— cleancheck:file-size— the two violations it reports (providers/page.tsx,tokenHealthCheck.ts) reproduce identically on an untouched base worktree;stream.tsitself is unchanged in sizeA type predicate has no runtime representation, so the emitted JavaScript is byte-identical. Behavior preservation here is a property of the language, not an empirical claim.
No test added — and this time that is the finding
The last four slices each turned up an untested path that was exactly the one the new type described (#8525 the
voidarm, #8528{ text }documents on the response adapter, #8531 theempty→ 502 arm, #8533 the ArrayBuffer backing). So I checked the same way here, expecting a fifth.It is already covered, from both sides.
tests/unit/stream-numeric-ids.test.ts:Those two tests are also what make the
neverdiagnosis provable rather than inferred: the branch TS typed as uninhabited has a passing test that executes it.Adding anything here would be padding, so I did not.
No inline comment either
stream.tssits at exactly its frozen cap —check:file-sizecounts 2887 against a frozen 2887, so a single added line fails the gate. I drafted a short doc comment, measured, and dropped it rather than spend a baseline bump on a frozen file for two lines of prose. The reasoning lives in the commit message (wheregit blameon this line surfaces it) and here.Scope
stream.tshas 9 other diagnostics, in four unrelated clusters —.output/.reduceonunknown(2), aUsageTokenRecordTS2345, a TS2556 spread, and.text/.thinkingonunknown(5). Different causes, left for their own slices per the group-by-kind convention. Tracked in #8484.