Skip to content

fix(pipeline): discriminate post-fold resends from new instances in reconcile-live-ids (#462) - #463

Merged
ranxianglei merged 3 commits into
masterfrom
2026-09-27_reid-echo-discriminator
Sep 27, 2026
Merged

ranxianglei merged 3 commits into
masterfrom
2026-09-27_reid-echo-discriminator

Conversation

@ranxianglei

Copy link
Copy Markdown
Owner

Fixes #462

What changed

reconcileLiveIdsNode (#459) renumbered EVERY live message whose id root was covered, on the assumption that "live id ∈ covered ⇒ new instance". That assumption is wrong for the folded originals themselves: stateless hosts resend the full raw history every turn, so the original re-derives exactly the covered bare id as an ECHO. Renumbering the echo minted a fresh ref and — because prune matches via baseIdOf (strips only #tail, not _n) — let the folded content escape prune and rejoin the wire beside its summary, every turn. On billion-context 0.0.96 this re-inflates the wire and kills downstream range anchoring (tests/e2e/e2e-codex-fake.test.ts overflow test → preflight 502 → turn lost).

The fix: cross-pass continuity

New optional CompressionState.lastPassIds — exact ids of the previous pass inbound, snapshotted at processTurn entry and written into the returned state:

  • exact id ∈ covered AND ∈ lastPassIds → echo → keeps its id; prune drops it (already summarized) and the first-user pin keeps a stable ref (also stops per-turn ref churn of the pinned message)
  • exact id ∈ covered AND ∉ lastPassIds → genuinely new identical instance (#1476) → renumbered to the first _k claimed by neither folded copies nor other live instances of the root
  • everything else keeps the converter numbering (renumbering those would only churn the prefix cache)
  • missing snapshot (pre-feature persisted state) → renumber nothing for one pass (0.0.95 semantics), self-heals next pass

Validation

  • kernel: 1186/1186 tests, tsc --noEmit clean
  • new tests: echo stays pruned (no _1 minted, original ref untouched), first-user pinned echo keeps id+ref across post-fold passes, missing-snapshot fallback, renumber skips live-claimed numbers; #1476 e2e now models the real two-pass flow (pass A without the instance → pass B with it)
  • end-to-end: packed tarball installed into billion-context → the failing codex overflow e2e (1 !== 0 at load turn 4) now passes; full e2e-codex-fake.test.ts 2/2
  • bisect correction: earlier attribution to fix(ccr): strict whole-body parser for stored-placeholder detection #457 was an artifact of a stale tarball; clean rebuilds show 6c9dcad (fix: re-mint live ids that collide with a folded copy (billion-context#1476) #459 merge) already fails

Known limitation (follow-up, not blocking)

billion-context preflight resolves advertised range endpoints over the RAW pre-pipeline array; a renumbered new-instance _1 id still will not anchor there in overflow situations. The echo path (the P0) resolves correctly now; the new-instance shape is the rare #1476 case and is documented for a bili-side resolution pass later.

Chain: merge this → release 0.0.97 → billion-context #1488 re-pins.

…econcile-live-ids (#462)

#459 renumbered every live id whose root was covered, but stateless hosts
resend the full raw history every turn, so the folded originals' own echoes
carry exactly the covered bare id — renumbering them minted fresh refs and
made them escape prune's covered check (baseIdOf strips only "#tail"), so
folded content rejoined the wire beside its summary every turn and
downstream range anchoring failed (billion-context e2e 502 on 0.0.96).

The discriminator is now cross-pass continuity via state.lastPassIds (the
previous pass's inbound ids, snapshotted in processTurn):
- exact id covered AND present last pass = the folded original's echo →
  keeps its id; prune drops it and the first-user pin keeps a stable ref
- exact id covered AND absent last pass = genuinely new identical
  instance (#1476) → renumbered past folded-claimed and live-claimed
  numbers
- everything else keeps the converter's numbering
- missing snapshot (pre-feature state) = renumber nothing for one pass
@github-actions

github-actions Bot commented Sep 27, 2026 •

Copy link
Copy Markdown
Contributor

📦 Built Package Artifact

Branch: 2026-09-27_reid-echo-discriminator (a220dd6)

Option A — Install from npm PR tag (recommended)

In your adapter project:

npm install acp-kernel@pr-463

Each push to this PR publishes a new version under the pr-463 npm tag.

Option B — Download artifact

  1. Download the artifact from the Actions run
  2. Extract the tarball and install:
tar xzf acp-kernel-pr463.tgz
npm install ./package

This comment is automatically updated on each push.

@ranxianglei

Copy link
Copy Markdown
Owner Author

🤖 Powered by ework · qwen3.8-27b

[bot] 🏷 收到,开始按规范审这个 PR(原 issue 方向 → 回归 → 代码 → 测试有效性 → diff 整洁度)。已拉取分支 pr-463(f8a8885),正在核对 #462 的根因分析、reconcile-live-ids 既有行为清单和 CI 状态,稍后给完整结论。

ranxianglei and others added 2 commits September 27, 2026 19:25
Adds the two regression tests missing from the PR diff: renumber must skip a _k claimed by the previous pass inbound (stale-ref aliasing guard in the while loop), and processTurn must write this pass's inbound ids into state.lastPassIds (the compress.ts hunk was previously only pinned indirectly via the e2e flow).
@ranxianglei

Copy link
Copy Markdown
Owner Author

🤖 Powered by ework · qwen3.8-27b

[bot] 🏷 Review complete — mergeable (human merge, per §7.3/§7.4). Full review below; I also pushed two small fixes directly to the branch (details in ④/⑥).

① Original issue (#462) — direction verified

Root cause reproduced against code, not just the report: prune matches coverage via baseIdOf, which strips only #tail — never _n. Stateless hosts resend the full raw history each turn, so a folded original re-derives exactly the covered bare id as an echo; #459's "live id ∈ covered ⇒ new instance" assumption then renumbered that echo to _1, letting the folded content escape prune and rejoin the wire beside its summary every turn — which is what breaks billion-context preflight range anchoring → 502 → the codex overflow e2e (1 !== 0 at load turn 4). The fix attacks the actual discriminator flaw, not the symptom. Design = cross-pass continuity via CompressionState.lastPassIds, matching the #462 OP exactly.

② Regression audit — existing behaviors touched

Behavior (pre-this-PR, i.e. #459) Post Verdict
Folded-original echoes renumbered _n every turn (ref churn + wire escape) Echo keeps its id; prune drops it; first-user pin keeps a stable ref Intended, disclosed in PR body
Live messages under a covered root but non-exact-covered: renumbered Keep converter numbering (avoids prefix-cache churn) Intended, disclosed
(n/a — new state field) Missing lastPassIds (pre-feature persisted state): renumber nothing for one pass (0.0.95 semantics), self-heals next pass Intended fallback, disclosed. Note: a fork-rebuilt state also lacks the snapshot, so a colliding instance waits one extra pass before renumbering — bounded, documented edge
Genuinely new identical instance (#1476) renumbered to first free _k Same, plus skips numbers claimed by previous-pass inbound (anti stale-ref aliasing) Strengthening, no regression

Untouched §7.3 contracts: wire-artifact format, node order (reconcile-live-ids remains first), prompts, protected-tool filtering, tool-pair atomicity. One structural note: lastPassIds is an additive optional CompressionState field — backward-compatible (absent ⇒ fallback path) but the state schema is human-gated, which independently requires a human merge.

Known limitation (already in PR body, agreed): if a host deletes messages from its raw history between passes, a new instance can slide into a covered slot present in prior and be misclassified as an echo. Rare #1476-variant shape; bili-side resolution pass later. Fine to ship.

③ Code review

Logic verified line-by-line: clusterRoot regex, conflict gate, mint loop (skip if !exactCovered || prior.has(fullId); else smallest k≥1 absent from coveredBases ∪ liveIds ∪ prior, #tail preserved), changed ? next : messages identity preservation (zero churn when nothing qualifies). Snapshot captured before runPipeline and written into the returned state — correct ordering.

One non-blocking hardening suggestion for a follow-up (semantic change ⇒ your call): when choosing _k, also exclude keys of state.messageRefs.byRaw. Strictly stronger than prior — it covers ref holders that are no longer present in the raw history (exactly the host-deletion case above). Not needed for the P0 echo path.

④ Test validity

Existing tests have teeth (assert exact ids/refs across two passes; the bug regression breaks them). Two gaps found and fixed directly on the branch:

  1. The prior.has(\${root}_${k}`)skip branch was untested → addedrenumber skips numbers claimed by the previous pass inbound (reconcile-live-ids (#459) re-numbers post-fold RESENDS as new instances: folded messages escape prune and re-inflate the wire (P0, breaks billion-context CI e2e on 0.0.96) #462)(folded[HASH], lastPassIds=[HASH_1], live [HASH]→ assertsHASH_2`).
  2. The compress.ts snapshot write was only pinned indirectly through the e2e flow → added processTurn writes this pass's inbound ids into state.lastPassIds (#462).

Result: 1188/1188 local (Node 22), typecheck + build clean.

⑤ Diff cleanliness

Clean: 4 files, all on-topic, no lockfile churn (my local npm install had regenerated package-lock.json; restored, not committed), no unrelated or whitespace-only noise beyond the prettier fix.

⑥ CI — was red on the original head, now green

Head f8a8885 failed CI on both Node 22 and 24 jobs at the format:check step (prettier line-wrapping on src/instance-reid.ts + the test file) — the test stage never actually ran, so the body's "1186/1186" (true locally) hadn't been CI-proven. Fixed via npm run format (per §7.2); you concurrently pushed the byte-identical style commit 965eb0d, so I dropped my duplicate and rebased my test commit onto it. Final head a220dd6: test (22) ✅ test (24) ✅ build-artifact ✅ pr-validation ✅.


Verdict: root cause correctly identified and fixed; all behavior deltas intended and disclosed; residual gaps documented. Ready for you to merge, then release 0.0.97 → billion-context #1488 re-pin.

中文摘要: 修复了 #459 把折叠后重发的原始消息误判为新实例、导致折叠内容绕过 prune 每轮重回 wire 的 P0 问题(用跨轮 lastPassIds 判别 echo 与真正的新实例);我直接在分支上补了两个缺失的回归测试并修好了 CI 的格式失败,可以合并。

@ranxianglei

Copy link
Copy Markdown
Owner Author

Reviewed the design. The core idea — cross-pass continuity via lastPassIds as the echo/new-instance discriminator — is exactly the missing signal, and the "no snapshot ⇒ one pass of 0.0.95 semantics" self-heal fallback is the right safety posture.

However, this PR is incomplete as-is: lastPassIds is snapshotted in processTurn but never threaded through the state merge/sync paths (src/sync.ts, src/persist/state-merge.ts are untouched). Any host path that rebuilds compression state through those functions drops the field, and the node then permanently degrades to the 0.0.95 fallback — safe (no leak) but silently loses the #1476 protection without any log line saying so.

#465 is a strict superset of this PR: same core plus exactly that threading (sync.ts + lastPassIds: next.lastPassIds ?? prev.lastPassIds). I verified both #465 and #466 empirically from the billion-context side (full suite 2884 tests: 55 failures on 0.0.96 → 0 with either branch; details in their threads).

Recommendation: close in favor of #465 or #466 rather than layering the threading changes onto this branch. My overall recommendation is #466 (stateless positional replay — no schema growth, no cross-pass id-stability assumption), but that's a maintainer call between the two.

ranxianglei pushed a commit that referenced this pull request Sep 27, 2026
…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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

1 participant