Repository navigation
fix: count the tokens billed by responses.compact - #4435
arthi-arumugam-git wants to merge 3 commits into
Conversation
run_compaction makes a real billed model call and read only compacted.output, so compacted.usage was discarded and those tokens never reached the run's totals. On a long conversation the compaction pass is often the largest call in the run, so the reported total can be a fraction of what was billed, and requests undercounts by one per pass. The session now accumulates compaction usage in a public compaction_usage attribute. Accounting is written defensively: a missing or unexpected usage shape is logged and skipped rather than raised, because usage bookkeeping must never break a run. Fixes openai#4434 Claude-Session: https://claude.ai/code/session_01KJrBqn4AgyHTazfdUuXbuA
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8e5fb9cb58
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| Compaction is a real model call, so it is billed, but it happens outside the | ||
| agent run loop and therefore never reaches ``RunResult.context_wrapper.usage``. | ||
| This accumulates it so the cost is visible rather than silently dropped. |
There was a problem hiding this comment.
When Runner triggers compaction, this new session-local accumulator is never merged into result.context_wrapper.usage, so the SDK's documented run-wide billing total still omits the potentially dominant responses.compact call; it also accumulates across the session rather than identifying the usage of the current run. save_result_to_session already passes the current wrapper through _call_session_method, so run_compaction can opt into that wrapper and add the parsed usage to its run-wide accumulator exactly once.
AGENTS.md reference: AGENTS.md:L153-L153
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed, and this is fixed in 2a6d651, which is now the head of this PR. run_compaction opts into the wrapper protocol that _call_session_method already implements, and the parsed usage is added to wrapper.usage exactly once per pass, so it reaches RunResult.context_wrapper.usage like any other model call.
On the second half of your point, accumulating across the session rather than the run: the session-local compaction_usage is now a separate concern from the run total. It exists for callers who drive compaction directly outside a run, where there is no wrapper to add to. The run-wide number comes from wrapper.usage and counts only that run's passes. If you would rather the session attribute not exist at all, say so and I will drop it.
Tests added for the wrapper path, for the opt-in detection itself, and for a call routed through _call_session_method so the real run-loop shape is exercised.
run_compaction now opts into the wrapper protocol that _call_session_method already implements, so the tokens billed by responses.compact land in wrapper.usage and therefore in RunResult.context_wrapper.usage, which is where the run's total is read from. The session-level compaction_usage attribute stays for callers that drive compaction directly, outside a run. Tests cover the wrapper path, the opt-in detection the run loop uses, and a call through _call_session_method itself. Claude-Session: https://claude.ai/code/session_01KJrBqn4AgyHTazfdUuXbuA
|
Updating this: my first version stopped short. It recorded compaction usage on the session, and I said in the description that flowing it into So 2a6d651 does the complete thing. New tests cover the wrapper path, the opt-in detection the run loop relies on, and a call routed through Sorry for the two-step. The first patch was a half fix and it should not have shipped that way. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2a6d651e0a
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
|
||
| compacted = await self.client.responses.compact(**compact_kwargs) | ||
|
|
||
| self._record_compaction_usage(getattr(compacted, "usage", None), wrapper) |
There was a problem hiding this comment.
Fresh evidence in this revision is that run_compaction now accepts wrapper, but automatic compaction still receives None: save_result_to_session() first replaces the supplied run wrapper with _get_session_wrapper(session, wrapper), and that helper rejects this decorator because its get_items, add_items, pop_item, and clear_session methods do not all accept wrapper. Consequently, when a real Runner triggers compaction, this call updates only session.compaction_usage and the returned run-wide usage still omits the compact request; the added test bypasses _get_session_wrapper() by calling _call_session_method() directly. Preserve the original run wrapper separately for the compaction dispatch while continuing to withhold it from legacy history methods.
AGENTS.md reference: AGENTS.md:L153-L153
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Both findings fixed in 91ffc3a, and the P1 taught me something about the design I had missed twice.
My first instinct was to opt the session's history methods into the wrapper protocol, which made _get_session_wrapper forward the wrapper. That broke test_compaction_session_keeps_context_aware_underlying_on_legacy_scope: the compaction session staying legacy-scoped for history is an intentional invariant, not an accident. So the fix lives at the dispatch instead: save_result_to_session keeps the unfiltered run wrapper for the run_compaction call only, while history calls still go through the filtered one. _call_session_method continues to gate on the method's own opt-in, so nothing changes for sessions that never declared wrapper.
The P2 is fixed by constructing Usage with the detail objects rather than assigning them afterwards, so __post_init__ normalizes None fields exactly as it does for every other provider shape; there is a test with model_construct-built details carrying None.
Green: tests/memory plus tests/test_usage.py at 234, tests/test_agent_runner.py plus tests/test_hitl_session_scenario.py at 211, including the legacy-scope test the first attempt broke.
| if isinstance(input_details, InputTokensDetails): | ||
| usage.input_tokens_details = input_details | ||
| output_details = getattr(compacted_usage, "output_tokens_details", None) | ||
| if isinstance(output_details, OutputTokensDetails): | ||
| usage.output_tokens_details = output_details |
There was a problem hiding this comment.
When the API returns an InputTokensDetails or OutputTokensDetails object whose optional fields are None—a case Usage.__post_init__() explicitly handles for provider and SDK-generated objects—these assignments occur after Usage construction and therefore bypass that normalization. Usage.add() normalizes the aggregate totals but synthesizes each RequestUsage entry from these unnormalized objects, leaving fields such as cache_write_tokens or reasoning_tokens as None in both compaction_usage.request_usage_entries and the run-wide entries; consumers performing the documented per-request cost arithmetic can then fail or receive inconsistent details. Pass the detail objects into the Usage constructor or normalize them before calling add().
AGENTS.md reference: AGENTS.md:L153-L153
Useful? React with 👍 / 👎.
…detail objects Two corrections from Codex review of 2a6d651. The wrapper never reached run_compaction on the automatic path: save_result_to_session filters the wrapper through _get_session_wrapper, which returns None unless every history method opts in, and the compaction session deliberately stays legacy-scoped for history (test_compaction_session_keeps_context_aware_underlying_on_legacy_scope asserts exactly that). The dispatch now passes the unfiltered run wrapper to run_compaction only; _call_session_method still gates on the method's own opt-in, and history calls keep the session-level contract. Usage details are now passed to the Usage constructor instead of being assigned after construction, so __post_init__ normalizes detail objects whose optional fields a provider left as None. Tests: the legacy-scope invariant is asserted alongside the compaction opt-in, the automatic dispatch path is exercised end to end through the same gate and helper the run loop uses, and None-field detail objects are covered. tests/memory + tests/test_usage.py: 234 passed. tests/test_agent_runner.py + tests/test_hitl_session_scenario.py: 211 passed. Claude-Session: https://claude.ai/code/session_01KJrBqn4AgyHTazfdUuXbuA
|
@codex review |
|
Security review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
|
Thanks, all three were fair and all three are addressed. The reviews ran against 8e5fb9c and 2a6d651; the branch is now at 91ffc3a. P1, route compaction tokens into the run usage accumulator. Fixed in 2a6d651. P1, preserve the run wrapper when dispatching compaction. This was the real one, and you were right that the first attempt did not fix it. So 91ffc3a fixes it at the dispatch site instead. The run wrapper is captured before the history gate narrows it: run_wrapper = wrapper
wrapper = _get_session_wrapper(session, wrapper)and compaction is dispatched with P2, normalize token details before recording. Fixed in 91ffc3a. The details are collected into 25 tests pass across |
|
#4446, which includes your contribution credit, resolved the issue. Thanks again for reporting and trying to resolve the issue! |
|
Thank you for taking it over the line, and for carrying the credit through to the commit. Closing the loop on the accounting was the part that mattered, and #4446 lands it cleanly, including keeping the history calls on the legacy scope, which my first attempt got wrong before the tests caught it. I learned a fair amount about the session protocol from reading your version against mine. Happy hunting with the SDK. |
Fixes #4434.
run_compactionmakes a real, billedresponses.compactcall and read onlycompacted.output.compacted.usagewas discarded, so those tokens never reached the run's totals. Compaction fires on long conversations, where the compacted history is large, so the dropped call is frequently the most expensive one in the run: in a scripted 12-turn run with one pass, the run reportstotal_tokens=1800while the compaction pass consumed 192,000.What this changes. The session accumulates compaction usage in a public
compaction_usageattribute, so the cost is visible instead of silently dropped.Written defensively on purpose. Usage bookkeeping must never break a run, so a missing
usage, or one whose token fields are not plain integers, is logged at debug and skipped rather than raised. This matters in practice: the first version of this patch validated the details objects eagerly and broke twelve existing tests that stub the response withMagicMock, which is exactly the shape a surprising provider response would take.Tests in
tests/memory/test_compaction_usage.py: usage recorded from a pass, accumulation across two passes, a missing-usage response tolerated, and the counter starting empty. All four fail on main.tests/memory/test_openai_responses_compaction_session.py(55) andtests/test_usage.py(21) pass unchanged.Left to you deliberately: whether this total should also flow into
RunResult.context_wrapper.usageautomatically. The session holds no reference to the run context today, so wiring it there is an architectural decision rather than a bug fix, and I did not want to make it on your behalf. Happy to follow up with that change if you want it.