Skip to content

fix(facts): a doc re-judged minimal left its previous numbers in the store - #44

Merged
sturlese merged 2 commits into
mainfrom
fix/bughunt-minimal-leaves-stale-facts
Jul 25, 2026
Merged

fix(facts): a doc re-judged minimal left its previous numbers in the store#44
sturlese merged 2 commits into
mainfrom
fix/bughunt-minimal-leaves-stale-facts

Conversation

@sturlese

@sturlese sturlese commented Jul 25, 2026

Copy link
Copy Markdown
Owner

Eleventh iteration of the autonomous bughunt sweep.

Bug

factstore states the contract in its own module docstring:

Idempotent per document: a reprocess REPLACEs the document's observations atomically; a deleted source deletes them — facts propagate deletions exactly like pages do.

The skipped path enforces it explicitly, with a comment saying why: "a doc downgraded to noise must not leave stale numbers behind". But that is one of two ways a pass can produce no facts.

A prose document re-judged minimal takes the other. The extraction branch declines, fact_rows stays None, and neither replace_facts nor delete_facts runs:

elif (method != "sheet" and out.representation in ("full", "digest")
      and prose_facts_processor and facts_dir):
    ...
if fact_rows is not None:
    factstore.replace_facts(...)      # <- and no else

So the previous pass's rows stay in the store, carrying a page_path that points at a page which no longer contains those figures. The answer layer can then serve a number whose own citation does not support it — precisely what the trust layer exists to prevent.

Fix

elif facts_dir and out.representation == "minimal":
    factstore.delete_facts(facts_dir, file_id)

The rule is what the page can support, not whether the extractor is on. agents.py defines minimal as "raw data without narrative, or a low-value doc -> pointer (title + type + source link)" — the numbers-heavy class. So the source may still be full of figures; it is the page that stops carrying them. Since every row cites a page via page_path, a pointer page cannot support a figure, whatever the extractor config happens to be.

A full/digest page whose extractor is switched off is the opposite case and deliberately untouched: it still carries its narrative and supports its rows. They go stale until the document's bytes change — classify_pending keys on the content hash, so a config flip alone never re-runs a document.

I got this wrong twice before the gate corrected me, and both errors came from the same place — reasoning about the content instead of the page:

  1. My first version cleared whenever no rows appeared, which would have made turning the extractor off destroy every fact it had ever extracted (the same over-reach as fix(graph): an approved merge was reverted by the next registry load #42).
  2. My second gated on whether the extractor was configured — which contradicted the skipped path I cite as precedent, since with CLEAN_FACTS_PROSE=off that path deletes unconditionally. Either both gate on config or neither does. Neither does: a skipped doc has no page, a minimal doc has a pointer page, and neither can support a citation.

I also had to delete a claim from the shipped comment: config-off rows are not "recoverable by turning the extractor back on" — verified, because the pending gate is content-hash-keyed and a config flip re-runs nothing.

Test

Both directions pinned, because for a fix that deletes data, only pinning one direction invites the opposite error:

  • test_process_one_minimal_representation_clears_stale_facts — mirrors the skipped sibling; fails today with the pass-1 rows still in the store.

  • test_switching_the_extractor_off_does_not_wipe_the_store — same document, same content, only the extractor removed; fails against the over-broad version.

  • test_a_sheet_pass_with_zero_kept_rows_still_clearsfact_rows is not None matters: an extraction that ran and kept nothing yields an empty list, which must still clear. The gate showed a one-token change to if fact_rows: silently reintroduced stale rows with the whole suite green.

Mutation-tested: reverting the clear, clearing whenever no rows appear, gating on config (my own earlier version), and if fact_rows: each fail. The last two survived until this round.

clean 259 (+3) · corpus 48 · graph 51 · slack 13 · fetch 33 · answer 66 · benchmark 3 — bare pytest (CI mode). ruff clean; golden scorecard 25/25, with all three facts: metrics unchanged (11 rows, 5/5 spot-checks, 2 rejected).

Honest scope note

The golden evals cannot exercise this branch: the fake backend never emits representation="minimal", and the golden state is full×7 + digest×1. So "scorecard 25/25, facts metrics unchanged" is true but is not evidence for this change — the unit tests are. Worth stating, because I originally offered it as if it were.

Also from the review: the deletion was the only destructive facts operation with no audit trail (both sibling deletes log). It now reports cleared and main surfaces it on the pass line.

sturlese and others added 2 commits July 25, 2026 07:46
…e store

factstore states the contract at the top of the file: "Idempotent per document: a
reprocess REPLACEs the document's observations atomically". The `skipped` path enforces
it explicitly — "a doc downgraded to noise must not leave stale numbers behind" — but
that is one of two ways a pass can produce no facts.

A prose document re-judged `minimal` takes the other: the extraction branch declines,
`fact_rows` stays None, and neither replace_facts nor delete_facts runs. The previous
pass's rows stay in the store carrying a page_path that points at a page which no
longer contains those figures — so the answer layer can serve a number whose own
citation does not support it, which is precisely what the trust layer exists to prevent.

The clear is deliberately conditional on the extractor being CONFIGURED. Producing no
rows because the document's content declined is a statement about the document; producing
none because CLEAN_FACTS_PROSE is switched off is a statement about the config, and a
config change must not silently wipe the store. Those rows go stale instead, which turning
the extractor back on repairs — deletion would not.

I narrowed it to that before the gate had to: the first version cleared whenever no rows
appeared, which would have made turning the extractor off destroy every fact it had ever
extracted. That is the same over-reach pattern as PR #42, where absorbing on any name
overlap deleted curated records.

Both directions pinned. `test_process_one_minimal_representation_clears_stale_facts`
mirrors the `skipped` sibling and fails today (the pass-1 rows survive);
`test_switching_the_extractor_off_does_not_wipe_the_store` fails against the over-broad
version. Mutation-tested: no clear at all, clear-whenever-no-rows, and checking the wrong
processor each fail exactly one.

clean 258 (+2). scorecard 25/25 with all three facts metrics unchanged, ruff clean.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…tractor is on

Gate review confirmed the behaviour but demolished the reasoning, and the reasoning was
load-bearing — it decided the condition.

I claimed `minimal` means "the content no longer carries facts". agents.py says the
opposite: `minimal` is "raw data without narrative, or a low-value doc -> pointer (title
+ type + source link)" — the numbers-heavy class. The SOURCE may still be full of
figures; it is the PAGE that stops carrying them. Since every row cites a page via
page_path, the actual invariant is that a pointer page cannot support a figure.

That reframing fixes the condition. It is not "is the extractor configured" —
config has nothing to do with what a page contains — it is `representation == "minimal"`.
Which also resolves an incoherence the reviewer found and I had shipped: with
CLEAN_FACTS_PROSE off (a reachable config), the `skipped` path I cite as precedent
deletes unconditionally, while my carve-out refused to. Either both gate on config or
neither does. Neither does: a skipped doc has no page at all, a minimal doc has a pointer
page, and neither can support a citation. Same reasoning, one step apart.

The full/digest-with-extractor-off case is still deliberately untouched, now for a reason
that survives inspection: that page carries its narrative and supports its rows.

Deleted a false claim from the shipped comment. I wrote that config-off rows are
"recoverable by turning it back on" — verified false: classify_pending keys on the content
hash and never on a config change, so flipping the extractor back on re-runs nothing. The
comment now says they go stale until the document's bytes change, which is what happens.

Also from the review: the deletion was the only destructive facts operation with no audit
trail. It now reports `cleared` and main surfaces it on the pass line. And the sheet half
of my old condition was unreachable — the extraction guard is byte-identical to it — so it
is gone rather than left as dead code with a comment.

Two mutants that previously survived now fail: the carve-out applied config-gating (my own
earlier version is now a failing mutant), and `if fact_rows:` instead of `is not None`,
which would silently stop clearing when an extraction runs and keeps nothing. The latter is
pinned by a new sheet test, since the reviewer showed the suite did not cover it.

Noted, not claimed: the golden evals cannot exercise this branch at all — the fake backend
never emits `representation="minimal"` — so "scorecard unchanged" is true but not evidence.
The two unit tests are.

clean 259 (+3). scorecard 25/25, ruff clean.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@sturlese

Copy link
Copy Markdown
Owner Author

Gate verdict

VERDICT: WEAKENED. Attack 1 (data loss / churn) was refuted with careful measurement. What did not survive was the reasoning — and here the reasoning was load-bearing, because it decided the condition. Reworked in 5970aab.

My premise was wrong, and it chose the wrong condition

I claimed minimal means "the content no longer carries facts". agents.py:60 says the opposite:

minimal: raw data without narrative, or a low-value doc -> pointer (title + type + source link).

That is the numbers-heavy class. The reviewer demonstrated a source still containing 4.2M EUR and 312 employees having its rows deleted — rows the same extractor would re-derive from that unchanged source. The source keeps its figures; it is the page that stops carrying them.

Since every row cites a page via page_path, the real invariant is: a pointer page cannot support a figure. So the test is not "is the extractor configured" — config has nothing to do with what a page contains — it is representation == "minimal".

That reframing also resolved an incoherence I had shipped

The reviewer found that with CLEAN_FACTS_PROSE=off — a reachable production config — the skipped path I cite as precedent deletes unconditionally, while my carve-out refused to:

=== extractor OFF + doc downgraded to `skipped` ===
rows after pass1 (prose ON, full):     1
rows after pass2 (prose OFF, skipped): 0   <- the carve-out says this must not happen

Either both gate on config or neither does. Neither does: a skipped doc has no page at all, a minimal doc has a pointer page, and neither can support a citation. Same reasoning, one step apart. The full/digest-with-extractor-off case stays untouched, now for a reason that survives inspection — that page carries its narrative and supports its rows.

A false claim in a shipped comment

I wrote that config-off rows are "recoverable by turning it back on". Verified false:

pass2 prose OFF  rows=1
pass3 prose ON   pending=0 calls=[] rows=1   <- not replaced: same extracted_at

classify_pending pends on new / hash-change / error / requeued / deleted / orphaned-duplicate — never on a config change. Flipping the extractor back on re-runs nothing. That was asserted in a code comment where it reads as doctrine; it now says the rows go stale until the document's bytes change, which is what actually happens.

Also fixed

  • The deletion was the only destructive facts operation with no audit trail — the sibling deletes both log. It now reports cleared and main surfaces it on the pass line.
  • The sheet half of my condition was unreachable, not merely untested: the extraction guard is byte-identical to it, so fact_rows is None and that guard are mutually exclusive. Removed rather than left as dead code.

Two previously-surviving mutants now fail

mutant before now
carve-out gated on config (my own earlier version) survived 1 failed
if fact_rows: instead of is not None survived 1 failed

The second is the more useful catch: an extraction that runs and keeps nothing yields an empty list, which must still clear the document's rows. The reviewer showed the suite did not cover it — a one-token change silently reintroduced stale rows with everything green. Now pinned by a sheet test.

Confirmed clean by the review, worth recording

  • No churn regression. A 0-row delete_facts is a no-op transaction — byte-identical file, identical mtime and inode. facts.jsonl is exported once per pass by main, not per document. And decisively: classify_pending keys on the content hash, so an unchanged minimal doc is not reprocessed at all — the delete fires once, on the pass where the content actually changed.
  • No facts dir or db springs into existence (delete_facts early-returns when facts.db is absent).
  • No double delete: the skipped path returns early.
  • The retry loop, dedup, dossiers/versions and the answer layer all stay consistent — nothing after the delete writes facts.

Honest scope note

The golden evals cannot exercise this branch: the fake backend never emits representation="minimal", and the golden state is full×7 + digest×1. So "scorecard 25/25, facts metrics unchanged" is true but is not evidence for this change. The unit tests are.

clean 259 (+3) · corpus 48 · graph 51 · slack 13 · fetch 33 · answer 66 · benchmark 3, bare pytest (CI mode) · ruff clean · scorecard 25/25.

@sturlese
sturlese merged commit ecda485 into main Jul 25, 2026
10 checks passed
@sturlese
sturlese deleted the fix/bughunt-minimal-leaves-stale-facts branch July 25, 2026 06:01
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.

1 participant