Uh oh!
There was an error while loading. Please reload this page.
fix(submit): make maxLine authoritative so the #208 drain test isn't vacuous - #212
Merged
Conversation
…us (review) Adversarial review of the earlier drain fix caught a vacuous test: bufio.Scanner's token cap is max(maxLine, cap(initialBuf)), so passing a 64 KB initial buffer with maxLine=1024 silently capped at 64 KB — the test's ~7.5 KB "oversized" line never tripped bufio.ErrTooLong, so the drain branch was never exercised (the test still passed with the drain deleted). Fix the helper so maxLine is authoritative: clamp the initial buffer to maxLine (64 KB otherwise). Production passes 16 MB, so the clamp is a no-op there — the initial buffer stays 64 KB and grows on demand exactly as before. The test now uses maxLine=4096 with a >4 KB line (guarded against future vacuity), which genuinely trips ErrTooLong. Verified by mutation: removing the drain now makes the test FAIL (InsertedRecords=0, the false exit 9), and restoring it passes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
saadqbal
self-requested a review
July 10, 2026 11:23
divyasinghds
approved these changes
Jul 10, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Follow-up to #208 (merged). The adversarial review of #208 caught — after merge — that its
streamDisplayAndParsedrain regression test is vacuous, and the root cause is a latent footgun in the helper itself.bufio.Scanner.Buffer(buf, max)sets the token cap tomax(max, cap(buf)).streamDisplayAndParsepassed a 64 KB initial buffer withmaxLineas the max, so anymaxLine < 64 KBwas silently raised to 64 KB. #208's test usedmaxLine=1024with a ~7.5 KB "oversized" line — under the 64 KB floor — so it never trippedbufio.ErrTooLongand the drain branch was never exercised. The test passed even with the drain deleted.Fix
maxLinesomaxLineis authoritative. Production behaviour is unchanged: it passes 16 MB, so the clamp is a no-op — the initial buffer stays 64 KB and grows on demand exactly as before.maxLine=4096with a >4 KB line and a setup guard (len(oversized) > maxLine) against future vacuity.Verification
go build ./...,go vet ./...,go test ./...— green;gofmtclean.io.Copydrain makesTestStreamDisplayAndParse_DrainsPastOversizedLineSoParserSeesBannerFAIL (InsertedRecords=0— the false exit 9); restoring the drain passes. The test is now a genuine guard for the exact behaviour fix: harden login env-validation, Inf/NaN schema inference, and log-scanner buffer (bug-hunt MED) #208 shipped.The production drain logic from #208 is correct and unchanged — this is a test-integrity + API-honesty fix.
🤖 Generated with Claude Code
Note
Low Risk
Test-hardening and a correctness fix for small
maxLinevalues; production still uses 16 MB with no behavior change to the drain path.Overview
streamDisplayAndParsenow clamps thebufio.Scannerinitial buffer capacity tomaxLine, because Go’sScanner.Buffereffective token limit ismax(maxLine, cap(initialBuf))— a fixed 64 KB start buffer meant callers passingmaxLine< 64 KB never hitbufio.ErrTooLongeven when they intended to.Production behavior is unchanged: watch still passes
displayLineMax(16 MB), so the initial cap stays 64 KB and grows on demand as before. The #208 post-ErrTooLongdrain logic is untouched.The drain regression test now uses
maxLine = 4096with a setup check that the fake tqdm line exceeds that cap, so removing the drain would fail instead of passing vacuously.Reviewed by Cursor Bugbot for commit dbb7f06. Bugbot is set up for automated code reviews on this repo. Configure here.