Skip to content

Pair renames by field ID, split widening from additive, record enum drift - #7

Merged
rubinder merged 2 commits into
mainfrom
feat/drift-classification
Aug 31, 2026
Merged

Pair renames by field ID, split widening from additive, record enum drift#7
rubinder merged 2 commits into
mainfrom
feat/drift-classification

Conversation

@rubinder

Copy link
Copy Markdown
Owner

Closes #1.

What was wrong

The agent reported checkNumber → check_reference as two findings — a breaking drop and an additive add — on the one table whose headline property is that a rename costs nothing because the field ID does not move. A reader was told a column had vanished and, separately, that an unfamiliar column had appeared, and left to connect them.

The evidence to do better was already being fetched and discarded: pyiceberg_engine.schema_history() returns field_ids; sensors.observe() read only ["columns"].

What changed

sensors.build_rename_map() groups every historical column name by Iceberg field ID. A name gone from the latest schema whose ID is still present is a rename; an ID that is gone is still a drop and still breaking. Transitive renames (a → b → c) resolve in one hop because the map keys on the ID rather than on consecutive pairs.

Severity vocabulary grows to breaking / renaming / widening / additive / enum_drift / benign. renaming is actionable — no data moved, but the published contract names a column that doesn't exist, and that needs a recorded decision. widening is not: an int → long and a new column are both safe, but only one means the contract is behind on a column that already existed.

Enum drift lands as enum_watch, a new contract key, not an expectation. Everything in expectations is a fail-closed gate and _evaluate raises on unknown types — which is what stops a typo'd check reading green forever. A watch that must never block would have needed a branch that always passes: the vacuous check that design exists to prevent. baseType keeps its accepted_values gate and is deliberately not watched — one column, one semantics.

Measured, at full scale, on a warehouse with no ops.monitor_results history

Sequence Before After
all → agent 0 findings 0 findings
all → drift-demo → agent 5 findings, 2 incidents 4 findings, 2 incidents
all → monitor → drift-demo → agent 6 findings, 3 incidents 5 findings, 3 incidents

Each drops by exactly one: the rename's phantom second finding. The 7 new enum watches add zero findings to the clean run.

A guard test in the first draft was vacuous

It supplied renames as a fixture, so it exercised detect() and never build_rename_map. A planted position-matching mutant passed all 19 tests.

The reason it survives is not obvious: a rename does not move a column, so ordinal matching gets plain renames right, and gets rename-plus-add right too since PyIceberg appends. It breaks only when a column is deleted from the middle — at which point it invents a rename that never happened and hides the real drop.

Three tests now build the map from a {a:1, b:2, c:3} → {a:1, c_new:3} history where position and field ID disagree. Both the position mutant and a name-similarity mutant fail against them, as does a real Iceberg table with a delete and a rename in one step.

Fixing the contract also surfaced a second latent gap: _FakeEngine in test_agent_graph.py had no category column, so all 7 watches correctly reported "not monitoring anything." The stub now seeds watched columns from the contract's own registered values, so it stays clean by construction.

Written up in docs/ai-sdlc/decisions/0006.

Known gap, stated not hidden

drift-demo's settlementDays int → long still reports as additive, because drift-demo adds that column before widening it — the contract never declared it, so there is no narrower type to compare against. widening fires when a declared column widens, covered against a real table by test_real_widening_classifies_as_widening. Declaring settlementDays in the contract to make the demo read better would make the clean run report a phantom dropped column.

Verification

  • 246 passed (224 before, 22 new), ruff clean.
  • Both drift sequences re-run end to end at full scale; every README number replaced with what was measured.
  • The stale checkNumber-3955ba01.md incident is removed — its slug changed because the evidence changed from dropped to renamed.

🤖 Generated with Claude Code

https://claude.ai/code/session_0199ePw5w34FyfGAB41mkv3G

rubinder and others added 2 commits August 31, 2026 12:43
…enum drift

Closes #1.

The agent reported `checkNumber -> check_reference` as two findings -- a
`breaking` drop and an `additive` add -- on the one table whose headline
property is that a rename costs nothing because the field ID does not move.
The evidence was already being fetched and thrown away: schema_history()
returns field_ids, observe() read only ["columns"].

`build_rename_map` groups every historical column name by field ID, so a name
gone from the latest schema whose ID is still present is a rename, not a drop.
Transitive renames (a -> b -> c) resolve in one hop because the map keys on the
ID rather than on consecutive pairs. A field ID absent from the latest schema
stays a genuine drop and stays breaking.

Measured at full scale, no ops.monitor_results history:
  all -> agent                        0 findings          (unchanged)
  all -> drift-demo -> agent          5 -> 4 findings, 2 incidents
  all -> monitor -> drift-demo -> agent   6 -> 5 findings, 3 incidents

`renaming` is ACTIONABLE: no data moved, but the published contract now names a
column that does not exist, and that needs a recorded decision. `widening` is
not -- an int -> long and a brand-new column are both safe, but only one means
the contract is behind on a column that already existed.

Enum drift lands as `enum_watch`, a new contract key, NOT an expectation.
Expectations are fail-closed gates and `_evaluate` raises on unknown types,
which is what stops a typo'd check reading green forever; a watch that must
never block would have needed a branch that always passes -- the vacuous check
that design exists to prevent. baseType keeps its accepted_values gate and is
deliberately not watched: one column, one semantics. A watch that reads no
values reports breaking rather than passing, and one aimed at a >200-cardinality
column reports itself misconfigured.

A guard test in the first draft was vacuous. It supplied `renames` as a fixture,
so it exercised detect() and never build_rename_map, and a planted
position-matching mutant passed all 19 tests. A rename does not move a column,
so ordinal matching gets plain renames right and breaks only when a column is
deleted from the middle. Three tests now build the map from a
{a:1,b:2,c:3} -> {a:1,c_new:3} history where position and field ID disagree;
the position mutant and a name-similarity mutant both fail against them, as does
a real Iceberg table with a delete and a rename in one step. Written up in
docs/ai-sdlc/decisions/0006.

Reviewed: 246 tests pass (224 before), ruff clean, both drift sequences re-run
end to end at full scale and the README numbers replaced with what was measured.
The settlementDays widening still reports as `additive` because drift-demo adds
that column before widening it, so no narrower type was ever declared; stated in
the README rather than papered over by editing the contract, which would make
the clean run report a phantom drop.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0199ePw5w34FyfGAB41mkv3G
…mpty column read clean

Two defects found by probing the shipped code rather than re-reading the diff.
Both are shapes this repository has fixed before.

1. An enum watch on a struct column raised `TypeError: unhashable type: 'dict'`
   out of both observe() and detect(). Bronze has three struct columns
   (amount, runningBalance, merchant), so it is a plausible config mistake, and
   the blast radius was not one bad column but a dead sensor for the whole
   table -- the same failure the unparseable-date guard exists to prevent.
   Guarded at the source and again in detect() for a hand-built ObservedState.
   Reports `not_comparable`, classifies benign. The reason lives in a separate
   `enum_errors` dict so "could not evaluate" can never share a representation
   with "no drift".

2. A watched column holding no values at all reported clean: zero values means
   zero new values. validator._evaluate already settled this in the other
   direction for `range` ("a column with nothing in it is not a column that is
   in range") and `freshness`; the enum watch now agrees and classifies
   `no_values` as breaking.

Four regression tests, including the healthy case, so the empty-column guard
cannot become noise on every run.

Reviewed: 250 tests pass (246 before), ruff clean. Both end-to-end sequences
re-run at full scale after the fix -- clean is still 0 findings with all seven
watches active, post-drift is still 4 findings and 2 incidents.

Also recorded a narrower limitation in decisions/0006: a rename whose old name
is immediately reused by a new column reports as `type_changed` rather than
`renamed`. Safe (still breaking, a human still looks) but describes the wrong
event; resolving it needs field IDs in contracts, which belongs with #3.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0199ePw5w34FyfGAB41mkv3G
@rubinder

Copy link
Copy Markdown
Owner Author

Review round 1 — two defects found, both fixed in 7fc9225

Reviewed by probing the shipped code adversarially rather than re-reading the diff. Both findings are shapes this repo has fixed before.

1. An enum watch on a struct column crashed the sensor.

TypeError: unhashable type: 'dict'

Raised out of observe() and detect(). Bronze has three struct columns (amount, runningBalance, merchant), so this is a config mistake a human could plausibly make — and the blast radius wasn't a bad reading on one column, it was a dead sensor for the whole table. The exact failure the unparseable-date guard exists to prevent: a monitor that crashes on bad data stops monitoring at the moment something has gone wrong.

Guarded at the source and again in detect(). The reason lives in a separate enum_errors dict, deliberately not in enum_values, so "could not evaluate" and "no drift" cannot share a representation.

2. An entirely-NULL watched column reported clean.

Zero values means zero new values, so the diff read green precisely when a categorical column had stopped being populated. validator._evaluate had already settled this in the other direction for range ("a column with nothing in it is not a column that is in range") and freshness. The enum watch now agrees: no_values classifies breaking.

Four regression tests, including the healthy case, so the empty-column guard can't become noise on every run.

Also recorded, not fixed

A rename whose old name is immediately reused by a new column reports as type_changed, not renamed — the old name is still present, so detect() never consults the rename map. Safe (still breaking, a human still looks) but it describes the wrong event. Resolving it needs field IDs in contracts, which belongs with #3. Written into decisions/0006.

Verification after the fix

  • 250 passed (246 before the review, 224 on main), ruff clean.
  • Both end-to-end sequences re-run at full scale: clean is still 0 findings with all seven watches active; post-drift is still 4 findings, 2 incidents.

@rubinder
rubinder merged commit 5fbe51e into main Aug 31, 2026
2 checks passed
@rubinder
rubinder deleted the feat/drift-classification branch August 31, 2026 16:57
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.

Drift classifier: detect renames by field ID, name widening as its own class, and record enum drift

1 participant