fix(sources): restore fail-closed refusal for byte-governed heads#3240
Conversation
Problem Another lane reported 2 failures on current master: tests/unit/sources/test_live_batch_support.py:: test_bundle_replay_respects_unconvertible_single_session_head [bundle_texts2-False-False] and [bundle_texts3-False-True] (assert result.failed == [older_bundle], failing shape assert [] == [...]). The working hypothesis was that polylogue-miwv's messages_fts_identity INSERT OR REPLACE fix (PR #3239) removed an IntegrityError abort that had been masking this failure. Diagnosis (bisected by checking out each of the three recently-merged commits directly, not by stash): that hypothesis is wrong. The exact same test failure reproduces identically on commit b3429fa (#3234), the commit immediately BEFORE messages_fts_identity (#3235) was even introduced -- the ledger and my INSERT OR REPLACE fix are unrelated. git bisect via commit checkout traced the real regression to PR #3211 ("perf(sources): tree-byte decoded layer + in-cohort head-retire drift fix"), landed 2026-07-20, ~10.5 hours before #3235. That PR's "drift fix" half removed a byte-governance refusal from ArchiveStore.apply_raw_membership_classification (storage/sqlite/archive_tiers/archive.py) on the premise that its branch is only reachable after a real membership-governance conversion, so a still-set raw_sessions.logical_source_key must be interrupted-pass drift, not live evidence. That premise is false for the accepted head specifically: _apply_membership_sessions (sources/live/batch.py) unconditionally injects the CURRENT raw_revision_heads accepted raw into the comparison cohort even when it was NEVER converted -- exactly PR #2718's original scenario (a byte-governed head being compared against membership-discovered content for the first time). With the guard gone, a later-discovered bundle raw whose content happens to strictly extend the byte-governed head's content (a pure string-prefix superset) silently replaced the head via membership governance: message_count moved 2->3, accepted_raw_id changed, even though the head still had a live, unresolved QUARANTINED append raw blocking ordinary byte-chain conversion. Confirmed via a standalone diagnostic script inspecting raw_revision_heads/raw_session_memberships state directly (not relying on the test's own asserts, which stop at the first failure) -- content genuinely lands, not merely a reporting-list discrepancy: the fail-closed contract was actually violated. #3211 shipped this change bundled into an unrelated perf PR without running tests/unit/sources/test_live_batch_support.py in its own verification section, so the regression was undetected (per-PR CI also skips the heavy test suite by design). Solution Restored a byte-governance refusal in apply_raw_membership_classification, narrower than #2718's original blanket `logical_source_key IS NOT NULL` check so #3211's own interrupted-pass-drift resumption (a stale un-nulled key with no dangling append) keeps working: refuse only when (a) replay is about to CHANGE which raw is accepted (accepted_raw_id != existing_raw_id -- a no-op re-affirmation of the same raw is never refused, matching #2718's original condition) AND (b) a live raw_sessions row elsewhere in this logical_source_key still chains a predecessor_source_revision off the existing head's own source_revision and isn't already part of the classified cohort -- i.e. genuine unresolved byte-append evidence this classification pass never saw, not merely "was this raw ever membership-converted". Verification - Bisected via direct commit checkout (not git stash): reproduced the exact failure on b3429fa (pre-#3235) and traced the introducing diff to 996a3d6 (#3211) via `git log -S` on the removed guard string. - devtools test tests/unit/sources/test_live_batch_support.py -k test_bundle_replay_respects_unconvertible_single_session_head -> 4 passed (all four bundle_texts/succeeds/census_head parametrizations, including the succeeds=True case that must still work) - devtools test tests/unit/sources/test_live_batch_support.py tests/unit/storage/test_revision_replay.py tests/unit/sources/test_revision_backfill.py -> 136 passed - devtools test tests/unit/storage/test_raw_retention.py -> 59 passed - mypy --strict polylogue/storage/sqlite/archive_tiers/archive.py tests/unit/sources/test_live_batch_support.py -> Success, no issues - devtools verify --quick -> exit 0 (also regenerated docs/topology-status.md, which was already out of sync on unmodified origin/master before this branch -- unrelated pre-existing drift, picked up incidentally by the routine gate) - Anti-vacuity: reverted just the new guard body (replaced with a no-op `pass`) and reran the 4-case test -> 2 of 4 failed with the exact original symptom; restored the fix and reran -> 4 passed AC matrix | AC | Status | | --- | --- | | Diagnose true mechanism (bisect between the 3 merged changes) | Satisfied -- root cause is #3211, not messages_fts_identity/miwv | | Determine whether content actually lands or reporting-only | Satisfied -- content genuinely landed (message_count 2->3, head moved) before this fix | | Fix production path to preserve explicit refusal | Satisfied | | head_after == head_before + message_count stays (2,) for succeeds=False | Satisfied (already asserted by the pre-existing test, now passing) | Ref polylogue-miwv Co-Authored-By: Claude <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Warning Review limit reached
Next review available in: 6 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 Run ID: 📒 Files selected for processing (3)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
|
Coordinator review (CodeRabbit rate-limited): read the full diff. The bisect-driven diagnosis (regression from #3211's guard removal, reproduced on the pre-ledger commit — my unmasking hypothesis disproven with evidence) is exactly the right kind of root-cause work. The restored guard is correctly narrower than #2718's original: fires only when replay would CHANGE the accepted raw AND live append evidence chains off the existing head, so #3211's interrupted-pass-drift resumption is preserved. Test docstring records the full mechanism for future readers. One noted edge (accepted): with an empty classified set the NOT IN (NULL) predicate makes the guard under-fire, but that branch requires a non-empty classification by construction. Merging. |
…ession) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QUsH3Rhq6oAZpYPWcsZqnZ
Summary
Fixes 2 test failures reported on current master (
test_bundle_replay_respects_unconvertible_single_session_head[bundle_texts2-False-False]and[bundle_texts3-False-True]intests/unit/sources/test_live_batch_support.py). Root cause is not related tomessages_fts_identity/polylogue-miwv's earlier work (PR #3239) — diagnosis below.Problem
The working hypothesis (from another lane) was that PR #3239's
messages_fts_identityINSERT OR REPLACEfix removed anIntegrityErrorabort that had been accidentally masking this test failure. That hypothesis is disproven: the exact same failure reproduces identically on commitb3429fae6(#3234), the commit immediately beforemessages_fts_identity(#3235) was even introduced.Bisecting by direct commit checkout (not
git stash) traced the real regression to PR #3211 ("tree-byte decoded layer + in-cohort head-retire drift fix", 2026-07-20, ~10.5h before #3235). That PR removed a byte-governance refusal fromArchiveStore.apply_raw_membership_classificationon the premise that its branch is only reachable after a real membership-governance conversion — but_apply_membership_sessions(sources/live/batch.py) unconditionally injects the current accepted head into the comparison cohort even when it was never converted (exactly PR #2718's original scenario: a byte-governed head compared against membership-discovered content for the first time). With the guard gone, a later-discovered bundle raw whose content happened to strictly extend the byte-governed head's content silently replaced the head via membership governance — confirmed with a standalone diagnostic script:message_countmoved 2→3,accepted_raw_idchanged, even though the head still had a live, unresolvedQUARANTINEDappend raw blocking ordinary byte-chain conversion. The fail-closed contract was genuinely violated, not merely a reporting-list discrepancy. #3211 shipped this bundled into an unrelated perf PR without runningtest_live_batch_support.pyin its own verification.Solution
Restored a byte-governance refusal in
apply_raw_membership_classification, narrower than #2718's original blanketlogical_source_key IS NOT NULLcheck so #3211's own interrupted-pass-drift resumption keeps working: refuse only when (a) replay is about to change which raw is accepted, and (b) a liveraw_sessionsrow elsewhere in this logical identity still chains apredecessor_source_revisionoff the existing head's ownsource_revisionand isn't already part of the classified cohort — genuine unresolved byte-append evidence, not merely "was this raw ever membership-converted".Verification
b3429fae6(pre-feat(storage): add messages_fts_identity ledger for rowid-reuse detection #3235); traced the introducing diff to996a3d6d3(perf(sources): tree-byte decoded layer + in-cohort head-retire drift fix #3211) viagit log -Son the removed guard string.devtools test tests/unit/sources/test_live_batch_support.py -k test_bundle_replay_respects_unconvertible_single_session_head→ 4 passed (all parametrizations, including thesucceeds=Truecase)devtools test tests/unit/sources/test_live_batch_support.py tests/unit/storage/test_revision_replay.py tests/unit/sources/test_revision_backfill.py→ 136 passeddevtools test tests/unit/storage/test_raw_retention.py→ 59 passedmypy --strict polylogue/storage/sqlite/archive_tiers/archive.py tests/unit/sources/test_live_batch_support.py→ Success, no issuesdevtools verify --quick→ exit 0 (also regenerateddocs/topology-status.md, already out of sync on unmodifiedorigin/masterbefore this branch — unrelated pre-existing drift, picked up incidentally)AC matrix
head_after == head_before+message_countstays(2,)forsucceeds=FalseRef polylogue-miwv