Skip to content

fix(preflight): mirror SequenceGroupValidator's null set for sequence ids (#239) - #240

Merged
aptracebloc merged 1 commit into
developfrom
fix/239-tsc-sequence-id-na-parity
Jul 13, 2026
Merged

fix(preflight): mirror SequenceGroupValidator's null set for sequence ids (#239)#240
aptracebloc merged 1 commit into
developfrom
fix/239-tsc-sequence-id-na-parity

Conversation

@LukasWodka

@LukasWodkaLukasWodka commented Jul 11, 2026

Copy link
Copy Markdown
Contributor

⚠️ Stacked on #218 — merge that first

This branch is based on the local feat/218-tsc-preflight-parity work (pandasDefaultNA), which is not on develop yet. Until #218 lands, this PR's diff shows two commits:

Once #218 is merged to develop, GitHub will drop fe46ff0 from the diff and this PR will show only the sequence-id fix. If #218 ships separately first, rebase this on the updated develop.


What

Closes #239. Same family as #218, a different validator not covered by it.

CheckSequenceRows / sequenceScanFrom (internal/push/preflight.go) decided whether a sequence_id cell is null by trimming it and testing the curated coercion.NA_SENTINELS map (naSentinels). But it previews the ingestor's SequenceGroupValidator, which plain-reads the CSV (pd.read_csv, keep_default_na=True) and computes null as:

ids=df[resolved]
null_mask=ids.isna() | (ids.astype(str).str.strip() =="")

So the faithful null set is pandas' default STR_NA_VALUES (matched on the raw cell — pandas tokenises NA before stripping) ∪ whitespace-only cells, not the coercion set. Two divergences:

sequence idpandas / ingestorold CLI (naSentinels+trim)direction
none (lowercase)real id → acceptin naSentinels → null → rejectover-reject (dangerous) — blocks an ingest the cluster accepts
#NAin STR_NA_VALUES → null → rejectnot in naSentinels → real id → missunder-reject
" NA " (padded)not an NA token → real idtrimmed to NA → nullover-reject

Fix

Reuse the pandasDefaultNA map #218 added for the grouped checks:

a sequence_id cell is null iff raw ∈ pandasDefaultNAORstrings.TrimSpace(raw) == "" — matched on the raw cell, dropping the pre-strip.

"" is already in pandasDefaultNA, so the whitespace clause only adds genuine whitespace-only cells. Distinct ids are counted on the raw value (pandas groups the object key verbatim, consistent with #218's labelConstantViolation / perGroupTimeViolation). naSentinels stays in use for the label-diversity read path (which the ingestor genuinely pins to coercion.NA_SENTINELS, keep_default_na=False).

Test plan

  • New parity casetsc-none-sequence-id (accept), driven against the realSequenceGroupValidatorgoldens.json regenerated at the pinned data-ingestors ref (7b4ecac); the goldens diff is exactly the one new accept entry, and scripts/sync-validator-goldens.sh --check is green.
  • Existing tsc-null-sequence-id (empty + NA) still rejects.
  • TestCheckSequenceRows gains none / #NA / whitespace-only / padded-NA boundaries, each ground-truthed against the validator's null_mask.
  • Mutation-proved: reverting sequenceScanFrom to naSentinels+trim fails the new parity case (over-reject) and the none/#NA/padded assertions (both directions).
  • go build / go vet / gofmt clean; go test ./... green; scripts/coverage-floor.sh green (internal/cli 73.9%, internal/submit 76.1%).

🤖 Generated with Claude Code


Note

Low Risk
Scoped to local preflight parity for time-series sequence id validation; behavior change only affects edge-case NA/id strings, with tests and parity pins guarding regressions.

Overview
Aligns CheckSequenceRows with ingestor SequenceGroupValidator: null sequence_id cells are detected with pandasDefaultNA on the raw CSV value plus whitespace-only, instead of trimmed coercion.NA_SENTINELS. Distinct sequence counts also use raw ids so grouping matches pandas.

This fixes over-reject when the id is the literal none (pandas keeps it) and under-reject for tokens like #NA, and treats padded values such as " NA " as real ids. Label diversity still uses naSentinels.

Adds unit boundaries and parity case tsc-none-sequence-id with updated goldens.

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

@LukasWodka
LukasWodka changed the base branch from develop to feat/218-tsc-preflight-parityJuly 11, 2026 14:41
@LukasWodka

Copy link
Copy Markdown
ContributorAuthor

Retargeted developfeat/218-tsc-preflight-parity to reconcile a cross-session snapshot mismatch. This PR was opened on the understanding that cli#218 (which added the pandasDefaultNA map this fix reuses) was unpushed with no PR — but #218 is in fact PR #234 (open, base develop). So this now stacks cleanly on #234 and its diff shows only the #239 sequence-id fix (8105679), not #218's commit.

Merge order: #234 first, then this. When #234 merges to develop (and its branch is auto-deleted), GitHub will retarget this PR to develop automatically. The fix itself is unchanged and green.

@aptracebloc
aptracebloc self-requested a review July 13, 2026 11:36
Base automatically changed from feat/218-tsc-preflight-parity to developJuly 13, 2026 11:42
… ids (#239)
CheckSequenceRows / sequenceScanFrom decided whether a sequence_id cell is
null by trimming the cell and testing the curated coercion.NA_SENTINELS map
(naSentinels). But it previews SequenceGroupValidator, which plain-reads the
CSV (pd.read_csv, keep_default_na=True) and computes null as
`ids.isna() | (ids.astype(str).str.strip() == "")` — i.e. pandas' DEFAULT
STR_NA_VALUES (matched on the raw cell) UNION whitespace-only cells, NOT the
coercion set. Two divergences resulted:
- "none" (lowercase): pandas keeps it → the ingestor ACCEPTS, but the CLI
treated it as null → false REJECT (the dangerous over-reject direction —
blocks an ingest the cluster accepts).
- "#NA": pandas drops it to NaN → the ingestor rejects, but the CLI's
coercion set lacks it → under-reject.
Reuse the pandasDefaultNA map cli#218 added for the grouped label/time checks:
null iff `raw ∈ pandasDefaultNA` OR `strings.TrimSpace(raw) == ""`, matched on
the raw cell (dropping the pre-strip), and count distinct ids on the raw value
(pandas groups the object key verbatim, consistent with the grouped previews).
Parity: new tsc-none-sequence-id case (accept), goldens regenerated against the
pinned data-ingestors ref; tsc-null-sequence-id (empty + NA) still rejects.
TestCheckSequenceRows gains none/#NA/whitespace/padded-NA boundaries — each
ground-truthed against the real validator's null_mask; reverting to naSentinels
fails the pins (mutation-proved).
Closes#239
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@aptracebloc
aptraceblocforce-pushed the fix/239-tsc-sequence-id-na-parity branch from 8105679 to 3bd3fe2CompareJuly 13, 2026 11:45
@aptracebloc
aptracebloc merged commit 17cad2f into developJul 13, 2026
17 checks passed
@aptracebloc
aptracebloc deleted the fix/239-tsc-sequence-id-na-parity branch July 13, 2026 11:48
LukasWodka added a commit that referenced this pull request Jul 14, 2026
…vs merged di#358 (cli#286)
di#365 adoption (ValidateImages/spec): the upstream ImageResolutionValidator
now validates min_size VALUES at construction — each side must be a positive
integer (integer-valued floats coerced; strings/bool/None/non-integral floats
rejected as a config error instead of surfacing as a phantom corrupt-image
error). No CLI code change is needed: ParseMinSize's WxH grammar (Atoi + >0)
is the equivalent gate at flag-parse time, so spec.file_options.min_size can
only ever carry the positive ints upstream accepts. Documented on
SpecArgs.MinSize + ValidateImages, and TestParseMinSize now pins the float
boundary ('16.5x32', '32.0x32' both rejected).
di#358 audit (CheckMaskIdColumn vs the merged MaskIdColumnValidator at
8f89aec) — semantics verified point for point, no divergence found:
- exact-lowercase 'mask_id' required after whitespace strip (ReadCSVHeader
trims like CSVIngestor's columns.str.strip())
- case/whitespace variant → rename hint (matchColumnIndex ≙ _match_column)
- empty scan tests the RAW untrimmed cell against NA_SENTINELS (naSentinels
is byte-identical, 12 tokens) + whitespace-only/missing — not trimmed-then-
matched (the #239/#240 padded-sentinel null-set trap)
- mid-read error fails closed on both sides
- duplicate stripped-equal headers: both sides inspect the FIRST exact match
- schema-declaration half satisfied by construction (buildImage always
declares mask_id VARCHAR(255)); csv_options dialect threading N/A (CLI
stages comma-separated UTF-8)
Recorded as a doc block on CheckMaskIdColumn.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
saadqbal pushed a commit that referenced this pull request Jul 14, 2026
…it di#358 (cli#286) (#305)
* chore(schema): bump data-ingestors pin to develop HEAD 8f89aec (cli#286)
Adopts the 6 upstream commits since the 7b4ecac (di#359) pin:
- di#358 — require-and-enforce semseg mask_id validator (backend#816)
- di#365 — min_size values validated as positive ints at construction (#348)
- di#369 — orphan-row reconcile before retry ingest (write path only)
- di#366 — content-comparison e2e (tests only)
- di#368 — ingest correlation id (write path only)
- the 0.7.0 version bump
Both sync scripts re-run against the new pin:
- scripts/sync-schema.sh: ingest.v1.json + layout.v1.json byte-identical
(no upstream schema change in the range) — no diff.
- scripts/sync-validator-goldens.sh: regenerated against the REAL
validators at 8f89aec — byte-identical goldens, so no verdict or
value-level drift for the existing corpus. di#358 only adds the semseg
MaskIdColumnValidator (no semseg parity cases yet) and di#365 only
changes verdicts for malformed min_size values the corpus never emits.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* docs(push): adopt di#365 min_size contract + audit CheckMaskIdColumn vs merged di#358 (cli#286)
di#365 adoption (ValidateImages/spec): the upstream ImageResolutionValidator
now validates min_size VALUES at construction — each side must be a positive
integer (integer-valued floats coerced; strings/bool/None/non-integral floats
rejected as a config error instead of surfacing as a phantom corrupt-image
error). No CLI code change is needed: ParseMinSize's WxH grammar (Atoi + >0)
is the equivalent gate at flag-parse time, so spec.file_options.min_size can
only ever carry the positive ints upstream accepts. Documented on
SpecArgs.MinSize + ValidateImages, and TestParseMinSize now pins the float
boundary ('16.5x32', '32.0x32' both rejected).
di#358 audit (CheckMaskIdColumn vs the merged MaskIdColumnValidator at
8f89aec) — semantics verified point for point, no divergence found:
- exact-lowercase 'mask_id' required after whitespace strip (ReadCSVHeader
trims like CSVIngestor's columns.str.strip())
- case/whitespace variant → rename hint (matchColumnIndex ≙ _match_column)
- empty scan tests the RAW untrimmed cell against NA_SENTINELS (naSentinels
is byte-identical, 12 tokens) + whitespace-only/missing — not trimmed-then-
matched (the #239/#240 padded-sentinel null-set trap)
- mid-read error fails closed on both sides
- duplicate stripped-equal headers: both sides inspect the FIRST exact match
- schema-declaration half satisfied by construction (buildImage always
declares mask_id VARCHAR(255)); csv_options dialect threading N/A (CLI
stages comma-separated UTF-8)
Recorded as a doc block on CheckMaskIdColumn.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
---------
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
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.

3 participants

@LukasWodka@aptracebloc@saadqbal