fix(compact): report upstream usage for native compact turns - #945
fix(compact): report upstream usage for native compact turns#945DevMello wants to merge 1 commit into
Conversation
The native branch buffers the upstream compact JSON and returns it without inspecting the body, so the request log row lands with no usage. Lift usage and response metadata from the buffered body the same way the routed branch gets it through handleResponses.
📝 WalkthroughWalkthroughThe native compact-response path now inspects successful buffered upstream JSON, records usage and response metadata in the request log, excludes synthetic buffer errors, and verifies that usage remains in the client response. ChangesNative compact usage reporting
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![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: 2ae3b5fc8d
ℹ️ 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".
| // Lift usage and response metadata from the buffered upstream JSON into the | ||
| // request log; the routed branch gets the same through handleResponses. The | ||
| // synthetic buffer errors are not upstream bodies and stay uninspected. | ||
| if (buffered.ok) inspectResponseLogJson(logCtx, await buffered.clone().text()); |
There was a problem hiding this comment.
When usage debugging is enabled, inspectResponseLogJson also copies the first 2 KiB of the body into usageDebugBodySample, which is later written to ~/.opencodex/usage-debug.jsonl. Native compact responses are replacement history derived from the conversation being compacted, so this new call can persist private conversation content just to extract token usage; parse the buffered JSON for usage/metadata without invoking the debug body sampler.
AGENTS.md reference: AGENTS.md:L214-L215
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/responses-compaction-routing.test.ts`:
- Around line 188-192: Update the response assertion in this test to validate
the complete upstream payload, not only body.usage. Assert that body also
preserves the expected id, status, and output fields while retaining the
existing usage and logCtx.usage checks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 7739ee1a-3d74-4c37-a3ae-932f1a61abb4
📒 Files selected for processing (2)
src/server/responses/compact.tstests/responses-compaction-routing.test.ts
| expect(response.status).toBe(200); | ||
| const body = await response.json() as { usage?: Record<string, unknown> }; | ||
| expect(body.usage).toMatchObject({ input_tokens: 10, output_tokens: 5, total_tokens: 15 }); | ||
| expect(logCtx.usage).toMatchObject({ inputTokens: 10, outputTokens: 5, totalTokens: 15 }); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the complete upstream payload.
Lines 189-190 check only body.usage. A regression that removes id, status, or output would still pass, although this PR must preserve the complete upstream body.
Proposed assertion
- const body = await response.json() as { usage?: Record<string, unknown> };
- expect(body.usage).toMatchObject({ input_tokens: 10, output_tokens: 5, total_tokens: 15 });
+ const body = await response.json();
+ expect(body).toEqual(completedPayload("native summary"));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/responses-compaction-routing.test.ts` around lines 188 - 192, Update
the response assertion in this test to validate the complete upstream payload,
not only body.usage. Assert that body also preserves the expected id, status,
and output fields while retaining the existing usage and logCtx.usage checks.
|
Carried onto the review stack as #953 (stack 3/3), unmodified. Your commits were taken with Verified on the stack: This PR stays open until #953 lands. If a maintainer prefers to take yours directly instead, that path is unaffected — the stack commits get dropped and this one merges. Once #953 merges I'll close this as carried, with the credit already in the commit history rather than in a comment. Stack: #951 (plan, base Thanks for the fix. |
2 participants
Summary
Native /v1/responses/compact turns buffer the upstream JSON and return it without reading the body, so every native compaction lands in /api/usage as unreported even though the response carries a full usage object. Compact turns are among the largest requests an account makes, so the undercount is biggest exactly where usage matters. The routed branch already reports through handleResponses. The native branch now inspects a clone of the buffered body with the existing inspectResponseLogJson helper, filling usage, resolved model, and service tier in the request log. The client body is unchanged and synthetic error responses are not inspected.
Verification
tests/responses-compaction-routing.test.ts: a native compact turn fills the request log usage from the upstream body and the client still receives the body intact.bun run test,typecheck,lint:gui,privacy:scan.Checklist
Summary by CodeRabbit
Bug Fixes
Tests