fix(storage): scope the ambiguous-membership refusal to the membership written - #3398
Merged
Merged
Conversation
…p written Problem The guard added in #3397 asks whether the *raw* has any ambiguous membership, but the write it guards is per-session. One retained raw routinely lowers to many independently-arbitrated sessions -- a Claude Code transcript plus its subagent sidechains, a bundle member set -- so a raw-scoped predicate suppresses every session that raw carries the moment a single sibling membership is ambiguous. Measured on the live archive: 295 raws carry a mix of decisions, together holding 489 sessions whose own membership is not ambiguous, and one raw carries 106 memberships. A concrete pair from that set: raw 0006e5ca2211 holds provider_session_id 4d4f8440-... recorded ambiguous and correctly absent from the index, alongside 14e9cdfe-...:agent-ab361be recorded applied and legitimately present. The raw-scoped predicate refuses both. That trades a fidelity downgrade for outright absence, which is the worse failure -- an unfetched attachment is visibly wrong, a missing session is not -- and it would have landed silently at the next full rebuild. Raised as a P1 on #3397 by review; I merged before triaging it, which was my error. This corrects it forward rather than reverting, since the underlying refusal is right and only its scope was wrong. What changed The predicate now matches `provider_session_id` as well as `raw_id`, so a membership is judged on its own recorded decision. Verification devtools test tests/unit/storage/test_revision_replay.py -k ambiguous 4 passed New `test_precedence_write_allows_a_non_ambiguous_sibling_membership_on_the_same_raw` builds the live shape -- one raw, two memberships, arbitrated differently -- and asserts both halves: the ambiguous membership is still refused, and its settled sibling is not collateral damage. Reverting the predicate to the merged raw-scoped form fails exactly that test and no other, so the scope is load-bearing and the original refusal is untouched. mypy --strict clean on the changed module. Ref polylogue-c737 Co-Authored-By: Claude <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 51 minutes 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 (3)
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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c8f3915653
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+1704
to
+1705
| SELECT 1 FROM raw_session_memberships | ||
| WHERE raw_id = ? AND provider_session_id = ? AND decision = 'ambiguous' |
There was a problem hiding this comment.
When one retained raw has GEMINI and DRIVE memberships with the same provider session ID but different decisions, this predicate treats them as one membership and suppresses the non-ambiguous write. That shape is supported because archive_ingest.py:170-180 deliberately shares a raw across those provider names, while raw_session_memberships is uniquely keyed by (raw_id, logical_source_key) precisely because the provider prefix distinguishes the memberships. Compare against f"{session.source_name.value}:{session.provider_session_id}" and logical_source_key so an ambiguous Gemini member cannot collateral-skip an applied Drive member (or vice versa).
AGENTS.md reference: AGENTS.md:L253-L258
Useful? React with 👍 / 👎.
Sinity
added a commit
that referenced
this pull request
Jul 30, 2026
…ps (#3400) ## Summary Extends the #3397/#3398 ambiguous-membership refusal to the daemon's default batch-ingest write path: `_write_session` in `polylogue/pipeline/services/ingest_batch/_core.py`. #3397/#3398 fixed the same defect on `ArchiveStore._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_session` had the exact same shape as the pre-#3397 `_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 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 recorded `ambiguous` verdict. 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'` and `parsed_at_ms` set: 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.** `ArchiveStore` keeps a lazily-opened, persistent `source.db` connection as instance state (`_ensure_source_conn`). `_core.py`'s `_write_session` is a free function operating on a plain `sqlite3.Connection` scoped to `index.db`, with no source.db handle in scope at all before this change. Extracting a shared predicate function would still require threading a `source_conn` through 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 recorded `ambiguous`) is, and that's now encoded identically in both places with mirrored tests, rather than glued together through a new cross-module coupling between `archive_tiers/archive.py` and `ingest_batch/_core.py` for one query. A read-only-use `source.db` connection is opened once per batch (alongside the pre-existing `blob_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 existing `finally`. The new `source_conn` parameter defaults to `None` everywhere it's threaded, so the ~40 existing direct `_write_session(conn, payload)` calls across `tests/unit/pipeline/test_ingest_batch*.py` are unaffected — no membership check runs when no source connection is supplied. **Scoping: `raw_id` AND `provider_session_id`, not `raw_id` alone.** 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, two `raw_session_memberships` rows — one `ambiguous`, one `applied` — and asserts **both halves**: the ambiguous membership's session is refused (no row in `sessions`), 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). ``` 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 devtools verify --quick exit 0 (20 steps, all green — format/lint/mypy/render-all-check/layering/closure-matrix/schema checks) ``` **Anti-vacuity, both directions checked directly against the test:** 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 per-membership 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 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 targeted `devtools test` runs above plus `devtools verify --quick` are the baseline this repo's CONTRIBUTING/CLAUDE.md establish as sufficient for a focused change, and per-PR CI does not run the heavy `test` suite anyway (it runs post-merge). Known-unrelated pre-existing failures on clean `origin/master` (not exercised by this change, not re-verified here): 5 subprocess tests in `tests/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 Co-authored-by: Claude <noreply@anthropic.com>
Sinity
added a commit
that referenced
this pull request
Jul 30, 2026
…elation (#3401) ## Summary Membership classification (`polylogue/archive/session_revision_membership.py` + `polylogue/pipeline/ids.py`) treated provider-volatile export artifacts as content identity: message array order, provider-reported generation duration, attachment id presence, and attachment acquisition state (the last already fixed by bu1i, #3394). Live-archive census (read-only, no mutation) over the full `claude-ai-export`/`chatgpt-export` ambiguous-cohort populations shows this quarantined 587 and 135 cohorts respectively as "ambiguous" branches when they were almost entirely the same conversation re-exported. This PR replaces the growing set of special-cased fixes with one invariant (polylogue-aggz): **a conversation is a SET of items (messages, attachments, events) keyed by content-derived identity, each carrying only content-bearing fields.** Two revisions are `equal`, one `contains` the other, or they `conflict` — total and decidable, no residual category. ## Problem - **c429**: Claude.ai's own export array order is not stable across export vintages. A strict positional-prefix dominance test refused both directions for a bare reorder of byte-identical messages. - **nuec**: ChatGPT's `generation_lifecycle` event re-derives `elapsed_duration_ms` from the raw export's own timing metadata on every export request; the value is not stable across requests for the SAME generation. - **d8al**: Claude.ai does not consistently emit a real attachment id across export vintages of the same conversation — one vintage carries a real UUID, the other has none. No id-minting scheme can make a real id and a synthetic hash collide. - All three (plus bu1i's already-merged attachment-acquisition-state fix) are symptoms of one thing: the comparison value contained things that are not properties of the conversation. ## Solution - `SessionRevisionProjection` (`ids.py`) now projects `message_contents`/`attachment_contents`/`event_contents` as `frozenset`s of `(identity, content)` pairs — never ordered tuples, never the array index. - Attachment identity drops the provider id unconditionally (`message_id, name, mime_type` only) instead of using it when present and falling back when absent. The strict/loose duality and its pairwise correlation machinery (`_correlate_attachments`, `_attachments_equivalent`, `AttachmentRecord`) are **deleted**, not bypassed. - Event content is built from an explicit per-event-type **allowlist** (`_EVENT_CONTENT_PAYLOAD_ALLOWLIST`), not a denylist of fields discovered volatile after shipping: a new field a parser adds later is excluded from comparison by construction. Only `generation_lifecycle` has a registered allowlist (`state`, `evidence_source`, `fidelity`); every other event type still compares its full payload. - `classify_membership_revisions` collapses what used to be four layered mechanisms (positional-prefix message test, denylist-stripped ordered event-hash prefix test, strict/loose attachment correlation, session-hash/metadata-timestamp tiebreak) into one `_relation` function applied uniformly to all three axes, plus a simple representative-collapse + adjacent-pair containment check. - `session_hash` (idempotency) is **unchanged**: it still covers the full, order-sensitive, unstripped payload, so a real reorder, duration change, or id change still triggers a re-write. Only the *comparison* layer is content-only. **Kept, not deleted**: `_provider_ordered_browser_snapshots`. Browser-captured DOM/native snapshots synthesize their own local message/attachment ids from DOM structure, not stable provider identity, so the content-only relation genuinely cannot correlate a DOM-to-native fidelity upgrade — deleting it risked silently breaking real browser-capture archival behavior this lane has no fixture coverage to verify independently. **Designed, tested, not wired**: `_maximal_evidence_fallback` — a deterministic (frontier + raw_id tiebreak, proven order-independent) presence-guarantee pick for a genuine, irreducible conflict, so a document is never simply absent from the archive. Wiring it into `classify_membership_revisions`'s return path trips a real, documented `archive.py` write-back invariant ("never retire an unrelated accepted head", with production incident history — polylogue-miwv, #3397/#3398) whenever the fallback pick differs from an already-established head. Proven by two failing integration tests during verification (`test_divergent_bundle_member_preserves_last_accepted_session` and its sibling) — not a theoretical concern. Landing it safely needs either the classifier/caller to carry existing-head context, or the write-back guard to accept a re-affirmed-quarantined outcome; both belong to `archive.py`'s write path, out of this lane's scope. A genuine conflict still quarantines exactly as before (`accepted_raw_ids == ()`, everything in `ambiguous_raw_ids`). ## Verification **Unit tests**: `devtools test tests/unit/archive/test_session_revision_membership.py tests/unit/pipeline/test_pipeline_ids.py tests/unit/sources/test_revision_backfill.py tests/unit/storage/test_revision_replay.py tests/unit/storage/test_browser_capture_origin_repair.py` — `129 passed`. **`devtools verify --quick`** — `exit_code 0` (format, lint, mypy --strict, render-all-check, layering, hash-boundary-census, schema-versioning, all clean). **Anti-vacuity**: mutated the core content-conflict clause in `_axis_relation` (dropped the "content disagrees under a shared identity" check) — 8 tests failed, confirming it is exercised and load-bearing. **Live-archive census** (read-only against `/realm/db/polylogue`, no mutation; full population, not a sample — replaying `parse_payload` → `session_revision_projection` → `classify_membership_revisions` for every ambiguous cohort, comparing this branch against `origin/master`): | origin | cohorts | old resolved | new resolved | regressed | |---|---|---|---|---| | `claude-ai-export` | 587 | 0 (0%) | **554 (94.4%)** | **0** | | `chatgpt-export` | 135 | 1 (0.7%) | **119 (88.1%)** | **0** | Zero regressions against cohorts that already resolved under the old logic, for both origins. The residual 33 + 16 = 49 cohorts are not resolved by this PR (the presence-guarantee fallback that would resolve genuine conflicts deterministically is designed/tested but not wired, per above) — consistent with the small number of real forks (message content genuinely edited, not just reordered) found during manual inspection of sampled residue. ## Acceptance-criteria honesty (polylogue-aggz) - ✅ Comparison value built from an explicit content-only allowlist (events) / content-derived-only identity (attachments/messages) — a volatile field added to a parser later is excluded by construction, tested directly (`test_non_allowlisted_event_type_keeps_its_full_payload_as_content` proves the allowlist boundary; every new-field-volatility test in this PR is the "add a volatile field, comparison unaffected" shape the bead asked for). - ✅ `superseded_prefix`/`superseded_equivalent` vocabulary distinction: confirmed (via research, not touched — `archive.py` is outside this lane's scope) that both already map to the identical `ApplicationDecision.SUPERSEDED` / `revision_authority = "byte_proven"` downstream; the only place they differ is a free-text audit `detail` string. No `archive.py` change was needed to satisfy this. - ✅ At least two special-case paths deleted: `_merge_attachment_id_presence_variants`, `_correlate_attachments`, `_attachments_equivalent`, `_attachment_evidence_preserved`, `_attachment_axis_grew`, `AttachmentRecord`/`attachment_records`, the `by_content`/`by_session_hash`/`metadata_variants`/timestamp-tiebreak layering, `_message_evidence_preserved`, `_strictly_dominates` — net **-255 lines** across the two production files. - ⏸️ Deferred, explicitly not silently dropped: the presence-guarantee fallback (needs `archive.py` write-path coordination) and `_provider_ordered_browser_snapshots` deletion (needs browser-capture parser id-stability verification this lane could not do safely in scope). - Invariant 2 (single write chokepoint) and Invariant 3 (versioned derived state) from polylogue-aggz are explicitly out of this lane's scope (owned by the `ingest_batch`/`archive.py`-owning lanes per this task's file boundaries) and not addressed here. ## Review follow-up: four defects fixed (chatgpt-codex-connector) Automated review found the content-only relation did not yet fully hold the invariant it claims, in four concrete ways. All four are fixed on this branch: - **P1 — source authority lost when collapsing equal revisions** (`session_revision_membership.py`). When a direct export and a browser capture projected to equal content, the equal-content collapse picked its representative by provider timestamp (then raw_id) BEFORE any source-authority ordering ran, so a browser capture could outrank its own authoritative export. New `_equal_content_representative` decides by authority first (direct export over browser capture, native over DOM), mirroring the ordering `_direct_export_precedence`/ `_browser_snapshot_dominates` already apply to the non-equal growth-chain case, falling back to timestamp/raw_id only when neither side's provenance outranks the other's. - **P2 — event identity unstable under a sibling appearing** (`ids.py`, the most serious of the four: it reintroduced exactly the identity-instability bug class this PR exists to eliminate). An event's canonical identity shifted from `base_identity` to `hash(base_identity, content)` purely because a SECOND event later shared its base identity, in the SAME revision — so an ordinary event-growth revision could compare as a disjoint conflict instead of containment. Now always `hash(base_identity, content)`, unconditionally: an item's identity must not depend on what else is in the set. - **P2 — colliding attachment identities silently dropped content** (`session_revision_membership.py`). Two acquired attachments sharing one identity (same message/name/mime, different bytes) fed a plain `dict(contents)`, so the second content hash silently overwrote the first — a real conflict could compare as equal, worse than either honest outcome. New `_content_by_identity` groups every content hash under its identity into a set, so a collision now always degrades to `conflict`. - **P2 — duration stripping keyed on `event_type` alone** (`ids.py`). The `generation_lifecycle` allowlist strip applied to every event of that type, including browser-capture's own DOM/UI observations (`duration_semantics` values like `dom_observed_wall`), which are a real first-party measurement, not the ChatGPT-parser-remeasured value nuec targeted. Now gated on `duration_semantics == "provider_reported_elapsed"` (wiring up the previously-dead `_PROVIDER_REPORTED_ELAPSED_MARKER_KEY`/ `_VALUE` constants). Both `_provider_ordered_browser_snapshots` (kept, not deleted) and `_maximal_evidence_fallback` (designed/tested, not wired — `archive.py` write-back invariant, out of scope) are unchanged per explicit instruction. **Tests**: one regression test per finding in `tests/unit/archive/test_session_revision_membership.py`, each verified by manually mutating the corresponding production line back to the buggy behavior and observing the exact expected failure (see commit `test(archive): cover the four aggz identity-invariant defects with mutation-proof tests` for the anti-vacuity detail per finding). **Re-run live census** (read-only, same methodology, apples-to-apples A/B on the identical ambiguous-cohort population — this branch's fixed `classify_membership_revisions` vs. the pre-fix `39591e17d`/`78d884b8c` state, same input rows for both runs): | origin | population | pre-fix resolved | fixed resolved | regressed | |---|---|---|---|---| | `claude-ai-export` | 587 | 554 (94.4%) | **554 (94.4%)** | **0** | | `chatgpt-export` | 136 | 119 (87.5%) | **120 (88.2%)** | **0** | (Population here is 136/587 for chatgpt-export/claude-ai-export — measured directly from `raw_session_memberships` without the earlier equal-message-count pre-filter, vs. this PR's original 135/587; the claude-ai-export count is unaffected either way.) The counts barely move — none of the four fixes change *whether* a cohort resolves for the vast majority of cases, only the P1 fix changes *which* raw is materialized as the accepted head for already-resolving equal-content cohorts (verified directly: the one real flip below is a P1 case, not P2). Zero regressions in either origin, confirmed by diffing the exact unresolved-cohort key sets between the two runs, not just the aggregate counts. One concrete flip, traced to root cause: `chatgpt:69d5383e-…` (4368 messages, 4 raw revisions — 1 direct export + 3 native browser captures, all pairwise-`equal` or `conflict` identically under old and new code). Pre-fix, the equal-content collapse of the export + 2 of the 3 natives picked the LATER-timestamped native as representative (losing the export's `browser_snapshot_fidelity=None` marker), so `_direct_export_precedence` then found zero non-browser candidates among the representatives and the cohort stayed quarantined. Post-fix, the export always survives that collapse, `_direct_export_precedence` fires as designed, and the cohort resolves. This is exactly the shape P1 describes, confirmed against a real archive row, not just the unit-test fixtures. **Residual population** (33 claude-ai-export + 16 chatgpt-export cohorts, unchanged in count from the original measurement modulo the one flip above) is the same population the unwired `_maximal_evidence_fallback` exists to serve: genuine, irreducible conflicts (real forks — content edited, not just reordered/re-measured/re-captured) where no containment chain exists at all. **This PR does not resolve them and does not claim to** — wiring the fallback needs `archive.py` write-path coordination (`_maximal_evidence_fallback`'s docstring states the concrete blocking invariant), which is explicitly out of this lane's scope; the residual count will not reach zero without that follow-up landing. Ref polylogue-aggz Ref polylogue-bu1i Ref polylogue-c429 Ref polylogue-nuec Ref polylogue-d8al Ref polylogue-hith 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude <noreply@anthropic.com>
Sinity
added a commit
that referenced
this pull request
Jul 30, 2026
…chive.py (#3406) ## Summary Splits the raw-revision-authority and membership-classification concern out of `polylogue/storage/sqlite/archive_tiers/archive.py` (13,338 lines) into a new module, `polylogue/storage/sqlite/archive_tiers/revision_governance.py` (2,841 lines), so that concern stops living inside the query-surface god-file. ## Problem `docs/architecture-hotspots.md` documents `archive.py`'s public contract as "`ArchiveStore` — every SELECT-shaped query surface (sessions, messages, blocks, insights reads, search)". The file also owned ~55 `raw_*`/write methods implementing revision/membership write authority — a different concern with different invariants, and the exact cluster where every defect found on 2026-07-30 lived (PRs #3394, #3396, #3397, #3398, #3401). Ref polylogue-1r9c (decomposition epic), polylogue-c737 (a defect from this cluster). ## Solution - New module `archive_tiers/revision_governance.py` owns raw-revision replay, membership classification/census, and the narrow raw-write paths that hand a parsed session to that authority — documented contract in the module docstring (what it owns / what it refuses). - Every governance function takes `store: RawRevisionGovernanceHost` (a `Protocol`) as its first argument instead of being a method on `ArchiveStore`. The protocol names exactly the seven `ArchiveStore` members governance code touches (`_conn`, `_ensure_source_conn`, `_blob_publisher`, `_pending_raw_parse_states`, `_preacquire_attachment_blobs`, `_write_counts`, `_skipped_counts`). `ArchiveStore` satisfies it structurally — no inheritance, no import of `ArchiveStore` from the new module (which would create an import cycle). - `ArchiveStore` keeps one-line delegating methods with unchanged signatures, so every external caller (`sources/live/batch.py`, `sources/live/append_ingest.py`, `sources/revision_backfill.py`, `storage/repair.py`, `pipeline/services/archive_ingest.py`, `api/archive.py`, and every test holding an `ArchiveStore` instance) is untouched. This is not a compatibility shim — there is exactly one implementation (in the new module), and the delegator body is the call site, same shape as any other extract-function-then-delegate refactor. - Updated `docs/plans/layering.yaml`'s `writer_modules` inventory: `archive.py`'s only remaining direct writer is `delete_sessions` (index-only); the `raw-membership-classification` twin-write contract and its 11 entrypoints moved to the new module's own entry. - Regenerated `docs/plans/topology-target.yaml` / `docs/topology-status.md`; updated `docs/architecture-hotspots.md`'s line-count row and `docs/plans/hash-boundary-registry.yaml`'s moved `hashlib.sha256` call site. ### Connection-interface decision (the design question this task turns on) Considered and rejected two alternatives: - **Bare `sqlite3.Connection`** — insufficient. Governance needs the lazily- opened `source.db` connection, the blob publisher, and the pending-raw- parse-state batch too, not just the index connection. - **A mixin `ArchiveStore` inherits from** — rejected because inheritance gives every moved method unrestricted `self` access to the other ~9,000 lines of read-surface internals, which is exactly the "reach back into ArchiveStore internals" this extraction is meant to make structurally impossible, not merely discouraged by convention. The `Protocol` makes the dependency surface an explicit, readable, narrow contract instead of "whatever `self` happens to have". ### A real regression found and fixed mid-PR Four tests monkeypatch an `ArchiveStore` method as a spy/crash-injection point (`_index_parsed_for_retained_raw`, `_write_parsed_precedence_result`, `mark_raw_parse_succeeded`, `record_revision_application_sync`). Under the old single-class shape, sibling governance methods called each other via `self.<method>()`, so patching the class attribute intercepted internal calls too. After the move, sibling governance *functions* call each other by direct module-global reference, bypassing the `ArchiveStore` delegator entirely — so those four tests silently stopped testing what they claimed to. Confirmed as a genuine regression (not pre-existing) by running the exact failing tests against a detached checkout of the pre-extraction parent commit — all passed there. Fixed by patching the `revision_governance` module attribute (the real internal call target) in the affected tests instead of the `ArchiveStore` delegator, across `tests/unit/storage/test_revision_replay.py`, `tests/unit/sources/test_revision_backfill.py`, `tests/unit/sources/test_live_batch_support.py`, and `tests/unit/sources/test_live_cursor_persistence.py`. This is the signal that behavior (specifically, internal call dispatch) moved, not a change in externally observable archive behavior. ## Non-goals / what was deliberately left alone - `polylogue/pipeline/ids.py`, `polylogue/archive/session_revision_membership.py`, `polylogue/sources/dispatch.py`, the parsers, and `polylogue/pipeline/services/ingest_batch/*` — untouched, per scope. - `write_hook_event` stays in `archive.py` — hook-event ingest is a different concern (evidence linked to a session, never itself a raw revision candidate; polylogue-31r1), not moved. - No import cycle formed; the new module never imports `ArchiveStore`. ## Verification - `mypy --strict` on every touched module: clean. - `devtools test` — mission's targeted files plus every file discovered by grepping for `monkeypatch.setattr(...)` on any of the 24 governance names called internally by a sibling governance function: **215 passed, 0 failed** (`tests/unit/storage/test_revision_replay.py`, `tests/unit/sources/test_revision_backfill.py`, `tests/unit/storage/test_raw_authority_ledger.py`, `tests/unit/sources/test_live_batch_support.py`, `tests/unit/storage/test_raw_revision_authority.py`, `tests/unit/sources/test_live_cursor_persistence.py`, `tests/unit/pipeline/test_archive_ingest_commit_batching.py`). Note: `tests/unit/storage/test_crud.py` named in the task no longer exists in this checkout (removed by prior test-infra churn) — confirmed via `git log --all -- tests/unit/storage/test_crud.py`, skipped. - `devtools verify --quick`: green (ruff format/check, mypy --strict, render all, topology, layering, hash-boundary-census, all other gates). - `archive.py`: 13,338 → 11,324 lines (-15.1%). New module: 2,841 lines. Not run: `devtools verify --all` (full suite) — out of scope for a `--quick` gate per repo convention; CI's post-merge `test` job will run it.
Sinity
added a commit
that referenced
this pull request
Aug 1, 2026
…o one function (#3505) ## Summary Consolidates a duplicated revision-authority refusal check that lived independently in two production session-write paths into one function, `revision_authority_refuses_write` (`storage/sqlite/archive_tiers/ingest_precedence.py`). ## Problem `ArchiveStore._write_parsed_precedence_result` (`archive_tiers/revision_governance.py`, the real implementation behind `write_parsed_for_retained_raw`/`write_raw_and_parsed`, used by the one-shot importer and the revision-authority-aware live batch path) and `_write_session` (`pipeline/services/ingest_batch/_core.py`, the daemon's default batch-ingest write path for most non-drive origins) each hand-carried their own copy of two checks: "has this session_id already been claimed by an accepted `raw_revision_heads` cohort winner" and "is this raw's own `raw_session_memberships` decision recorded `ambiguous`". polylogue-c737 is the concrete symptom this duplication produced: the live archive had 28 `aistudio-drive` cohorts genuinely recorded `ambiguous` (correctly refused a winner) whose sessions were nonetheless materialized in `index.db` with 641 attachments stuck `unfetched`. PR #3397 fixed the ambiguous-membership refusal in `_write_parsed_precedence_result`; PR #3398 then had to independently re-derive and hand-apply the identical fix to `_write_session` because it was a separate copy that had never been patched — "the signature of duplicated semantics rather than a missing check" (polylogue-aggz Invariant 2). I read both write functions in full (`revision_governance.py` ~2900 lines, `ingest_batch/_core.py` ~1970 lines) before touching anything, and traced real callers rather than assuming duplication from file/line coordinates alone. Note: the coordinator's initial lead (`_write_parsed_precedence_result`/`write_parsed` duplicated between `revision_governance.py` and `archive.py`) turned out to be a false positive — `archive.py`'s copies are genuine one-line delegating methods to the sole implementation in `revision_governance.py`, exactly as that module's own docstring documents (an already-completed extraction, not a live duplication). The real duplication was the `_core.py` / `revision_governance.py` pair named in polylogue-c737's own closure notes as a "known sibling hole." ## Solution - Added `revision_authority_refuses_write(conn, source_conn, *, session_id, raw_id, provider_session_id)` to `ingest_precedence.py` — the module that already owns the sibling precedence primitives shared by these same two write paths (`should_skip_stale_replace`, `browser_capture_precedence`, `session_has_parser_ingest_flag`, `stored_message_count`), so this is a continuation of an established consolidation pattern (see that module's own `should_skip_stale_replace` docstring, itself a prior 3-copy→1 consolidation for polylogue-t83e), not a new one-off shim. - `_write_parsed_precedence_result` and `_write_session` now call this one function instead of carrying their own inline SQL + comment block. The two duplicated blocks (~59 lines and ~65 lines including their explanatory comments) are **deleted**, not left as bypassable dead alternates, per this repo's surgical-renewal rule. - `session_id`/`raw_id`/`provider_session_id` are required keyword arguments with no defaults — a caller cannot invoke the gate without supplying identity, and (per the anti-vacuity check below) cannot silently skip calling it without a production regression test failing. ## Acceptance criteria (Invariant 2 only — this lane's scope per coordinator reshape) - "Exactly one code path can write a session, and it cannot be called without authority": **partially satisfied**. The specific duplicated-semantics bug shape polylogue-c737/#3397/#3398 exhibited (independently hand-maintained copies of the revision-authority refusal check) is now structurally impossible — there is exactly one implementation of that refusal decision. This PR does **not** merge the two write paths themselves into one function; `_write_parsed_precedence_result` and `_write_session` remain separate entry points serving genuinely different callers (one-shot importer / revision-authority-aware live batch vs. the daemon's default batch-ingest path), each still carrying its own freshness/browser-capture-precedence logic downstream of the shared gate. A full single-function merge of the two write paths is a larger, riskier change spanning ~4900 lines across two files and many call sites; I judged landing this smaller, verified slice safer than forcing that merge in one lane, per this lane's explicit "acceptable to land a smaller slice" guidance. - "At least two existing special-case paths are DELETED, not merely bypassed": satisfied for this slice — both inline duplicate-check blocks (with their explanatory comments) are deleted from `revision_governance.py` and `ingest_batch/_core.py`. ## Anti-vacuity Production callers exercised: `ArchiveStore.write_parsed_for_retained_raw` (→ `_write_parsed_precedence_result`) is reached from the one-shot importer (`pipeline/services/archive_ingest.py`) and the revision-authority-aware live batch path; `_write_session` is reached from the daemon's default batch-ingest path (`_write_session_entry` → `_process_ingest_batch_sync`, the daemon's primary write path for most non-drive origins). Mutation proof (temporarily replacing each call site's `if revision_authority_refuses_write(...): ...` with `if False: ...`, one at a time, then reverting): - `revision_governance.py` mutation → `tests/unit/storage/test_revision_replay.py::test_precedence_write_refuses_a_raw_recorded_ambiguous` fails (`assert (1,) == (0,)`, i.e. the ambiguous raw's session gets written). - `ingest_batch/_core.py` mutation → `tests/unit/pipeline/test_ingest_batch.py::test_write_session_refuses_a_raw_recorded_ambiguous_membership` fails (`assert True is False`). Both pass again once reverted (see Verification below for the clean run). ## Follow-ups (not in scope here) - A genuine single-function merge of `_write_parsed_precedence_result` and `_write_session` into one literal chokepoint (rather than one shared authority-gate they both call) remains open, if the full Invariant 2 shape is wanted. I have not filed a new bead for this per this lane's bd-write restrictions (worktree `.beads/issues.jsonl` reimport hazard) — reporting it here for the coordinator to file. - `ArchiveStore.write_parsed`/`SessionRepository.save_parsed_session` is a third code path that writes directly to `sessions` via `write_parsed_session_to_archive` with **no** revision-authority consultation at all (no raw_id, no governed-head check, no ambiguous- membership check). Traced its only production caller (`repository_writes.py::save_parsed_session`) and that method itself has no real production caller — only a docstring example and test infrastructure reference it. Currently dead in the real ingest pipeline, but it is a public method reachable from `SessionRepository`/`ArchiveStore`, so it remains a structurally-possible fourth way to write a session bypassing authority if anything ever calls it for real. Worth a follow-up bead to either delete it or route it through the same gate. ## Verification ``` python -m devtools test tests/unit/storage/test_revision_replay.py \ tests/unit/pipeline/test_ingest_batch.py \ polylogue/storage/sqlite/archive_tiers/ingest_precedence.py \ polylogue/storage/sqlite/archive_tiers/revision_governance.py \ polylogue/pipeline/services/ingest_batch/_core.py \ tests/unit/storage/test_archive_tiers_archive.py # 127 passed python -m devtools verify --quick # exit_code: 0 (ruff format/check, mypy --strict, render all --check, layering, closure-matrix, schema policies) ``` Ref polylogue-aggz Co-authored-by: Claude <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Corrects the scope of the guard merged in #3397. The refusal is right; it was keyed on the raw instead of on the membership being written, which would have deleted 489 legitimately-indexed sessions at the next full rebuild.
Problem
#3397 stopped
_write_parsed_precedence_resultfrom writing a session for a raw whose membership was recordedambiguous. That fixed a real leak — 28 live aistudio-drive cohorts had both members materialized, 641 attachment refs reportedunfetchedwhile the bytes sat in the blob store.But the predicate asks whether the raw has any ambiguous membership, while the write it guards is per-session. One retained raw routinely lowers to many independently-arbitrated sessions: a Claude Code transcript plus its subagent sidechains, a bundle member set. So a single ambiguous sibling suppresses every session that raw carries.
Measured on the live archive:
A concrete pair from that set — one raw, two memberships, arbitrated differently:
The raw-scoped predicate refuses both. That trades a fidelity downgrade for outright absence, which is the worse of the two failures — an unfetched attachment is visibly wrong, a missing session is not — and it would have landed silently at the next full index rebuild rather than at merge time.
This was raised as a P1 by review on #3397. I merged before triaging it, which was my error; this corrects it forward rather than reverting, because the underlying refusal is correct and only its scope was wrong.
Solution
The predicate now matches
provider_session_idalongsideraw_id, so each membership is judged on its own recorded decision.session.provider_session_idis already in scope at the call site, so no plumbing was needed.Verification
test_precedence_write_allows_a_non_ambiguous_sibling_membership_on_the_same_rawbuilds the live shape — one raw, two memberships, oneambiguousand oneapplied— and asserts both halves: the ambiguous membership is still refused, and its settled sibling is not collateral damage. Asserting only the second half would pass against a guard that had been deleted outright.Anti-vacuity: reverting the predicate to the merged raw-scoped form (
WHERE raw_id = ? AND decision = 'ambiguous') fails exactly that one test and no other — so the scope is load-bearing, and #3397's original refusal is demonstrably untouched by this change.Ref polylogue-c737