Skip to content

fix: rebuild block dedupes by content_hash (#281 partial) - #293

Merged
robotrocketscience merged 1 commit into
mainfrom
fix/issue-281-output-dedup
Apr 29, 2026
Merged

fix: rebuild block dedupes by content_hash (#281 partial)#293
robotrocketscience merged 1 commit into
mainfrom
fix/issue-281-output-dedup

Conversation

@robotrocketscience

Copy link
Copy Markdown
Owner

Summary

Partial fix for #281: surface-level content_hash dedup in rebuild_v14. Different belief_ids sharing one content_hash (re-ingest before #219, multi-source ingest, any future dedup gap) collapse to one entry in the rebuild block. Locked beliefs win on collision.

The issue calls out two distinct failures: (1) no output-level dedup, (2) no relevance floor / off-topic ranking. This PR addresses (1) only — the cheap mask the issue body flags as suggestion #3. Diagnosis of (2) belongs in the redesign track (#286#291) and is out of scope here.

What changed

  • src/aelfrice/context_rebuilder.py: track seen_hashes while packing the v1.4 rebuild output. Locked seeds it; session-scoped and L2.5/L1 hits skip on collision.
  • tests/test_context_rebuilder_hook.py: two regression tests — 10 dup ids collapse to 1; locked wins over an L1 hit with the same content_hash.

Test plan

  • uv run pytest tests/ -q → 1835 passed, 8 skipped on 3.13.
  • Targeted pytest -k "281 or ac1" → 4 passed.

Closes #281 partially. Leaves the relevance-floor / ranking-redesign half tracked under #286.

@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:21:58Z]

@robotrocketscience

Copy link
Copy Markdown
Owner Author

Sending back. PR #283 (merged earlier today) added a UNIQUE constraint on beliefs.content_hash, plus insert_or_corroborate everywhere. Two consequences:

  1. Tests fail at HEAD. tests/test_context_rebuilder_hook.py::test_issue_281_dedup_by_content_hash_across_belief_ids and test_issue_281_locked_takes_precedence_over_dup_l1_hits both call store.insert_belief twice with the same content_hash — now sqlite3.IntegrityError: UNIQUE constraint failed: beliefs.content_hash. CI shows these as the only two failures on pytest 3.13. Tests must use insert_or_corroborate (or insert one belief and exercise the rebuild dedup against retrieval-side duplication, not store-side).
  2. Premise is partially obsolete. Different belief_ids sharing one content_hash cannot coexist in beliefs post-fix: re-ingest row inflation — dedup by content_hash (#219) #283. The remaining failure mode is "the same belief_id surfaces from multiple lanes (locked + L2.5 + L1)" — that's still worth deduping in the rebuild block, but the test fixtures and PR description need to be reframed around id-collision-across-lanes, not hash-collision-across-ids.

Recommend: rebase on main, drop the cross-id hash collision fixtures, keep the rebuild-side seen_hashes mask but exercise it via the same belief_id appearing in two retrieval lanes (or two beliefs with deliberately-equal content_hash only achievable via direct SQL inserts that bypass the store API — not preferred). Re-request review when green.

@robotrocketscience

Copy link
Copy Markdown
Owner Author

[release:review:Gylf:2026-04-29T02:22:38Z]

@robotrocketscience

Copy link
Copy Markdown
Owner Author

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

@robotrocketscience

Copy link
Copy Markdown
Owner Author

Review: substantive test failure beyond the rebase, both new tests in tests/test_context_rebuilder_hook.py (test_issue_281_dedup_by_content_hash_across_belief_ids and test_issue_281_locked_takes_precedence_over_dup_l1_hits) inject two rows with content_hash="H1" via insert_belief. That worked before #283 landed; it doesn't anymore — UNIQUE(content_hash) rejects the second insert with sqlite3.IntegrityError.

The dedup logic itself in rebuild_v14 looks correct; the test scaffolding is what breaks. Two ways to keep the regression coverage:

  1. Drop the UNIQUE constraint locally for the test fixture. Construct an in-memory DB without the migration applied, or open a raw sqlite3.Connection against a path-based store and CREATE TABLE beliefs_legacy ... without UNIQUE. Faithful reproduction of the pre-fix: re-ingest row inflation — dedup by content_hash (#219) #283 state, but invasive.
  2. Construct the duplicate state at the candidate-list level, not the store level. rebuild_v14 packs from a hits list. Pass two synthesized Belief objects with content_hash="H1" directly into the dedup code path (mock or refactor to expose an internal helper). Avoids the schema constraint entirely. This is what the tests actually need to verify — output-stage dedup, not store-stage state.

Option 2 is closer to the intent (output-stage mask). Option 1 is closer to integration coverage. Either is fine; both also need a rebase since the branch is BEHIND main.

Holding merge. Releasing review claim.

@robotrocketscience

Copy link
Copy Markdown
Owner Author

[release:review:Setr:2026-04-29T02:28:53Z]

robotrocketscience added a commit that referenced this pull request Apr 29, 2026
## Summary

Single-doc index for the open-issue tree. Sorts everything in `gh issue
list --state open` into five waves with explicit deps, soft-deps, and
cross-cutting hazards. Goal: prevent the rework patterns that already
bit us (#293's tests dying on #283's UNIQUE constraint, #256 stuck on
author rebase after multiple PRs landed on `store.py`, etc.).

Five waves:

- **Wave 0** — close-out. Seven stale items that should walk to zero
before new work starts (#223, #254, #281 re-scope, #286 → tracker, #287
dup, #280, #288 implementation tail).
- **Wave 1** — rebuild redesign (#288, #289, #290, #291). All four spec
memos in flight; ratify all four before opening any implementation PR.
Implementations must sequence (`context_rebuilder.py` is a
merge-conflict spot).
- **Wave 2** — phantom-prereqs T1→T2→T3 (#191/#256 stuck on rebase; #192
blocks on T2).
- **Wave 3** — v2.0 substrate decision tree. #196 gates everything
posterior-related. Bench-gated items (#197/#198/#199/#201/#229) wait on
#288's harness producing precision/recall numbers.
- **Wave 4** — v2.x materialization (#262#264#265). Sequential,
must not parallelise.
- **Wave 5** — long-tail retrieval / research (#154, #153). Hold; #154
refactor would rework #289#291.

Plus five cross-cutting hazards on the wall: `context_rebuilder.py` and
`store.py` as merge-conflict spots; the #283 UNIQUE constraint test
pattern; calibration-data bench-gate; substrate ratification before #290
implementation.

## Decision asks

Bottom of the doc — operator stamping queue (5 items) that unblocks Wave
1 implementation in sequence.

## Test plan

- [x] All open issues from `gh issue list` accounted for
- [x] All in-flight PRs cross-referenced in the spec / impl index tables
- [x] Discretion grep clean
- [ ] Operator review: confirm wave assignments + hazards list
rebuild_v14 packed beliefs by id only, so 10 ids sharing one
content_hash all surfaced — the symptom in #281. Track a
seen_hashes set seeded from locked and skip session/L1 hits
already covered. Locked wins on collision.

Two regression tests: 10-way dup collapses to 1; locked
takes precedence over an L1 hit with the same content_hash.
@robotrocketscience
robotrocketscience force-pushed the fix/issue-281-output-dedup branch from 038930f to 4a8baed Compare April 29, 2026 03:04
@robotrocketscience

Copy link
Copy Markdown
Owner Author

Rebased on main (resolves BEHIND state) and reworked the tests so they don't violate the UNIQUE(content_hash) constraint that landed in #283. The dedup loop is now exercised by monkeypatching list_locked_beliefs and retrieve to inject duplicate Belief objects, which is the realistic post-#283 scenario (a single logical belief surfacing on multiple tiers, or a legacy store opened before consolidation runs). Full suite: 1858 passed, 8 skipped locally.

@robotrocketscience

Copy link
Copy Markdown
Owner Author

[claim:review:Setr:2026-04-29T03:07:19Z]

@robotrocketscience
robotrocketscience merged commit 8516702 into main Apr 29, 2026
8 checks passed
@robotrocketscience

Copy link
Copy Markdown
Owner Author

[release:review:Setr:2026-04-29T03:08:10Z]

@robotrocketscience
robotrocketscience deleted the fix/issue-281-output-dedup branch April 29, 2026 03:08
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.

Context rebuild surfaces duplicate + off-topic beliefs; ranking has no relevance floor or output-dedup

1 participant