test(desktop): pin the context meter to a round that completes no tool - #337
Merged
Merged
Conversation
#332 moved the meter's sampling from tool completion to the provider's own usage, on the grounds that a round which calls no tool reports nothing at all under the old scheme. Its e2e could not show that. Every round the mock drives calls a tool, so sampling per completed tool and sampling per API response produce the same series — I said so in that PR rather than let the green tick imply more than it proved. This is the test that was missing. The turn now contains one round that names a tool the agent cannot have. That name is rejected in `conversation_loop` before the executor is ever entered (`_invalid_tool_retries`), so the round finishes a real API call, reports real usage, appends a result saying the tool does not exist, and continues — with no `tool_start`, no `tool_complete`, and therefore no orphaned tool card. It is a branch on a string, so nothing about it is timing-dependent. Three other mechanisms were considered and rejected. Malformed tool arguments never reach the executor's block path — the loop validates argument JSON first and either aborts the turn or silently re-issues the identical request, and the streaming transport repairs them earlier still. A `length` continuation works but injects a `user` message mid-turn and duplicates text into the final response. A guardrail block halts the loop by construction, which fails the "turn must continue" requirement outright. Two supporting changes: - **The mock counted tool results across the whole conversation, not the turn.** A second turn in the same chat therefore started already at the cap and collapsed to a single round — so the existing "still tracks a session the user typed" test was passing on turn-boundary events, not the mid-turn frames it was written to check. Anchoring the count to the last user message fixes it: that test goes from 2 readings to 16. Only this spec has multi-turn chats, so no other spec that shares the mock changes behaviour. - **The probe's served prompt count is exported rather than hardcoded.** The usage schedule is a function of conversation length, so a literal in the spec would drift the moment the system prompt or tool list changes size. The expected value is formatted through the status bar's own `compactNumber`, because the gauge is what the user actually reads. The evidence, which is the only reason this test is worth having: with e38eacb 15k -> 18k -> 21k -> 24k (t=3.0 / 4.4 / 5.6 / 6.0s) with it reverted FAILS — 18k never appears 18k is the round that completed no tool. Reverting only the three source files of #332 and re-running turns the new test red on exactly that assertion, and restoring them turns it green again. A test that passes either way would prove nothing, so that check is the point of the change, not a footnote to it. Co-authored-by: Omar Baradei <omar@kostudios.io> Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
OmarB97
added a commit
that referenced
this pull request
Aug 2, 2026
`main` does not typecheck. #337 asserted the context meter's tool-free round by formatting the expected figure with the status bar's own `compactNumber`, which meant importing `src/lib/format` from a spec: e2e/context-meter-spawn.spec.ts(30,31): error TS6307: File 'apps/desktop/ src/lib/format.ts' is not listed within the file list of project 'apps/desktop/tsconfig.e2e.json'. `tsconfig.e2e.json` carries `"exclude": ["src", "electron"]`. That is the design, not an oversight — the e2e project is deliberately a separate island from renderer source, and the two compile under different lib/DOM assumptions. Adding the file to that project's include would fight the boundary to buy one test a prettier assertion. Re-implementing the formatter in the spec is the other obvious move and is worse: `compactNumber`'s own docstring calls itself THE compact-number formatter and says not to hand-roll `/ 1000` display math elsewhere, which is exactly what a copy in a test would be. So the assertion compares numbers instead of labels — against the count the mock actually served, never a literal, since the usage schedule is a function of conversation length. The gauge renders one decimal in `k`, so a reading round-trips to within half a step of the real figure; ±100 tokens covers that, and the rounds are 3k apart, so the slack cannot let a neighbouring round pass for the one under test. `npm run typecheck` fails on `main` at this commit's parent and passes here, which is the whole of the change. Co-authored-by: Omar Baradei <omar@kostudios.io> Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
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.
#332 moved the meter's sampling from tool completion to the provider's own
usage, on the grounds that a round which calls no tool reports nothing at all
under the old scheme. Its e2e could not show that. Every round the mock drives
calls a tool, so sampling per completed tool and sampling per API response
produce the same series — I said so in that PR rather than let the green tick
imply more than it proved. This is the test that was missing.
The turn now contains one round that names a tool the agent cannot have. That
name is rejected in
conversation_loopbefore the executor is ever entered(
_invalid_tool_retries), so the round finishes a real API call, reports realusage, appends a result saying the tool does not exist, and continues — with no
tool_start, notool_complete, and therefore no orphaned tool card. It is abranch on a string, so nothing about it is timing-dependent.
Three other mechanisms were considered and rejected. Malformed tool arguments
never reach the executor's block path — the loop validates argument JSON first
and either aborts the turn or silently re-issues the identical request, and the
streaming transport repairs them earlier still. A
lengthcontinuation worksbut injects a
usermessage mid-turn and duplicates text into the finalresponse. A guardrail block halts the loop by construction, which fails the
"turn must continue" requirement outright.
Two supporting changes:
A second turn in the same chat therefore started already at the cap and
collapsed to a single round — so the existing "still tracks a session the user
typed" test was passing on turn-boundary events, not the mid-turn frames it
was written to check. Anchoring the count to the last user message fixes it:
that test goes from 2 readings to 16. Only this spec has multi-turn chats, so
no other spec that shares the mock changes behaviour.
usage schedule is a function of conversation length, so a literal in the spec
would drift the moment the system prompt or tool list changes size. The
expected value is formatted through the status bar's own
compactNumber,because the gauge is what the user actually reads.
The evidence, which is the only reason this test is worth having:
18k is the round that completed no tool. Reverting only the three source files
of #332 and re-running turns the new test red on exactly that assertion, and
restoring them turns it green again. A test that passes either way would prove
nothing, so that check is the point of the change, not a footnote to it.
Local Desktop E2E was run at reduced machine load; the four other specs that share the mock were last exercised while other suites had the box at 46-79 load average and failed at boot with zero assertion failures, so CI is the authoritative check on those.