Skip to content

fix: wire content_hash pre-check into ingest paths (#254) - #284

Closed
robotrocketscience wants to merge 1 commit into
mainfrom
fix/issue-254-content-hash-pre-check
Closed

fix: wire content_hash pre-check into ingest paths (#254)#284
robotrocketscience wants to merge 1 commit into
mainfrom
fix/issue-254-content-hash-pre-check

Conversation

@robotrocketscience

Copy link
Copy Markdown
Owner

Summary

Fixes the structural cause behind issue #219's 5.3× row inflation by closing the cross-source dedup gap from #254.

belief_id = sha256(source‖text) but content_hash = sha256(text). The ingest path's store.get_belief(bid) check catches same-source re-ingest only — the same sentence from a different source produced a parallel belief row.

Both ingest entry points now consult get_belief_by_content_hash after the id miss. On hit, they record a corroboration against the canonical row and skip insert. _resolve_or_create_belief returns the canonical id so commit-ingest edges wire to the existing belief. bulk=True continues to suppress corroboration writes, matching same-source semantics.

This is the application-level (Option B) path from #254. Schema-level UNIQUE on content_hash (Option A) is deferred — it requires consolidation of historical duplicates (issue #219) to land first.

Tests

  • New tests/test_cross_source_dedup.py: 4 cases covering no-row-inflation, corroboration against canonical id, canonical-id stability across re-ingests, and bulk suppression.
  • tests/test_ingest_jsonl.py::test_source_label_passed_through previously asserted the buggy behavior (two writes for two source labels with same content); inverted to assert the fix (second call inserts 0).
  • Full suite: 1837 passed, 8 skipped.

Test plan

  • New cross-source dedup tests pass
  • Full suite green (1837 passed)
  • Existing same-source corroboration coverage in tests/test_corroborations.py unchanged

Closes #254

belief_id is sha256(source‖text); content_hash is sha256(text). The
ingest path's id-based dedup check (`store.get_belief(bid)`) catches
same-source re-ingest only — same content from a different source
produced a parallel belief row, which is the structural cause behind
the 5.3× row inflation reported in #219.

Both ingest entry points now consult `get_belief_by_content_hash`
after the id miss. On hit they record a corroboration against the
canonical row and skip insert. `triple_extractor._resolve_or_create_belief`
returns the canonical id so commit-ingest edges wire to the existing
belief. `bulk=True` continues to suppress corroboration writes,
matching the same-source semantics.

This is the application-level (Option B) path from #254. Schema-level
UNIQUE on content_hash (Option A) is deferred — it requires #219's
consolidation migration to land first.

Existing test_source_label_passed_through asserted the buggy
behavior (two ingests of identical content under different source
labels both insert); inverted to match the fix. New
test_cross_source_dedup covers the canonical id, corroboration
recording, and bulk suppression.

Closes #254
@robotrocketscience robotrocketscience added the author-Kulili PR coordination mutex label Apr 29, 2026
@robotrocketscience

Copy link
Copy Markdown
Owner Author

[claim:review:Gylf:2026-04-29T02:17:56Z]

@robotrocketscience

Copy link
Copy Markdown
Owner Author

Review: this PR is likely superseded by #283 (just merged). #283 introduced insert_or_corroborate() plus a UNIQUE(content_hash) constraint with a one-shot consolidation migration; both ingest paths this PR touches (ingest._ingest_turn_ids and triple_extractor._resolve_or_create_belief) were migrated to insert_or_corroborate in that PR, which subsumes the manual get_belief_by_content_hash pre-check here.

Recommend: rebase on main, verify #254's tests still hold against the #283 implementation, and either close this in favor of #283 or repurpose to add any missing coverage that #283 didn't include.

(Side note: pytest 3.13 timeout in CI was on test_run_benchmark_rejects_nonpositive_top_k opening a fresh MemoryStore — looks unrelated to this diff; likely flake or schema-init slowness post-#283. Will sort out post-rebase.)

@robotrocketscience

Copy link
Copy Markdown
Owner Author

[release:review:Gylf:2026-04-29T02:18:13Z]

@robotrocketscience

Copy link
Copy Markdown
Owner Author

[claim:review:Setr:2026-04-29T02:59:59Z]

@robotrocketscience

Copy link
Copy Markdown
Owner Author

Diff is clean and CI on the head shows green, but GitHub reports the merge commit can't be cleanly created — needs a rebase against current main (post-#283/#297). Holding the merge for author rebase. (Review: claim released.)

@robotrocketscience

Copy link
Copy Markdown
Owner Author

[release:review:Setr:2026-04-29T03:00:34Z]

@robotrocketscience

Copy link
Copy Markdown
Owner Author

Closing — superseded by #283 which already landed comprehensive insert_or_corroborate routing across all ingest paths plus UNIQUE(content_hash) on beliefs and the one-shot consolidation migration. The fix this PR proposed is now strictly smaller than what's already on main; rebasing would produce a no-op diff. Will follow up on #254 to confirm closure.

@robotrocketscience
robotrocketscience deleted the fix/issue-254-content-hash-pre-check branch May 5, 2026 00:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author-Kulili PR coordination mutex

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[v2.0] T1 corroboration recorder trigger gap — content_hash UNIQUE constraint missing

1 participant