Repository navigation
fix(gateway): log missing tool_call_id as client_error - #2748
Conversation
The Responses API transform threw a plain Error when a tool message lacked tool_call_id, which app.onError mapped to a generic 500 with no log row. Introduce a typed MissingToolCallIdError so the gateway maps it to a 400 and writes a client_error insertLog entry, surfacing the rejected request in activity history like the other pre-upstream input errors. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
WalkthroughIntroduces an exported ChangesRequestError typed validation error
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Replace MissingToolCallIdError with a generic RequestError carrying a statusCode (default 400) so it can be reused for other pre-upstream request validation failures. The gateway maps it to the carried status and writes a client_error log row. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/actions/src/prepare-request-body.ts (1)
39-43: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winConstrain
RequestError.statusCodeto the client-error range.
statusCodeis currently accepted verbatim, then used for response rendering/log classification downstream. Guarding to400-499keeps this type aligned with pre-upstream client validation failures.Suggested change
export class RequestError extends Error { public readonly statusCode: number; public constructor(message: string, statusCode = 400) { super(message); this.name = "RequestError"; - this.statusCode = statusCode; + if (!Number.isInteger(statusCode) || statusCode < 400 || statusCode > 499) { + throw new RangeError( + "RequestError statusCode must be an integer between 400 and 499", + ); + } + this.statusCode = statusCode; } }🤖 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 `@packages/actions/src/prepare-request-body.ts` around lines 39 - 43, The RequestError constructor accepts any statusCode value without validation, but it should be constrained to the client-error range of 400-499 to ensure proper response rendering and log classification downstream. Add validation in the RequestError constructor to ensure the statusCode parameter is within the 400-499 range, either by rejecting values outside this range or defaulting/normalizing invalid values to a valid client-error code like 400.packages/actions/src/prepare-request-body.spec.ts (1)
511-545: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAssert
statusCodein addition to error type.The regression currently checks only the class instance. Add a
statusCode: 400assertion to lock the new contract introduced byRequestError.🤖 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 `@packages/actions/src/prepare-request-body.spec.ts` around lines 511 - 545, The test function "should throw a typed RequestError for tool messages without tool_call_id" currently only validates that the thrown error is an instance of RequestError using toBeInstanceOf. To properly lock in the contract, add an additional assertion to verify that the error has a statusCode property set to 400. Update the expect block to check both the error type and the statusCode value together.
🤖 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.
Nitpick comments:
In `@packages/actions/src/prepare-request-body.spec.ts`:
- Around line 511-545: The test function "should throw a typed RequestError for
tool messages without tool_call_id" currently only validates that the thrown
error is an instance of RequestError using toBeInstanceOf. To properly lock in
the contract, add an additional assertion to verify that the error has a
statusCode property set to 400. Update the expect block to check both the error
type and the statusCode value together.
In `@packages/actions/src/prepare-request-body.ts`:
- Around line 39-43: The RequestError constructor accepts any statusCode value
without validation, but it should be constrained to the client-error range of
400-499 to ensure proper response rendering and log classification downstream.
Add validation in the RequestError constructor to ensure the statusCode
parameter is within the 400-499 range, either by rejecting values outside this
range or defaulting/normalizing invalid values to a valid client-error code like
400.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 1abd7be6-5d86-4f06-b87f-45dd7c7552d5
📒 Files selected for processing (4)
apps/gateway/src/app.tsapps/gateway/src/chat/chat.tspackages/actions/src/prepare-request-body.spec.tspackages/actions/src/prepare-request-body.ts
Tool result messages are converted to Responses API function_call_output items, which require a call_id referencing a prior function_call. #2748 made the missing-tool_call_id case a clean 400 (RequestError) instead of a 500, but still rejected recoverable requests. The Chat Completions spec requires tool_call_id on tool messages, but real callers omit it: legacy `function` role results carry only `name`, and some clients drop the id when replaying history. Recover the id from preceding unmatched function calls, but only when the pairing is unambiguous (explicit id, unique name match, or exactly one unmatched call); ambiguous cases throw RequestError rather than risk attaching a result to the wrong call. Also handle the legacy `function` role. Reuses the existing RequestError (no new error class / wiring). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Problem
When a tool message reached the OpenAI Responses API transform without a
tool_call_id,transformMessagesForResponsesApithrew a plainError.app.onErrormapped this to a generic 500, and crucially no log row was written, so the rejected request never appeared in the user's activity history.Fix
Introduce a reusable typed
RequestErrorinprepare-request-body.ts(exported via the package barrel) for invalid client requests detected before hitting the upstream provider. It carries astatusCode(default 400) so it can be reused for other similar pre-upstream validation failures.RequestErrorfor the missingtool_call_idcase.chat.ts, include it in theprepareRequestBodycatch block so aclient_errorinsertLogentry is written, using the error'sstatusCode.app.ts, map it to the carried status code instead of a generic 500.The rejected request now shows up in history as a client error, like the other pre-upstream input validation errors.
Test
Added a unit test asserting
prepareRequestBodyrejects withRequestErrorfor a tool message lackingtool_call_idunder the Responses API.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests