Skip to content

fix(push): fail closed on labels.csv read errors in label + sequence preflight - #222

Merged
saadqbal merged 1 commit into
developfrom
fix/preflight-csv-read-failclosed
Jul 10, 2026
Merged

fix(push): fail closed on labels.csv read errors in label + sequence preflight#222
saadqbal merged 1 commit into
developfrom
fix/preflight-csv-read-failclosed

Conversation

@saadqbal

@saadqbalsaadqbal commented Jul 10, 2026

Copy link
Copy Markdown
Collaborator

What

Two more preflight mirror-checks swallowed a non-EOF csv.Reader.Read error with continue — the same fail-open class as #221. Follow-up Bugbot findings on the promote PR #219.

  • readLabelColumnValues (backs CheckLabelDiversity + ReadLabelValues): a mid-read failure returned a partial class set with Found=true. An under-counted column could false-reject good data or let a bad file pass. Now returns an error → the diversity gate fails closed; 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 makes the whole internal/push package uniform.

Trigger (same correction as #221)

Not malformed CSV content: openCSVReader sets LazyQuotes + FieldsPerRecord=-1, so every bad shape parses like pandas. The branch is only reachable via a genuine I/O read error.

Test

Each scan is split into a reader-based core (labelColumnValuesFrom / sequenceScanFrom) so the branch is exercised with an injected fail-after-header reader (reusing failAfterReader from #221). Parsing is byte-for-byte unchanged; the value parity harness passes, proving the relocation is faithful.

Scope

Bugbot drive-bys on #219; roll up under release ticket #220 (no separate kanban item). Targets develop → joins the v0.8.0 payload with #221.

🤖 Generated with Claude Code


Note

Low Risk
Localized preflight error-handling only; happy-path parsing is unchanged and parity harness coverage is noted in the PR.

Overview
Label and sequence CSV preflight no longer treat a mid-file csv.Reader I/O error as a skippable row. That path used to continue and could return a partial class set or pass a scan whose tail never ran—matching the fail-open bug fixed in #221 for text/CrossCheckLabels.

readLabelColumnValues now returns an error on mid-read failure; CheckLabelDiversity propagates it and fails closed. ReadLabelValues still ignores that error and reports Found=false so previews do not show wrong counts. The scan logic moves into labelColumnValuesFrom on an injected reader for tests.

CheckSequenceRows does the same for sequence ids via sequenceScanFrom: read errors surface as wrapped failures instead of hiding null/missing ids in unread rows.

New unit tests use the existing failAfterReader helper to assert both paths abort with the underlying error and do not mix read failures with domain null-id errors.

Reviewed by Cursor Bugbot for commit fc97b90. Bugbot is set up for automated code reviews on this repo. Configure here.

…preflight
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>
@saadqbal
saadqbal merged commit f05d01d into developJul 10, 2026
20 checks passed
@saadqbal
saadqbal deleted the fix/preflight-csv-read-failclosed branch July 10, 2026 15:36
@saadqbal

Copy link
Copy Markdown
CollaboratorAuthor

Added the root-cause fix on top of the fail-closed change: openCSVReader set FieldsPerRecord=-1 but not LazyQuotes, so a bare/unescaped quote (which pandas tolerates) made Read error. That meant every preflight mirror-check false-rejected a CSV the ingestor accepts — and made the fail-closed change reachable via a bare-quote row (fail-open → false-reject). Now LazyQuotes is set centrally (+ in InferSchema's own reader), so the readers are pandas-faithful and the remaining read error is genuinely I/O-only. Full parity harness still passes + added a bare-quote guard test. This is the real fix behind the read-error findings Bugbot kept surfacing.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@saadqbal@LukasWodka