Skip to content

chore(combo): extract pure error predicates and quota status helpers to comboPredicates - #8548

Merged
diegosouzapw merged 1 commit into
diegosouzapw:release/v3.8.49from
MumuTW:chore/combo-predicates-extract
Jul 26, 2026
Merged

diegosouzapw merged 1 commit into
diegosouzapw:release/v3.8.49from
MumuTW:chore/combo-predicates-extract

Conversation

@MumuTW

@MumuTW MumuTW commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

Summary

Extract 8 pure helper/predicate functions (clampPercent, quotaRemainingPercentFromQuota, normalizeConnectionStatus, hasFutureRateLimitUntil, getConnectionStatusQuotaCutoffReason, isContextOverflow400, isParamValidation400, isModelScoped400) from open-sse/services/combo.ts to open-sse/services/combo/comboPredicates.ts.

Re-export them from combo.ts for backward compatibility.

Changes

  • open-sse/services/combo/comboPredicates.ts: Added pure implementations and required regex pattern imports (CONTEXT_OVERFLOW_PATTERNS, MODEL_ACCESS_DENIED_PATTERNS).
  • open-sse/services/combo.ts: Removed inline predicates and re-exported from comboPredicates.ts (file size reduced from 3,629 to 3,546 lines).

Verification

  • npm run typecheck:core passed with 0 errors.
  • npm run check:cycles passed.
  • Unit tests (combo-strategies.test.ts, combo-param-validation-fallback-4519.test.ts, repro-6637-kimi-token-limit.test.ts) passed.

@MumuTW
MumuTW requested a review from diegosouzapw as a code owner July 25, 2026 08:59
@diegosouzapw

Copy link
Copy Markdown
Owner

Thanks for this — I verified the extraction end-to-end in an isolated worktree: typecheck, lint and check:cycles are clean, all consuming unit tests pass (23/23 and 20/20 across the combo suites), and the merge onto the current release tip is automatic with no conflicts. The 4 helpers you don't re-export have no external consumers (they were never exported to begin with), and the 4 you do re-export keep combo-param-validation-fallback-4519.test.ts and friends working unchanged.

One correction to the framing, so the record is accurate: on the current tip open-sse/services/combo.ts is at 3640 lines against a frozen cap of 3642 — it is under the cap, and check:file-size does not currently flag it (the two files it does flag, providers/page.tsx and tokenHealthCheck.ts, are unrelated inherited base-red). Your branch is 75 commits behind, where the cap was still 3630, which is where the "over the cap" reading comes from. So this PR isn't repairing a red gate — it's creating headroom, taking the file from 3640 to 3543. That's still genuinely valuable: two lines of slack is nothing, and several open PRs want to add lines to that exact file.

The thing worth settling before any of these merge: you have three open PRs extracting the same predicates from the same hunk of combo.ts, into two different destinations.

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

Only one of the two designs should land — they touch the identical hunk. My read is that this one is the better target: it reuses a module that already has mutation coverage wired up, and it extracts more. But you know the reasoning behind splitting out errorClassification.ts better than I do, so if there was a deliberate reason for a separate module, say so and we'll go that way instead. Either way, the second commit on #8558 (the #7847 structural JSON-size fix) is independent and should land regardless.

Could you also add a changelog.d/ entry here, matching what you already did on #8555?

@MumuTW
MumuTW force-pushed the chore/combo-predicates-extract branch from 1841844 to 31d5107 Compare July 25, 2026 14:36
@MumuTW

MumuTW commented Jul 25, 2026

Copy link
Copy Markdown
Contributor Author

Agreed on both counts — going with this PR's design, and #8555 is closed.

On the framing correction: you're right and my reading was the stale one. combo.ts is under the 3642 cap on the current tip, so this isn't repairing a red gate — it's creating headroom (3640 → 3543). There was no deliberate architectural reason for the separate errorClassification.ts: it came out of a branch that predated my noticing comboPredicates.ts already existed in the same directory. Reusing a module that's already in stryker's mutate list with a tracked score, and extracting 8 helpers instead of 3, is plainly the better target. So: #8555 closed, and #8558 rebased so only its independent #7847 JSON-size commit remains.

changelog.d: already in — changelog.d/maintenance/combo-predicates-extract.md. It landed in the force-push a few minutes before your comment, so you were looking at the pre-push head.

Rebased onto 4053e2314. The base-red gates are fixed upstream now (#8544 ESLint, #8534 check:db-rules, #8539 backoff assertions, #8561 check:file-size rebaseline). Re-verified on the rebased head: combo-param-validation-fallback-4519 + chatcore-compression-combo-predicates 9/9, typecheck:core clean, check:cycles clean (379 files), eslint clean on both touched files, check:file-size OK.

Merge ordering is no longer a concern. With #8555 dropped, the computeCompatRejectedTargets reformat exists in only one place. I test-merged the three remaining combo PRs pairwise against the current tip with git merge-tree --write-tree: #8548 × #8553, #8548 × #8558, #8553 × #8558 — all clean, in any order. This PR now only touches lines ~166/301/681 of combo.ts; #8553 touches ~1804/2921/3183/3199 and #8558 is a single 2-line hunk at ~1819.

@MumuTW

MumuTW commented Jul 25, 2026 •

Copy link
Copy Markdown
Contributor Author

Correction to my comment above: Fast Quality Gates will still be red after the rebase, and it isn't this PR. #8561 fixed check:file-size and thereby unmasked a fifth base-red gate behind it in the same job — check:complexity-ratchets (complexity 2169 > baseline 2130, cognitiveComplexity 956 > 951).

I measured it on pristine detached checkouts with an empty working tree: 4053e2314 (current tip) and 30709255c (the base you reviewed against) both report 2169 / 956 — identical to this PR's head, so it predates #8561 and is not caused by anything here. It was hidden because check:file-size ran earlier in the same bash -e job and short-circuited it. Full measurement table in my comment on #8546.

The gate is in quality.yml's fast-gates job, so it's red for every PR against release/v3.8.49 regardless of content. Everything else on this PR is green and verified locally.

@diegosouzapw

Copy link
Copy Markdown
Owner

Thanks @MumuTW — 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. 🚀

@diegosouzapw diegosouzapw mentioned this pull request Jul 28, 2026
HouMinXi pushed a commit to HouMinXi/OmniRoute that referenced this pull request Aug 2, 2026
@MumuTW
MumuTW deleted the chore/combo-predicates-extract branch September 5, 2026 10:18
muhamadgalihsaputra pushed a commit to niyatna/NiyatnaRoute that referenced this pull request Sep 27, 2026
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