Skip to content

refactor(sse): extract combo 400-classification predicates into a leaf module - #8555

Closed
MumuTW wants to merge 1 commit into
diegosouzapw:release/v3.8.49from
MumuTW:refactor/combo-error-classifiers
Closed

MumuTW wants to merge 1 commit into
diegosouzapw:release/v3.8.49from
MumuTW:refactor/combo-error-classifiers

Conversation

@MumuTW

@MumuTW MumuTW commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

Unblocks #8553 and the follow-up #7847 work. Behavior-preserving move, no logic change.

The problem this solves

combo.ts is 3640 lines against a frozen file-size ratchet of 3642. The gate is shrink-only, so any change that adds a line to that file is rejected — including a one-line bug fix plus the comment explaining it.

Both in-flight #7847 fixes hit this. Worse, one of them shipped a violation: lint-stageds Prettier reflowed an unrelated over-long line during pre-commit

-  const compatRejectedTargets = computeCompatRejectedTargets(evalRankedTargets, filteredTargets, body);
+  const compatRejectedTargets = computeCompatRejectedTargets(
+    evalRankedTargets,
+    filteredTargets,
+    body
+  );

— +4 lines, after the gate had already been checked. That is a trap for anyone touching this file: verifying check-file-size before committing is not sufficient, because the formatter runs later and can grow the file on its own.

Trimming comments until they fit is optimizing the wrong thing. This restores real headroom.

What moves

The three 400-classification predicates — isContextOverflow400, isParamValidation400, isModelScoped400 — decide whether an upstream 400 should advance the combo to the next target or hard-stop it. They are pure string classifiers that share the canonical pattern lists in accountFallback.ts; nothing about them belongs inside the routing loop.

combo.ts 3640 → 3610 (30 lines of headroom), new leaf is 55 lines.

Behavior preservation

Verified mechanically, not by eye: each body was extracted from both revisions and compared after normalizing whitespace.

✅ isContextOverflow400
✅ isParamValidation400
✅ isModelScoped400

combo.ts imports them for its own fallback guard and re-exports them, so the public surface is unchanged — tests/unit/combo-param-validation-fallback-4519.test.ts still imports them from combo.ts with no modification and passes 6/6.

One thing worth flagging for anyone repeating this pattern: a bare export { x } from "./leaf" re-exports without binding the name locally, so combo.tss own use of the predicates needed a separate import. typecheck:core caught it — a decomposition that only re-exports will compile-fail rather than fail silently.

Verification

check result
body faithfulness 3/3 byte-identical
combo-param-validation-fallback-4519 (existing suite, unmodified) 6/6 pass
combo regression sweep pass, except the base-red below
typecheck:core, check-file-size, check:any-budget:t11 clean

Inherited base-red (not from this PR)

Confirmed red on upstream/release/v3.8.49 untouched: live repo: no NEW unexported db modules beyond the frozen allowlist. Plus the repo-wide lint (stale suppression — #8544) and file-size (providers/page.tsx, tokenHealthCheck.ts — #8532 / #8524).

…f module

combo.ts sits at 3640 lines against a frozen file-size ratchet of 3642. The gate is
shrink-only, so ANY change that adds a line to this file is rejected -- including a
one-line bug fix plus its explanatory comment. Two in-flight diegosouzapw#7847 fixes both hit this,
and one shipped a violation because lint-staged's Prettier reflowed an unrelated over-long
line (+4 lines) AFTER the gate had already been checked.

Squeezing comments to fit is optimizing the wrong thing. This restores real headroom
instead: the three 400-classification predicates are pure string classifiers over upstream
error text, sharing the canonical pattern lists in accountFallback.ts, and have no business
inside the routing loop. Moving them takes combo.ts to 3610 -- 30 lines of headroom.

Behavior-preserving. The bodies were extracted from both revisions and compared after
normalizing whitespace: 3 of 3 byte-identical. combo.ts imports them for the fallback guard
and re-exports them, so the public surface is unchanged and
tests/unit/combo-param-validation-fallback-4519.test.ts still imports them from combo.ts
without modification (6/6 pass).

Note for anyone repeating this: a bare re-export does not bind the name locally, so
combo.ts's own use of the predicates needed a separate import. typecheck caught it.
@diegosouzapw

Copy link
Copy Markdown
Owner

Reviewed this in an isolated worktree against the actual PR head (7172e25).

Verified mechanically, not by eye:

  • combo.ts is 3610 lines / errorClassification.ts is 55 lines after the move -- matches the PR description exactly.
  • The three extracted function bodies are byte-identical to the originals (diffed directly).
  • tests/unit/combo-param-validation-fallback-4519.test.ts runs unmodified and passes 6/6, confirming the combo.ts re-export surface is unchanged.
  • typecheck:core, check:any-budget:t11, and eslint on both touched files are clean.
  • Re the "inherited base-red" claims: I checked out the untouched origin/release/v3.8.49 tip in the same worktree and reproduced the identical check:file-size (providers/page.tsx, tokenHealthCheck.ts) and check:db-rules (compressionDetailNormalizers.ts) failures -- confirmed pre-existing, not introduced by this PR.
  • The combo/ leaf-module + backward-compat re-export pattern also matches the existing comboPredicates.ts in the same directory, so this isn't a new decomposition style.

This looks solid and ready to merge as-is. Since #8558 is stacked on top of this commit, it'll need this one merged first.


One coordination note before this merges. You have a third PR, #8548, that extracts the same isContextOverflow400 / isParamValidation400 / isModelScoped400 predicates out of the same hunk of combo.ts — but into the existing combo/comboPredicates.ts rather than a new errorClassification.ts:

destination extracts shrinks combo.ts
this PR (+#8558) combo/errorClassification.ts (new module) 3 helpers −30 lines
#8548 combo/comboPredicates.ts (exists, already in stryker's mutate list with a tracked score) 8 helpers −97 lines

Only one of the two designs can land — they touch the identical hunk. My read leans toward #8548's target, since it reuses a module that already has mutation coverage wired up and extracts more, but you have the context on why errorClassification.ts was worth splitting out separately. Whichever you prefer, the second commit on #8558 (the #7847 structural JSON-size fix) is independent and should land either way.

@MumuTW

MumuTW commented Jul 25, 2026

Copy link
Copy Markdown
Contributor Author

Closing in favour of #8548, per the coordination note there and here.

You read it right: there was no deliberate architectural reason for a separate errorClassification.ts. It came out of a branch that predated my noticing comboPredicates.ts already existed in the same directory, and once that's on the table #8548 is the better target on both counts you named — already in stryker's mutate list with a tracked score, and it extracts 8 helpers / −97 lines instead of 3 / −30.

#8558 has been rebased with --onto past this commit, so it drops out cleanly and only the independent #7847 JSON-size fix remains there. Verified no errorClassification remnant is left on that branch and typecheck:core is clean. Dropping this also removes the computeCompatRejectedTargets reformat conflict flagged on #8553 — #8548, #8553 and #8558 now merge clean pairwise in any order.

@MumuTW MumuTW closed this Jul 25, 2026
@MumuTW
MumuTW deleted the refactor/combo-error-classifiers branch September 5, 2026 10:19
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