test(responses): assert the stream completed, not that it lacks the digits 502 - #4860
Conversation
…igits "502"
Windows shard 4/9 of run 35180376537 went red on a turn that had succeeded
perfectly. The relay stamps every chunk with a random chatcmpl-<hex> id, and this
run drew chatcmpl-05021785ecf5440c96ca31be. expect(text).not.toContain("502")
searched the whole stream, found those three characters inside the id, and failed.
tests/images/loop.test.ts already retired the identical assertion for "504" and
measured it: roughly one id in 137 contains a given three-digit string, which
reddened about one run in 69 for no reason at all.
The assertion could not do its job either. This relay's failure mode carries an
error frame and ends the turn; the number 502 never appears in the body, so a real
failure would have slipped straight past it. It was simultaneously flaky and blind.
Assert the terminal shape instead: the stream reached [DONE] and carried no error
frame. That detects the failure the case was written for and cannot be moved by a
random identifier.
No local suite, focused test, typecheck, build, or install was run.
|
✅ Deterministic PR hygiene checks passed. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
github-actions
Bot
added
the
chore
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review. 📝 WalkthroughWalkthroughThe deferred-tool streaming relay test replaces a broad ChangesDeferred-tool stream validation
Priority: ⬇️ Low Estimated code review effort: 1 (Trivial) | ~3 minutes Change: Other Merge Risk: ⚪ Minimal · up to This test-only change improves detection of relay failures without changing production behavior. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
리뷰 · 우선순위 77 / 80이 PR은 왜 바꿨는지가 분명합니다. Windows shard 4/9 실행 (Actions run 35180376537)에서 이미 성공한 턴이 빨개졌습니다. 릴레이가 청크마다 더 큰 문제는 이 검사가 진짜 실패도 못 본다는 점입니다. 이 릴레이가 실패할 때는 본문에 새 검사는 두 줄입니다. 현재 라인 196 - 메인테이너의 판단이 필요한 지점
너의 추천
이 댓글은 grok-bot이 작성했습니다 |
…igits "502" (lidge-jun#4860) Windows shard 4/9 of run 35180376537 went red on a turn that had succeeded perfectly. The relay stamps every chunk with a random chatcmpl-<hex> id, and this run drew chatcmpl-05021785ecf5440c96ca31be. expect(text).not.toContain("502") searched the whole stream, found those three characters inside the id, and failed. tests/images/loop.test.ts already retired the identical assertion for "504" and measured it: roughly one id in 137 contains a given three-digit string, which reddened about one run in 69 for no reason at all. The assertion could not do its job either. This relay's failure mode carries an error frame and ends the turn; the number 502 never appears in the body, so a real failure would have slipped straight past it. It was simultaneously flaky and blind. Assert the terminal shape instead: the stream reached [DONE] and carried no error frame. That detects the failure the case was written for and cannot be moved by a random identifier. No local suite, focused test, typecheck, build, or install was run.
1 participant
Summary
Windows shard 4/9 of run 35180376537 went red on a turn that had succeeded perfectly:
The relay stamps every chunk with a random
chatcmpl-<hex>id. This run drewchatcmpl-05021785ecf5440c96ca31be, the assertion searched the whole stream for the three characters502, found them inside that id, and failed.tests/images/loop.test.tsalready retired the identical assertion for"504"and measured the rate: roughly one id in 137 contains a given three-digit string, which reddened about one run in 69 for no reason at all.The assertion could not do its job either. This relay's failure mode carries an error frame and ends the turn; the number 502 never appears in the body, so a real failure would have slipped straight past it. It was simultaneously flaky and blind — which is the worst combination a test can have, and exactly the pattern this round has been removing.
It now asserts the terminal shape: the stream reached
[DONE]and carried no error frame. That detects the failure the case was written for and cannot be moved by a random identifier.Verification
No local suite, focused test, typecheck, build, or install was run; this lane is hosted-CI-only by task contract. Verification is static plus exact-head hosted CI.
Static checks:
not.toContainon a bare 4xx/5xx number) found three other sites. Two are intests/adapters/anthropic/anthropic-pool-toggle-copy.test.tsand assert that UI copy strings do not mention429— those operate on short authored strings, not on payloads carrying random ids, so they are sound and untouched. The third is the already-fixed comment intests/images/loop.test.ts. This was the last live instance.git diff --checkclean.Ablation: a relay failure emits an error frame and no
[DONE], so both new assertions go red on the behaviour the case exists to catch — which the old assertion did not.Checklist
devSummary by CodeRabbit