Skip to content

fix: base-id coverage for sub-id projections in prune + sync (#231) - #233

Open
ranxianglei wants to merge 1 commit into
masterfrom
2026-09-09_subid-base-coverage
Open

fix: base-id coverage for sub-id projections in prune + sync (#231)#233
ranxianglei wants to merge 1 commit into
masterfrom
2026-09-09_subid-base-coverage

Conversation

@ranxianglei

Copy link
Copy Markdown
Owner

Fixes #231.

Bug

prune() decided coverage with exact string membership (covered.has(message.id), covered built verbatim from block.effectiveMessageIds). When a block recorded sub-id projections (base#callId, base#r0, …) and the current view emitted the plain base (or vice versa), coverage failed and already-compressed messages re-entered the visible view resurrected (762K-token leak measured in replay).

Fix

Normalize to the base id on both sides before the membership test:

  • baseIdOf(id) — everything before the first # (a # at position 0 or #-free ids map to themselves, so h_abcde is never confused with covered base h_abc — no prefix matching).
  • isCovered(id, coveredBases) — O(1) lookup against a precomputed base set (built once per pass), instead of per-message linear scans.
  • Summary anchoring uses the earliest index of the base group, so a summary no longer jumps to index 0 when only projection forms are present.

Sibling fix (same root cause, found during triage)

syncBlocks() (src/sync.ts) had the same exact-match presence check and runs before prune in the pipeline: in the primary direction it deactivated the block (summary lost) before prune could cover the message. Fixed with the same base-set normalization. Fixing prune alone would not have stopped the leak.

Note: the isCovered in closed PR #225 early-returned for #-free ids, so it missed the primary direction (block recorded sub-ids, view emits base); this normalizes both sides.

Tests

8 new regression tests (5 in tests/state-prune.test.ts, 3 in tests/sync-config.test.ts), covering: block-sub-id/view-base, block-base/view-sub-id, sibling sub-ids, distinct-id negative case, summary anchor placement, and sync deactivation in both directions. All 588 tests pass; typecheck + build clean.

Remaining same-class exact-match sites (recommend.ts, decompress.ts, compress.ts overlap detection, boundaries.ts) are tracked in a follow-up issue.

@ranxianglei

Copy link
Copy Markdown
Owner Author

🤖 Powered by ework · qwen3.8-27b

[bot] 🏷 Independent review — verified, looks good to merge (pending the 2nd-agent + human merge per repo policy).

I didn't trust the PR description — I re-ran everything and re-derived the logic from the diff.

Re-verification (clean checkout of the PR head, not the description's claims):

  • tsc --noEmit — clean
  • npm test — 588/588 pass
  • npm run build (tsup + tsc --emitDeclarationOnly) — clean
  • PR is a single commit on top of current master (04bd5ed), so no merge drift.

The regression tests are meaningful, not vacuous. I checked out master (pre-fix) with only the two new test files applied and ran them: 6 of 8 fail (prune ×4 + syncBlocks ×2), while the 2 that pass are the negative controls that already worked (h_abcde not covered by base h_abc; base truly gone → deactivate). That's exactly the "6 fail on master" claim, so the tests genuinely pin the bug.

Fix logic — sound, and it closes all three leak directions:

  • baseIdOf(id) = everything before the first #; a # at position 0 or a #-free id maps to itself. This is what keeps it from degrading into prefix matching — h_abcde is never confused with covered base h_abc (negative test confirms).
  • isCovered(id, coveredBases) normalizes both sides to the base and does an O(1) lookup against a set built once per pass. Because both sides normalize, all three combinations are covered: block-sub-id/view-base (your primary), block-base/view-sub-id, and sibling sub-ids of the same base. This is the correct fix for the root cause (exact-string membership in a world where one original message has several id forms), not a band-aid.
  • baseIndexById (earliest index of the base group) is the right call for anchoring — a summary now lands at the base group's start instead of jumping to index 0 when only projection forms are present.
  • sync.ts presentBases is the load-bearing sibling fix: it runs before prune in the pipeline, so without it the block would be deactivated (summary lost) before prune could cover the message. Correctly fixed with the same normalization; the summary-id branch is left exact (right — summary ids carry no #).

I walked the non-obvious cases by hand: non-contiguous base groups (summary anchors at the earliest projection, all projections still removed), and false-positive coverage (impossible, since the base is the full original id and sub-ids are base#suffix, so distinct originals always have distinct bases). No cross-message false coverage.

Deferred sites are genuinely latent, and properly tracked. I confirmed the kernel only emits plain base ids — deriveMessageId is h_<sha256> and the cluster counter appends _<N> (src/wire/message-id.ts), never a #. Sub-id forms come from adapters, so the remaining exact-match sites can't fire today. I also spot-checked the 7 sites listed in #234 against master and the line numbers/checks all match. #234 carries the required source tag (来源: …#231 分析…). Keeping those out of this PR to stay focused on the actual resurrection path (prune + sync) is the right scoping call.

Nit (non-blocking): exporting baseIdOf/isCovered from src/index.ts is the right move — it gives #234 a stable seam to reuse instead of re-deriving the normalization at each site.

Nothing to fix here. Ready for the second-agent sign-off and human merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

prune: isCovered misses base-id coverage when block effective ids use sub-id forms (base#suffix)

1 participant