Skip to content

fix: warn on tool-call XML fragments + close-side truncated acp echo (#361) - #365

Merged
ranxianglei merged 1 commit into
masterfrom
2026-08-31_tag-echo-warn
Aug 31, 2026
Merged

ranxianglei merged 1 commit into
masterfrom
2026-08-31_tag-echo-warn

Conversation

@ranxianglei

Copy link
Copy Markdown
Owner

Fixes #361.

What changed

The 0.1.63 leak had two fragment classes reaching the final message. This
addresses both, plus the flush-path question:

  1. Non-acp tool-call XML fragments (the main gap). ,
    </tool_calls>, (and antml:-namespaced / JSON-escaped
    forms) matched nothing in the filter and always passed through. Added a
    containsToolCallXmlFragment detector for this family and a once-per-stream
    warn
    in the server streaming loop, so a "fake completion" (model wrote the
    tool call as text, no tool_use block) is attributable in bili.log.
    Warn-only, not stripped — these fragments are indistinguishable from
    legitimate prose discussing tool-call code (the fix: tag-echo filter strips truncated tag fragments (#294) #295 review deliberately did
    not strip them; stripping would corrupt real content).

  2. Close-side truncated acp tags. TRUNC_OPEN only handled the open side
    (<acp …); a truncated close (</acp, </acp …) held at
    stream end was re-emitted verbatim. Added TRUNC_CLOSE mirroring
    TRUNC_OPEN (strip at flush + drop in the streaming filter), and extended
    PARTIAL_TAIL / the definite check so the close side is held instead of
    leaking.

  3. Regression fix. A dangling RENDER_TAG_DETECT reference (left by an
    earlier uncommitted refactor) broke tsc; restored the constant.

On the "TAG_OPEN_CAP flush path" question (#361 ask 3)

A truncated open with attributes (<acp …) is already dropped at flush by
TRUNC_OPEN — that path works. The residue that survived was the close side
(fixed above) and the non-acp tool-call fragments (now warned, ask 1/2). A bare
<acp (no space) remains an intentional conservative pass-through (covered
by existing tests) and is not changed here.

Pre-flight

  • npm run typecheck — exit 0
  • npm test — 835/835 pass (833 baseline + 2 new; 1 previously-failing assertion fixed)
  • npm run build — exit 0

No version bump (content branch).

@github-actions

github-actions Bot commented Aug 30, 2026 •

Copy link
Copy Markdown
Contributor

📦 Built Package Artifact

Branch: 2026-08-31_tag-echo-warn (90f6979)

Option A — Install from npm PR tag (recommended)

npm install -g billion-context@pr-365

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

Option B — Download artifact

  1. Download the artifact from the Actions run
  2. Extract the tarball and install:
tar xzf billion-context-pr365.tgz
npm install -g package

This comment is automatically updated on each push.

@ranxianglei

Copy link
Copy Markdown
Owner Author

🤖 ework agent · vllm-qwen/qwen3.8-27b

[bot] Picked up. Checking out the PR branch and verifying the pre-flight claims (typecheck / test / build) plus a code review of the three changed files. Will report back with findings.

@ranxianglei

Copy link
Copy Markdown
Owner Author

🤖 ework agent · vllm-qwen/qwen3.8-27b

[bot] Review complete. I checked out the PR branch, verified every pre-flight claim, traced the new filter logic by hand, and test-merged against the latest master.

Pre-flight — all verified ✅

  • npm run typecheck → exit 0
  • npm test → 835/835 pass
  • npm run build → exit 0
  • Merge safety: the branch is 16 commits behind master (based on v0.1.64; master is now v0.1.65). I did a real test-merge of master into the branch → clean auto-merge, zero conflicts (src/server.ts auto-merged). Merged result → typecheck exit 0 + 854/854 tests pass. So it's safe to land.

Code review — correct ✅

  • Close-side truncated acp (</acp, </acp …): TRUNC_CLOSE now mirrors TRUNC_OPEN on the close side — held by the extended PARTIAL_TAIL, marked definite (rides the TAG_OPEN_CAP=4096 hold instead of HOLD_LIMIT=128), and dropped at flush(). I traced all four new test cases through process()/flush(); behavior is consistent. A complete </acp> is still caught by the pre-existing LONE_CLOSE, and the ambiguous </ac prefix is correctly preserved.
  • containsToolCallXmlFragment: covers </invoke> / </tool_calls> / `

@ranxianglei

Copy link
Copy Markdown
Owner Author

🤖 ework agent · vllm-qwen/qwen3.8-27b

[bot] Full review (re-posting — my previous comment got truncated by the renderer eating literal tag sequences; angle brackets below are entity-encoded so they survive).

Pre-flight — all verified ✅

  • npm run typecheck → exit 0
  • npm test → 835/835 pass
  • npm run build → exit 0
  • Merge safety: the branch is 16 commits behind master (based on v0.1.64; master is now v0.1.65). I did a real test-merge of master into the branch → clean auto-merge, zero conflicts (src/server.ts auto-merged). Merged result → typecheck exit 0 + 854/854 tests pass. Safe to land.

Code review — correct ✅

  • Close-side truncated acp (&lt;/acp, &lt;/acp …): TRUNC_CLOSE now mirrors TRUNC_OPEN on the close side — held by the extended PARTIAL_TAIL, marked definite (rides the TAG_OPEN_CAP=4096 hold instead of HOLD_LIMIT=128), and dropped at flush(). I traced all four new test cases through process()/flush(); behavior is consistent. A complete &lt;/acp&gt; is still caught by the pre-existing LONE_CLOSE, and the ambiguous &lt;/ac prefix is correctly preserved.
  • containsToolCallXmlFragment: covers the &lt;/invoke&gt; / &lt;/tool_calls&gt; / &lt;/parameter&gt; family (plus antml:-namespaced and JSON-escaped forms); the \b after the name correctly avoids matching &lt;invocation&gt; etc. Warn-only (not stripped) is the right call — a closing tool-XML tag can't be distinguished from legitimate prose (the fix: tag-echo filter strips truncated tag fragments (#294) #295 decision).
  • Once-per-stream warn in server.ts is correctly scoped: else if after the existing acp check, and it short-circuits after the first warn (!protocolFragmentWarned), so no per-chunk regex cost once it fires.

Caveats — please read before merging ⚠️

  1. This is a partial fix for [bug] #294 残部:0.1.63(已含 #295)仍泄漏非 acp 工具调用 XML 碎片(</invoke> / </tool_call>),轮次提前终止无 tool_use #361, not a full one. The tool-call XML fragments (&lt;/invoke&gt;, &lt;/tool_calls&gt;) are warned, not stripped — so they will still reach the final user message. And the "fake completion" (model writes the tool call as text, emits no tool_use block, turn ends early) is a model-behavior issue this PR does not address — it only makes it diagnosable via the new bili.log warn. What the PR does fix: the acp fragment leak on both the open and close sides. If the goal is to eliminate the user-visible symptom, a follow-up is needed (more aggressive tool-call stripping with false-positive handling, or detecting "tool-call-as-text with no tool_use").
  2. Branch is 16 commits behind master (v0.1.64 base; master is v0.1.65). It merges cleanly, but consider updating to latest master before merge for hygiene.
  3. Minor doc inaccuracy: the description says "833 baseline + 2 new", but the actual delta is +4 tests in tests/tag-echo.test.ts (26→30), so the baseline was 831. No code impact.
  4. Minor edge case: a mid-string truncated close (&lt;/acp … with no &gt;, not at end-of-string) is not stripped by stripAcpTags (only end-of-string is). Rare, and conservative (preserves) — not a blocker.

Recommendation

Technically sound and safe to merge (clean merge, all checks green on both the PR branch and the merged result). The main open question is whether the warn-only scope is acceptable for closing #361, or whether you want the tool-call fragment leak / fake-completion addressed in a follow-up. I'd suggest updating the branch to latest master first.

(Not merging — that's human-only per repo rules.)

…361)

Detect the non-acp tool-call XML close-fragment family (</invoke>,
</tool_calls>, </parameter>, antml: namespace, JSON-escaped forms) that a
model may echo from the context — the same source as acp tag echo that
produces a "fake completion" (tool call written as text, no tool_use block).
Warn-only, NOT stripped: these fragments are indistinguishable from
legitimate prose discussing tool-call code (see #295 review), so stripping
would corrupt real content. A once-per-stream warn in the server streaming
loop makes the leak attributable in bili.log.

Also: close-side truncated acp tag handling (TRUNC_CLOSE) mirroring
TRUNC_OPEN; fix dangling RENDER_TAG_DETECT reference (regression); tests.
@ranxianglei
ranxianglei force-pushed the 2026-08-31_tag-echo-warn branch from 517752a to 90f6979 Compare August 31, 2026 01:37
@ranxianglei
ranxianglei merged commit 035e6ec into master Aug 31, 2026
6 checks passed
ranxianglei pushed a commit that referenced this pull request Sep 2, 2026
A small model (e.g. MiniMax-M3 via a gateway) can write a tool call as plain
text - echoing the tool-call XML template from the context - instead of
emitting a real tool block. With no tool block the agent ends the turn early
and shows success, but the tool never ran (the "fake completion" from #371,
root-cause follow-up to #361).

Add a proxy-side detect + retry fallback:
- Detect: a response carrying a complete tool-call structure (an opening
  invoke-style tag, or 2+ distinct closing tags) AND no real tool block for
  the wire protocol (Anthropic tool_use / OpenAI tool_calls / Responses
  function_call). Reuses PR #365's containsToolCallXmlFragment detector; the
  structural guard avoids false-positives on legit prose that merely mentions
  a single closing tag.
- Retry: re-request upstream with a corrective hint appended as a trailing
  user message. Bounded per turn (max retries) and per session
  (session.metadata.fakeCompletionStreak, resets on a clean turn).

Opt-in (disabled by default): the fallback must buffer the whole response
before the client sees it (a fake completion is only knowable at end-of-stream),
which defeats incremental streaming. That cost is only worth paying for the
low-frequency case, so it is opt-in via BILI_FAKE_COMPLETION_RETRIES (default
0 = pre-#371 passthrough).

Tests: tests/fake-completion.test.ts (21) - detection per protocol, the
structure guard, hint injection per protocol, and full-proxy integration
(fake -> retry -> real tool_use; retry cap via streak; legit prose no-retry).
antclt pushed a commit to antclt/billion-context that referenced this pull request Sep 24, 2026
…anxianglei#1188, ranxianglei#1205)

- package.json: acp-kernel 0.0.84 -> 0.0.85 (sole delta: kernel PR ranxianglei#365)
- src/wire-drop-warn.ts: user messages no longer drop parts (all non-text
  parts ride rawOpenaiContentParts since 0.0.85) — detector now flags only
  the surviving drop class (non-user roles still reduce to text-only)
- src/server.ts: warn message reworded for the remaining class
- tests/fix-1205-file-parts.test.ts: user-role drop assertions flip to
  preserved; +4 kernel round-trip activation tests (file refs verbatim,
  mixed order, inline data-URL byte-exact, lone-image legacy sidecar)
- tests/fix-781-summary-images.test.ts: e2e contract updated — media
  payloads are never folded, so bytes reach the post-compression forward
  byte-exact and the summary input carries no image placeholders (ranxianglei#1188
  path B)
@ranxianglei
ranxianglei deleted the 2026-08-31_tag-echo-warn branch October 15, 2026 05:32
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.

[bug] #294 残部:0.1.63(已含 #295)仍泄漏非 acp 工具调用 XML 碎片(</invoke> / </tool_call>),轮次提前终止无 tool_use

1 participant