Uh oh!
There was an error while loading. Please reload this page.
fix(push): text preflight fails closed on a labels.csv read error - #221
Merged
Conversation
…reflight) manifestReferencedTextNames skipped a failed CSV read with `continue`, returning a partial referenced set — so the text files named by the unread rows would silently escape the enforced record-format check (local fail-open), and a *persistent* read error would spin the loop forever. The image mirror-check (CrossCheckLabels) already aborts on the same read error; make the enforced-text path match and fail closed. The trigger is I/O, not malformed CSV content: openCSVReader sets LazyQuotes + FieldsPerRecord=-1, so every bad CSV shape parses cleanly (like pandas) — the branch is only reachable via a genuine read failure. The parse loop is split into referencedTextNames(*csv.Reader) so that branch can be exercised with an injected failing reader. Addresses the Bugbot finding on #219. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
LukasWodka
approved these changes
Jul 10, 2026
Uh oh!
There was an error while loading. Please reload this page.
saadqbal added a commit
that referenced
this pull request
Jul 10, 2026
…preflight (#222) Two more mirror-checks swallowed a non-EOF csv.Reader.Read error with `continue`, same class as #221: - readLabelColumnValues (CheckLabelDiversity / ReadLabelValues): a mid-read failure returned a PARTIAL class set with Found=true — under-counting classes could false-reject good data or pass a bad file. Now returns an error; the diversity GATE fails closed, while the value PREVIEW (ReadLabelValues) degrades to Found=false. - CheckSequenceRows: a mid-read failure skipped the unread tail, so null/missing sequence ids there never surfaced locally. Now fails closed. CrossCheckLabels and the tabular schema scan already abort on the same error; this brings the whole package to uniform fail-closed behavior. The trigger is I/O, not malformed CSV (LazyQuotes + FieldsPerRecord=-1 tolerate every bad shape). Each scan is split into a reader-based core (labelColumnValuesFrom / sequenceScanFrom) so the branch is exercised with an injected failing reader; parsing is unchanged and the value parity harness validates the relocation. Addresses the two follow-up Bugbot findings on #219. Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
This was referenced Jul 29, 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.
What
manifestReferencedTextNames(enforced-text preflight) skipped a failedlabels.csvread withcontinue, returning a partial referenced set — so the text files named by the unread rows would silently escape the enforced record-format check (local fail-open). A persistent read error would also spin the loop forever.The image mirror-check
CrossCheckLabelsalready aborts on the same read error; this makes the enforced-text path match and fail closed.Bugbot framing vs. reality
Bugbot flagged this as "skips bad CSV rows." That specific scenario isn't reproducible:
openCSVReadersetsLazyQuotes+FieldsPerRecord=-1, so every malformed CSV shape (bare/unbalanced quotes, ragged rows, null bytes, stray CR) parses cleanly — exactly like pandas. Verified empirically. Theerr != nilbranch is therefore reachable only via a genuine I/O read error, not content. The fix is still correct (fail closed + consistent with the image sibling + removes the latent infinite loop); the code comment and test say so honestly rather than repeating the "bad row" framing.Test
The branch can't be hit through a real file, so the parse loop is split into
referencedTextNames(*csv.Reader)and exercised with an injected reader that yields the header then fails — asserting the walk returns the wrapped read error instead of a partial set.Scope
Bugbot drive-by on promote PR #219; rolls up under release ticket #220 (no separate kanban item). Targets
developso it joins the v0.8.0 payload before staging QA.🤖 Generated with Claude Code
Note
Medium Risk
Changes local dataset preflight validation for enforced text record formats; incorrect behavior could let some manifest-referenced files skip checks, but the change tightens fail-closed behavior to match existing image preflight.
Overview
Enforced-text preflight no longer keeps walking
labels.csvwhen a row read fails with a real I/O error. It now aborts and returns a wrappedreading labels.csverror, matching howCrossCheckLabelshandles the same failure for image layouts.The manifest walk is split into
referencedTextNames(*csv.Reader)so that behavior can be tested without a real file. A test uses an injected reader that serves the header then fails mid-stream, asserting discovery does not return a partial referenced filename set (which would skip validation for unread rows).Comments clarify that with
LazyQuotesandFieldsPerRecord=-1, malformed CSV rows are not what hit this branch—only genuine read failures do.Reviewed by Cursor Bugbot for commit 1831a54. Bugbot is set up for automated code reviews on this repo. Configure here.