Skip to content

fix: host-declared summaryOfBlockId — plain ranges skip host checkpoints (#335) - #336

Closed
ranxianglei wants to merge 1 commit into
masterfrom
2026-09-20_host-checkpoint-barrier
Closed

ranxianglei wants to merge 1 commit into
masterfrom
2026-09-20_host-checkpoint-barrier

Conversation

@ranxianglei

Copy link
Copy Markdown
Owner

Closes #335

Problem (reproduced on v0.0.80)

applyCompression only treats kernel-rendered acp_summary_* ids as view-only summary placeholders (isSummaryMessageId, src/prune.ts:21). A host-managed-surface integrator (billion-context-dsh) projects its own compression checkpoint as an ordinary user/message; a PLAIN range spanning it folded the checkpoint into the new block's effectiveMessageIds, silently dropping the previous distillation from the visible context with no supersedence signal in the result.

Repro pre-fix: the new regression test below asserts the checkpoint is NOT in the new block's coverage and fails on current master with host checkpoint must not be folded into the new block's coverage.

Fix

Host-declarable marker instead of the kernel-private prefix:

  • CoreMessage.summaryOfBlockId?: string (src/types.ts) — the host declares "this message is my surface's rendering of block X, not primary content" (follows the existing thinkingTokens host-field precedent).
  • applySingleRange (src/compress.ts): for PLAIN ranges only (boundaryKind !== "block", i.e. both ends are mNNNNN refs), messages carrying the marker are excluded from rangeMessageIds — the same choke point where acp_summary_* ids are already filtered, applied after pair-boundary adjustment so it also covers widened spans. Block-boundary ranges (T2/T3 tier distillation) deliberately cross checkpoints and keep folding them.

Unchanged by design: nested-block consumption/lineage (directBlockIds, parents), tier output (plain range stays T1), soft protection, Bug-39 protected-tool filtering, unmarked-message behavior (the marker is opt-in).

Tests — tests/checkpoint-span.test.ts (new)

Note: the test file referenced in #335 was never merged to main (no such file or PR exists upstream); this PR provides it.

  1. plain range spanning a marked checkpoint → not folded into eff/direct ids; nested-block lineage intact (directBlockIds [b1], b1 deactivated); prune keeps the checkpoint visible alongside the new rendered summary — the reported defect. Red pre-fix, green post-fix.
  2. plain range over an UNMARKED host message → still folded (scope guard against over-exclusion).
  3. block-boundary distillation (b1..b2) with a marked checkpoint inside the span → still folded, T2 produced with full inherited coverage (must-not-barrier invariant).
  4. control: checkpoint inside the recent-protection window → excluded via soft protection WITHOUT any marker (documents that in-window non-folding was never a barrier).

Evidence: npm run typecheck clean; npm test 865/865 pass; npm run build OK. Diff: +22 lines across src/types.ts and src/compress.ts plus the new test file; no lockfile/generated churn.

中文摘要:修复了宿主自管 surface 的 checkpoint 消息被 plain range 当普通消息折叠的问题(#335)——新增 CoreMessage.summaryOfBlockId 宿主声明标记,plain range(两端均为 mNNNNN)跳过带标记的消息、block-boundary 蒸馏路径行为不变;issue 中提到的回归测试此前并未合入 main,本 PR 已补齐。

@github-actions

Copy link
Copy Markdown

📦 Built Package Artifact

Branch: 2026-09-20_host-checkpoint-barrier (7f40e1f)

Option A — Install from npm PR tag (recommended)

In your adapter project:

npm install acp-kernel@pr-336

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

Option B — Download artifact

  1. Download the artifact from the Actions run
  2. Extract the tarball and install:
tar xzf acp-kernel-pr336.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] 🏷 Reviewing PR #336 now. Initial status: CI is green (test 24/22, pr-validation, build-artifact all pass) and the diff is tight (+306/−1 across src/compress.ts, src/types.ts, tests/checkpoint-span.test.ts — no lockfile/generated churn). Pulling the branch to run local verification (typecheck/test/build + red-pre-fix check on master) and reading the full diff before giving a verdict. Will follow up with findings.

@ranxianglei

Copy link
Copy Markdown
Owner Author

🤖 Powered by ework · qwen3.8-27b

[bot] 🏷 Review verdict: APPROVED — ready for human merge. Second-agent review complete (satisfies AGENTS.md §6 two-reviewer rule alongside the implementing session). No code changes needed; I found nothing to fix directly.

Independent verification (not just trusting CI)

  • CI: green — test (24), test (22), pr-validation, build-artifact all success; mergeable_state: clean; branch is exactly 1 commit ahead of current master (1ac05bb, v0.0.80), no divergence.
  • Local run on 7f40e1f: npm run typecheck clean; npm test 865/865; npm run build OK. Master baseline is 861/861, so 865 = 861 + the 4 new tests — evidence claim checks out.
  • Red pre-fix: detached checkout of true master + the new test file → test 1 fails with exactly host checkpoint must not be folded into the new block's coverage; tests 2–4 pass (they're guards/controls, as intended). Red/green claim verified on the actual PR base.
  • Prettier: all three touched files pass prettier --check. The repo-wide format:check failure (114 files) is pre-existing on master (prettier 3.9.6 drift, no config committed) — not caused by this PR.
  • Duplicate screening: searched all open+closed issues/PRs for checkpoint/summaryOfBlockId/host-surface — no duplicates; applyCompression: isSummaryMessageId 只认 acp_summary_* 前缀,宿主携带的 checkpoint 会被 plain range 当普通消息折叠 #335 is the sole source and is closed by this PR. Design is consistent with the existing host-declared-field precedent (thinkingTokens, and PR feat(report): host-declared surface meta in status reports #280's surface meta).

Diff cleanliness

Tight: src/compress.ts +14/−1, src/types.ts +8/−0, tests/checkpoint-span.test.ts +284. Every line serves the stated purpose; no lockfile/generated churn (I hit the known lockfile-version drift locally during npm install and discarded it — matches the note in #335).

Code-level findings

  1. Placement correct — the hostSummaryIds filter sits at the same choke point as isSummaryMessageId (src/compress.ts:802), applied after applyPairBoundaryAdjustments, so widened tool-pair spans are covered too. Gated on boundaryKind !== "block", so T2/T3 distillation deliberately crosses checkpoints (pinned by test 3).
  2. Degenerate cases safe — a plain range consisting solely of marked checkpoints yields an empty rangeMessageIds and fails loudly via validateCompressionRange ("Range contains no compressible messages…") or the livelock guard when nested blocks are consumed — identical behavior class to ranges spanning only kernel acp_summary_* messages. No silent empty blocks.
  3. Turn-integrity preservedcomputeIntegrityWithdrawals runs downstream over the final effectiveMessageIds (src/compress.ts:944), so even a pathological marked tool message can't split a call/result pair across the fold; such pairs are withdrawn and stay visible.
  4. Lineage/tier/prune semantics intact — test 1 pins directBlockIds: ["b1"], b1 deactivated, tier stays 1, and prune keeps the checkpoint visible alongside acp_summary_b2.
  5. Design note (accepted, non-blocking): marker semantics are presence-only — the summaryOfBlockId value is not validated against state.blocks. That's the right call (opt-in host declaration, auditable, zero state coupling), consistent with how thinkingTokens is trusted.
  6. Nit (non-blocking): the minCompressRange char count (src/compress.ts:294) still counts marked-checkpoint text toward range size. Negligible, and arguably correct since the model still summarizes that span.

One process note: strictly per AGENTS.md §7.4 this adds a small opt-in adapter-facing field rather than being a pure fix: — additive and backward-compatible, but flagging that the capability-surface item is why this goes through normal review rather than auto-merge. Merge is human-only regardless.

中文摘要:独立复核了 PR #336(第二审查人)——在真实 master(v0.0.80)上验证了修复前红/修复后绿、本地 typecheck+865/865 测试+build 全过、diff 干净无无关改动、无重复条目;修复位置正确(与 isSummaryMessageId 同一 choke point、pair 调整之后)、空 range 与 turn-integrity 边界情况均安全,无需任何修改,可以合并。

@ranxianglei

Copy link
Copy Markdown
Owner Author

Superseded by #338, which implements the same host-declared marker with two strict improvements over this PR: (1) liveness check — only carriers of still-active blocks are skipped, so stale carriers (block already consumed / unknown id) fold like ordinary messages and a dead block's leftover checkpoint cannot linger forever (this PR would have pinned it permanently); (2) kernel-side result.warnings reporting of each exclusion, making the skip explicit in the result string rather than only in the host's own display.

Verified before closing: read the full #338 diff; ran its head locally — typecheck clean, 868/868 tests pass, build OK; and cross-ran all four regression tests from this PR (#336) against #338 — all pass (liveness-at-call-start keeps the characterization flip intact even when the same call consumes the carrier's block). Closing to keep one issue = one open PR; #335 should track #338 going forward.

@ranxianglei

Copy link
Copy Markdown
Owner Author

🤖 Powered by ework · qwen3.8-27b

[bot] 🏷 Acknowledged — supersession verified independently, no objection to closing.

I checked both claims against the merged #338 diff (6ee4190, in master via 4d7d238, now shipped in v0.0.82):

  1. Liveness check — present: carriers are skipped only when blockById(state, summaryOfBlockId)?.active (src/compress.ts, applySingleRange). Stale carriers (consumed block / unknown id) fold like ordinary messages. This does fix a real weakness of this PR that I had flagged as "accepted" in my review: presence-only semantics would have pinned a dead block's leftover checkpoint permanently visible. Liveness-at-call-start also keeps test 1's characterization flip intact when the same call consumes the carrier's block (b1 active at scan time).
  2. Kernel-side reporting — present: each exclusion pushes an Excluded N checkpoint message(s) … from the compression range entry into result.warnings, with a pointer to use bN..bM refs to fold deliberately.

Local runs at #338 head: npm run typecheck clean, 869/869 tests pass (one more than your 868 — the head commit adds the tool-pair-growth case), npm run build OK. Cross-ran all four regression tests from this PR (#336) against #338's code: 4/4 pass.

Closing is correct — one issue = one open PR; #335 tracks #338 going forward. Nothing further needed on #336.

中文摘要:已独立核实 #338 的两项改进(liveness 判定 + kernel 侧 warnings 上报)确实存在于已合并 diff 中,本地 typecheck/869 测试/build 全绿、本 PR 的 4 条回归测试在 #338 代码上全部通过;同意关闭 #336,后续以 #335#338 为准。

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.

applyCompression: isSummaryMessageId 只认 acp_summary_* 前缀,宿主携带的 checkpoint 会被 plain range 当普通消息折叠

1 participant