[codex] centralize gateway paid request flow#168
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
✨ 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 |
|
@codex @claude please perform a full pre-merge review. Please inspect:
Review focus:
Please report blockers first with severity and exact file/line references. |
|
Claude finished @AnkanMisra's task in 3m 10s —— View job Pre-merge Review:
|
| Scenario | Old code | New code |
|---|---|---|
| Missing sig/nonce | c.JSON(402, ...) |
c.JSON(http.StatusPaymentRequired, ...) = 402 ✅ |
| Verifier timeout | respondError(c, 504, ...) |
respondError(c, http.StatusGatewayTimeout, ...) = 504 ✅ |
| Verifier unavailable | respondError(c, 502, ...) |
respondError(c, http.StatusBadGateway, ...) = 502 ✅ |
| Invalid sig / replay / mismatch | respondVerificationFailure(c, verifyResp) |
unchanged ✅ |
respondVerificationFailure → verifierFailureResponse is untouched and correctly maps nonce_already_used→409, chain_id_mismatch→400, invalid_signature→403.
Verifier Timeout / Unavailable / Replay Mappings
All three paths route through verifyPaidRequest() identically for cache-hit and cache-miss:
- Timeout:
errors.Is(err, context.DeadlineExceeded)→504 verifier_timeout✅ - Unavailable (non-timeout error or non-200 non-business status):
502 verification_unavailable✅ - Replay (
nonce_already_used): caught byisVerifierBusinessRejectioninverifyPayment, returned as a validVerifyResponse{IsValid:false}, then dispatched throughrespondVerificationFailure→409 nonce_already_used✅
The error path for a 5xx from the verifier with non-JSON body correctly falls through to verification_unavailable without leaking body content — confirmed by TestCacheHitMapsVerifierUnavailable. ✅
Receipt Generation on Cached Responses
sendPaidResult() is a thin wrapper:
// payment_flow.go:81-89
func sendPaidResult(c *gin.Context, payment *verifiedPayment, requestBody []byte, result string) error {
if payment == nil { ... }
return generateAndSendReceipt(c, payment.PaymentContext, payment.RecoveredAddress, requestBody, result)
}The cache-hit path in cache.go:135 calls sendPaidResult(c, verified, requestBody, cached.Result), passing the cached AI result as the result argument — receipt will hash the cached response body, which is correct since the cache key guarantees the same text produces the same result. ✅
TestCacheHitSuccessEmitsReceiptHeader confirms X-402-Receipt is present and the response body carries "result": "cached summary". ✅
Low-Severity Issues
1. Dead code: nil guard on verifyResp is unreachable
payment_flow.go:46-49:
if verifyResp == nil {
respondError(c, http.StatusBadGateway, "verification_unavailable", fmt.Errorf("missing verifier response"))
return nil, false
}verifyPayment() never returns (nil, *, nil) — it either returns an error (caught at line 36) or a non-nil *VerifyResponse. This guard is permanently unreachable. It's harmless but adds dead code. Fix this →
2. Stale TODO comments in main.go
main.go:407-414 still contains comments referencing generateAndSendReceipt that predate this refactor:
// generateAndSendReceipt sends error response if it fails?
// No, it returns error, we might have already written status if we aren't careful.
// Let's implement generateAndSendReceipt to handle sending response.These should be removed — the behavior is now documented by sendPaidResult's implementation. Fix this →
3. Sleep-based timeout test is timing-sensitive
payment_flow_test.go:69-88 (TestVerifyPaidRequestMapsVerifierTimeout) and errors_test.go:210-229 (TestCacheHitMapsVerifierTimeout) both use time.Sleep(1500ms) with a 1s verifier timeout. The 500ms margin is tight on loaded CI runners and could cause intermittent failures. Consider increasing the sleep to 3s or the margin to 2s.
4. No test for sendPaidResult(nil) guard
The nil-payment guard in sendPaidResult (payment_flow.go:83-86) is never reachable from verifyPaidRequest, but it could be exercised by a direct unit test. Low priority since the guard itself cannot be triggered through normal call paths.
Test Coverage Assessment
New tests cover the critical paths:
| Test | Path covered |
|---|---|
TestVerifyPaidRequestWritesPaymentChallengeForMissingHeaders |
402 challenge completeness |
TestVerifyPaidRequestReturnsVerifiedPayment |
Happy path |
TestVerifyPaidRequestMapsVerifierTimeout |
504 from verifier timeout |
TestVerifyPaidRequestRequiresRecoveredAddress |
502 on empty recovered_address |
TestCacheHitMapsVerifierNonceReplay |
409 replay via cache-hit |
TestCacheHitMapsVerifierTimeout |
504 via cache-hit path |
TestCacheHitMapsVerifierUnavailable |
502 via cache-hit + no sensitive leak |
TestCacheHitSuccessEmitsReceiptHeader |
Receipt header present on cache-hit |
Coverage is solid. The matrix of (cache-hit, cache-miss) × (timeout, unavailable, replay, success) is fully covered.
CONTEXT.md
Accurate and useful. One minor gap: CONTEXT.md defines Verifier Result but the domain vocabulary in errors.go handles several error codes (timestamp_expired, timestamp_future, timestamp_missing, chain_id_mismatch) that aren't mentioned. Worth adding a note that error_code values have defined HTTP mappings in errors.go:verifierFailureResponse. Non-blocking.
Verdict
Approve with minor cleanup. The refactor is clean, behavior-preserving, and well-tested. The three low-severity items (dead nil-guard, stale comments, timing margin) are all cosmetic — none affect correctness or safety. Safe to merge.
![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: 6742ffd9ae
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| verified, ok := verifyPaidRequest(c) | ||
| if !ok { | ||
| return | ||
| } |
There was a problem hiding this comment.
For cache-disabled requests, or any route path where CacheMiddleware is bypassed because Redis is unavailable, this now calls the verifier before reading and validating the JSON body. Previously handleSummarize rejected malformed JSON, empty text, and oversized bodies before verifyPayment; after this change a valid signed retry can have its nonce marked used and still receive a 400/413 with no receipt, while an invalid signature on a malformed request returns a verifier error instead of the public request-body error. Please keep the shared verifier call after the request-body validation on the cache-miss/non-cache path, while preserving the cache-hit pre-serve verification.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 018a76e. handleSummarize now keeps missing-header 402 challenges early, but validates signed cache-miss/non-cache request bodies before calling the verifier. Added TestHandleSummarizeValidatesSignedBodyBeforeVerifier for malformed JSON and empty text, asserting the verifier is not called.
AnkanMisra
force-pushed
the
codex/gateway-x402-flow-module
branch
from
6742ffd to
2c7dbd9
Compare
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Co-authored-by: codex <codex@users.noreply.github.com>
Co-authored-by: codex <codex@users.noreply.github.com>
AnkanMisra
force-pushed
the
codex/gateway-x402-flow-module
branch
from
2c7dbd9 to
881c6fc
Compare
|
@codex review @claude please re-review the latest head after the PR branch was rebased onto current Current head: Please focus on:
|
|
Claude finished @AnkanMisra's task in 2m 35s —— View job Re-review:
|
Co-authored-by: codex <codex@users.noreply.github.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 881c6fcb73
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| timestampValue, err := strconv.ParseUint(timestampHeader, 10, 64) | ||
| if err != nil || timestampValue == 0 { | ||
| respondError(c, 400, "invalid_timestamp", fmt.Errorf("invalid X-402-Timestamp header")) | ||
| verified, ok := verifyPaidRequest(c) |
There was a problem hiding this comment.
When Redis caching is disabled or redisClient is nil, CacheMiddleware skips body reading entirely, so this new early verification runs before the handler enforces the 10MB body limit. A signed retry with a valid signature and an oversized body will be accepted by the verifier and claim the nonce, then the gateway returns 413 while producing no AI result or receipt; the pre-refactor code read and rejected body read/size failures before calling verifyPayment. Please keep the body read/limit failure path ahead of payment verification for the no-cache/cache-miss handler path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Covered in 85f3af1. TestHandleSummarizeValidatesSignedBodyBeforeVerifier now includes a signed body over 10MB, expects HTTP 413, and asserts verifierCalls == 0, so the oversized-body path is locked before payment verification.
|
@codex review @claude please re-review latest head |
|
Claude finished @AnkanMisra's task in 2m 14s —— View job Re-review:
|
| Item | Status |
|---|---|
Dead nil-guard on verifyResp |
Removed in 881c6fc ✅ |
Stale TODO comments in main.go |
Removed in 881c6fc ✅ |
withSlowVerifier 3s margin (vs old 1.5s sleep) |
Present, using select { case <-r.Context().Done() } ✅ |
Minor Gap: No Regression Test for Oversized Body → No Verifier Call
The code correctly rejects a 10MB+ body (main.go:369-378) before verifyPaidRequest, but TestHandleSummarizeValidatesSignedBodyBeforeVerifier only covers malformed JSON and empty text — not the 413 path. The fix is correct in code; the test coverage gap is P3. Fix this →
Double-Check Pattern Note (Non-Blocking)
The missing-header early-exit at main.go:354-357 calls verifyPaidRequest(c) with its return values discarded:
if c.GetHeader("X-402-Signature") == "" || c.GetHeader("X-402-Nonce") == "" {
verifyPaidRequest(c) // writes 402, return values ignored
return
}This is correct and Go-idiomatic for using a function's side-effect only. It centralizes the challenge payload in verifyPaidRequest rather than duplicating it. No issue.
Verdict
Approve. The body validation ordering fix is correct and complete. The regression test is deterministic and proves the nonce is not consumed on malformed or empty bodies. All prior cleanup items remain resolved. The oversized-body test gap is P3 and can be addressed in a follow-up.
|
Codex Review: Didn't find any major issues. Can't wait for the next one! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Co-authored-by: codex <codex@users.noreply.github.com>
1 participant
Summary
CONTEXT.mddomain vocabulary for Payment Context, Signed Retry, Paid Request, Receipt, Receipt Store, and Response Cache.Type Of Change
Affected Areas
gateway/)verifier/)web/)tests/,run_e2e.sh)bench/)deploy/, Docker, env, workflows)Contributor Checklist
Verification
List the exact commands you ran and their result.
Notes For Reviewers
@codex @claude please review this PR deeply before merge.
Focus areas:
There are no intended public API, header, status-code, verifier, or web-client behavior changes.