Repository navigation
fix: a run record must include the turn's closing model call - #41
Merged
Merged
Conversation
The run history reported `steps` and `costUSD` one attempt short on every turn. Measured in the Desktop app on 2026-10-05: `steps` equalled the session log's `assistant/message` count minus one in 8 of 8 runs. The cause is ordering, not a missing drain. `spendSettledUsage` runs at `agent/pre-step` and at `agent/request`, but a turn's LAST attempt is settled only as the `agent/request` handler RETURNS — after the `turn/end` listener has already snapshotted the budget and written the record. So the record is built from a budget that has not yet been charged for its final step, and the step it misses is the turn's most expensive one: the closing answer, on the longest context. `turn/end` now drains before it builds the record. The gate check that was already there stays first in the same block — it is the phase-advance that must not be reordered behind a price read — and the duplicated `agentOfSession` lookup is folded into the one call that now needs it. test/plugin-approval.test.ts gains a fixture that models the real ORDER: a step boundary drains with the closing answer still unmaterialised, the answer lands, then `turn/end` arrives. It fails `steps: 0 !== 1` on the old code and passes on the new one. A log that already contained the answer at the first boundary would pass either way, which is why the fixture orders the events instead of just declaring them. 689 tests pass, typecheck clean, pre-commit scan clean.
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
1 participant
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.
What was wrong
The run history reported
stepsandcostUSDone attempt short on every turn. Measured in the Desktop app against the session log on 2026-10-05:stepsequalled the session'sassistant/messagecount minus one in 8 of 8 runs.Root cause — ordering, not a missing drain
spendSettledUsagealready ran at two seams:agent/pre-step(before each step) andagent/request(per attempt). But a turn's last attempt is settled only as theagent/requesthandler returns — andturn/endfires before that return. So the record is built from a budget that has not yet been charged for its closing call.The step it drops is the turn's most expensive one: the closing answer, on the longest context. So the undercount is not spread evenly — it is systematically the worst step.
The fix
turn/endnow drains before it builds the record (src/plugin.ts). The existing gate check stays first inside the same block, because that is the phase-advance that must not be reordered behind a price read; the duplicatedagentOfSessionlookup is folded into the one call that now needs it.Test
test/plugin-approval.test.tsgains a fixture that models the real order — a step boundary drains while the closing answer is still unmaterialised, then the answer lands, thenturn/endarrives. On the old code it fails0 !== 1; on the new one it passes. A log that already contained the answer at the first boundary would pass either way, which is why the fixture orders the events rather than merely declaring them.Verification
make test— 689 pass, 0 failmake typecheck— clean.githooks/pre-commit --all— cleandocs/PRD.md§8 updated: the "largest correctness gap" entry described a missing price path; the real defect was cadence, and it is now closed.Not verified here: a fresh Desktop run re-measured against the session log. This PR needs merging and a profile refresh before the app picks it up.