Repository navigation
fix: segment compressible ranges by array adjacency, not ref arithmetic (#207) - #209
Conversation
📦 Built Package ArtifactBranch: Option A — Install from npm PR tag (recommended)In your adapter project: npm install acp-kernel@pr-209Each push to this PR publishes a new version under the Option B — Download artifact
tar xzf acp-kernel-pr209.tgz
npm install ./packageThis comment is automatically updated on each push. |
Reviewed PR #209 end-to-end. Verdict: favorable — the fix is correct, minimal, and correctly scoped. Details below. Reproduction (verified, not taken on faith)
So 3/4 fail without the patch and all 4 pass with it: the tests catch a real defect, not tautology. Both reported symptoms trace exactly — Layer judgment — root cause correctly located. Recommendation inferred continuity from ref arithmetic; the apply side is already order-agnostic. Confirmed in Solution assessment
Pre-flight (on PR head): typecheck ✓ · tests 584/584 ✓ · build ✓. Non-blocking observations (out of scope for #207, reporting only)
Neither feeds the compression decision or the benefit floor, and neither can emit a descending pair (entries are sorted ascending / strictly contiguous). On surface-replace hosts they may cosmetically under-/over-merge in rendered text. Leaving them is defensible here; if you want display parity on replace hosts that'd be a small follow-up, not a blocker for this PR. Minor nit (no action): the patch moves the Dup screen: no related open/closed issue or competing PR; single PR closing #207 (the documented alternative to reverted #176, not a duplicate). Per AGENTS.md this needs a second independent review before merge, and merge is human-only. Ready for that. |
Fixes #207.
Problem
buildCompressibleRanges(src/recommend.ts) segmented ranges withhasGap = info.refNum > prevRefNum + 1— correct only when refNum order equals message-array order. Surface-replacing hosts (e.g. billion-context-dsh) break that assumption two ways:hasGapfires spuriously → every range fragments;assignRefsgives it a fresh HIGH ref → the previous group flushes and a descending pair (startRef > endRef) is emitted (live example:m00110295..m00106762).Result on the affected host: fragmented ranges, large tool results dropping out of recommendations, and the benefit floor suppressing every nudge while the session keeps growing.
Fix
Segment by array adjacency, never ref arithmetic: a range splits only when a numbered-ref message is physically skipped between two list entries (protected tool, synthetic/covered, protected zone) or at user-turn boundaries. Entries carry a
gapBeforeboolean instead ofrefNum. Unrefed/BLOCKEDmessages consume no slot and never interrupt a range.assignRefsis untouched — append-only, no recycling, no pruning. The Revert "fix: reclaim dead message refs, reuse exhausted capacity (#176)" — ref slot reuse is unsafe #191 invariant ("ref numbers are never reused or duplicated within a session — ever") holds.boundaries.tsresolves anchors by message index), so no other module changes were needed.Behavior audit
On dense append-only hosts (pai-acp / billion-context-pi / opencode-acp) a numbered-ref message is skipped between two entries iff a refNum gap exists — all five skip categories verified one-to-one against the old rule, so recommended ranges are identical and the existing suite stays green with zero test edits.
On replace-style hosts holes are no longer interruptions, and mid-array summary nodes extend the range instead of flushing it.
Tests
New regression tests in tests/recommend.test.ts:
[a,b,c,d,e], surface shrinks to[a,e]→ one rangem00001..m00005, count 2);[a, summary(m00006), d, e]→ one range, count 4);[Compressed conversation section]nodes still split (preserved legacy behavior);Verification:
npm run typecheck✓ ·npm test584/584 pass ✓ ·npm run build✓