Skip to content

Conversation

sukumargaonkar
Copy link
Contributor

@sukumargaonkar sukumargaonkar commented Oct 21, 2025

Description
This PR removes the wrapping double-quotes returned by GCP when using guided_regex

GCP doesn't natively support REGEX response modes, so we instead express them as json schema.
This causes the response to be wrapped in double-quotes.
E.g. "positive" (the double-quotes at the start and end are unwanted)

@codecov-commenter
Copy link

codecov-commenter commented Oct 21, 2025

Codecov Report

❌ Patch coverage is 90.62500% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 78.63%. Comparing base (f134ecd) to head (06983d4).

Files with missing lines Patch % Lines
internal/extproc/translator/gemini_helper.go 89.28% 3 Missing ⚠️

❌ Your project status has failed because the head coverage (78.63%) is below the target coverage (86.00%). You can increase the head coverage or adjust the target coverage.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1402      +/-   ##
==========================================
+ Coverage   78.61%   78.63%   +0.02%     
==========================================
  Files         139      139              
  Lines       13745    13763      +18     
==========================================
+ Hits        10805    10823      +18     
  Misses       2285     2285              
  Partials      655      655              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@sukumargaonkar sukumargaonkar marked this pull request as ready for review October 21, 2025 19:20
@sukumargaonkar sukumargaonkar requested a review from a team as a code owner October 21, 2025 19:20
// This causes the response to be wrapped in double-quotes.
// E.g. `"positive"` (the double-quotes at the start and end are unwanted)
// Here we remove the wrapping double-quotes.
if len(part.Text) > 2 && part.Text[0] == '"' && part.Text[len(part.Text)-1] == '"' {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm curious whether there's a reason to not use strings.Trim() here: https://pkg.go.dev/strings#Trim

Copy link
Contributor Author

@sukumargaonkar sukumargaonkar Oct 22, 2025

Choose a reason for hiding this comment

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

nice catch,
mainly being defensive for scenarios where first and last words are quoted but not connected
"ERROR" Unable to connect to database "DatabaseModule"
trim would remove more than intended
added a test case for this
thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

I see... makes sense. TrimPrefix(), TrimSuffix() can be used to trim exact strings so it'd work to limit to the single char. https://pkg.go.dev/strings#TrimPrefix

Copy link
Member

Choose a reason for hiding this comment

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

+1 to strings.Trim*

Copy link
Member

Choose a reason for hiding this comment

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

@sukumargaonkar i think this is the only pending unresolved comment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed in c73f6aa

Copy link
Member

Choose a reason for hiding this comment

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

i don't think this if conditions is necessary as strings.Trim* is noop if not applicable

TrimPrefix returns s without the provided leading prefix string. If s doesn't start with prefix, s is returned unchanged.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

addressed in f2d8620

@sukumargaonkar
Copy link
Contributor Author

/retest

@sukumargaonkar
Copy link
Contributor Author

The ubuntu based CI failed but macOS based passed

error in ubuntu based CI
https://github.com/envoyproxy/ai-gateway/actions/runs/18718793788/job/53391903589?pr=1402

--- FAIL: TestRun_enabled (0.00s)
    pprof_test.go:33: 
        	Error Trace:	/home/runner/work/ai-gateway/ai-gateway/internal/pprof/pprof_test.go:33
        	Error:      	Received unexpected error:
        	            	Get "http://localhost:6060/debug/pprof/cmdline": dial tcp [::1]:6060: connect: connection refused
        	Test:       	TestRun_enabled

seems unrelated to this PR. maybe @mathetake knows more

@mathetake
Copy link
Member

yeah sorry you can ignore the flake

Comment on lines 36 to 40
ResponseModeNone ResponseMode = "NONE"
ResponseModeText ResponseMode = "TEXT"
ResponseModeJSON ResponseMode = "JSON"
ResponseModeEnum ResponseMode = "ENUM"
ResponseModeRegex ResponseMode = "REGEX"
Copy link
Member

Choose a reason for hiding this comment

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

unexport

Copy link
Member

@mathetake mathetake Oct 22, 2025

Choose a reason for hiding this comment

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

also prefix gemini*

Copy link
Contributor Author

Choose a reason for hiding this comment

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

addressed in f871ff7

)

// ResponseMode represents the type of response mode for Gemini requests
type ResponseMode string
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
type ResponseMode string
type geminiResponseMode string

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed in f871ff7


// ResponseFormat and guidedJSON/guidedChoice/guidedRegex are mutually exclusive.
// Verify only one is specified.
if formatSpecifiedCount > 1 {
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens when formatSpecifiedCount == 0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nothing really
this is only to validate more than one format specifiers are not specified

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.

5 participants