diff --git a/scripts/audit_evidence_snippets.py b/scripts/audit_evidence_snippets.py index 3ffaaaca..2a10c7ea 100644 --- a/scripts/audit_evidence_snippets.py +++ b/scripts/audit_evidence_snippets.py @@ -32,9 +32,15 @@ this audit exists to catch, so it asks for a check against the source rather than asserting a defect. -Baseline ratchet, same shape as audit_causal_graphs.py: pre-existing findings -are frozen in ``conf/evidence_snippet_baseline.tsv`` and never fail; anything -new exits 1. +Baseline ratchet: pre-existing findings are frozen in +``conf/evidence_snippet_baseline.tsv`` and never fail; anything new exits 1. + +NOT the same shape as audit_causal_graphs.py, which is set membership on a +4-tuple carrying the locator index and a detail fragment. That shape rotted here +(#270): the index renumbers when an evidence item is deleted, so an improvement +read as a new finding. This keys without the index and compares OCCURRENCE +COUNTS, plus a magnitude ratchet for REUSED_SNIPPET, whose locator has no index +and whose severity lives in its detail (#291). Usage: just audit-snippets @@ -48,6 +54,7 @@ import re import sys from pathlib import Path +from typing import NamedTuple import yaml @@ -272,15 +279,141 @@ def audit(check_reports: bool = True) -> list[dict[str, str]]: return findings -def _key(row: dict[str, str]) -> tuple[str, str, str, str]: - return (row["file"], row["locator"], row["defect"], row["detail"]) +_INDEX_RE = re.compile(r"\[\d+\]$") + + +def _key(row: dict[str, str]) -> tuple[str, str, str]: + """Baseline identity: file, locator WITHOUT its array index, and defect. + + The index has to go. `evidence[1]` renumbers to `evidence[0]` when item 0 is + deleted, so an unchanged finding became a new one and `just qc` failed on a + change that strictly improved the corpus — 114 baselined findings sat at + index >= 1. The remedy a curator reaches for is `--write-baseline`, which + re-freezes anything genuinely new in the same PR. That is how a ratchet rots + (#270). + + `detail` goes too, for the same reason at a different granularity: it + carries the full snippet for ELLIPTICAL/UNSUPPORTIVE and the DOI for + MISSING, so retyping a still-elliptical snippet or correcting the DOI on a + still-snippet-less reference flipped the key. audit_causal_graphs.py learned + this first and keys on only the leading fragment of its detail. + + Dropping both collapses several findings onto one key, which is why the + baseline carries a COUNT rather than a set of keys — see compare(). + """ + return (row["file"], _INDEX_RE.sub("[]", row["locator"]), row["defect"]) + + +# Defects whose detail leads with a magnitude that can get WORSE without the +# finding count changing. REUSED_SNIPPET is the only aggregate this audit emits: +# one finding per (graph, snippet) whatever the number of items sharing it, with +# the number in the detail. Everything else is per-item, so more of it means +# more findings and the occurrence count already catches it. +# +# UNSUPPORTIVE_SNIPPET is deliberately NOT here even though its detail also leads +# with an integer: that integer is a character count, where larger is BETTER, so +# ratcheting it would flag a snippet growing from 6 chars to 10 as a regression. +MAGNITUDE_DEFECTS = {"REUSED_SNIPPET"} + +def _magnitude_key(row: dict[str, str]) -> tuple[str, str, str, str] | None: + """Identity for a magnitude, or None where magnitude is not ratcheted. + + REUSED_SNIPPET's locator is `{graph_id}:*`, so every reused snippet in one + graph shares a _key(). Keying the magnitude on the graph alone would let the + smaller of an uneven pair grow up to the larger unnoticed — + `trophic_type_classification_axes:*` already carries two rows. The snippet + itself is the discriminator (#291). + + Cost of that choice, tracked in #292: editing the shared snippet's TEXT + produces an unseen key, so its baselined magnitude reads as 0 and any count + beats it — a reworded quote is reported as new when nothing got worse. + Falling back to the graph's per-key max would fix that and restore the + sheltering this key exists to remove, so it fails closed instead. + """ + if row.get("defect") not in MAGNITUDE_DEFECTS: + return None + match = re.search(r"share one snippet: (.+)$", row.get("detail", "")) + return (*_key(row), match.group(1) if match else "") + + +def _magnitude(row: dict[str, str]) -> int: + """The leading integer of `detail`, for defects where more is worse. + + Ratcheted as a VALUE, not folded into the key. Putting it in the key would + reintroduce the rot #270 fixed from the other side: 3 -> 2 is an + improvement, and a key carrying the count would make it an unbaselined + finding and fail. audit_causal_graphs.py keys on the leading token for the + same information and accepts that cost; a value comparison does not have to. + """ + if row.get("defect") not in MAGNITUDE_DEFECTS: + return 0 + match = re.match(r"\s*(\d+)", row.get("detail", "")) + return int(match.group(1)) if match else 0 + + +class Baseline(NamedTuple): + """What the frozen backlog accepted: how many, and how bad. + + Both maps travel together rather than one being a module global, so + compare() is a pure function of its arguments and the tests exercise the + magnitude branch by constructing a Baseline instead of poking a global. + """ + counts: dict[tuple[str, str, str], int] + magnitudes: dict[tuple[str, str, str, str], int] -def load_baseline(path: Path) -> set[tuple[str, str, str, str]]: + +def load_baseline(path: Path) -> Baseline: + """Baselined occurrences per key. + + A count, not a set. Keying without the index means `evidence[0]` and + `evidence[1]` share a key, so set membership would let a THIRD missing + snippet at `evidence[2]` match a baselined key and pass silently — trading a + false positive for a false negative inside the integrity mechanism. The + count says "two of these were accepted"; a third is new. + """ + counts: dict[tuple[str, str, str], int] = {} + magnitudes: dict[tuple[str, str, str, str], int] = {} if not path.exists(): - return set() + return Baseline(counts, magnitudes) with path.open() as fh: - return {_key(row) for row in csv.DictReader(fh, delimiter="\t")} + for row in csv.DictReader(fh, delimiter="\t"): + key = _key(row) + counts[key] = counts.get(key, 0) + 1 + mkey = _magnitude_key(row) + if mkey is not None: + magnitudes[mkey] = max(magnitudes.get(mkey, 0), _magnitude(row)) + return Baseline(counts, magnitudes) + + +def compare(findings: list[dict[str, str]], + baseline: Baseline) -> list[dict[str, str]]: + """Findings in excess of what the baseline accepted. + + Two ways to exceed it: more occurrences at a key than were frozen, or an + aggregate finding whose magnitude is worse than the frozen one. Fewer + occurrences, or a smaller magnitude, is an improvement and passes. + + Note which ROW is reported for a count excess: findings are walked in audit + order and the ones PAST the baselined count are marked, so the row named is + the last at that key rather than the one that is new. Adding a snippet-less + reference at evidence[0] points a curator at evidence[2]. The exit code is + right and a count-based key cannot do better — the occurrences are + interchangeable by construction. + """ + seen: dict[tuple[str, str, str], int] = {} + new: list[dict[str, str]] = [] + for row in findings: + key = _key(row) + seen[key] = seen.get(key, 0) + 1 + if seen[key] > baseline.counts.get(key, 0): + new.append(row) + elif _magnitude(row) > baseline.magnitudes.get(_magnitude_key(row), 0): + # Same key, same occurrence count, but worse — a graph going from 3 + # shared snippets to 9 is one REUSED_SNIPPET finding either way + # (#291). + new.append(row) + return new def write_tsv(path: Path, rows: list[dict[str, str]]) -> None: @@ -316,7 +449,7 @@ def main(argv: list[str] | None = None) -> int: return 0 baseline = load_baseline(Path(args.baseline)) - new = [r for r in findings if _key(r) not in baseline] + new = compare(findings, baseline) if args.fail_on == "any": blocking = findings elif args.fail_on == "error": diff --git a/tests/test_audit_evidence_snippets.py b/tests/test_audit_evidence_snippets.py index 21ec9c6c..76b2a9cf 100644 --- a/tests/test_audit_evidence_snippets.py +++ b/tests/test_audit_evidence_snippets.py @@ -203,10 +203,16 @@ def test_a_short_snippet_does_not_match_by_coincidence(tmp_path, monkeypatch): def test_the_committed_baseline_matches_the_corpus(): """The ratchet is only a ratchet if the frozen set is current.""" - from audit_evidence_snippets import DEFAULT_BASELINE, _key, load_baseline + from audit_evidence_snippets import DEFAULT_BASELINE, compare, load_baseline baseline = load_baseline(DEFAULT_BASELINE) - assert baseline, "baseline is empty — run `just audit-snippets --write-baseline`" - new = [r for r in audit() if _key(r) not in baseline] + assert baseline.counts, "baseline is empty — run `just audit-snippets --write-baseline`" + # compare(), not key membership: `_key(r) not in baseline.counts` would + # check presence only, ignoring both the occurrence count and the + # REUSED_SNIPPET magnitude, so a third snippet-less reference or a graph + # growing from 5 shared snippets to 50 would fail `just qc` while this test + # passed. This is the only test comparing the committed baseline to the live + # corpus, so it has to assert what qc enforces (#291). + new = compare(audit(), baseline) assert new == [], f"{len(new)} findings are not baselined, e.g. {new[:2]}" @@ -233,3 +239,118 @@ def test_the_audit_report_is_not_tracked(tmp_path): assert out == "", ( "reports/evidence_snippet_audit.tsv is tracked again; either untrack it " "or wire it into audit-derived-reports on a `git show HEAD:` basis") + + +# --- baseline identity (#270) ------------------------------------------- + +def _row(file, locator, defect, detail=""): + return {"file": file, "locator": locator, "defect": defect, + "severity": "WARN", "detail": detail} + + +def test_the_key_ignores_the_evidence_array_index(): + """evidence[1] renumbers to evidence[0] when item 0 is deleted — an + improvement that used to fail qc as a new finding (#270).""" + from audit_evidence_snippets import _key + assert _key(_row("f.yaml", "evidence[1]", "MISSING_SNIPPET")) == \ + _key(_row("f.yaml", "evidence[0]", "MISSING_SNIPPET")) + assert _key(_row("f.yaml", "g1:a->b[2]", "MISSING_SNIPPET")) == \ + _key(_row("f.yaml", "g1:a->b[0]", "MISSING_SNIPPET")) + + +def test_the_key_ignores_volatile_detail(): + """detail carries the full snippet and the DOI, so retyping a still-bad + snippet flipped the key. audit_causal_graphs learned this first.""" + from audit_evidence_snippets import _key + assert _key(_row("f.yaml", "evidence[0]", "ELLIPTICAL_SNIPPET", "a ... b")) == \ + _key(_row("f.yaml", "evidence[0]", "ELLIPTICAL_SNIPPET", "c ... d")) + + +def test_the_key_still_separates_files_locators_and_defects(): + from audit_evidence_snippets import _key + base = _row("f.yaml", "evidence[0]", "MISSING_SNIPPET") + assert _key(base) != _key(_row("g.yaml", "evidence[0]", "MISSING_SNIPPET")) + assert _key(base) != _key(_row("f.yaml", "g1:a->b[0]", "MISSING_SNIPPET")) + assert _key(base) != _key(_row("f.yaml", "evidence[0]", "ELLIPTICAL_SNIPPET")) + + +def test_fewer_occurrences_than_baselined_is_an_improvement(): + from audit_evidence_snippets import Baseline, compare + rows = [_row("f.yaml", "evidence[0]", "MISSING_SNIPPET")] + baseline = Baseline({("f.yaml", "evidence[]", "MISSING_SNIPPET"): 2}, {}) + assert compare(rows, baseline) == [] + + +def test_one_more_occurrence_than_baselined_is_new(): + """The false negative a set-membership key would have introduced: a THIRD + missing snippet matching a baselined pair and passing silently (#270).""" + from audit_evidence_snippets import Baseline, compare + rows = [_row("f.yaml", f"evidence[{i}]", "MISSING_SNIPPET") for i in range(3)] + baseline = Baseline({("f.yaml", "evidence[]", "MISSING_SNIPPET"): 2}, {}) + new = compare(rows, baseline) + assert len(new) == 1 + + +def test_an_unbaselined_key_is_new(): + from audit_evidence_snippets import Baseline, compare + rows = [_row("f.yaml", "evidence[0]", "ELLIPTICAL_SNIPPET")] + assert len(compare(rows, Baseline({}, {}))) == 1 + + +# --- magnitude ratchet for aggregate defects (#291) ---------------------- + +def _reused(n, graph="g1:*", file="f.yaml"): + return _row(file, graph, "REUSED_SNIPPET", + f"{n} evidence items share one snippet: 'virulence factors'") + + +def test_a_worse_reused_count_is_new_despite_the_same_key(): + """REUSED_SNIPPET has no index and carries its magnitude in detail, so + dropping detail from the key let 3 -> 9 pass as one unchanged finding.""" + from audit_evidence_snippets import Baseline, _key, _magnitude_key, compare + baseline = Baseline({_key(_reused(3)): 1}, {_magnitude_key(_reused(3)): 3}) + assert len(compare([_reused(9)], baseline)) == 1 + + +def test_a_better_reused_count_still_passes(): + """The rot #270 fixed, from the other side: a count in the KEY would make + 3 -> 2 an unbaselined finding and fail on an improvement.""" + from audit_evidence_snippets import Baseline, _key, _magnitude_key, compare + baseline = Baseline({_key(_reused(3)): 1}, {_magnitude_key(_reused(3)): 3}) + assert compare([_reused(2)], baseline) == [] + assert compare([_reused(3)], baseline) == [] + + +def test_a_character_count_is_not_ratcheted(): + """UNSUPPORTIVE_SNIPPET's leading integer is a length, where larger is + BETTER — ratcheting it would flag 6 chars growing to 10 as a regression.""" + from audit_evidence_snippets import _magnitude + assert _magnitude(_row("f.yaml", "evidence[0]", "UNSUPPORTIVE_SNIPPET", + "6 chars, supports nothing specific: 'toxins'")) == 0 + assert _magnitude(_reused(7)) == 7 + + +def test_the_real_baseline_records_reused_magnitudes(): + """Guards the wiring: a baseline read without magnitudes silently disarms.""" + from audit_evidence_snippets import DEFAULT_BASELINE, load_baseline + magnitudes = [v for v in load_baseline(DEFAULT_BASELINE).magnitudes.values() if v] + assert magnitudes, "no REUSED_SNIPPET magnitudes captured from the baseline" + assert max(magnitudes) >= 3 + + +def test_two_reused_snippets_in_one_graph_get_separate_magnitudes(): + """`{graph_id}:*` means every reused snippet in a graph shares a _key(), so + a per-key max would let the smaller of an uneven pair grow to the larger + unnoticed — trophic_type_classification_axes already carries two (#291).""" + from audit_evidence_snippets import Baseline, _key, _magnitude_key, compare + a = _row("f.yaml", "g1:*", "REUSED_SNIPPET", + "3 evidence items share one snippet: 'carbon source'") + b = _row("f.yaml", "g1:*", "REUSED_SNIPPET", + "8 evidence items share one snippet: 'energy source'") + assert _key(a) == _key(b) + assert _magnitude_key(a) != _magnitude_key(b) + baseline = Baseline({_key(a): 2}, + {_magnitude_key(a): 3, _magnitude_key(b): 8}) + worse_a = dict(a, detail="7 evidence items share one snippet: 'carbon source'") + assert len(compare([worse_a, b], baseline)) == 1, \ + "the smaller snippet grew to below the larger's magnitude and passed"