fix(compact): report upstream usage for native compact turns - #945
fix(compact): report upstream usage for native compact turns#945DevMello wants to merge 2 commits into
Conversation
📝 WalkthroughWalkthroughThe native compact-response path now parses successful buffered upstream JSON and applies usage and response metadata to the request log. Synthetic buffering errors are excluded. A regression test verifies payload preservation, token usage recording, and usage-debug field handling. 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. |
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.
c1dbe0a to
8192ea4
Compare
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 170-206: Extend the “buffered upstream body” test around
handleResponsesCompact to return response data whose model and service_tier
differ from the initial route metadata, then assert logCtx.resolvedModel and
logCtx.responseServiceTier are populated from that response. Add a focused
non-OK or synthetic buffering case that exercises the buffered.ok exclusion and
verifies existing request-log metadata remains unchanged.
🪄 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: cdea9032-d8c1-41ce-af15-de6b01ccaa10
📒 Files selected for processing (2)
src/server/responses/compact.tstests/responses-compaction-routing.test.ts
| describe("native compact usage reporting", () => { | ||
| test("the buffered upstream body fills the request log usage and stays intact for the client", async () => { | ||
| const config = { | ||
| defaultProvider: "openai-apikey", | ||
| providers: { | ||
| "openai-apikey": { | ||
| adapter: "openai-responses", | ||
| baseUrl: "https://api.openai.com/v1", | ||
| authMode: "key", | ||
| apiKey: "sk-test", | ||
| }, | ||
| }, | ||
| } as unknown as OcxConfig; | ||
| globalThis.fetch = (async () => jsonResponse(completedPayload("native summary"))) as typeof fetch; | ||
| const logCtx: RequestLogContext = { model: "", provider: "" }; | ||
| const previousUsageDebug = process.env.OPENCODEX_USAGE_DEBUG; | ||
| process.env.OPENCODEX_USAGE_DEBUG = "1"; | ||
| let response: Response; | ||
| try { | ||
| response = await handleResponsesCompact( | ||
| compactionRequest(baseCompactionBody({ model: "openai-apikey/gpt-5.5" })), | ||
| config, | ||
| logCtx, | ||
| ); | ||
| } finally { | ||
| if (previousUsageDebug === undefined) delete process.env.OPENCODEX_USAGE_DEBUG; | ||
| else process.env.OPENCODEX_USAGE_DEBUG = previousUsageDebug; | ||
| } | ||
| expect(response.status).toBe(200); | ||
| expect(await response.json()).toEqual(completedPayload("native summary")); | ||
| expect(logCtx.usage).toMatchObject({ inputTokens: 10, outputTokens: 5, totalTokens: 15 }); | ||
| // The compact body is replacement history; even with usage debug on it must | ||
| // never be sampled into the debug log. | ||
| expect(logCtx.usageDebugBodyKind).toBeUndefined(); | ||
| expect(logCtx.usageDebugBodySample).toBeUndefined(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Cover the remaining response-log branches.
This test checks usage and complete body preservation only. It does not exercise model or service_tier extraction in src/server/request-log.ts, Lines 489-504. It also does not verify the buffered.ok exclusion at src/server/responses/compact.ts, Line 513.
Add assertions for logCtx.resolvedModel and logCtx.responseServiceTier using response values that differ from the initial route metadata. Add a non-OK or synthetic buffering case and assert that existing request-log metadata remains unchanged.
As per path instructions, source behavior changes require focused regression coverage in tests/; cover these new branches here.
🤖 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 170 - 206, Extend
the “buffered upstream body” test around handleResponsesCompact to return
response data whose model and service_tier differ from the initial route
metadata, then assert logCtx.resolvedModel and logCtx.responseServiceTier are
populated from that response. Add a focused non-OK or synthetic buffering case
that exercises the buffered.ok exclusion and verifies existing request-log
metadata remains unchanged.
Source: Path instructions
|
Leave it as draft. Thanks. It doesnt need any more changes as you can see in juns comment on which you didn't respond to. |
3 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