refactor(push): extract checkFileSize + pin the single-file-cap boundary#260
Merged
Conversation
The `size > MaxSingleFileBytes` guard was duplicated at four sites (stream, tabular, text ×2), and its boundary was untestable at every one: MaxSingleFileBytes is a 500 MB const, so the `>` vs `>=` distinction could only be exercised with a 500 MB fixture. Mutation testing (gremlins) confirmed the CONDITIONALS_BOUNDARY mutant survived at all four sites. Extract checkFileSize(relPath, size) so the guard lives in one place — DRYing the four call sites (behavior-identical: each still returns its own arity with the same sizeError) — and add a unit test that asserts the boundary at exactly MaxSingleFileBytes using int64 values, no fixture. Mutation-proven: flipping the helper's `>` to `>=` fails TestCheckFileSize/exactly_at_cap_is_allowed. make ci green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… too Bugbot (cli#260): the first pass extracted checkFileSize but Discover in walk.go still inlined the same size > MaxSingleFileBytes / sizeError pattern for labels.csv and images/ — leaving 2 boundary sites the extraction was meant to kill (gremlins flagged walk.go:169/238) and making the 'exactly one place' comment false. Route both through checkFileSize; now all 6 sites (image-layout, stream, tabular, text) share the single tested guard. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
Author
|
bugbot run |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit f0bb6c7. Configure here.
saadqbal
approved these changes
Jul 14, 2026
saadqbal
left a comment
Collaborator
There was a problem hiding this comment.
LGTM 👍 All six call sites are faithful 1:1 extractions (same name + size args, same sizeError), and centralizing the guard finally makes the > vs >= boundary assertable at exactly the cap without a 500 MB fixture. Clean.
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 free
to 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
Mutation testing (gremlins) surfaced a real gap: the
size > MaxSingleFileBytessingle-file-cap guard was duplicated at 4 sites (stream.go,tabular.go,text.go×2), and its boundary was untestable at every one —MaxSingleFileBytesis a 500 MBconst, so>vs>=could only be exercised with a 500 MB fixture. TheCONDITIONALS_BOUNDARYmutant survived at all four.Extract
checkFileSize(relPath, size)so the guard lives in one place (behavior-identical — each caller still returns its own arity with the samesizeError), and add a unit test that asserts the boundary at exactlyMaxSingleFileByteswith int64 values — no fixture.Mutation-proven: flipping the helper
>to>=failsTestCheckFileSize/exactly_at_cap_is_allowed.Test plan
make cigreen (build, vet, gofmt -s,go test -race, errcheck/ineffassign/misspell, schema-check).🤖 Generated with Claude Code
Note
Low Risk
Behavior-preserving refactor across push validation paths plus a new unit test; no API or cap value changes.
Overview
Centralizes the duplicated
size > MaxSingleFileBytesguard intocheckFileSize(relPath, size)inwalk.goand routes discovery, tabular, text sidecar, and stream-time tar paths through it instead of inline checks. Customer-facing behavior stays the same: over-cap still returns the existingsizeErrorwith the same file path and messaging.Adds
TestCheckFileSizeto assert the cap boundary with int64 sizes only—especially that a file exactly atMaxSingleFileBytesis allowed (>not>=)—so mutation testing can catch a>=flip without a 500 MB fixture.Reviewed by Cursor Bugbot for commit f0bb6c7. Bugbot is set up for automated code reviews on this repo. Configure here.