Skip to content

fix: unify tool-message accounting across status report surfaces (issue #390) - #177

Open
ranxianglei wants to merge 1 commit into
masterfrom
2026-08-31_status-report-accounting
Open

fix: unify tool-message accounting across status report surfaces (issue #390)#177
ranxianglei wants to merge 1 commit into
masterfrom
2026-08-31_status-report-accounting

Conversation

@ranxianglei

Copy link
Copy Markdown
Owner

What

Fixes the four kernel-side defects behind billion-context issue #390 (acp_status CONTEXT BREAKDOWN under-counts tool output by ~99%, prints two contradictory tool/text splits, mixes two token estimators without labeling, and can print bucket percents summing to >100%):

  1. report.ts field-selection bugcollectVisible classified messages by message.toolName ?? "text", but wire converters (anthropic.ts, openai.ts, responses.ts) only set toolName on the call side; every tool-result therefore landed in the text bucket. Now classification uses the shared isToolMessage() (contentType-based), and result-side tool names are resolved through the shared toolCallId so both halves of an exchange land in the same named tool bucket (Top tools / drilldown tool: filter now work for results too).
  2. Two conflicting "tool" definitions — new leaf module src/message-kind.ts exports the single isToolMessage() definition; report.ts, recommend.ts (local copy deleted), and compress.ts computeContextBreakdown (inline copy deleted) all use it. One definition, three surfaces.
  3. pct floorpct() no longer clamps to 1%: tiny buckets print 0% and three buckets can no longer print 1%+1%+1% with sum >100.
  4. panel scale contractunprunedTokens docstring/comment claimed "chars/4" but the nudge breakdown actually runs on the core's estimator (defaultCountTokens, CJK-aware, unless a host injects one into createCore). Comments corrected to state the real contract: same estimator on both sides of the Session-only subtraction (issue fix: hide all orphaned compress calls #18 family). No arithmetic change — the bili host already computes unprunedTokens with defaultCountTokens.

Tests

  • tests/report-accounting.test.ts (new): 51K-char tool-result lands in the tool bucket (12.8K, 100%); breakdown tool split corroborates buildCompressibleRanges toolPct on the same message set; no 1% floor (sums ≤100); injected-estimator scale check (CJK 4000 chars → 4.0K with defaultCountTokens vs 1.0K with estimateTokensFast); orphan result stays in tool bucket under tool; drilldown filters results by resolved name.
  • tests/panel.test.ts: new same-scale Session-only case with CJK originals (1000 CJK chars = 1.0k, not 250).

Pre-flight

  • npm run typecheck — clean
  • npm test — 556/556 pass (549 baseline + 7 new)
  • npm run build — clean

Note: npm run format:check fails on 89 files at clean master (pre-existing, CI does not gate on it); edits follow each file's local style.

Follow-up (separate repo)

billion-context must (a) pass defaultCountTokens instead of estimateTokensFast to buildStatusReport in its three acp_status paths, and (b) bump acp-kernel to this release. Tracked in billion-context#390.

@github-actions

Copy link
Copy Markdown

📦 Built Package Artifact

Branch: 2026-08-31_status-report-accounting (87f4e02)

Option A — Install from npm PR tag (recommended)

In your adapter project:

npm install acp-kernel@pr-177

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

Option B — Download artifact

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

This comment is automatically updated on each push.

@ranxianglei

Copy link
Copy Markdown
Owner Author

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

[bot] 🏷 Reviewing this PR now — pulling branch 2026-08-31_status-report-accounting, verifying the four fixes, and running the pre-flight (typecheck / test / build). Will report findings shortly.

@ranxianglei

Copy link
Copy Markdown
Owner Author

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

[bot] 🏷 Review: LGTM — all four fixes verified against the branch (2026-08-31_status-report-accounting @ 87f4e02).

What I ran (full pre-flight, not just trusting the description):

  • npm run typecheck — clean
  • npm test — 556/556 pass on the branch; master baseline is 549/549, so exactly +7 new tests as claimed (6 in tests/report-accounting.test.ts, 1 in tests/panel.test.ts)
  • npm run build — clean

Fix-by-fix verification:

  1. report.ts field-selection bug — confirmed. collectVisible now classifies via isToolMessage() (contentType-based) and builds a nameByCallId map from calls carrying both toolCallId + toolName, so results resolve into the same named bucket; unresolvable orphans fall back to "tool". The map is built in a first pass over all messages, so ordering (result-before-call) and calls inside compressed blocks don't matter. I verified the root-cause claim in the wire codecs: anthropic.ts:94-108, openai.ts:136-148, and responses.ts:241-247 all set toolCallId on tool-result and never toolName — so the old message.toolName ?? "text" really did dump every result into the text bucket.
  2. Single definition — confirmed. src/message-kind.ts is a clean leaf module (imports only types.ts); report.ts, recommend.ts (local copy deleted, now isTool: isToolMessage(msg) at src/recommend.ts:195 feeding toolPct/textPct), and compress.ts computeContextBreakdown (inline copy deleted) all import it. Exported from src/index.ts — reasonable, hosts may want the same predicate.
  3. pct floor — confirmed. Math.max(1, Math.round(...))Math.round(...) at src/report.ts:10-13; tiny buckets now print 0%.
  4. panel scale contract — confirmed comment-only. unprunedTokens docstring and the sessionOnly derivation comment now state "same estimator as the core's nudge breakdown (defaultCountTokens unless injected)" instead of "chars/4"; the arithmetic (Math.max(0, unprunedTokens - sentTotal)) is untouched, and the new CJK panel test pins the scale (1000 CJK chars → 1.0k, not 250).

Test quality: the new tests cover the right regressions — 51K-char result landing in the tool bucket (12.8K/100%), cross-surface agreement between the breakdown and buildCompressibleRanges toolPct on the same message set, no-floor sum ≤100, estimator-scale check (CJK 4000 chars → 4.0K defaultCountTokens vs 1.0K estimateTokensFast), orphan result under tool, and drilldown filtering results by resolved name.

Minor nits (non-blocking, no changes required):

  • renderOverview's toolTypeMap (src/report.ts:155-158) still includes text messages under the key "text", so when text dominates, "Top tools" can print text (NN%) and the Tip line can suggest tool:"text". Pre-existing (the old toolName ?? "text" had the same shape), but now that isTool is a first-class field, skipping !m.isTool entries there would be a one-line cleanup for a follow-up.
  • Bucket percents can still sum to 101 in a pure-rounding edge case (e.g. 33.5+33.5+33.0 → 34+34+33). The PR's claim is correctly scoped to the floor bug (1%+1%+1%), so this is just a note, not a defect.
  • src/message-kind.ts uses 4-space indent (matches report.ts, not the 2-space majority) and the branch adds 2 more format:check warnings (90 at master → 92). CI doesn't gate on format (checked ci.yml), and the pre-existing 90-file failure makes this cosmetic — but worth knowing for the eventual repo-wide format pass.
  • Optional follow-up: src/protected.ts:36-61 has its own negated inline contentType checks that could also adopt !isToolMessage(); out of scope here since it's protection logic, not accounting.

Verdict: correct, minimal, well-tested, and the description matches the code exactly. Ready for human merge (per repo rules I don't merge PRs myself). Don't forget the tracked follow-up in billion-context#390: pass defaultCountTokens (not estimateTokensFast) to buildStatusReport in the three acp_status paths, and bump to this release — otherwise the host-side estimator mismatch the panel comment now documents stays live.

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