Repository navigation
fix(acp): persist token usage events for ACP sessions - #1137
Conversation
ACP turns never set agent.Options.OnUsage, so provider token counts were dropped on the floor. zero exec and the TUI persist the same counts as session usage events, and `zero usage report` totals those events, which left every editor-driven ACP session at zero requests and zero tokens. runTurn now queues each report through usage.EventUsagePayload, the same writer exec uses for a non-escalating run, and commits it in the turn's existing buffered batch under the session id. A hard run failure still returns before persistTurn, so that turn's usage is dropped with its history. StopCancelled stays a successful outcome and keeps the usage already reported. session/load and session/resume still skip event types other than messages, compaction, and tools, so the restored prompt and the wire replay do not gain a usage channel. ACP has no model escalation, so the payload has no "model" key and no prompt, tool argument, or other raw input. This sits beside the permission-mode field persisted on the session (Twigpine#1134). It does not change the ACP wire protocol, exec, the TUI, or the report reader. Refs Twigpine#1118
The persistence test showed that a successful turn records token counts, and the hard-failure test showed that an aborted turn records nothing. Neither one locked the two edges that are easy to get backwards. A client cancel is a StopCancelled outcome, not a hard failure, so the tokens the provider already reported stay in the session log. session/load and session/resume read that same log; the replay and the next prompt must still be the conversation, with no usage payload on the wire and no token counts folded into the resumed prompt. The persisted JSON is also checked to be token-count keys only, so a prompt or tool argument cannot ride along in the usage record. Refs Twigpine#1118
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughACP now persists provider token-usage reports as session events in the turn’s buffered batch. Tests cover persistence, turn failures, cancellation, and session load and resume. ChangesACP usage persistence
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix · Severity of issue fixed: Medium Suggested reviewers: Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 62.50% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 8 functions across 2 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly and concisely describes the main change: persisting token usage events for ACP sessions. Linked Issues check The PR meets the coding requirements in [ #1118].internal/acp/agent.gowiresagent.Options.OnUsagethroughusage.EventUsagePayloadinto the existing buffered turn batch. The batch persists `sess…Out of Scope Changes check The changes stay within [ #1118]. The source change is limited to ACP usage persistence ininternal/acp/agent.go. The added tests ininternal/acp/usage_test.godirectly verify the issue behavior an…
- Fix all pre-merge checks with AI
✨ Finishing Touches: 💡 1
-
🧪 Generate unit tests (beta)
-
- Create a new PR
🛠️ Fix failing CI checks: 💡
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Comment @coderabbitai help to get the list of available commands.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
CI note: the only red job is This PR only touches Could a maintainer re-run the failed job? The other nine checks pass. I am not pushing an empty commit to re-trigger it. |
2 participants
Summary
ACP sessions dropped every provider token count, so
zero usage reportshowed zero requests and zero tokens for editor-driven work.zero execand the TUI already persist those counts as session usage events. This wires the same writer into the ACP turn.The full suite, including Windows and macOS, already passed on the fork CI (FabioLeitao#12, merged).
This pull request was prepared with AI assistance. The change was reviewed against the current
runTurnpath, including the session permission mode from #1134, and checked locally: the regression tests fail whenOnUsageis removed, and an isolated local OpenAI-compatible stub showed zero usage on the upstream binary and the stub's token counts after the change.Problem
internal/acp/agent.gorunTurnsetOnText,OnReasoning,OnToolCall,OnToolResult, andOnPermissionRequest, and leftagent.Options.OnUsageunset. The agent loop still received provider usage. Nothing appended asessions.EventUsagerecord.zero usage report(including--session) totals only those records, so an ACP session that had user messages, assistant messages, and tool events still reported zero tokens. Reproduced onmainat99721c76and on v0.9.0. #929 (usage on the ACP wire) is a separate change and stays out of this PR.Cause
The exec runtime records usage with
usage.EventUsagePayloadfromOnUsage. The ACP turn runner never installed that callback, so the counts never reached the session store.Fix
runTurnqueues each report throughusage.EventUsagePayloadinto the turn's existing buffered batch and commits it withpersistTurnunder the session id created byStore.Create. That is the idzero usage report --sessionalready filters on.The payload matches a non-escalating
zero execrun:promptTokens,completionTokens,totalTokens, pluscachedInputTokens,cacheWriteTokens, andreasoningTokensonly when non-zero. There is nomodelkey, because ACP has no mid-run model escalation. The payload has no prompt text, tool arguments, or other raw input.A hard
RunAgentfailure still returns beforepersistTurn, so that turn's usage is discarded with its history.StopCancelledremains a successful outcome, and usage the provider already reported stays in the log.session/loadandsession/resumestill project only messages, compaction summaries, and tool events, so the restored prompt and the wire replay stay the conversation.The permission mode persisted on the session (#1134) is unchanged. The ACP wire protocol, exec, the TUI, and the report reader are unchanged.
Before / after
Before: a finished ACP turn left messages and tool events and no
usageevents.zero usage report --session <id> --jsonshowed"requests": 0and zero tokens.After: the same turn leaves one
usageevent per provider report. Against a local OpenAI-compatible stub that emitted 1200 prompt tokens and 34 completion tokens, with an isolated home directory:mainBoth turns ended
end_turn.totalCoststayed 0 because the stub model id is not in the price registry. The token counts are what the report starts to see.Tests
TestACPPromptPersistsProviderUsage: two provider reports in one turn become two usage events in the report's shape, beside the turn's two messages. Payload keys are token counts only.TestACPHardTurnFailureDropsItsUsageWithTheTurn: a hardRunAgenterror leaves no durable events.TestACPCancelledTurnKeepsReportedUsage:context.Canceledcommits the reported usage withstopReasoncancelled.TestACPLoadAndResumeIgnoreUsageEvents:session/loadreplays the user and assistant text only, and the resumed prompt contains the conversation and no token counts.Removing the
OnUsagewiring makesTestACPPromptPersistsProviderUsagefail withagent.Options.OnUsage is not wiredandusage events = []. The cancel and load/resume tests fail the same way.Risk
Usage joins the turn batch, so a crash mid-write has the same boundary it already had for messages. A cancelled turn that already received a provider usage report now counts those tokens; a hard failure still does not. Cost for an unknown model stays a zero estimate, as it does for any other session whose model id is absent from the registry.
OnUsageruns on the agent loop goroutine, the same placeOnToolCallalready appends to the turn buffer.How to verify
gofmt -lon the two files,go vet ./internal/acp/...,go test ./internal/acp ./internal/usage/... -race -count=1,go run ./cmd/zero-release build, andgo run ./cmd/zero-release smoke.End to end, with an isolated home directory and a local stub provider:
zero acp(session/new,session/prompt), thenzero usage report --session <id> --json. The fixed binary counts the stub's tokens. The upstream binary reports zeros.Scope
internal/acp/agent.goandinternal/acp/usage_test.goonly. No UI change.Linked issue
Closes #1118
The parent issue has the
issue-approvedlabel.Checklist
issue-approvedlabel.go build ./...,go vet ./..., andgo test ./...pass locally.gofmtclean.-racewhere relevant).Local commands run on this branch:
gofmt -lon both touched files (clean),go vet ./internal/acp/...,go test ./internal/acp ./internal/usage/... -race -count=1,go run ./cmd/zero-release build, andgo run ./cmd/zero-release smoke. The full repository suite, including Windows and macOS smoke, passed on the fork CI for the same tests (FabioLeitao#12). There is no UI change, so there is no screenshot.Summary by CodeRabbit