fix: surface spend-precheck denial as RATE_LIMITED - #3749
Conversation
When the Convex /stream endpoint denies a model call via spend-precheck
it returns 200 OK with Content-Type: application/json and a body like
{ok:false, code:"user_rate_limit", isRetryable:true, retryAfter:N}.
Two seams failed to catch this:
1. processOneStep in mcpjam-stream-handler.ts only checked !res.ok or
!res.body, so a 200 OK JSON body passed through to processStream.
processStream saw no SSE events, produced empty contentParts, and the
agentic loop completed "successfully" with runSucceeded=true.
2. agent.ts checked !result.turnTrace to detect engine failures, but
because runSucceeded was true, onConversationComplete fired and set
capturedTurnTrace -- so result.turnTrace was defined even though
onEngineError had fired with the denial payload.
Fix:
- In processOneStep, treat a 200 OK with Content-Type: application/json
as a non-stream denial (same error path as non-OK responses).
onEngineError fires with the parsed body and httpStatus:200.
- In agent.ts, check lastEngineError in addition to !result.turnTrace
when deciding whether to return an error. A precheck denial fires
onEngineError with code:"user_rate_limit", which also now maps to
RATE_LIMITED via the rateLimitCodes set (covering both the 429-status
and the 200-with-JSON-code paths).
Tests: new cases in mcpjam-stream-handler.test.ts (engine-level) and
agent.test.ts (route-level RATE_LIMITED mapping).
Fixes MCPJam#3708
Signed-off-by: Christian Sidak <christian@sentineltech.eu>
Signed-off-by: Christian-Sidak <61099993+Christian-Sidak@users.noreply.github.com>
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
dosubot
Bot
added
size:S
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe stream handler now treats HTTP 200 JSON responses as denial errors and preserves their code, status, body, message, prompt, and step data. The agent route now fails turns that contain recorded engine errors, even when a trace exists. It maps 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
mcpjam-inspector/server/routes/v1/agent.ts (1)
1141-1161: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPreserve spend-precheck retry metadata in the RATE_LIMITED response.
onEngineErrorcurrently only acceptsmessage,code, andhttpStatus, so{ code: "user_rate_limit", retryAfter: 60, ... }from the 200 OK JSON denial is stored and emitted withoutretryAfter. Store that metadata, enrichlastEngineErrorwith it, and return it in theRATE_LIMITEDdetails before callingv1Error. Also cover this in the spend-precheck test assertions.🤖 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 `@mcpjam-inspector/server/routes/v1/agent.ts` around lines 1141 - 1161, Update mcpjam-inspector/server/routes/v1/agent.ts:1141-1161 and the related error-handling flow around onEngineError to retain retryAfter and other spend-precheck metadata, enrich lastEngineError, and include that metadata in RATE_LIMITED errorDetails before calling v1Error. Update assertions in mcpjam-inspector/server/utils/__tests__/mcpjam-stream-handler.test.ts:3001-3012 and mcpjam-inspector/server/routes/v1/__tests__/agent.test.ts:412-429 to verify retryAfter is preserved in the response.
🤖 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.
Outside diff comments:
In `@mcpjam-inspector/server/routes/v1/agent.ts`:
- Around line 1141-1161: Update
mcpjam-inspector/server/routes/v1/agent.ts:1141-1161 and the related
error-handling flow around onEngineError to retain retryAfter and other
spend-precheck metadata, enrich lastEngineError, and include that metadata in
RATE_LIMITED errorDetails before calling v1Error. Update assertions in
mcpjam-inspector/server/utils/__tests__/mcpjam-stream-handler.test.ts:3001-3012
and mcpjam-inspector/server/routes/v1/__tests__/agent.test.ts:412-429 to verify
retryAfter is preserved in the response.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d5561c91-1b03-4864-996a-d11f4d341ad7
📒 Files selected for processing (4)
mcpjam-inspector/server/routes/v1/__tests__/agent.test.tsmcpjam-inspector/server/routes/v1/agent.tsmcpjam-inspector/server/utils/__tests__/mcpjam-stream-handler.test.tsmcpjam-inspector/server/utils/mcpjam-stream-handler.ts
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 4 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
… fixes)
parseEngineErrorBody only extracted `code` when the body had an `error`
field, but the spend-precheck denial is {ok:false, code:"user_rate_limit",
...} — so the agent route's new rateLimitCodes check never saw a code and
the 429 mapping only worked because the raw body text embedded in the
fallback message happened to match classifyFailure's rate-limit regex.
Now the top-level `code` is surfaced even without `error`.
Also tighten both new tests so they exercise the fix:
- engine test asserts event.code === "user_rate_limit"
- route test keeps turnTrace PRESENT (the actual MCPJam#3708 scenario — the old
!turnTrace check alone never fired) and uses a message the regex cannot
catch, so it fails without the lastEngineError + code-set branches
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
dosubot
Bot
added
size:M
`res.headers.get(...)` optional-chains the RESULT of `.get`, not `headers`
itself. Callers that stub a Response without `headers` — the eval runner's
`backendStreamResponse()` returns {ok, status, statusText, body, text} — hit
a TypeError that the outer catch turns into a failed turn, breaking 7 tests
in evals-runner.test.ts and runner-parity.test.ts against current main.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
hey @Christian-Sidak, thanks for fixing this! Added a commit on top and merging! Anything else we can help you with? |
3 participants
Summary
When the Convex
/streamendpoint denies a model call via the spend-precheck it returns200 OKwithContent-Type: application/jsonand a body like{ok:false, code:"user_rate_limit", isRetryable:true, retryAfter:N}. Two seams in the engine+route pipeline silently swallowed this, producing an empty-reply success envelope instead of a proper error:processOneSteponly checked!res.ok || !res.body, so a200 OKJSON body fell through toprocessStream, which found no SSE events and returned emptycontentParts. The agentic loop completed withrunSucceeded=true.agent.tschecked!result.turnTraceto detect failures, but becauserunSucceededwas true,onConversationCompletefired and setcapturedTurnTrace, soresult.turnTracewas defined even thoughonEngineErrorhad fired with the denial payload.Fix
processOneStep(mcpjam-stream-handler.ts), detectContent-Type: application/jsonon a200 OKresponse as a non-stream denial and route it through the same error path as non-OK responses.onEngineErrornow fires with the parsed body andhttpStatus:200.agent.ts, checklastEngineErroralongside!result.turnTracewhen deciding whether to return an error. TherateLimitCodesset covers both thehttpStatus === 429path and the200 OKpath wherecodeis"user_rate_limit"or"org_rate_limit".Test plan
mcpjam-stream-handler.test.ts: feeds a200 OK+application/jsonresponse, assertsonEngineErrorfires withhttpStatus:200and the raw body.agent.test.ts: mocksonEngineErrorwithcode:"user_rate_limit"andhttpStatus:200, asserts the route returns429 RATE_LIMITED.Fixes #3708
Summary by cubic
Maps Convex
/streamspend-precheck denials (200 OK +application/json) to 429 RATE_LIMITED instead of an empty 200. Fixes #3708 so clients consistently see rate limit errors.mcpjam-stream-handler.ts, treat 200 OK withapplication/jsonas a non-stream denial; callonEngineErrorwith parsed body andhttpStatus: 200. Guardres.headersin the content-type check to avoid crashes with stubbed responses.agent.ts, also branch onlastEngineError(even whenturnTraceexists) and mapuser_rate_limit/org_rate_limitto 429.parseEngineErrorBody, surface top-levelcodefrom JSON bodies without anerrorfield so rate-limit mapping works reliably.Written for commit 3c001e0. Summary will update on new commits.