Skip to content

fix(ccr): strict whole-body parser for stored-placeholder detection - #457

Merged
ranxianglei merged 1 commit into
masterfrom
2026-09-27_strict-stored-placeholder-parser
Sep 27, 2026
Merged

ranxianglei merged 1 commit into
masterfrom
2026-09-27_strict-stored-placeholder-parser

Conversation

@ranxianglei

Copy link
Copy Markdown
Owner

What

Replaces the CCR placeholder detector's substring check with a strict whole-body parser, fixing the misclassification of ordinary text that merely mentions [acp-stored — cross-repo fix for ranxianglei/billion-context#1456 (kernel-side root cause; plan step 1 of that issue).

Old: isStoredPlaceholderText = stripLeadingTag(text).includes("[acp-stored")
New: parseStoredPlaceholder(text) — matches ONLY the exact two-line shape emitted by buildStoredPlaceholder (📦 line with 5-digit zero-padded ref + groupThousands token count + optional backtick title; retrieve-hint line citing the SAME ref), modulo one trusted leading render tag and one optional trailing newline. isStoredPlaceholderText is redefined on top of it (public API kept, semantics fixed).

Call sites:

Path Old gate New gate
storeLargeResults substring skip skip iff parsed AND (ref unknown OR parsed.ref === ownRef)
storeCoveredOriginals substring skip skip iff parsed?.ref === ownRef
isAbsorbCandidate substring (via shared fn) strict shape (via redefined fn; no ref context at this site by design)

Behavior change disclosure (intended)

Ordinary text merely CONTAINING [acp-stored (source defining the constant, docs quoting a placeholder example, logs) is NO LONGER treated as an internal CCR placeholder:

  • old: skipped from arrival storage / fold-time archiving / absorb → after fold, acp_retrieve(ref) missed with no stored original (dangling advertised ref);
  • new: flows through normal CCR (stored + replaced when over minToolTokens) and absorb candidacy.

Canonical self-citing placeholders: UNCHANGED — still skipped everywhere; the #432/#1340 anti-poisoning guarantee holds (retrieve-by-ref stays an honest miss when the store entry was lost). Placeholder wire format: UNCHANGED.

New public API: parseStoredPlaceholder() (+ ParsedStoredPlaceholder type), exported for host use — billion-context will tighten fork-adoption.citedPlaceholderRefs() (the host-side fourth instance of the same root-cause family) with it after the pin bump.

Test-fixture change (deliberate)

The #1340 regression test fixture cited a FOREIGN ref (m00099 inside message m00001). CCR replacement always cites the hosting message's OWN ref, so the faithful poisoning fixture is self-citing — corrected to m00001. Under the old over-wide detector both fixtures behaved identically; under the strict detector the foreign-ref case is now correctly archived as a genuine original. A new dedicated test pins both directions, including "the cited foreign ref never gains a fake hit".

Evidence

  • Live repro against 0.0.95 before the fix: doc embedding the canonical example → isStoredPlaceholderText=true, storedCount=0, fold-time store empty, retrieveByRef not-found; canonical control still detected.
  • After the fix, green on this head: npm run typecheck, npm test (1168/1168, incl. 9 new regression tests covering all three affected paths + round-trip suite + 12 negative shapes), npm run build, npm run format:check.

中文摘要:修复了「正文含 [acp-stored 的普通源码/文档被误判为 CCR placeholder」——用严格整段 parser(parseStoredPlaceholder)替换子串判断,arrival/fold-time/absorb 三处调用点同步收紧;canonical self-citing placeholder 的处理与线格式均不变,#432/#1340 防投毒保证保留。对应 ranxianglei/billion-context#1456 计划第 1 步(kernel 侧根因修复)。

@github-actions

github-actions Bot commented Sep 27, 2026 •

Copy link
Copy Markdown
Contributor

📦 Built Package Artifact

Branch: 2026-09-27_strict-stored-placeholder-parser (4814aaa)

Option A — Install from npm PR tag (recommended)

In your adapter project:

npm install acp-kernel@pr-457

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

Option B — Download artifact

  1. Download the artifact from the Actions run
  2. Extract the tarball and install:
tar xzf acp-kernel-pr457.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] 🏷 Review complete — approved for merge. Verified on head ab036f3 (single commit, base = current master c43e2da, no rebase needed): local npm run typecheck ✓, npm test 1168/1168 ✓, npm run build ✓, npm run format:check ✓; GitHub CI runs on this head both success. Per repo rules I won't merge — please merge yourself: #457

① Original issue (direction) — PASS

Read ranxianglei/billion-context#1456 in full. Triage there reproduced the bug on the pinned 0.0.95 (all three paths) and correctly located the root cause kernel-side: the detector conflates "text mentions the marker" with "message is a canonical placeholder". This PR fixes the decision point itself (strict whole-body parser), not a symptom filter downstream — right layer. The issue's proposed API (parseStoredPlaceholder + ParsedStoredPlaceholder, 8 parser requirements) is implemented exactly as specified, including requirement 7 (callers with known ref compare parsed.ref === ownRef) and requirement 8 (embedded-in-larger-text → null). All 11 acceptance criteria map to passing tests.

② Regression review — existing behavior inventory on touched paths

Verified against pre-change code, not just the diff:

Path Existing behavior Verdict
storeLargeResults (src/ccr.ts:335) non-tool-result / CCR-disabled / empty-text / ACP-tool / excludeTools / protected / below-minToolTokens / unmapped-or-BLOCKED-ref / hasStoredRef first-write-wins gates all untouched, same order-of-effect (early ref computation is pure, no behavior delta)
genuine self-citing placeholder skips storage old substring skip → new parse+ref-equality skip preserved — every kernel-emitted placeholder provably round-trips (see ③), so parse always succeeds and parsed.ref === ownRef (or !ref conservative branch, matching old behavior for unmapped messages)
storeCoveredOriginals (src/ccr.ts:458) covered-only, reasoning-skip, tag-strip, unmapped-ref guard untouched; the !ref guard at line 478 runs before the new gate, so ref is always defined where compared
#432/#1340 anti-poisoning (honest miss when store entry lost) self-citing placeholder never persisted as its own original preserved — fixture corrected to the faithful self-citing form (m00099→m00001; CCR replacement always cites the hosting message's own ref, confirmed at src/ccr.ts:399-406)
isAbsorbCandidate (src/absorb.ts:92) placeholder-marked results excluded from absorb preserved for canonical placeholders; site has no ref context by design — foreign-ref placeholder-shaped text stays excluded here too, which is the conservative (safe) direction
Placeholder wire format byte-stable two-line shape UNCHANGED — buildStoredPlaceholder not touched
Public API isStoredPlaceholderText kept kept, semantics narrowed (disclosed in PR body); parseStoredPlaceholder/ParsedStoredPlaceholder are additive barrel exports

Intended changes (disclosed in PR, confirmed correct): marker-mention-only text and foreign-ref placeholder-shaped text now flow through normal CCR/absorb/archiving instead of being silently skipped (which previously produced dangling advertised refs after fold).

No §7.3 kernel-owned contract is touched (no wire format, node ordering, ref semantics, or state schema change). Note: the new public export makes this strictly outside the §7.4 auto-merge gate anyway — human merge is the right path regardless.

Round-trip safety proof (the critical regression risk): a strict parser is only as good as its match to the generator. Checked each generator input: head goes through normalizeHead (src/ccr.ts:93-97, collapses ALL whitespace incl. newlines) and command through extractCommand (same \s+→space normalization), so kernel-emitted placeholders can never be multi-line; kind comes from fixed KIND_LABELS; refs are always m + exactly 5 digits (REF_WIDTH = 5 const, src/refs.ts:3); groupThousands clamps negatives to 0 and emits exactly the (0|[1-9]\d{0,2}(?:,\d{3})*) shape the regex accepts. Every placeholder the kernel can produce parses back to non-null — the #1340 guarantee cannot regress through this change.

③ Code review — clean

Parser (src/ccr.ts:191-229): anchored whole-body grammar, cross-line ref equality enforced, tolerance limited to one leading render tag (reuses existing LEADING_TAG_RE) + one trailing newline. Regex backtracking correctly resolves ambiguity when kind contains · or title contains backticks (greedy-to-end anchoring). Redundant-but-harmless defensive null checks on guaranteed captures — not worth churning. No as any, no @ts-ignore, comments carry rationale (billion-context#1456 citations). Doc comment honestly documents the residual byte-equal ambiguity and defers structured provenance — agrees with the issue's non-goals.

④ Test validity — real teeth

  • Round-trip suite: 5 shapes (backticks-in-title, kind-with-middle-dots, custom retrieve tool name, multi-group token count, tiny counts) with field-level equality incl. title.
  • 12 negative shapes (embedded example, source defining the constant, prefix/suffix/third line, wrong arrow glyph, wrong indent, wrong trailing phrase, six-digit ref, ungrouped count, missing box glyph, CRLF) each asserting BOTH parse === null AND isStoredPlaceholderText === false.
  • Path tests assert storedCount, replacement happened, retrieveByRef returns the exact original bytes, and "the cited foreign ref never gains a fake hit". Under the old detector these fail (doc fixture → storedCount=0), so they genuinely pin the regression.
  • The #1340 fixture correction is faithful, not cosmetic: the old foreign-ref fixture could never occur from the kernel's own replacement path.

⑤ Diff cleanliness — clean

3 files, +452/−14, every line serves the fix: src/ccr.ts (parser + 2 call-site gates), src/index.ts (+2 exports), tests/ccr.test.ts (regression suite + fixture correction). No lockfile/version churn, no unrelated files, single well-scoped commit.

Remaining cross-repo step (tracked in billion-context#1456, not this PR): after this merges and CI publishes, bump the pin there and tighten fork-adoption.citedPlaceholderRefs() with the new API.

中文摘要:修复了「正文含 [acp-stored 的普通源码/文档被误判为 CCR placeholder」——严格整段 parser 替换子串判断、三处调用点收紧;我核对了 parser 与 buildStoredPlaceholder 生成器的逐字段匹配(round-trip 不会回归)、#432/#1340 防投毒保证保留、线格式与既有门控零漂移,本地四项检查与 CI 全绿、测试有牙且钉住三路径回归,diff 干净,可以合并。

isStoredPlaceholderText used stripLeadingTag(text).includes("[acp-stored"),
which misclassified ANY text merely mentioning the marker — source defining
the constant, docs quoting a placeholder example — as an internal CCR
placeholder. Such content escaped arrival storage (storeLargeResults),
fold-time archiving (storeCoveredOriginals → dangling retrieve-by-ref)
and absorb candidacy (isAbsorbCandidate).

parseStoredPlaceholder() parses ONLY the exact two-line shape emitted by
buildStoredPlaceholder (5-digit zero-padded ref, groupThousands token
count, cross-line ref equality, one trusted leading render tag, optional
trailing newline). Callers that know the message's own ref compare
parsed.ref === ownRef:

- storeLargeResults: skip only self-citing (or unknown-ref) placeholders;
  marker-mentioning content stores normally.
- storeCoveredOriginals: skip only self-citing placeholders; foreign-ref
  placeholder-shaped bytes store as genuine originals (entries are written
  under the hosting message's own ref only, so no fake hit is possible).
- isAbsorbCandidate inherits the fixed detector via isStoredPlaceholderText.

Placeholder wire format unchanged; canonical self-citing placeholders keep
all prior guarantees (#432/#1340 honest miss preserved — the #1340
regression fixture is corrected to be self-citing, which is what CCR
replacement actually emits).

Cross-repo fix for ranxianglei/billion-context#1456 (kernel-side root cause).
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.

1 participant