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

[realmarcin]

Closes #270, deferred out of #267 because getting it right needed more than the obvious fix.

The bug

The baseline keyed on (file, locator, defect, detail), and locator embeds the evidence-array index. 114 baselined findings sat at index ≥ 1, so deleting a duplicate evidence item renumbered the survivors and turned unchanged findings into new ones — just qc failing on a change that strictly improved the corpus. 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.

detail had the same sensitivity one level down: 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 also flipped the key. audit_causal_graphs.py learned this first and keys on only the leading fragment of its detail; this script's docstring claimed "same shape as audit_causal_graphs.py" and the key was the one place it wasn't.

Why the obvious fix was wrong, and what this does instead

Stripping the index collapses evidence[0] and evidence[1] onto one key — so under set membership a third missing snippet at evidence[2] would match a baselined key and pass silently. That trades a false positive for a false negative inside the integrity mechanism, which is why #267 deferred it rather than patching it at the tail of a merging PR.

So the baseline is read as a count per key: "two of these were accepted." Only occurrences in excess of that are new; fewer than baselined is an improvement and passes.

Canaried on the record the issue names

delete biofilm_formation evidence[0]   → 2736 findings, 0 new, exit 0   ← used to fail
add a third snippet-less item          → 2738 findings, 1 new, exit 1

The first is the case #270 is about; the second is the false negative the naive fix would have introduced. Both are pinned by tests, along with the key still separating files, locators and defects.

No baseline regeneration. The file format is unchanged — load_baseline aggregates the existing rows by key — so the diff is code and tests only.

Verification

348 tests pass  (6 new)
just qc         exit 0

🤖 Generated with Claude Code

[culturebot-reviewer]

Claude finished @realmarcin's task in 2m 54s —— View job

---

Review

The count-keyed baseline is the right call, and the reasoning about why the naive index-strip was wrong is correct. One hole, in the defect this audit most needs to hold.

---

🟡 Dropping detail disarms the ratchet on REUSED_SNIPPET

REUSED_SNIPPET is the one aggregate finding this audit emits: its locator is {graph_id}:* (scripts/audit_evidence_snippets.py:263) with no index, and the magnitude lives in detail — "3 evidence items share one snippet: 'virulence factors'". So dropping detail from _key costs nothing on the index problem here and loses the only thing that distinguishes bad from worse:

baseline:  animal_pathogen_metazoan_adaptation:*  "3 evidence items share one snippet: 'virulence factors'"
after:     animal_pathogen_metazoan_adaptation:*  "9 evidence items share one snippet: 'virulence factors'"

Same key, count still 1, 1 > 1 is false → passes, exit 0. On main that flips the key and fails. All 13 baselined REUSED_SNIPPET rows (conf/evidence_snippet_baseline.tsv) are exposed; gc_content.yaml is already at 5 and could go to 50 unnoticed.

This is the same trade the sibling script already made and reversed. scripts/audit_causal_graphs.py:220-230 is explicit about it:

Detail MUST lead with the component count, because _key takes the leading whitespace-delimited token as the baseline discriminator. Leading with the node count instead made the ratchet fail open in both directions […] 3 components -> 4 keeps the node count, so real backsliding stayed suppressed

The PR cites that script as precedent for dropping detail, but it keeps a leading fragment and deliberately made that fragment the count for its one whole-graph defect. This is the same defect shape, and the module docstring's stated purpose (line 15) is "refuse to let the backfill make it worse" — pasting one phrase across newly backfilled edges is precisely how #183 makes it worse.

A generic leading-token discriminator won't work here (MISSING's leading token is the DOI you're trying to shed). Two options that fit the new count model:

• Emit n - MAX_REUSE_PER_GRAPH findings instead of one, so excess reuse is expressed as occurrences and the count comparison handles it for free; or
• Keep a defect-specific discriminator in _key — the leading token of detail for REUSED_SNIPPET only, "" for the rest.

Fix this →

---

🔵 compare() flags the wrong occurrence

compare (line 326) walks findings in audit order and marks the occurrences past the baselined count, so the row printed to stderr is the last one at that key, not the one that is actually new. If evidence[0] is a freshly added snippet-less reference and [1]/[2] were baselined, the curator is pointed at evidence[2]. The exit code is right and there's no perfect answer under a count key, but it's worth a line in the docstring so the next reader doesn't trust the locator.

🔵 Stale cross-reference

The module docstring line 35 still reads "Baseline ratchet, same shape as audit_causal_graphs.py". The two now differ in the one place this PR is about — set membership vs. per-key counts. Worth updating, since the previous divergence from that claim is what the PR body calls out.

---

The index-stripping regex is correctly $-anchored for every locator iter_evidence produces, write_tsv's sorted(rows, key=_key) stays stable within a key so --write-baseline output is deterministic, and no other module consumes load_baseline's changed return type. Tests cover the two canaries in the PR body.

I could not run pytest or just qc — Bash execution was declined in this environment, so the "348 tests pass" claim is unverified on my side.

gh pr review 290 --request-changes

· branch fix/snippet-baseline-key

[culturebot-reviewer]

Count-keyed baseline is the right call. One hole: dropping detail from _key disarms the ratchet on REUSED_SNIPPET, whose locator ({graph_id}:*) carries no index and whose magnitude lives entirely in detail — a baselined "3 evidence items share one snippet" now silently covers "9 evidence items share one snippet". That is the failure audit_causal_graphs.py:220-230 explicitly reversed for its own whole-graph defect, and it is the defect #183's backfill most directly worsens. Full detail in the review comment.

[culturebot-reviewer]

Claude finished @realmarcin's task in 2m 26s —— View job

---

Review

Re-reviewed after 183533af. The magnitude fix is the right shape — ratcheting the count as a value rather than folding it into the key avoids reintroducing #270's rot from the improvement side, which keying on the leading token (the audit_causal_graphs.py approach) would have done. MAGNITUDE_DEFECTS correctly excludes UNSUPPORTIVE_SNIPPET, whose leading integer is a length where bigger is better, and the comment says why. The prior 🟡 is closed.

One defect remains, plus the two 🔵s from the last pass that are still open.

---

🟡 The baseline-currency test still uses the semantics this PR abandoned

tests/test_audit_evidence_snippets.py:209 was not migrated:

new = [r for r in audit() if _key(r) not in baseline]

baseline is now a dict, so in tests key presence and the assertion silently degrades to "every current finding's key appears somewhere in the baseline" — no count comparison, no magnitude comparison. Its own docstring says "The ratchet is only a ratchet if the frozen set is current," and it is the only test that checks the committed baseline against the live corpus.

Concretely: add a third snippet-less reference to a file with two baselined at the same key, and just qc fails (correctly, via compare) while this test passes. Grow gc_content_composition_drivers:* from 5 to 50 and the same split happens. Before this PR the 4-tuple set key would have caught both. Every other call site moved to compare(); this one is the leftover — new = compare(audit(), baseline) restores it.

Fix this →

---

🔵 BASELINE_MAGNITUDE is a per-key max, and one key already holds two rows

load_baseline keeps max(magnitude) across all rows sharing a key, and REUSED_SNIPPET's locator ({graph_id}:*) means every reused snippet in one graph shares a key. conf/evidence_snippet_baseline.tsv:2696-2697 is already such a pair — trophic_type_classification_axes:* with 'carbon source' and 'energy source', both at 3. Harmless while the magnitudes are equal; once any graph carries an uneven pair, the smaller one can grow up to the larger unnoticed. Per-key max over (key, snippet) findings is the loose part — a {key: {snippet: n}} map, or folding the snippet prefix into the magnitude lookup, would tighten it. Not blocking, since nothing in the corpus is uneven today.

Related: compare() takes baseline as a parameter but reads magnitudes from a module global. The tests have to aes.BASELINE_MAGNITUDE.clear() and poke it by hand (lines 307-308, 318-319) to exercise the branch, which is the coupling showing through. test_the_real_baseline_records_reused_magnitudes guards the wiring, so it holds — but returning both maps from load_baseline would make it structural rather than guarded.

🔵 Two notes from the previous review are unaddressed

• Module docstring line 35 still reads "Baseline ratchet, same shape as audit_causal_graphs.py". That script is set membership on a 4-tuple; this one is now per-key counts plus a magnitude ratchet. The PR body's own argument is that the previous divergence from this claim is what caused Snippet baseline keys on an array index, so improving the corpus can fail qc #270.
• compare() walks findings in audit order and marks the occurrences past the baselined count, so the row printed to stderr is the last at that key, not the one that is new — a curator who adds a snippet-less ref at evidence[0] gets pointed at evidence[2]. The exit code is right and there is no better answer under a count key, but the docstring should say so.

---

Checked and clean: _INDEX_RE is $-anchored and matches every indexed locator iter_evidence emits while leaving {graph_id}:* alone; write_tsv's sorted(rows, key=_key) is stable within a key so --write-baseline stays deterministic; --write-baseline bypasses compare entirely; no other module imports load_baseline from this script, so the changed return type is contained. _magnitude reads defect via .get, so baseline rows and live findings both work.

Not verified: pytest and just qc — Bash execution for test runs was declined in this environment, so the "348 tests pass" and "just qc exit 0" claims are unchecked on my side. Everything above is from reading the code and the committed baseline.

gh pr review 290 --request-changes

· branch fix/snippet-baseline-key

[culturebot-reviewer]

Claude finished @realmarcin's task in 2m 42s —— View job

---

Review

Re-reviewed after 21f8a763. All three items from the last pass are closed, and closed properly rather than papered over:

• Baseline as a NamedTuple carrying both maps makes compare() a pure function of its arguments — the tests now construct a baseline instead of poking aes.BASELINE_MAGNITUDE, so the wiring is structural rather than guarded (scripts/audit_evidence_snippets.py:348).
• _magnitude_key discriminates on the snippet, so trophic_type_classification_axes:*'s two rows (conf/evidence_snippet_baseline.tsv:2696-2697) can no longer shelter each other; test_two_reused_snippets_in_one_graph_get_separate_magnitudes pins exactly that.
• test_the_committed_baseline_matches_the_corpus now calls compare() (line 215), so the only test comparing the frozen file to the live corpus asserts what just qc enforces.
• The module docstring's cross-reference is corrected and the audit_causal_graphs.py characterization it gives is accurate (scripts/audit_causal_graphs.py:241-249 is set membership on a 4-tuple with a leading-fragment discriminator). compare()'s docstring now says which row gets named on a count excess.

No 🔴 or 🟡. One 🔵 below, which is a tradeoff to document rather than a defect to fix.

---

🔵 The magnitude branch is an identity check for any snippet it hasn't seen

compare() line 405 reads baseline.magnitudes.get(_magnitude_key(row), 0), and _magnitude_key folds the snippet text in. So a REUSED_SNIPPET whose shared snippet is edited — not diversified, just rewritten — gets mkey absent → 0, and any magnitude beats 0:

baseline:  cellulolysis_cellulase_systems:*  3 items share 'Complete degradation of cellulose…'
after:     cellulolysis_cellulase_systems:*  3 items share 'Complete degradation of cellulose to glucose…'   ← quote extended
           count at key unchanged (1 ≤ 1), mkey unseen, 3 > 0  →  new  →  qc exits 1

That is a false positive on a finding that got neither better nor worse, and the PR's own thesis is that a false positive is what sends a curator to --write-baseline and rots the ratchet. Same shape when one reused snippet is replaced by a different one at the same count: {A@3, B@3} → {A@3, C@3} fails, where the equivalent substitution under any other defect is absorbed by the count.

I don't think the obvious remedy is free. Falling back to the per-key max when mkey is unseen would absorb the text edit, but it would also let a retyped snippet grow 3 → 7 under a sibling baselined at 8 — the hole _magnitude_key was added to close, back in a narrower form. Erring toward failing closed is the defensible call here; it just isn't stated anywhere, and the module is otherwise unusually good about writing down why each direction was chosen. A line in _magnitude_key's docstring saying that an unseen snippet is treated as unbaselined by choice, and why the max-fallback was rejected, would keep the next reader from "fixing" it.

Two smaller things in the same neighbourhood:

• detail truncates the snippet at 100 chars (scripts/audit_evidence_snippets.py:277), so _magnitude_key discriminates on the first 100 characters only. cellulolysis is already truncated in the baseline (line 1306). Two reused snippets in one graph sharing a 100-char prefix would collide and re-open the uneven-pair hole. Nothing in the corpus does this today.
• MAGNITUDE_DEFECTS at line 316 is followed by one blank line before def _magnitude_key. Ruff's default select (E4,E7,E9,F) doesn't include E3 so this won't fail lint, but it's the only spot in the file that breaks the two-line convention.

---

Checked and clean: _INDEX_RE is $-anchored and matches every indexed locator iter_evidence emits while leaving {graph_id}:* alone; _magnitude's re.match and _magnitude_key's re.search both work on baseline rows and live findings (.get on defect/detail); write_tsv's sorted(rows, key=_key) is stable within a key and audit() emits reused snippets via sorted(counts.items()), so --write-baseline is deterministic; --write-baseline bypasses compare entirely; only justfile and the test module reference this script, so the changed load_baseline return type is contained. No data/traits/** change, so no history/ record is expected.

Not verified: pytest and just qc. Bash execution for test runs was declined in this environment again, so "348 tests pass" and "just qc exit 0" are unchecked on my side — everything above is from reading the code, the tests, and the committed baseline.

gh pr review 290 --approve

· branch fix/snippet-baseline-key

[culturebot-reviewer]

Claude finished @realmarcin's task in 2m 8s —— View job

---

Review

Re-reviewed after 0a7ab21d. The last pass's 🔵 is closed the way it should have been — _magnitude_key's docstring now records that an edited snippet reads as unbaselined by choice, and names the max-fallback it rejects and why (scripts/audit_evidence_snippets.py:327-333). That's a docstring-only commit; nothing else moved.

I re-checked the whole mechanism rather than just the delta.

No 🔴 or 🟡. Two 🔵 nits below, both narrow and neither blocking.

---

What I verified this pass

• _INDEX_RE covers every locator. iter_evidence emits exactly two shapes — evidence[i] and {graph_id}:{subject}->{obj}[j] (lines 180-198) — both index-terminal, so the $-anchor is right, and {graph_id}:* is left alone. An edge locator strips to g1:a->b[], which can't collide with the :* aggregate key.
• The magnitude branch under ordering. audit() emits reused snippets via sorted(counts.items()), and compare()'s elif sits behind the count check. I walked the awkward cases: a new snippet sorting before a baselined one hits mkey unseen → 0 → flagged; a baselined pair where only the smaller grows is caught by _magnitude_key's snippet discriminator; a snippet dropping below MAX_REUSE_PER_GRAPH while its sibling grows still fails. When a new snippet sorts first, both it and the surviving baselined row get reported — over-report, correct exit code, and compare()'s docstring already covers it under "the occurrences are interchangeable by construction."
• The corpus matches the model. All 13 REUSED_SNIPPET rows read; trophic_type_classification_axes:* is the only shared-_key pair and both sit at 3, so nothing is sheltering anything today. gc_content_composition_drivers:* at 5 is now ratcheted as a value.
• Blast radius. --write-baseline bypasses compare entirely; write_tsv's sorted(rows, key=_key) is stable within a key so output stays deterministic; only justfile and the test module touch this script, so the Baseline return type is contained. _magnitude/_magnitude_key both .get their fields, so baseline rows and live findings work identically. No data/traits/** change, so no history/ record is expected.

---

🔵 detail truncates at 100 chars, so _magnitude_key discriminates on a prefix

audit() writes {snippet[:100]!r} (line 277) and _magnitude_key recovers the snippet from that string. Two reused snippets in one graph sharing a 100-character prefix would collide back onto one mkey and re-open the max() sheltering that _magnitude_key exists to close. cellulolysis_cellulase_systems:* is already truncated (conf/evidence_snippet_baseline.tsv:1306), so the prefix path is live, not theoretical — though nothing in the corpus collides.

Worth noting because it's the shape #183's backfill produces: pasting one long passage across several edges and varying only the tail is exactly how two >100-char snippets in one graph would end up sharing a prefix. A sentence in _magnitude_key's docstring would be enough; the module is otherwise unusually careful about writing down where each key is load-bearing.

🔵 Blank-line nit

MAGNITUDE_DEFECTS (line 316) is followed by one blank line before def _magnitude_key, where every other top-level definition in the file uses two (_INDEX_RE at 282 → _key at 285). pyproject.toml:63-65 sets no select, so ruff's default E4,E7,E9,F won't catch it — cosmetic only.

---

Not verified: pytest and just qc. Bash execution for test runs was declined in this environment again, so "348 tests pass" and "just qc exit 0" remain unchecked on my side. Everything above is from reading the code, the tests, and the committed baseline.

gh pr review 290 --approve

· branch fix/snippet-baseline-key