feat(request): transform input and tools format in requests #1174
Conversation
Enhance request handling by transforming input to messages and validating tools format. This improves compatibility with the expected request structure.
This reverts commit 728af08
WalkthroughA new middleware is added to the Changes
Sequence DiagramsequenceDiagram
participant Client
participant Middleware
participant Parser
participant Transformer
participant Next
Client->>Middleware: POST /v1/chat/completions
Middleware->>Parser: Parse JSON body
alt JSON Valid
Parser-->>Middleware: Parsed body
Middleware->>Transformer: Inspect for non-standard shapes
alt Transformation Needed
Transformer->>Transformer: Convert input→messages,<br/>wrap tools as functions
Transformer-->>Middleware: Transformed body
Middleware->>Middleware: Create new Request<br/>with transformed body
else No Transformation
Transformer-->>Middleware: Use original body
end
Middleware->>Next: next()
else JSON Parse Error
Parser-->>Middleware: Error + Log
Middleware->>Next: next()
end
Next-->>Client: Response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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.
Actionable comments posted: 1
🧹 Nitpick comments (4)
apps/gateway/src/app.ts (4)
112-120: Consider aligningtoolsValidwith thetype: "function"schema
toolsValidcurrently checks only that each tool has a"function"key, but not thattype === "function". Given the sharedtoolschema from@llmgateway/db:export const tool = z.object({ type: z.literal("function"), function: toolFunction, });you may get “valid” tools here that still fail downstream schema validation (e.g.,
{ function: {...} }withouttype). To keep behavior consistent across layers, consider tightening the validity check:- const toolsValid = - !hasTools || - (Array.isArray(bodyObj.tools) && - bodyObj.tools.every((tool: unknown) => { - return ( - typeof tool === "object" && tool !== null && "function" in tool - ); - })); + const toolsValid = + !hasTools || + (Array.isArray(bodyObj.tools) && + bodyObj.tools.every((tool: unknown) => { + return ( + typeof tool === "object" && + tool !== null && + "function" in tool && + // keep existing valid tools that already match the OpenAI shape + ("type" in tool ? (tool as any).type === "function" : true) + ); + }));or, more strictly, require
type: "function"when present or after your own transformation.Not strictly required for this PR to function, but it will reduce surprises if downstream schemas assume the stricter shape.
123-144: Verify assumptions aboutinputarray shape before treating it asmessagesFor array
input, you currently do:} else if (Array.isArray(bodyObj.input)) { // If input is already an array, assume it's messages bodyObj.messages = bodyObj.input; delete bodyObj.input; needsTransformation = true; }This assumes all callers send
inputas an array of message objects already compatible with your/v1/chat/completionshandler. If any client sendsinputasstring[]or some other lightweight shape, this will produce amessagesarray that likely fails downstream validation.Two options:
- Keep behavior but add a lightweight shape check (e.g. verify that elements are objects with
roleandcontentbefore treating as messages, otherwise leaveinputuntouched and skip transformation).- Document and enforce the contract: if this route is only supporting Cursor-style
inputarrays (message objects), confirming that no other producer sendsstring[]is enough.Given this is user‑facing behavior, I’d at least add a brief shape check or a comment clarifying the expected
inputarray structure.
184-189: Differentiate parse errors from transformation errors in loggingThe
catchblock logs everything as a failure to parse:logger.error( "Failed to parse request body for transformation", error instanceof Error ? error : new Error(String(error)), );But this
tryalso covers transformation logic (e.g., issues in the tools mapping), not justJSON.parse. That can make debugging noisy/ambiguous.Two small improvements:
- Narrow the
tryaroundJSON.parseif you only want this message for parse errors, or- Adjust the log message to something broader like
"Failed to process request body for transformation"and, optionally, include a hint about which phase failed if you later split the try/catch.Behavior-wise you’re already correctly falling back to the original request; this is just about clearer observability.
172-181: Confirm overridingc.req.rawis the preferred Hono patternCreating a new
Requestand assigning it toc.req.rawis a neat way to supply the transformed body to downstream handlers:const newReq = new Request(c.req.raw.url, { method: c.req.raw.method, headers: c.req.raw.headers, body: transformedBody, }); c.req.raw = newReq;Assuming
HonoRequestdelegates its body-reading methods tothis.rawon access, this should work as intended and keep middleware non-breaking.It’s worth double‑checking against Hono’s recommended patterns that mutating
c.req.rawin place is stable across runtimes (Node, edge, etc.). If not, a safer alternative would be to construct a newHonoRequestaroundnewReqand replacec.reqentirely.No change strictly required here if you’ve already exercised this on your supported platforms.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/gateway/src/app.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/gateway/src/app.ts (2)
packages/db/src/types.ts (1)
tool(18-21)packages/logger/src/index.ts (2)
logger(181-181)error(153-160)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: build / run
- GitHub Check: generate / run
- GitHub Check: lint / run
- GitHub Check: test / run
- GitHub Check: e2e-shards (5)
- GitHub Check: e2e-shards (3)
- GitHub Check: e2e-shards (2)
- GitHub Check: e2e-shards (1)
- GitHub Check: e2e-shards (4)
- GitHub Check: autofix
🔇 Additional comments (1)
apps/gateway/src/app.ts (1)
99-121: Overall middleware approach is sound and non-invasiveThe middleware stays scoped to
/v1/chat/completions, only touches POSTs, and falls back to the original request on any failure. Cloning the request body, doing transformations in-memory, and then re-wrapping into a newRequestbefore callingnext()is a clean pattern for backwards‑compatible adaptation.No blocking issues here from a control‑flow / safety perspective.
| // Transform tools format: wrap in function object | ||
| if (hasTools && !toolsValid) { | ||
| logger.info("Transforming tools format", { | ||
| userAgent: c.req.header("User-Agent"), | ||
| }); | ||
|
|
||
| bodyObj.tools = bodyObj.tools.map((tool: unknown) => { | ||
| if ( | ||
| typeof tool === "object" && | ||
| tool !== null && | ||
| !("function" in tool) | ||
| ) { | ||
| return { | ||
| type: "function", | ||
| function: tool, | ||
| }; | ||
| } | ||
| return tool; | ||
| }); | ||
| needsTransformation = true; | ||
| } | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
Guard against non-array tools to avoid unnecessary runtime errors
When tools is present but not an array, toolsValid becomes false, so hasTools && !toolsValid is true, and the code executes:
bodyObj.tools = bodyObj.tools.map((tool: unknown) => { ... });If bodyObj.tools is an object (or any non-array), this will throw (.map is not a function). The error is caught by the outer try/catch, but it will log on every such request and skip transformation.
A small guard avoids this:
- // Transform tools format: wrap in function object
- if (hasTools && !toolsValid) {
+ // Transform tools format: wrap in function object, only when tools is an array
+ if (hasTools && Array.isArray(bodyObj.tools) && !toolsValid) {
logger.info("Transforming tools format", {
userAgent: c.req.header("User-Agent"),
});
- bodyObj.tools = bodyObj.tools.map((tool: unknown) => {
+ bodyObj.tools = bodyObj.tools.map((tool: unknown) => {
if (
typeof tool === "object" &&
tool !== null &&
!("function" in tool)
) {
return {
type: "function",
function: tool,
};
}
return tool;
});
needsTransformation = true;
}For non-array tools, this will now simply skip transformation and proceed with the original body instead of throwing and logging.
I'll verify the review comment by examining the codebase to understand how toolsValid is determined and whether the suggested guard is necessary.
🏁 Script executed:
#!/bin/bash
# Find and examine the app.ts file
find . -name "app.ts" -path "*/gateway/*" | head -5Length of output: 25
🏁 Script executed:
#!/bin/bash
# Extract lines 140-180 to see full context around the reviewed code
sed -n '140,180p' apps/gateway/src/app.tsLength of output: 1001
🏁 Script executed:
#!/bin/bash
# Search for where toolsValid is defined/set
rg -n "toolsValid" apps/gateway/src/app.ts -B5 -A2Length of output: 523
🏁 Script executed:
#!/bin/bash
# Search for the validation logic that determines hasTools and toolsValid
rg -n "hasTools.*=|toolsValid.*=" apps/gateway/src/app.ts -B3 -A3Length of output: 309
🏁 Script executed:
#!/bin/bash
# Look for try-catch block around this code
sed -n '100,200p' apps/gateway/src/app.ts | grep -n "try\|catch" -A5 -B5Length of output: 606
🏁 Script executed:
#!/bin/bash
# Search for where tools is validated as an array
rg -n "Array.isArray.*tools|typeof.*tools" apps/gateway/src/app.ts -B2 -A2Length of output: 156
🏁 Script executed:
#!/bin/bash
# Get more context: show lines 1-200 to understand the full flow
head -200 apps/gateway/src/app.ts | tail -100Length of output: 2566
🏁 Script executed:
#!/bin/bash
# Look at the entire validation section
sed -n '120,180p' apps/gateway/src/app.tsLength of output: 1527
🏁 Script executed:
#!/bin/bash
# Search for all references to toolsValid to understand its purpose
rg -n "toolsValid" apps/gateway/src/app.tsLength of output: 63
🏁 Script executed:
#!/bin/bash
# Get a broader view of the code structure
sed -n '100,200p' apps/gateway/src/app.tsLength of output: 2619
🏁 Script executed:
#!/bin/bash
# Check what happens with tools validation
rg -n "tools" apps/gateway/src/app.ts -B1 -A1 | head -80Length of output: 532
🏁 Script executed:
#!/bin/bash
# Look at the validation logic more carefully
rg -nP "tools.*instanceof|tools.*Array|toolsValid\s*=" apps/gateway/src/app.ts -B3 -A3Length of output: 275
🏁 Script executed:
#!/bin/bash
# Examine if there's validation that ensures tools is an array
sed -n '130,175p' apps/gateway/src/app.tsLength of output: 1178
🏁 Script executed:
#!/bin/bash
# Check the type of tools validation
ast-grep --pattern 'if ($hasTools && !$toolsValid) { $$$ }'Length of output: 0
🏁 Script executed:
#!/bin/bash
# Simple extract to see the code clearly
cat apps/gateway/src/app.ts | sed -n '140,180p'Length of output: 1001
🏁 Script executed:
#!/bin/bash
# Check for any Array.isArray checks on tools before line 146
sed -n '1,145p' apps/gateway/src/app.ts | grep -n "Array.isArray\|tools\|toolsValid" | tail -20Length of output: 211
🏁 Script executed:
#!/bin/bash
# See where toolsValid is actually set to understand validation
sed -n '1,145p' apps/gateway/src/app.ts | grep -n "toolsValid\s*=" -B5 -A5Length of output: 408
🏁 Script executed:
#!/bin/bash
# Get the exact validation logic
rg -n "toolsValid" apps/gateway/src/app.ts -B10 | head -50Length of output: 792
🏁 Script executed:
#!/bin/bash
# Look at the full validation section
sed -n '80,160p' apps/gateway/src/app.tsLength of output: 2252
🏁 Script executed:
#!/bin/bash
# Extract all validation related to tools
rg -n "(tools|toolsValid)" apps/gateway/src/app.ts | head -30Length of output: 370
🏁 Script executed:
#!/bin/bash
# Get a comprehensive view from line 90 onwards
sed -n '90,200p' apps/gateway/src/app.tsLength of output: 2863
🏁 Script executed:
#!/bin/bash
# Focus on understanding toolsValid assignment
rg "toolsValid\s*=" apps/gateway/src/app.ts -B10 -A1Length of output: 380
🏁 Script executed:
#!/bin/bash
# Check what toolsValid check actually means
sed -n '80,200p' apps/gateway/src/app.ts | cat -nLength of output: 4014
🏁 Script executed:
#!/bin/bash
# Look for the exact toolsValid definition and usage
rg -n "toolsValid" apps/gateway/src/app.tsLength of output: 63
🏁 Script executed:
#!/bin/bash
# Simple line extraction
awk 'NR>=80 && NR<=200' apps/gateway/src/app.tsLength of output: 3167
🏁 Script executed:
#!/bin/bash
# Search for tools validation comprehensively
rg -nP "(hasTools|toolsValid|bodyObj\.tools)" apps/gateway/src/app.ts -B1 -A1 | head -100Length of output: 479
🏁 Script executed:
#!/bin/bash
# Find the complete validation logic
rg -nP "tools.*Array|toolsValid\s*=" apps/gateway/src/app.ts -C5Length of output: 408
🏁 Script executed:
#!/bin/bash
# Last attempt - directly show the problematic section and surrounding code
python3 << 'EOF'
with open('apps/gateway/src/app.ts', 'r') as f:
lines = f.readlines()
for i in range(max(0, 80), min(len(lines), 200)):
print(f"{i+1:3d}: {lines[i]}", end='')
EOFLength of output: 3752
🏁 Script executed:
#!/bin/bash
# Check the validation chain for tools
python3 << 'EOF'
with open('apps/gateway/src/app.ts', 'r') as f:
content = f.read()
# Find lines with toolsValid or hasTools
for i, line in enumerate(content.split('\n')[80:200], start=81):
if 'toolsValid' in line or 'hasTools' in line or ('tools' in line and 'map' in line):
print(f"{i:3d}: {line}")
EOFLength of output: 196
🏁 Script executed:
#!/bin/bash
# Extract validation logic to understand if tools is guaranteed to be array
sed -n '100,180p' apps/gateway/src/app.ts | cat -nLength of output: 2748
🏁 Script executed:
#!/bin/bash
# Look at where toolsValid becomes false
python3 << 'EOF'
with open('apps/gateway/src/app.ts', 'r') as f:
lines = f.readlines()
# Show lines 100-180 to understand the flow
for i in range(99, min(len(lines), 180)):
print(f"{i+1:3d}: {lines[i]}", end='')
EOFLength of output: 2586
🏁 Script executed:
#!/bin/bash
# Check if we can see the validation logic for tools being an array
rg -nP "\.isArray\(.*tools\)|Array\.isArray" apps/gateway/src/app.ts -C3Length of output: 546
🏁 Script executed:
#!/bin/bash
# Final comprehensive check
python3 << 'EOF'
import re
with open('apps/gateway/src/app.ts', 'r') as f:
lines = f.readlines()
print("=== Full context 80-180 ===")
for i in range(79, min(len(lines), 180)):
print(f"{i+1:3d}: {lines[i]}", end='')
print("\n\n=== Searching for toolsValid definition ===")
full_text = ''.join(lines)
matches = re.finditer(r'toolsValid\s*=.*', full_text)
for match in matches:
# Find line number
line_num = full_text[:match.start()].count('\n') + 1
print(f"Line {line_num}: {match.group()}")
EOFLength of output: 3331
🏁 Script executed:
#!/bin/bash
# Find tools validation definition
python3 << 'EOF'
with open('apps/gateway/src/app.ts', 'r') as f:
lines = f.readlines()
for i, line in enumerate(lines, 1):
if 'toolsValid' in line:
# Print context around it
start = max(0, i-5)
end = min(len(lines), i+5)
print(f"\n=== Context around line {i} ===")
for j in range(start-1, end):
print(f"{j+1:3d}: {lines[j]}", end='')
EOFLength of output: 832
🏁 Script executed:
#!/bin/bash
# Show lines around the error location
python3 << 'EOF'
with open('apps/gateway/src/app.ts', 'r') as f:
lines = f.readlines()
# Show lines 100-200 with line numbers
for i in range(99, min(len(lines), 200)):
print(f"{i+1}: {lines[i]}", end='')
EOFLength of output: 3124
🏁 Script executed:
#!/bin/bash
# Check validation for tools - specifically looking for the part where toolsValid is set
python3 << 'EOF'
with open('apps/gateway/src/app.ts', 'r') as f:
content = f.read()
# Find the section with tools validation
start_idx = content.find('toolsValid')
if start_idx != -1:
# Get 500 chars before and 1000 chars after
snippet = content[max(0, start_idx-1000):start_idx+1500]
print(snippet)
EOFLength of output: 2500
🏁 Script executed:
#!/bin/bash
# Extract just the validation section
python3 << 'EOF'
with open('apps/gateway/src/app.ts', 'r') as f:
lines = f.readlines()
# Look for lines 120-170
print("".join(lines[119:170]))
EOFLength of output: 1269
🏁 Script executed:
#!/bin/bash
# Review entire validation logic starting from line 100
awk 'NR>=100 && NR<=175' apps/gateway/src/app.ts | nl -v 100Length of output: 40
Add Array.isArray(bodyObj.tools) guard to prevent runtime errors on non-array tools
The review comment is correct. When tools exists but is not an array, toolsValid evaluates to false (because Array.isArray(bodyObj.tools) is false in the AND expression), triggering the condition hasTools && !toolsValid at line 147. This causes .map() to be called on a non-array, throwing an error that's caught by the outer try-catch and logged unnecessarily.
The suggested fix is valid and prevents this by adding the Array.isArray(bodyObj.tools) guard:
- // Transform tools format: wrap in function object
- if (hasTools && !toolsValid) {
+ // Transform tools format: wrap in function object, only when tools is an array
+ if (hasTools && Array.isArray(bodyObj.tools) && !toolsValid) {This ensures the transformation only runs when tools is actually an array, allowing the original body to pass through for non-array tools values.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Transform tools format: wrap in function object | |
| if (hasTools && !toolsValid) { | |
| logger.info("Transforming tools format", { | |
| userAgent: c.req.header("User-Agent"), | |
| }); | |
| bodyObj.tools = bodyObj.tools.map((tool: unknown) => { | |
| if ( | |
| typeof tool === "object" && | |
| tool !== null && | |
| !("function" in tool) | |
| ) { | |
| return { | |
| type: "function", | |
| function: tool, | |
| }; | |
| } | |
| return tool; | |
| }); | |
| needsTransformation = true; | |
| } | |
| // Transform tools format: wrap in function object, only when tools is an array | |
| if (hasTools && Array.isArray(bodyObj.tools) && !toolsValid) { | |
| logger.info("Transforming tools format", { | |
| userAgent: c.req.header("User-Agent"), | |
| }); | |
| bodyObj.tools = bodyObj.tools.map((tool: unknown) => { | |
| if ( | |
| typeof tool === "object" && | |
| tool !== null && | |
| !("function" in tool) | |
| ) { | |
| return { | |
| type: "function", | |
| function: tool, | |
| }; | |
| } | |
| return tool; | |
| }); | |
| needsTransformation = true; | |
| } |
🤖 Prompt for AI Agents
In apps/gateway/src/app.ts around lines 146 to 167, the transformation block can
call .map() on bodyObj.tools when tools exists but is not an array, causing a
runtime error; update the if condition to only run the mapping when
Array.isArray(bodyObj.tools) is true (i.e., change the guard to hasTools &&
Array.isArray(bodyObj.tools) && !toolsValid) so non-array tools are left
untouched and transformation only runs on actual arrays, and leave
needsTransformation set as before.
Summary by CodeRabbit
New Features
Bug Fixes