fix(semantic): a replayed repair needs corroboration in the frame it lands on - #489
Merged
Merged
Conversation
…lands on
A repair learned on one dataset was replayed onto another whose column meant
something different, and applied automatically:
A: segment = M/F -> learns M -> male
B: segment = S/M/L -> becomes S/male/L/male (status=automatic,
human_review=False,
confidence 0.95)
Clothing sizes silently rewritten as genders.
The conflict machinery already existed and worked: _conflict_proposal turns a
deterministic-vs-memory clash into a high-risk unsafe_ambiguous record that is
never auto-applied. But it only fires on disagreement. On B the
CategorySynonymExpert correctly abstains -- the column is not gender-like --
and an abstention is not a disagreement, so _merge_proposals passed the
replayed repair through carrying the full confidence it earned elsewhere:
det_list = det_by_key.get(key)
if not det_list:
merged.append(mem_p) # no deterministic proposal at all
continue
Every other branch was careful. Only total silence had no handling, and silence
is exactly what happens when the evidence behind a learned repair is absent.
The stored column_signature/value_signature would not have caught it: both
columns profile as role="categorical", semantic_type=None, free_text=False.
A replayed repair whose issue type is context-dependent -- category_synonym,
boolean_synonym, reference_value, where the same token means different things
in different columns -- is now demoted to a review-required suggestion when
nothing in this frame corroborates it. Context-free repairs (encoding_repair,
format_alignment, numeric_format and the rest) are untouched: they mean the
same thing in every column, so demoting them would cost the feature without
buying any safety.
sizes + gender memory S/M/L kept, suggested / risk=high / human_review
genders + gender memory still auto-applied -- the feature still works
Provenance survives the demotion, which matters more for a held decision than
an applied one: memory_influenced, model_id ending :memory, backend="memory",
evidence kinds memory_replay and a new memory_uncorroborated note carrying the
reason.
4 of the 5 new tests fail on main; the one that passes is the
feature-still-works control.
This also flips five tests that #481 pinned as current behaviour when it found
the defect. They were tripwires for the fix and the fix tripped them; each now
asserts the repaired behaviour with its history kept in the docstring. Two
findings in that file stay pinned because they remain true: freshdata_version
is written and never read, and the SQLite store overwrites on a dataset_id
collision.
Full suite 7098 passed / 0 failed, coverage 94.94%; ruff clean repo-wide.
Contributor
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 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. Comment |
FreshData benchmark report —
|
| fixture | n_rows | n_cols | p50 s | p95 s | peak MB | repair % | false-repair % | preserve % | trust | monotonic | export % |
|---|
Authored-code reduction (Metric 6)
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 free
to 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.
Summary
A repair learned on one dataset was replayed onto another whose column meant something different, and applied automatically:
Clothing sizes silently rewritten as genders. This is the trap the brief names directly: "Dataset A:
M -> Male. Dataset B:M -> Medium. The library must not blindly replay Dataset A's decision into Dataset B."Found by the Phase 16 lane (#481) and reproduced independently before fixing.
Root cause
The conflict machinery already existed and worked —
_conflict_proposalturns a deterministic-vs-memory clash into a high-riskunsafe_ambiguousrecord that is never auto-applied. But it only fires on disagreement.On B the
CategorySynonymExpertcorrectly abstains (the column is not gender-like), and an abstention is not a disagreement:Every other branch was careful — agreement kept the higher confidence, disagreement produced
unsafe_ambiguous, flag-vs-repair was resolved deliberately. Only total silence had no handling, and silence is exactly what happens when the evidence behind a learned repair is absent from the new frame.The stored signatures would not have caught it: both columns profile as
role="categorical", semantic_type=None, free_text=False.The fix
A replayed repair whose issue type is context-dependent —
category_synonym,boolean_synonym,reference_value, where the same token means different things in different columns — is demoted to a review-required suggestion when nothing in this frame corroborates it.Context-free repairs (
encoding_repair,format_alignment,numeric_format, …) are untouched: they mean the same thing in every column, so demoting them would cost the feature without buying any safety.S/male/L/male, auto-appliedS/M/Lkept —suggested/risk=high/human_review=TrueProvenance survives the demotion, which matters more for a held decision than an applied one:
memory_influenced,model_idending:memory,backend="memory", evidence kindsmemory_replayand a newmemory_uncorroboratednote carrying the reason.Note for reviewers
This flips five tests that #481 pinned as current behaviour when it found the defect. They were tripwires for the fix and the fix tripped them; each now asserts the repaired behaviour with its history kept in the docstring.
Two findings in that file stay pinned because they remain true:
freshdata_versionis written and never read, and the SQLite store overwrites on adataset_idcollision.This is the third pinned-defect collision in this programme (after #485 and #488) — worth grepping
DEFECT/FINDING (Sbefore landing any behaviour change.Verification
main, the one that passes being the feature-still-works control.ruff check .clean repo-wide.