refactor(storage): extract raw revision/membership governance from archive.py - #3406
Merged
Merged
Conversation
…chive.py Problem: archive_tiers/archive.py (13,338 lines) mixes its documented query-surface contract with ~55 raw-revision-authority and membership- classification methods -- a different concern with different invariants. Every 2026-07-30 defect (PRs #3394/#3396/#3397/#3398/#3401) landed in this cluster or its callers (polylogue-c737 et al, polylogue-1r9c hotspot map). Solution: moved the raw revision/membership governance surface to a new module polylogue/storage/revision_governance.py as free functions taking `store: RawRevisionGovernanceHost` (a Protocol naming 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) instead of `self`. 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. Considered and rejected: (a) passing the bare sqlite3.Connection -- insufficient, governance needs the lazy source-conn, blob publisher, and pending-raw-parse-state batch too; (b) 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, exactly the "reach back into internals" the extraction is meant to make impossible. archive.py: 13,338 -> 11,318 lines (-15.1%). New module: 2,838 lines (documented contract + moved implementation). Verification: mypy --strict clean on both files; ruff format/check clean; python -c "import polylogue.storage.sqlite.archive_tiers.archive" succeeds.
…after governance extraction Problem: the previous commit extracted raw-revision/membership governance to a new module but (1) put it at polylogue/storage/revision_governance.py, outside layering.yaml's writer_modules.mutation_roots scan boundary (archive_tiers/), so `devtools verify layering`/`verify topology` failed; (2) left archive.py's docstring claiming ownership of both index and source tiers when its only remaining direct writer (delete_sessions) is index-only; (3) broke four tests that monkeypatch an ArchiveStore method as a spy/crash point (_index_parsed_for_retained_raw, _write_parsed_precedence_result, mark_raw_parse_succeeded, record_revision_application_sync) -- these functions are now called by sibling governance functions as direct module-internal references, not through `self.` dynamic dispatch, so patching the ArchiveStore delegator no longer intercepts the call the test actually exercises (proven via a pre-extraction baseline run: all 10 tests below pass on origin/master, 6 initially failed here). What changed: - Move polylogue/storage/revision_governance.py to polylogue/storage/sqlite/archive_tiers/revision_governance.py (writer-module scanning is scoped to archive_tiers/, matching every sibling writer module). - Split archive.py's writer_modules.yaml entry: delete_sessions stays, index-only; the twin-write raw-membership-classification contract and its 11 entrypoints move to the new module's own writer_modules entry. - Fix docs/plans/hash-boundary-registry.yaml's moved hashlib.sha256 call site and regenerate docs/plans/topology-target.yaml / docs/topology-status.md. - Add "ArchiveRawParsedWriteResult" to archive.py's __all__ (mypy --strict --no-implicit-reexport requires it, since it's now imported rather than defined locally). - Fix the 6 broken monkeypatch spy points across tests/unit/storage/test_revision_replay.py, tests/unit/sources/test_revision_backfill.py, and tests/unit/sources/test_live_batch_support.py / test_live_cursor_persistence.py: patch the revision_governance module attribute (the real internal call target) instead of the ArchiveStore delegator method (which only intercepts external callers). Verification: devtools verify --quick green (mypy --strict, ruff, render all, topology, layering, hash-boundary-census all pass). devtools test across the 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. Cross-checked the 4 originally-failing tests against a genuine pre-extraction baseline (detached checkout of the parent commit) to confirm the regression was real, not flaky, before fixing.
|
Warning Review limit reached
Next review available in: 23 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 (11)
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 |
Sinity
added a commit
that referenced
this pull request
Aug 3, 2026
…e tie-break (#3616) ## Summary Fixes a cross-feature interaction between PR #3574 (byte-identical duplicate collapse) and the pre-existing (#3406) membership-census guard: a duplicate of the accepted revision baseline could make `plan_revision_replay` misclassify a cohort with one unambiguous baseline as ambiguous, which then routed backfill/rebuild into a fallback path that tripped `ActiveByteRevisionChainError`. ## Problem PR #3574 collapses byte-identical full-revision captures into one representative plus "duplicate" decisions. A follow-up fix (#3580) made a duplicate's `acquisition_generation` mirror its representative's generation for correct display ordering. Together these made a duplicate of the accepted baseline/head share that node's generation number. `plan_revision_replay` (`polylogue/archive/revision_replay.py`) selects the unique baseline by finding the single FULL, byte-proven candidate with the newest `acquisition_generation`. A duplicate sharing that generation now looked like a second competing baseline, so the tie-break declared the cohort ambiguous and returned an empty `accepted_raw_ids` for a cohort that in fact has one unambiguous baseline. Backfill/rebuild callers (`sources/revision_backfill.py`) treat an empty `accepted_raw_ids` as "no accepted chain" and fold every full-only raw for the identity into membership governance via `replace_raw_membership_census(..., retire_full_revision_governance=True)`. That path's guard then raised `ActiveByteRevisionChainError` the moment it tried to retire the baseline raw, because the duplicate's own `baseline_raw_id` column still durably points at it. Confirmed via `git log -S "ActiveByteRevisionChainError"` that the guard itself is unchanged since #3406; the trigger is #3574/#3580's newly-created generation-sharing shape, not the guard. ## Solution The guard is correct and intentionally strict: a raw genuinely still pointed at by another raw's `predecessor_raw_id`/`baseline_raw_id` must not be retired out of byte governance (a real dependent chain shouldn't lose its baseline out from under it). The defect is upstream, in `plan_revision_replay`'s inability to distinguish a harmless duplicate from a genuinely competing baseline. A FULL, byte-proven candidate with no `predecessor_raw_id` that is NOT itself the cohort's `baseline_raw_id` can only be such a duplicate -- the classifier that writes these columns (`archive/revision_authority.py`) guarantees at most one true root per cohort (`relation="baseline"`). `plan_revision_replay` now excludes that signature from the baseline candidate pool via `_is_full_duplicate_signature`; excluded duplicates still fall through to the existing trailing loop and are marked `DEFERRED` against whatever head the genuine evidence accepts. No schema change; this is a pure classification/replay-planning fix in `polylogue/archive/revision_replay.py`, shared correctly by both the backfill/rebuild path (`revision_governance.classify_raw_revision_cohort`) and the live watcher (`sources/live/batch.py`), which both build `RevisionCandidate` lists from the same `raw_sessions` columns. ## Verification - `devtools test tests/unit/sources/test_revision_backfill.py::test_backfill_content_cache_across_pages_reduces_parses_and_matches_uncached_archive tests/unit/storage/test_rebuild_paging_content_order.py::test_rebuild_content_order_paging_dedups_first_time_classification_via_content_cache` -- 2 passed (previously failing with `ActiveByteRevisionChainError`; this is the reproduction the tracking item names). - `devtools test tests/unit/storage/test_revision_replay.py tests/unit/storage/test_raw_revision_authority.py tests/unit/sources/test_revision_backfill.py tests/unit/storage/test_rebuild_paging_content_order.py tests/unit/sources/test_live_batch_support.py -k "revision or membership or duplicate or backfill or paging"` -- 127 passed. - Full-file runs of the same five files -- 169 passed, 3 pre-existing failures in `test_live_batch_support.py` confirmed identical with this change reverted (`git stash`); unrelated to this fix. - Red-first: the two new tests in `test_revision_replay.py` were confirmed failing against the unpatched `revision_replay.py` (`git stash`), then passing after the fix. - `devtools verify --quick` -- exit 0 (format, lint, mypy --strict, render all --check, layering, closure-matrix, schema roundtrip, schema-versioning, classifier-fingerprints, schema-promotion-audit, etc., all passed). Anti-vacuity: the failing production path exercised is `sources/revision_backfill.py`'s `if not plan.accepted_raw_ids:` fallback into `archive.replace_raw_membership_census(..., retire_full_revision_governance=True)`; the mutation that makes the new tests fail is reverting the `_is_full_duplicate_signature` exclusion in `plan_revision_replay` (confirmed via `git stash` on `polylogue/archive/revision_replay.py` alone). The guard invariant itself (a real dependent chain must block retirement) is separately re-asserted inside the new archive-level test by calling `replace_raw_membership_census` directly and expecting `ActiveByteRevisionChainError`, and is unaffected by this change (pre-existing `test_membership_sweep_defers_sibling_retirement_instead_of_quarantining_current_raw` in `test_live_batch_support.py` still passes unmodified). Ref polylogue-qhk8z ## Scope not addressed None -- this closes the full interaction described in the tracking item (root-cause fix in the replay planner, not a guard loosening), with a regression test reproducing the exact reported failure plus a unit-level test pinning the underlying tie-break defect.
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
Splits the raw-revision-authority and membership-classification concern out
of
polylogue/storage/sqlite/archive_tiers/archive.py(13,338 lines) into anew 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.mddocumentsarchive.py's public contract as"
ArchiveStore— every SELECT-shaped query surface (sessions, messages,blocks, insights reads, search)". The file also owned ~55
raw_*/writemethods 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
archive_tiers/revision_governance.pyowns 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).
store: RawRevisionGovernanceHost(aProtocol) as its first argument instead of being a method onArchiveStore. The protocol names exactly the sevenArchiveStoremembersgovernance code touches (
_conn,_ensure_source_conn,_blob_publisher,_pending_raw_parse_states,_preacquire_attachment_blobs,_write_counts,_skipped_counts).ArchiveStoresatisfies itstructurally — no inheritance, no import of
ArchiveStorefrom the newmodule (which would create an import cycle).
ArchiveStorekeeps 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
ArchiveStoreinstance) is untouched. This is nota 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.
docs/plans/layering.yaml'swriter_modulesinventory:archive.py's only remaining direct writer isdelete_sessions(index-only); the
raw-membership-classificationtwin-write contract andits 11 entrypoints moved to the new module's own entry.
docs/plans/topology-target.yaml/docs/topology-status.md;updated
docs/architecture-hotspots.md's line-count row anddocs/plans/hash-boundary-registry.yaml's movedhashlib.sha256call site.Connection-interface decision (the design question this task turns on)
Considered and rejected two alternatives:
sqlite3.Connection— insufficient. Governance needs the lazily-opened
source.dbconnection, the blob publisher, and the pending-raw-parse-state batch too, not just the index connection.
ArchiveStoreinherits from — rejected because inheritancegives every moved method unrestricted
selfaccess to the other ~9,000lines 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
Protocolmakes the dependency surface an explicit, readable, narrowcontract instead of "whatever
selfhappens to have".A real regression found and fixed mid-PR
Four tests monkeypatch an
ArchiveStoremethod as a spy/crash-injectionpoint (
_index_parsed_for_retained_raw,_write_parsed_precedence_result,mark_raw_parse_succeeded,record_revision_application_sync). Under theold single-class shape, sibling governance methods called each other via
self.<method>(), so patching the class attribute intercepted internalcalls too. After the move, sibling governance functions call each other by
direct module-global reference, bypassing the
ArchiveStoredelegatorentirely — 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_governancemodule attribute (the real internal call target) in the affected tests
instead of the
ArchiveStoredelegator, acrosstests/unit/storage/test_revision_replay.py,tests/unit/sources/test_revision_backfill.py,tests/unit/sources/test_live_batch_support.py, andtests/unit/sources/test_live_cursor_persistence.py. This is the signalthat 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, andpolylogue/pipeline/services/ingest_batch/*— untouched, per scope.write_hook_eventstays inarchive.py— hook-event ingest is a differentconcern (evidence linked to a session, never itself a raw revision
candidate; polylogue-31r1), not moved.
ArchiveStore.Verification
mypy --stricton every touched module: clean.devtools test— mission's targeted files plus every file discovered bygrepping for
monkeypatch.setattr(...)on any of the 24 governance namescalled 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.pynamed in the task no longer exists inthis 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, renderall, 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--quickgate per repo convention; CI's post-merge
testjob will run it.