Skip to content

fix: don't zod check ai gateway on no_mapping#5099

Merged
colegottdank merged 1 commit intomainfrom
connor/no-zod-on-nomapping
Oct 22, 2025
Merged

fix: don't zod check ai gateway on no_mapping#5099
colegottdank merged 1 commit intomainfrom
connor/no-zod-on-nomapping

Conversation

@connortbot
Copy link
Collaborator

No description provided.

@vercel
Copy link

vercel bot commented Oct 22, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

3 Skipped Deployments
Project Deployment Preview Comments Updated (UTC)
helicone Skipped Skipped Oct 22, 2025 8:15pm
helicone-bifrost Skipped Skipped Oct 22, 2025 8:15pm
helicone-eu Skipped Skipped Oct 22, 2025 8:15pm

@claude
Copy link
Contributor

claude bot commented Oct 22, 2025

Claude finished @connortbot's task —— View job


Code Review Analysis

Todo List:

  • Read the changed file to understand the modification
  • Analyze the change for potential issues
  • Provide detailed review feedback
  • Give confidence score and summary

Review Summary

Score: 7/10 - Good change with sound logic, but needs documentation and test coverage

Analysis

The change adds a condition to skip OpenAI payload validation when bodyMapping is set to NO_MAPPING. Here's the specific modification:

File: worker/src/lib/ai-gateway/SimpleAIGateway.ts:141

// Before
if (attempt.authType === "ptb") {

// After  
if (attempt.authType === "ptb" && this.requestWrapper.heliconeHeaders.gatewayConfig.bodyMapping !== "NO_MAPPING") {

Detailed Feedback

Positive Aspects

  1. Logical Consistency: The change aligns with the existing pattern in the codebase where NO_MAPPING bypasses response transformation (line 351-353). It makes sense to also bypass request validation when no mapping is expected.

  2. Performance Improvement: Skipping unnecessary Zod validation for NO_MAPPING requests reduces computational overhead.

  3. Targeted Fix: The change is minimal and surgical, only affecting the specific case where validation should be skipped.

⚠️ Areas for Improvement

  1. Missing Documentation: The change lacks inline comments explaining why validation is skipped for NO_MAPPING. Consider adding:

    // Skip OpenAI validation when NO_MAPPING is used since we're not transforming the request/response
    if (attempt.authType === "ptb" && this.requestWrapper.heliconeHeaders.gatewayConfig.bodyMapping !== "NO_MAPPING") {
  2. Test Coverage: No evidence of test cases covering this scenario. The change should include tests for:

    • PTB requests with NO_MAPPING should skip validation
    • PTB requests with OPENAI mapping should still validate
    • Invalid payloads with NO_MAPPING should still pass through (expected behavior)
  3. Error Handling Consideration: With validation bypassed, downstream errors from malformed requests may be less clear. This might be acceptable for NO_MAPPING use cases but should be documented.

🔍 Security & Safety

  • Low Risk: Bypassing validation for NO_MAPPING is acceptable since these requests are passed through without transformation
  • Auth Type Check: The ptb auth type check remains intact, preserving the security boundary

🚀 Suggestions for Enhancement

  1. Add JSDoc comments to the validation function explaining when it's used
  2. Consider logging when validation is skipped for debugging purposes
  3. Document the behavior in relevant API documentation

Code Quality Assessment

  • Readability: ✅ Clear and concise
  • Maintainability: ✅ Follows existing patterns
  • Performance: ✅ Improves efficiency
  • Security: ✅ No security concerns
  • Testing: ⚠️ Needs test coverage

Recommendation

This change is safe to merge but would benefit from documentation and test coverage. The logic is sound and aligns with the existing NO_MAPPING behavior pattern in the codebase.

@colegottdank colegottdank merged commit c675d03 into main Oct 22, 2025
9 checks passed
@colegottdank colegottdank deleted the connor/no-zod-on-nomapping branch October 22, 2025 20:18
Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Greptile Overview

Summary

Fixes validation logic to skip OpenAI schema validation when NO_MAPPING body mapping is configured. When users set the Helicone-Gateway-Body-Mapping: NO_MAPPING header, they're sending requests in the provider's native format (e.g., Anthropic format) rather than OpenAI format. The previous code would incorrectly validate these native-format requests against the OpenAI schema, causing validation errors for valid requests.

  • Prevents false validation errors when using provider-native request formats
  • Maintains validation for OpenAI-formatted requests
  • Aligns validation behavior with response mapping logic (which already respects NO_MAPPING)

Confidence Score: 5/5

  • This PR is safe to merge with minimal risk
  • The change is a simple, logical fix that adds an appropriate condition check. It addresses a clear bug where OpenAI validation was incorrectly applied to non-OpenAI request formats. The fix is consistent with existing NO_MAPPING handling in the response mapping logic (line 351-353). The condition is straightforward and doesn't introduce any edge cases or race conditions.
  • No files require special attention

Important Files Changed

File Analysis

Filename Score Overview
worker/src/lib/ai-gateway/SimpleAIGateway.ts 5/5 Added condition to skip OpenAI payload validation when NO_MAPPING is configured, preventing validation errors for non-OpenAI request formats

Sequence Diagram

sequenceDiagram
    participant Client
    participant SimpleAIGateway
    participant validateOpenAIChatPayload
    participant AttemptExecutor
    participant Provider

    Client->>SimpleAIGateway: Request with NO_MAPPING header
    Note over SimpleAIGateway: Parse request body
    
    SimpleAIGateway->>SimpleAIGateway: Build attempts (ptb auth)
    
    alt bodyMapping == "NO_MAPPING"
        Note over SimpleAIGateway: Skip OpenAI validation
        SimpleAIGateway->>AttemptExecutor: Execute attempt with native format
    else bodyMapping == "OPENAI" 
        SimpleAIGateway->>validateOpenAIChatPayload: Validate OpenAI schema
        alt Validation fails
            validateOpenAIChatPayload-->>SimpleAIGateway: Error
            SimpleAIGateway->>Client: Return validation error
        else Validation succeeds
            validateOpenAIChatPayload-->>SimpleAIGateway: OK
            SimpleAIGateway->>AttemptExecutor: Execute attempt
        end
    end
    
    AttemptExecutor->>Provider: Forward request
    Provider-->>AttemptExecutor: Response
    AttemptExecutor-->>SimpleAIGateway: Response
    
    alt bodyMapping == "NO_MAPPING"
        Note over SimpleAIGateway: Return response without mapping
    else bodyMapping == "OPENAI"
        Note over SimpleAIGateway: Map response to OpenAI format
    end
    
    SimpleAIGateway-->>Client: Final response
Loading

1 file reviewed, no comments

Edit Code Review Agent Settings | Greptile

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants