Skip to content

test(domains): cross-pack domain-validation lane - #482

Merged
kevincostner17 merged 1 commit into
mainfrom
test/domain-validation
Sep 16, 2026
Merged

kevincostner17 merged 1 commit into
mainfrom
test/domain-validation

Conversation

@kevincostner17

Copy link
Copy Markdown
Contributor

Lane: domain validation (Phase 8)

A cross-cutting suite for src/freshdata/domains/ — the eight packs, the shared
ConfigDrivenValidator engine, the registry and the bundled reference layer.
The per-pack suites under tests/domains/ already check that each pack flags the
right rows; this file covers what only shows up when the packs are compared with
one another, or handed input they were never meant to see.

One new file, tests/test_domain_validation_lane.py (66 tests). No source
changes — the findings below are reported, not fixed, so the team can decide the
disposition of each. Every test asserts the behaviour that exists today, so a
deliberate fix has exactly one place to update.

Full suite: pytest -m "not online and not large" -p no:randomly
6723 passed, 26 skipped, 0 failures, coverage 94.02% (gate 93%).
ruff check . passes repo-wide.

Verified — these hold

Property How it was checked
No pack silently validates another pack's data Full 8×8 matrix of good frames × packs. Only the diagonal is (1.0, True); every off-diagonal cell is exactly (0.0, False).
No rule ever reports passed for a field that is absent Swept: drop each mapped column in turn from each pack's own good frame, assert no rule naming that canonical field returns passed. Zero offenders across all 8 packs.
run_domain never mutates its input SHA-256 digest over columns, index, dtypes and every cell value — not assert_frame_equal, which tolerates dtype drift. Holds for all 8 packs on dirty frames that actually trigger repairs, on both the unique-index and the non-unique-index (internal re-index) path.
Declared bounds are inclusive stop_lat/stop_lon, Modbus register_address [0, 65535], the two nested soil-pH bands, product_description ≤ 200, runtime_seconds ≤ 86400, the 2-decimal amount scale — exact boundary passes, one step outside fails.
Identifier columns are never imputed or dropped Blank every mapped id_field per pack; row count unchanged and the cell stays null.
Reference snapshots never silently rewrite an unknown value Every repair: coerce reference rule (AG-007, AG-010, ENG-008) logs unresolvable and leaves the cell exactly as it was.
Version and schema version reach the report All 8 packs carry version + schema_version on ValidationReport, in to_dict() and in summary(). Healthcare versions each FHIR resource separately; finance_mode="tick" versions separately.
Selector packs raise rather than guess Healthcare → AmbiguousFHIRResourceError, media → AmbiguousMediaTypeError, transport → DomainError, on every one of the other packs' frames.
Skipped rules never affect the trust score The one property CONTRIBUTING_DOMAINS.md states. Transport single-file mode skips 10 of 14 rules and still scores 1.0.
Rule ids are globally unique Across all 8 packs and all sub-schemas.

Findings

# Severity Finding
1 S2 ConfigDrivenValidator._check_regex stringifies a float64 column with a .0 suffix, so a numeric code column loaded from CSV with one blank cell fails a digit-only pattern. Hits both regex rules in the repo: GS1-008 [0-9]{8} and FIN-008 [A-Za-z0-9]{4,12}. 10000266 in an int64 column passes; the same code in a float64 column becomes "10000266.0" and is flagged. Both are warning severity so passed does not flip, but every row raises a false finding and the trust score drops. The repo already solves exactly thisretail/validator.py::_integral_float_text, whose docstring describes this same CSV-blank-cell scenario — so the intended behaviour is settled and the shared engine simply does not apply it. Test: test_regex_check_misreads_an_integral_float_column.
2 S2 Packs disagree on whether "absent from a documented subset" means "invalid". bundled/uom_codes.json says in its own _meta: "Curated subset … not the exhaustive UN/CEFACT list" (50 of ~1800 Rec-20 codes), yet GS1-006 is named "net_content_uom is a valid UN/CEFACT code" and is error severity — a real Rec-20 code outside the subset sets passed=False and trust to 0.0. energy/quality_codes.json ("NOT an exhaustive or vendor-specific quality model; specific historians may expose finer-grained sub-statuses") behaves the same via ENG-008, as do the curated agriculture unit sets. Healthcare is the counter-example and the model to follow: HC-C005 / HC-O004 / HC-O010 are warning severity and are named "… is in the documented common set". Nothing is mutated, so this is a wrong disposition, not corruption. Decision needed: make subset-gated reference rules warnings pack-wide, or complete the sets. Tests: test_absent_from_a_disclaimed_subset_is_reported_as_error, test_healthcare_treats_absence_from_a_subset_as_a_warning.
3 S3 + spec gap domain_trust_score is not comparable across domains. Measured formula: clamp01(1 − Σ_violated weight(severity) × min(rows_violated / n_rows, 1)), weight = {error: 1.0, warning: 0.25, info: 0.05}, rounded to 4 dp; skipped and passed rules contribute nothing; a missing required column is charged a row fraction of 1.0 regardless of frame size. Three measured reasons it cannot be compared between packs: (a) the penalty is an unnormalised sum over rules, so denser packs fall faster — max accruable penalty is 21.00 for media (24 rules) but 6.75 for finance (9), i.e. the 0.0 floor is reached after ~5% of media's rules versus ~15% of finance's; (b) packs cover the same defect with different rule counts — one bad cell in four rows costs 0.25 in finance (one rule) but 0.3125 in agriculture (an error and a warning rule on soil pH); (c) severity conventions differ per pack for the same kind of check (finding 2). It also saturates: one fully-violated error rule and five fully-violated rules both score exactly 0.0, so it does not rank even within a pack at the low end. The repo documents none of this — CONTRIBUTING_DOMAINS.md says only "skipped rules never affect the trust score". Missing decision: is this an absolute, cross-pack data-quality index, or a within-pack/within-schema indicator? Tests: TestTrustScore (7 tests).
4 S3 Missing fields and rule violations are not commensurate. A missing required column costs the full severity weight and silences every other rule on that field (they are skipped). In a 1000-row finance frame, dropping currency scores 0.0 while one bad currency value scores 0.999 — a step, not something proportional to lost data. Correct as a conservative choice; recorded because it is the answer to "how do missing fields vs rule violations affect the score". Test: test_a_missing_column_costs_the_full_weight_and_silences_its_other_rules.
5 S3 MISSING_REQUIRED_FIELD is an in-band sentinel in the row-label channel. _trust_score decides "table-level finding" by testing whether any violation_rows entry equals that literal string, so a frame whose index carries that label has an ordinary one-row violation charged as a whole-table one (0.75 → 0.0). No built-in pack uses repair: reject, so today the blast radius is the score alone — but the sentinel belongs on RuleResult as a flag. Test: test_missing_required_field_sentinel_collides_with_a_row_label.
6 S3 FIN-006 declares tolerance: 0.01 but compares raw binary floats: abs(100.00 − 99.99) is 0.010000000000005116, so a one-cent rounding difference — exactly what a 0.01 tolerance exists to absorb — is reported as an error. The tolerance admits nothing at its own boundary. Fix would be to round the difference to the rule's scale before comparing. Test: test_balanced_tolerance_never_admits_a_one_cent_imbalance.
7 S3 An incorrect column_map fails quietly in three distinct ways, none of which raises or logs a diagnostic: a typo'd source column is dropped and detection silently falls through to the normal exact/alias search, so the user gets a different column than they asked for with method: "exact" in the log; an unknown canonical field ("currency_code" instead of "currency") is never consulted, because detection only iterates the pack's own field names; two columns mapped to one canonical field silently keeps only the last, because the override dict is inverted. Validating column_map keys against canonical_fields and its values against df.columns would cost one loop. Tests: TestColumnMapOverrides (6 tests).
8 S3 describe() has no stable shape for healthcare before resource detection. Every other pack answers with the same eight keys; an un-activated HealthcareValidator answers with a five-key placeholder, so a caller building an audit trail from describe()["schema_version"] raises KeyError for healthcare and only for healthcare. Documented by a note key, so arguably deliberate — but it is an unannounced shape change in a JSON audit surface. Test: test_describe_is_uniform_across_packs_except_unresolved_healthcare.
9 S4 media_type swaps the effective rule set but not the schema_version — both sub-schemas report eidr-ddex-2024, unlike healthcare (per-resource) and finance (finance-tick/2025.06). describe() reports media_type; ValidationReport does not, so a stored report identifies the sub-schema only indirectly, via the MD-C* / MD-R* rule-id prefix. Test: test_media_sub_schema_is_not_distinguished_by_schema_version.
10 S4 With a non-unique input index, run_domain re-indexes internally and maps findings back to the caller's labels, so a duplicated label identifies a group of rows rather than the flagged row: findings ["a", "b"] where df.loc[["a", "b"]] selects three rows. Labels are always valid; they are just not unique. Test: test_reported_row_labels_are_translated_back_but_stay_ambiguous.

Suspected, then disproved

  • "Retail silently validates finance data via the description alias." The alias match is real — retail's product_description does bind to finance's description column, and GS1-007 genuinely returns passed on it. But gtin is absent, GS1-001 fires with MISSING_REQUIRED_FIELD, and the frame comes back passed=False, trust 0.0. Partial alias luck never adds up to a validation. Pinned as test_a_shared_column_name_alone_never_produces_a_pass.
  • "Column aliases do not match names containing spaces." They do not — re.fullmatch(r"txn_?id", "Txn ID") fails, so detect_columns leaves transaction_id unmapped for the repo's own messy_finance fixture. This was my harness error, not a defect: fd.clean normalises column labels before the pack runs (_normalized_column_map, cfg.column_names), so through the public entry point "Txn ID" is already txn_id and the alias matches. Direct get_validator(...).detect_columns(raw_df) is a lower-level surface that does not get that normalisation.
  • "run_domain's shallow copy for a non-unique index leaks the RangeIndex back to the caller." df.copy(deep=False) gives the copy its own axes, so assigning working.index does not touch the input. Verified by digest for all 8 packs: the input's index, dtypes and values are byte-identical afterwards.
  • "A MISSING_REQUIRED_FIELD row label could get a row dropped by repair: reject." No built-in pack declares reject anywhere (coerce and flag_only only), so finding 5 is confined to the score today.
  • "Aliases might grab misleadingly-named neighbours." They do not: matching is re.fullmatch, so credit_limit and salary_credit are both left alone while a bare cr binds and is logged with method: "regex".

Notes

  • Every pack's per-canonical-field mapping decision is logged with its method (exact / case_insensitive / regex / override / missing), one entry per canonical field, with no gaps — that audit log is what makes findings 7 and the by-design "a swapped column_map validates cleanly" case reviewable rather than silent.
  • The bundled sets all carry version and source in _meta, and every pack's reference_sources() stays JSON-serialisable.

Adds tests/test_domain_validation_lane.py: a cross-cutting suite for the
eight domain packs, covering the properties that only show up when packs
are compared with one another or handed input they were never meant to see.

What it pins:

- every pack scores 1.0 on its own good frame, and the engine's reported
  layer/severity/check/repair for every rule matches its rules.yaml entry;
- all declared numeric and length bounds are inclusive (lat/lon, Modbus
  register, soil pH's two nested bands, description length, runtime);
- an 8x8 wrong-domain matrix: only the diagonal validates, everything
  off-diagonal is trust 0.0 / passed False;
- no rule ever reports "passed" for a field that is absent, swept over
  every mapped column of every pack;
- the exact arithmetic of _trust_score, per severity weight, plus three
  measured reasons the score is not comparable across domains;
- subset-based reference sets never silently rewrite an unrecognised
  value (coerce logs "unresolvable");
- version / schema_version reach ValidationReport for all eight packs;
- run_domain never mutates its input, verified with a SHA-256 digest over
  columns, index, dtypes and values, on both the unique and non-unique
  index paths.

Behaviours the tests record as findings (current behaviour asserted, so a
deliberate fix has one obvious place to update):

- _check_regex stringifies a float64 column as "10000266.0", so FIN-008
  and GS1-008 raise false findings for a CSV column with one blank cell,
  although retail/_integral_float_text already solves exactly this;
- FIN-006 declares tolerance 0.01 but compares raw binary floats, so a
  one-cent imbalance is never within tolerance;
- retail, energy and agriculture report "absent from our documented
  subset" as error severity where healthcare reports it as a warning;
- MISSING_REQUIRED_FIELD is an in-band row-label sentinel, so a frame
  indexed by that literal string mis-scores;
- a column_map entry naming an absent column or an unknown canonical
  field is silently ignored;
- healthcare's describe() has no schema_version before resource detection.
@coderabbitai

coderabbitai Bot commented Sep 16, 2026

Copy link
Copy Markdown
Contributor

Important

  • 🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: 59c3a5ed-fd47-4888-a7d9-efcd4392273d


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@kevincostner17
kevincostner17 merged commit 422ab04 into main Sep 16, 2026
19 checks passed
Sign up for free to 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.

1 participant