Skip to content

test(sse): regression guard for PROVIDER_BREAKER_FAILURE_STATUSES import (#8405) - #8424

Closed
MumuTW wants to merge 3 commits into
diegosouzapw:release/v3.8.49from
MumuTW:fix/breaker-failure-statuses-reference-error
Closed

MumuTW wants to merge 3 commits into
diegosouzapw:release/v3.8.49from
MumuTW:fix/breaker-failure-statuses-reference-error

Conversation

@MumuTW

@MumuTW MumuTW commented Jul 24, 2026 •

Copy link
Copy Markdown
Contributor

Rebased onto tip (release/v3.8.49). Reduced to regression-test-only as the production fix landed in #8390.

Adds unit test tests/unit/breaker-failure-statuses-8405.test.ts to assert that chatPredicates.ts exports PROVIDER_BREAKER_FAILURE_STATUSES as a Set with the expected failure statuses [408, 500, 502, 503, 504] and that chat.ts explicitly imports it without ReferenceError.

@MumuTW

MumuTW commented Jul 24, 2026

Copy link
Copy Markdown
Contributor Author

Aligned with the diagnosis on #8405. Local check: node --import tsx --test tests/unit/breaker-failure-statuses-8405.test.ts → 2/2 pass. Ready for review.

@diegosouzapw diegosouzapw added duplicate This issue or pull request already exists manual-review Aguardando revisão manual do dono (scope/contaminação) labels Jul 25, 2026
@diegosouzapw

Copy link
Copy Markdown
Owner

Thanks for the fast turnaround on this, @MumuTW — the diagnosis and fix approach (export the const from chatPredicates.ts, import it in chat.ts) are exactly right, and the regression test is a nice touch.

Timing caught this one: the identical two-line fix was independently merged into release/v3.8.49 via #8390 about 43 seconds before this PR was opened, so the production change here is now redundant — src/sse/handlers/chat.ts and chatPredicates.ts on the current tip already have your exact fix.

We tried a real merge of this branch against the current tip to double check, and it does produce a duplicate PROVIDER_BREAKER_FAILURE_STATUSES entry in the import block from chatPredicates.ts (both #8390's insertion and this PR's insertion land on different lines of the same import statement, so git doesn't flag it as a conflict, but it is a real duplicate identifier). So we won't be merging this as-is.

Your tests/unit/breaker-failure-statuses-8405.test.ts file does add something the existing coverage doesn't have (a direct assertion that chat.ts imports the symbol cleanly) — if you'd like, feel free to rebase onto the latest release/v3.8.49 and resubmit with just that test file (dropping the now-redundant chat.ts/chatPredicates.ts hunks); we'd be happy to take that as extra regression coverage. Otherwise we'll close this one as subsumed by #8390. Thanks again for catching and fixing this — much appreciated.

@MumuTW
MumuTW force-pushed the fix/breaker-failure-statuses-reference-error branch from 30ab319 to febfcbc Compare July 25, 2026 06:19
@MumuTW

MumuTW commented Jul 25, 2026

Copy link
Copy Markdown
Contributor Author

Took the second option — rebased onto the current release/v3.8.49 tip and dropped the now-redundant production hunks. Force-pushed as febfcbc49; the PR is now a single file, tests/unit/breaker-failure-statuses-8405.test.ts, with zero changes to chat.ts / chatPredicates.ts. That removes the duplicate-import problem you found entirely.

Good call on #8390 — I confirmed the tip already carries the fix (chatPredicates.ts:4 exports the Set, chat.ts:76-80 imports it), so no assertion changes were needed.

Verified it is a real guard, not a vacuous assertion:

Tree Result
current tip (7a8f9156d, post-#8390) 2/2 pass
3b4f4afc9^ (the commit just before #8390) 2/2 fail
eslint + prettier --check on the file exit 0 / clean

So it goes red exactly on the state #8405 reported and green on the fixed state. The second test also pins the import in chat.ts specifically, which is the part #8390's own coverage doesn't assert — that was the failure mode in #8405, so it should keep the re-export from silently drifting back out.

@MumuTW

MumuTW commented Jul 25, 2026

Copy link
Copy Markdown
Contributor Author

Heads-up on the red Fast Quality Gates check here — it is not from this PR.

The failing step is check:file-size, and the two violations are files this PR does not touch:

✗ src/app/(dashboard)/dashboard/providers/page.tsx: 1990 > congelado 1927
✗ src/lib/tokenHealthCheck.ts: 843 > congelado 841

I checked out release/v3.8.49 at its tip (7a8f9156d) clean and ran npm run check:file-size — identical two violations, no PR content involved. So the fast-gates job is currently red for every PR→release. Both offenders are already merged (#8349 for page.tsx, #8426 for tokenHealthCheck.ts), so there is no branch left to fix in place.

Opened #8524 with the baseline bump + justification entry, following the _rebaseline_2026_07_25_v3849_basered_filesize precedent already in the file. Once that lands this check should go green here without any change on this branch.

… import

Rebased onto release/v3.8.49 and reduced to the test file only — the
production fix landed in diegosouzapw#8390. Asserts chatPredicates exports the Set
with the documented statuses and that chat.ts imports the symbol cleanly.
@MumuTW
MumuTW force-pushed the fix/breaker-failure-statuses-reference-error branch from febfcbc to 2fcd4ad Compare July 26, 2026 09:31
@MumuTW MumuTW changed the title fix(resilience): import PROVIDER_BREAKER_FAILURE_STATUSES in chat.ts (Fixes #8405) test(sse): regression guard for PROVIDER_BREAKER_FAILURE_STATUSES import (#8405) Jul 26, 2026
Same tip fix as diegosouzapw#8657 so Merge integrity is green without waiting for
that PR to land. Regenerated via generate-agent-skills --apply.
@diegosouzapw

Copy link
Copy Markdown
Owner

Closing — the guard this PR adds is already enforced on the release tip

Thanks for this one. Verified against origin/release/v3.8.49 before closing: the import you were guarding is present and exercised.

PROVIDER_BREAKER_FAILURE_STATUSES is exported from open-sse/services/chatPredicates.ts and imported and used in src/sse/handlers/chat.ts:

src/sse/handlers/chat.ts:79     PROVIDER_BREAKER_FAILURE_STATUSES,
src/sse/handlers/chat.ts:1318   PROVIDER_BREAKER_FAILURE_STATUSES.has(breakerFailureStatus)

The dedicated regression file from this PR does not exist on the tip, but the same declaration is already asserted by tests/unit/nvidia-quota-phase1.test.ts, and chat.ts is imported by dozens of other unit tests — a broken export would fail the suite loudly, which is the outcome your guard was protecting.

Closing as already-covered. If you spot a gap the current coverage misses, please reopen with the specific case — the concern behind this PR was a real one.

@MumuTW
MumuTW deleted the fix/breaker-failure-statuses-reference-error 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

duplicate This issue or pull request already exists manual-review Aguardando revisão manual do dono (scope/contaminação)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants