Key the snippet baseline by occurrence count, not by array index (#270)

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":

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"