test(server): budget ACP tool call emissions at the adapter boundary - #122
Merged
Conversation
The thread transfer budget in server.test.ts caps HTTP and WebSocket bytes, but it replays recorded fixtures through Codex and Claude. Neither is an ACP provider, and the harness never starts an adapter, so nothing measures how many provider events an ACP tool call produces in the first place. That decision is `decideToolCallUpdateEmission`, shared by Grok, Cursor, and Prime Agent, and it was until recently withholding every in-progress update for command tools. Adds a streaming-command scenario to the ACP mock agent and a two-sided budget over it. The ceiling keeps coalescing honest. The floor is the part that matters: a ceiling alone reads "withheld everything" as a pass.
Thread transfer impact✅ Thread transfer remains within every enforced ceiling.
Baseline: Scenario and decoded snapshot size10 historical turns, 5 command tools per turn, 878.9 KiB retained MCP result per historical turn, and a 1.05 MiB retained result in the measured turn.
Updated in place by a trusted workflow. PR artifacts are strictly validated and never executed. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes a coverage gap I hit while reviewing #118, where I could describe a websocket-traffic risk but not measure it.
The gap
apps/server/src/server.test.tsalready enforces a thread transfer budget — five metrics per provider, hard ceilings, CI fails on a violation. But it coverscodexandclaudeAgentonly, and it drives them by queueing recorded fixtures into an adapter harness rather than starting a real adapter.So two things are invisible to it. ACP providers aren't in the provider list, and no adapter runs at all — which means
decideToolCallUpdateEmission, the function that decides how many provider events a tool call generates in the first place, is never executed. That function is shared by Grok, Cursor, and Prime Agent.Adding an ACP provider to
TRANSFER_BUDGETSwould not have fixed this. It would measure the same fixture-driven pipeline under a different label and still never reach the emission decision. The measurement has to happen at the adapter boundary.What this adds
A
T3_ACP_STREAM_COMMAND_CHUNKSscenario in the ACP mock agent that streams a long-running command the way a real terminal tool does: onetool_call_updateper chunk, cumulative text onrawOutput.stdout, and atitlethat never changes. Then a budget over it, driven through the Grok adapter and asserting on emittedProviderRuntimeEvents.The budget is two-sided, and that is the point. A ceiling alone would have passed on the exact bug #118 fixed: because command tools keep
detailequal to the command, coalescing that measured onlydetailsaw no growth and suppressed every in-progress update, delivering the whole build log at completion. To a ceiling that reads as "wonderfully few messages." To a user it is a dead UI during a build.Verification
For 40 chunks of 64 characters — 2,560 characters of stdout:
pylon)I confirmed the second row by temporarily restoring the old detail-only coalescing in
AcpRuntimeModel.tsand running the budget against it, then restoring the file. So this test is demonstrably capable of catching the regression it exists for, rather than merely passing today.Bounds are 4–16 against an observed 11, deliberately loose. Tuning the coalescing thresholds should not fail this test; losing streaming or losing coalescing should.
t3typecheck clean, lint clean on both files.No production code changes — mock agent and test only.
Model: Claude Opus 5. Harness: Claude Code.
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.