Repository navigation
fix: reconcile-live-ids keeps positional replays of folded originals - #466
ranxianglei wants to merge 4 commits into
Conversation
📦 Built Package ArtifactBranch: Option A — Install from npm PR tag (recommended)In your adapter project: npm install acp-kernel@pr-466Each push to this PR publishes a new version under the Option B — Download artifact
tar xzf acp-kernel-pr466.tgz
npm install ./packageThis comment is automatically updated on each push. |
[bot] 🏷 PR #466 reviewed — verified independently, mergeable (merge itself is human-only) ① Root-cause direction — confirmed in pre-PR code. The bug path is real: ② Regression audit of touched paths:
③ Gates on head sha ④ Test quality:
中文摘要: 修复了 reconcile-live-ids 把「代理端整段重放的折叠原文」误判为新实例而重新编号(内容回流、ref 抖动、去活块下新副本被静默丢弃)的问题——按当前输入结构区分「重放」与「真新副本」,本地四项检查与 CI 全绿,无 §7.3 契约变更;注意 #463/#465 修同一根因(走 lastPassIds 状态字段路线),三者只能合一个,建议合本 PR,可以合并。 |
|
Reviewed the design + verified empirically from the main consumer side (billion-context). Design. The positional fingerprint is the right discriminator for the shape #459 got wrong. A live occurrence carrying a covered base is kept only when ALL blocks covering that base are replaying (every covered raw present by base id AND the rendered summary absent); anything else is re-minted as a genuinely new instance. That holds for both host classes:
No new persisted state, nothing to thread through merge/sync paths, deterministic per (state, body) — survives state restore trivially and stays prefix-cache friendly. Empirical (billion-context, the repo whose full suite went red on 0.0.96). Injected this branch's build into bili's
One doc nit. The docblock says ambiguous single-message fingerprints "are treated as a new instance and re-minted", but the code has no such special case: for a 1-base block with the base present and the summary absent, Verdict: recommend merging this over #463/#465 — both of which I also verified green under identical conditions (details in their threads). This one has the least surface: no schema growth, no cross-pass id-stability assumption. |
…461) v0.0.96's reconcile-live-ids node re-minted every live message whose content-hash root appeared in coveredMessageIds, conflating two distinct situations: a host resending the very originals a block was folded from (full-history resend after server-side compress) and a genuinely new duplicate occurrence (#1476). Re-minting the resends broke the pre-0.0.96 contract "covered => pruned + replaced by rendered summary at anchor": folded content returned to the wire every turn, the summary lost its carrier, refs churned, and blocks deactivated. Classify each covered-id occurrence by positional fingerprint: a block is REPLAYING when its rendered summary is absent AND all of its covered bases are present; occurrences under exclusively-replaying coverage keep their ids (prune drops them, the summary takes the anchor). Every other shape keeps the 0.0.96 re-mint behavior, now renumbering only the conflicting members (non-conflicting siblings no longer churn) and skipping numbers claimed by folded copies or retained by kept members. Coverage is judged per sync's own activation predicate (isBlockStillPresent, extracted from sync.ts and recomputed against the current input) instead of the stored active flag: a block that sync would reactivate this turn counts as covering even while currently inactive, closing a silent-drop chain in which a deactivated block's coverage vanished before a colliding new instance could get its instance id and then got pruned. Fixes #461
The docblock listed an 'ambiguous single-message fingerprint' shape that is re-minted, but the code has no such special case: a 1-base block with its base present and the summary absent classifies as replaying and keeps the id. Reword to match the code.
Unit test for the one-message-fold echo shape: 1 covered base present, summary absent => positional replay => id kept. Makes the docblock intent explicit and guards against drift.
…ngerprint Replaces #463's cross-pass snapshot (shipped in v0.0.97) with reconcile-live-ids' per-(state,body) positional classification: deterministic, no cross-pass id-stability assumption, and correct on the FIRST pass after any state restore — the snapshot was dropped by mergeCompressionState's field whitelist on every reload, which degraded exactly one pass to 0.0.95 semantics (renumber nothing), re-exposing both #461 and #1476 for that pass. No persisted-state compat concern: the load path never carried the field, so old blobs' stray key was already ignored; adapters never set it directly. Behavior change vs v0.0.97: echo-vs-new-instance discrimination is now structural (rendered-summary presence x covered-base presence) instead of prior-pass membership.
64171ab to
848eee9
Compare
[bot] 🏷 Doc nit fixed + branch rebased onto current master (now supersedes #463) ① Your doc nit — confirmed and fixed. You're right: the old docblock (
② Master moved mid-review — rebase was required. Timeline (UTC): my review reply 12:22 → #463 merged 12:40 (then release v0.0.97, then #469) → your design review 15:47. #465 was closed unmerged. That left #466 Per your verdict ("recommend merging this over #463/#465"), I rebased onto current master (
③ Why replacement is safe — plus a concrete gap found in #463. ④ Behavior change vs v0.0.97 (disclosed): echo/new-instance discrimination moves from prior-pass membership ( ⑤ Gates on rebased head Ready for you to merge when you are. 中文摘要: 按你的 review 修了 docblock 的不实描述(改写 + 单测钉住 1-base 重放分类);因 master 在 review 期间合入了 #463 并发布 v0.0.97,已将本 PR rebase 到最新 master,用无状态指纹替换 lastPassIds 机制(顺带发现并披露了 #463 的缺口: |
Root cause
v0.0.96's
reconcile-live-idsnode (src/instance-reid.ts, first pipeline node) re-minted every live message whose content-hash root appeared incoveredMessageIds. Its discriminator — "any live message carrying a covered id is a new instance, not the original" — conflates two distinct situations:For case 1, pre-0.0.96 behavior was load-bearing: resent covered originals were pruned and replaced by the rendered summary block at the anchor. v0.0.96 re-minted them instead — folded content returned to the wire every turn (payload bloat), the summary lost its carrier in both proxy and plugin mode, refs churned after each fold, blocks deactivated, and ranges computed from state refs no longer matched post-remint live ids (fail-fast 502 loop against the real codex CLI). Measured impact: 56/2919 unit failures + e2e lane red on billion-context with a pin-only bump.
Fix
Classify each covered-id occurrence by positional fingerprint — no new config surface, so all full-history-resending hosts are fixed pin-only:
Two adjacent restorations in the same root cause:
isBlockStillPresent(extracted fromsrc/sync.ts, behavior-identical refactor), recomputed against the current input instead of the storedactiveflag. This closes a data-loss chain found during analysis: a block deactivated last turn (view lacked both raws and summary) reactivates this turn when the summary returns, but under the stored-flag check its coverage vanished before reconcile ran, so a colliding new instance kept its bare id, got ref'd to nothing new, and was silently pruned.Behavior change disclosure (old → new)
_Neach turn; escaped prune; summary lost carrier; block deactivatedroot_1..kWHY: the v0.0.96 discriminator was built for #1476 without distinguishing replay from new instances; restoring the pre-0.0.96 contract for the replay shape is the fix, not a semantic drift — #1476 protection is preserved and now tested for both shapes.
No §7.3 contract touched: node ordering unchanged, wire format unchanged, ref immutability intact (no slot recycling), state schema unchanged, zero runtime deps unchanged.
Verification
Minimal repro (40 alternating messages → fold m00001..m00015 → resend full history):
acp_summary_b1at anchortests/issue461-replay.test.ts: 7 unit tests (replay identity, partial-presence conservatism, sibling no-churn, deactivated-block coverage, expanded never covers, …) + 4 end-to-end pipeline tests covering all shapes above.tests/issue1476-reid.test.tsupdated to the two-shape fixtures (its old "renumbers every live instance" expectation encoded the buggy group-wide discriminator).npm run typecheck,npm test(1195 pass / 0 fail),npm run build,npm run format:check.Update (2026-09-27): rebased onto current master — supersedes #463's mechanism
While this PR was under review, #463 (cross-pass
lastPassIdssnapshot) was merged and shipped in v0.0.97. Per the owner verdict in this thread ("recommend merging this over #463/#465"), the branch is now rebased onto current master and REPLACES that mechanism with the stateless positional fingerprint:state.lastPassIds) to the structural replay fingerprint. Concrete gap closed:mergeCompressionState's field whitelist never carriedlastPassIdsacross a state reload, so v0.0.97 degraded exactly ONE pass after every restore to 0.0.95 semantics (renumber nothing) — re-exposing both remintCoveredLiveIds: full-history resend after fold escapes prune — covered originals re-minted instead of replaced by summary #461 and #1476 for that pass. The stateless classification is correct on the first pass after any restore.CompressionState.lastPassIdsremoved (src/types.ts, snapshot write insrc/compress.ts). No persisted-state compat concern: the load path never carried the field (old blobs' stray key already ignored) and no adapter sets it directly.Gates green on the rebased head (Node 22): typecheck,
npm test(1196 pass / 0 fail), build, format:check.Fixes #461