test(cloud): regression-lock the server-generated /v1/messages billing requestId (#12994 follow-up, #11588 class) - #13016
Conversation
…g requestId (#12994, #11588 class) #12994 fixed /v1/messages deriving the billing requestId (the affiliate- earnings dedupe sourceId) from the client-controllable idempotency key, but shipped without a regression test — the existing messages suites only assert the requestId is a stable truthy string, and they drive the internal handleStream hook, below the header-derivation seam. This adds the /v1/messages counterpart of the chat/completions coverage (chat-completions-optimistic-billing.test.ts "billing requestId is server-generated"): - drives the REAL route handler (app.request) with Idempotency-Key / X-Request-Id pinned AND the request-context ALS populated the way the bootstrap populates it (positive control asserts the pinned key IS visible to the route — under the pre-#12994 code these tests fail with requestId === the pinned key; verified by locally reverting the fix) - asserts the billUsage context requestId is a fresh server uuid, not the pinned client key, with the affiliate leg in play - asserts two real billed requests pinning the SAME client key get DIFFERENT billing requestIds (the #11588 forgery: the second request's cashable affiliate credit deduped away while its org charge landed) Deliberately does NOT mock @/lib/utils/credit-reservation (the abort suite in this package tests the real settler; a process-wide module mock here would strand it) — the fake reservation returned by the mocked reserveCredits feeds the real settler instead.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
I validated this locally on a rebased copy over current What passed:
Blocker before merge: the related one-process set in the PR description did not pass for me. Command: bun test packages/cloud/api/__tests__/messages-billing-requestid-server-generated.test.ts \
packages/cloud/api/__tests__/messages-abort-partial-settle.test.ts \
packages/cloud/api/__tests__/messages-reasoning-floor.test.ts \
packages/cloud/api/__tests__/shared-agent-messages-route.test.ts \
packages/cloud/api/__tests__/shared-agent-messages-stream.test.ts \
packages/cloud/api/__tests__/chat-completions-optimistic-billing.test.tsResult: 5 failures, all in
Given the PR explicitly calls out cross-file safety with |
|
Found a blocking test-isolation issue in the advertised cross-file safety run. Passed locally on
Failed locally: bun test \
packages/cloud/api/__tests__/messages-billing-requestid-server-generated.test.ts \
packages/cloud/api/__tests__/messages-abort-partial-settle.test.ts \
packages/cloud/api/__tests__/messages-reasoning-floor.test.ts \
packages/cloud/api/__tests__/shared-agent-messages-route.test.ts \
packages/cloud/api/__tests__/shared-agent-messages-stream.test.ts \
packages/cloud/api/__tests__/chat-completions-optimistic-billing.test.tsWhen the new test runs before Representative failures:
Please make the new test restore/isolate its module mocks strongly enough for the combined one-process run to pass; the PR body currently claims that cross-file safety pass, but this checkout reproduces the opposite order-dependent failure. |
2 participants
What
Follow-up to #12994, which merged the one-line fix without a regression test. This adds the /v1/messages counterpart of the chat/completions coverage (
chat-completions-optimistic-billing.test.ts→ billing requestId is server-generated, not copied from x-request-id): a route-level suite locking in that the billingrequestId— the affiliate-earnings dedupe sourceId — is server-generated, never the client-controllable idempotency key.Why
The existing /v1/messages suites only assert the billed requestId is a stable truthy string, and they drive the internal
handleStreamtest hook — below the header-derivation seam #12994 changed. Nothing failed if the route went back togetRequestIdempotencyKey() ?? crypto.randomUUID()(the #11588 forgery: pinX-Request-Id/Idempotency-Keyacross two real billed requests → the second cashable affiliate/creator credit dedupes away while both org charges land).How
app.request, not test hooks) with the pinned headers AND the request-context ALS populated exactly the way the bootstrap populates it. A positive control asserts the pinned key IS visible to the route viagetRequestIdempotencyKey(), so the test can't silently pass on an empty ALS.billUsagecontext requestId is a fresh server uuid ≠ the pinned client key, with the affiliate leg in play (x-affiliate-codethreaded through).mock.module@/lib/utils/credit-reservation:messages-abort-partial-settle.test.tstests the REAL settler and a process-wide registry replacement here strands it (verified — mocking it broke 5 abort tests when the files run in one process). The fake reservation returned by the mockedreserveCreditsfeeds the real settler instead.Verification (local, worktree at develop
1af5ec03ec)getRequestIdempotencyKey() ?? crypto.randomUUID()makes both tests fail withrequestId === "req-pinned-by-client-12994"— the suite genuinely locks the fix.messages-abort-partial-settle+messages-reasoning-floor+shared-agent-messages-route+shared-agent-messages-stream+chat-completions-optimistic-billingin ONE bun process, both orders: 29 pass / 0 fail.bunx @biomejs/biome checkclean on the new file.Test-only change; no product code touched.
— [cloud-security]