fix(pipeline): refuse ingest-batch writes for ambiguous raw memberships - #3400
Conversation
## Summary Extends the #3397/#3398 ambiguous-membership refusal to the daemon's default batch-ingest write path, `_write_session` in `pipeline/services/ingest_batch/_core.py`. ## Problem `_write_session` had the exact same shape as the pre-#3397 `ArchiveStore._write_parsed_precedence_result`: its only revision- authority check was against `raw_revision_heads`, populated only when a cohort has an ACCEPTED winner. A cohort `classify_membership_revisions` genuinely refused to arbitrate never gets an accepted head, so that check stays silent and the ordinary freshness/browser-precedence fallback below it writes the session unconditionally on the raw's next reparse -- last-writer-wins over the recorded `ambiguous` verdict. This path is the daemon's default for most non-drive origins, which is why quarantined-but-parsed counts concentrate here (chatgpt-export 7,050, codex-session 3,633, claude-code-session 2,450, claude-ai-export 1,562 raws with `revision_authority='quarantined'` and `parsed_at_ms` set). Left unfixed, a planned full index rebuild would re-corrupt every one of those origins even after #3397/#3398 fixed the drive-specific write path. ## Solution Added the same membership check directly to `_write_session`, not routed through a shared helper with `archive.py`. The two write paths don't share a connection: `ArchiveStore` keeps a lazily-opened persistent `source.db` connection as instance state (`_ensure_source_conn`), while `_core.py`'s writer is a free function operating on a plain `sqlite3.Connection` for index.db with no existing source.db handle in scope. Extracting a shared predicate would require either threading a `source_conn` through both call graphs anyway (no real duplication saved beyond the six-line SQL predicate) or introducing a new cross-module coupling between `archive_tiers/archive.py` and `ingest_batch/_core.py` for a single SELECT. The duplication that mattered was the *authority semantics* (never write a session for a membership recorded ambiguous), not the six-line SQL string; both copies now encode identical semantics and are covered by mirrored tests. A read-only-use `source.db` connection is opened once per batch (alongside the existing `blob_publisher`, which already opens its own source.db handle for a different purpose) and threaded through `_consume_ingest_results` -> `_drain_ingest_result` -> `_drain_ready_session_entries` -> `_write_session_entry` -> `_write_session`, closed in the batch's existing `finally`. The parameter defaults to `None` everywhere, so the ~40 existing direct `_write_session(conn, payload)` calls in `tests/unit/pipeline/ test_ingest_batch.py` and friends are unaffected. The predicate matches `raw_id` AND `provider_session_id`, not raw_id alone -- #3398's exact scoping lesson. One retained raw routinely lowers to many independently-arbitrated sessions (a Claude Code transcript plus its subagent sidechains, a bundle member set); a raw-scoped predicate would suppress every session on the raw the moment one sibling membership is ambiguous, turning a fidelity downgrade into outright absence at the next full rebuild. ## Verification `test_write_session_refuses_a_raw_recorded_ambiguous_membership` builds one raw with two `raw_session_memberships` rows -- one `ambiguous`, one `applied` -- and asserts BOTH halves: the ambiguous membership is refused, its settled sibling on the same raw is written. Asserting only the refusal would pass against an over-broad raw-scoped predicate; this was checked directly (see anti-vacuity below). ``` devtools test tests/unit/pipeline/test_ingest_batch.py -k test_write_session_refuses_a_raw_recorded_ambiguous_membership 1 passed devtools test tests/unit/pipeline/test_ingest_batch.py 56 passed devtools test tests/unit/pipeline/test_ingest_batch_fts_repair.py tests/unit/pipeline/test_ingest_append_replay.py tests/unit/pipeline/test_ingest_batch_resource_bounds.py 8 passed devtools test tests/unit/pipeline/test_ingest_batch_wal_checkpoint.py tests/unit/pipeline/test_blob_publication_crash_matrix.py tests/unit/pipeline/test_parsing_service.py 41 passed mypy --strict polylogue/pipeline/services/ingest_batch/_core.py Success: no issues found in 1 source file ``` **Anti-vacuity, run and reverted twice:** 1. Reverting the predicate to the raw-scoped form (`WHERE raw_id = ? AND decision = 'ambiguous'`, dropping the `provider_session_id` match) fails the settled-sibling assertion (`assert False is True`) -- proves the scoping is load-bearing, not incidental. 2. Short-circuiting the guard entirely (`if False and source_conn is not None ...`) fails the ambiguous-refusal assertion (`assert True is False`) -- proves the guard itself, not an unrelated earlier clause, is what refuses the write. Ref polylogue-c737 Co-Authored-By: Claude <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 1 second Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
1 participant
Summary
Extends the #3397/#3398 ambiguous-membership refusal to the daemon's default batch-ingest write path:
_write_sessioninpolylogue/pipeline/services/ingest_batch/_core.py. #3397/#3398 fixed the same defect onArchiveStore._write_parsed_precedence_result(used by the one-shot importer and other non-membership-governed callers) but explicitly left this file out of scope. This PR closes that residual scope.Problem
_write_sessionhad the exact same shape as the pre-#3397_write_parsed_precedence_result: its only revision-authority check was againstraw_revision_heads, populated only when a cohort has an ACCEPTED winner. A cohortclassify_membership_revisionsgenuinely refused to arbitrate never gets an accepted head, so that check stayed silent and the ordinary freshness/browser-precedence fallback below it wrote the session unconditionally on the raw's next reparse — last-writer-wins over the recordedambiguousverdict.This path is the daemon's default write path for most non-drive origins, which is why quarantined-but-parsed counts concentrate here (raws with
revision_authority='quarantined'andparsed_at_msset: chatgpt-export 7,050, codex-session 3,633, claude-code-session 2,450, claude-ai-export 1,562 — the four largest origins). Left unfixed, a planned full index rebuild would re-corrupt every one of those origins even after #3397/#3398 fixed the aistudio-drive-specific path.Solution
Direct check, not a shared helper — and here's why.
ArchiveStorekeeps a lazily-opened, persistentsource.dbconnection as instance state (_ensure_source_conn)._core.py's_write_sessionis a free function operating on a plainsqlite3.Connectionscoped toindex.db, with no source.db handle in scope at all before this change. Extracting a shared predicate function would still require threading asource_connthrough both call graphs independently — the six-line SQL SELECT isn't the part that was actually duplicated risk; the authority semantics (never write a session for a membership recordedambiguous) is, and that's now encoded identically in both places with mirrored tests, rather than glued together through a new cross-module coupling betweenarchive_tiers/archive.pyandingest_batch/_core.pyfor one query.A read-only-use
source.dbconnection is opened once per batch (alongside the pre-existingblob_publisher, which already opens its own separate source.db handle for a different purpose) and threaded through_consume_ingest_results→_drain_ingest_result→_drain_ready_session_entries→_write_session_entry→_write_session, closed in the batch's existingfinally. The newsource_connparameter defaults toNoneeverywhere it's threaded, so the ~40 existing direct_write_session(conn, payload)calls acrosstests/unit/pipeline/test_ingest_batch*.pyare unaffected — no membership check runs when no source connection is supplied.Scoping:
raw_idANDprovider_session_id, notraw_idalone. This is #3398's exact lesson, reapplied here on purpose. One retained raw routinely lowers to many independently-arbitrated sessions (a Claude Code transcript plus its subagent sidechains, a bundle member set). A raw-scoped predicate would suppress every session that raw carries the moment one sibling membership is ambiguous — turning a fidelity downgrade into outright absence at the next full rebuild, exactly the regression #3398 had to correct on the sibling path (295 raws with mixed decisions, 489 sessions that would have vanished under a raw-scoped predicate).Verification
test_write_session_refuses_a_raw_recorded_ambiguous_membership(tests/unit/pipeline/test_ingest_batch.py) builds the live shape directly: one raw, tworaw_session_membershipsrows — oneambiguous, oneapplied— and asserts both halves: the ambiguous membership's session is refused (no row insessions), and its settled sibling on the same raw is written (row present). Asserting only the refusal half would pass against an over-broad raw-scoped predicate that suppresses everything on the raw; this was checked directly, not assumed (see anti-vacuity below).Anti-vacuity, both directions checked directly against the test:
WHERE raw_id = ? AND decision = 'ambiguous', dropping theprovider_session_idmatch) fails the settled-sibling assertion (assert False is True) — proves the per-membership scoping is load-bearing, not incidental.if False and source_conn is not None ...) fails the ambiguous-refusal assertion (assert True is False) — proves the new guard itself, not an earlier unrelated clause, is what refuses the write.Not run:
devtools verify --seed-testmon --skip-slow(the seeded full local gate) — the seeding path was unreliable on this checkout for the full 3+ minute run this session; the targeteddevtools testruns above plusdevtools verify --quickare the baseline this repo's CONTRIBUTING/CLAUDE.md establish as sufficient for a focused change, and per-PR CI does not run the heavytestsuite anyway (it runs post-merge). Known-unrelated pre-existing failures on cleanorigin/master(not exercised by this change, not re-verified here): 5 subprocess tests intests/unit/cli/test_status.py,tests/unit/cli/test_terminal_snapshots.py::TestCommandOutputs::test_check_output_snapshot,tests/unit/devtools/test_testmon_mutation_proof.py::test_real_testmon_mutation_proof.Ref polylogue-c737