test(gom-47): add E2E and contract tests for embeddings endpoint#105
Closed
SantiagoDePolonia wants to merge 7 commits into
Closed
test(gom-47): add E2E and contract tests for embeddings endpoint#105SantiagoDePolonia wants to merge 7 commits into
SantiagoDePolonia wants to merge 7 commits into
Conversation
- Add embeddings handler to mock LLM server with dimension support - Add 10 E2E tests: basic input, array input, parameters, usage, errors, concurrency - Add 7 coverage tests for chat completions and responses API - Add 5 contract tests for OpenAI, Gemini, Groq, xAI embeddings - Add embeddings endpoint configs to recordapi and Makefile record-api target - Add test generation requirements section to CLAUDE.md Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds embeddings support across tooling, tests, and mock server: new /v1/embeddings endpoints and CLI record targets, contract and e2e embedding tests and fixtures, mock embeddings handler/forwarding, CI contract job, and updated Record API Makefile steps and golden data. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Gateway
participant MockServer
participant RecordAPI
Client->>Gateway: POST /v1/embeddings (model,input,encoding,dimensions)
Gateway->>MockServer: forward request to /embeddings
MockServer->>MockServer: validate & normalize input
MockServer->>MockServer: generate embeddings, apply dimensions, compute usage
MockServer-->>Gateway: 200 OK + EmbeddingResponse
Gateway->>RecordAPI: (Makefile/record-api) invoke recorder to save golden
RecordAPI-->>Gateway: ack
Gateway-->>Client: return EmbeddingResponse
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 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 |
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/contract/openai_test.go`:
- Around line 256-281: Replace the loose dynamic decoding into resp
(map[string]interface{}) with a typed response struct: define a Response struct
with Object string, Data []DataItem, Usage Usage; DataItem should have Object
string and Embedding json.RawMessage; Usage should have PromptTokens int (or
float if needed); unmarshal data into that Response type instead of
map[string]interface{}; update the test assertions in the "Contract" subtest to
use resp.Object, iterate resp.Data and assert item.Object == "embedding", and
check item.Embedding by attempting to interpret the json.RawMessage as either a
quoted string (base64) or a JSON array of numbers (try unmarshalling into string
first, then []float64) to keep polymorphic behavior while preserving strong
typing.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (13)
CLAUDE.mdMakefilecmd/recordapi/main.gotests/contract/gemini_test.gotests/contract/groq_test.gotests/contract/openai_test.gotests/contract/xai_test.gotests/e2e/chat_test.gotests/e2e/embeddings_test.gotests/e2e/helpers_test.gotests/e2e/main_test.gotests/e2e/mock_provider.gotests/e2e/responses_test.go
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Record embeddings golden files (OpenAI, Gemini) and streaming golden files (all providers). Fix Gemini embedding model to gemini-embedding-001 and remove Groq/xAI embedding targets from Makefile (unsupported). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/test.yml:
- Around line 87-107: Update the contract tests README to document the new CI
behavior introduced by the "test-contract" job: note that contract tests now run
non-blocking (continue-on-error) via the go test command (go test -v
-tags=contract -timeout=5m ./tests/contract/...) and that a "Warn on contract
test failure" step will emit a warning rather than block the PR; add guidance on
the golden-file refresh workflow including the exact command to run locally
(make record-api) and what contributors should expect in CI when golden files
are out of date.
In `@Makefile`:
- Around line 68-71: The Makefile always runs the Gemini recording command in
the record-api target, which hard-fails without credentials; modify the Makefile
so the Gemini step is only executed when GEMINI_API_KEY is set (or move the
Gemini command into a separate target like record-api-gemini and call it
explicitly). Update the record-api recipe to check the GEMINI_API_KEY
environment variable (using an ifdef/conditional or conditional shell execution)
before running the go run ./cmd/recordapi -provider=gemini -endpoint=embeddings
... command so users can run make record-api without Gemini credentials.
In `@tests/contract/testdata/groq/chat_completion_stream.txt`:
- Line 11: Split the single combined chunk into two chunks to match the
OpenAI-compatible streaming spec: keep the existing chunk with choices[0].delta:
{} and finish_reason: "stop" but remove any usage fields from that chunk (remove
both "usage" and "x_groq.usage"), then add a separate final chunk that contains
only the usage information with choices: [] (no deltas or finish_reason) and the
usage/x_groq usage objects; update the test fixture so the first chunk mirrors
the finish-reason-only structure and the second chunk is the usage-only object.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (10)
.github/workflows/test.ymlMakefiletests/contract/testdata/anthropic/messages_stream.txttests/contract/testdata/gemini/chat_completion_stream.txttests/contract/testdata/gemini/embeddings.jsontests/contract/testdata/groq/chat_completion_stream.txttests/contract/testdata/openai/chat_completion_stream.txttests/contract/testdata/openai/embeddings.jsontests/contract/testdata/openai/embeddings_base64.jsontests/contract/testdata/xai/chat_completion_stream.txt
Comment on lines
+87
to
+107
| test-contract: | ||
| name: Contract Tests | ||
| runs-on: ubuntu-latest | ||
| continue-on-error: true | ||
| steps: | ||
| - uses: actions/checkout@v6 | ||
|
|
||
| - name: Set up Go | ||
| uses: actions/setup-go@v6 | ||
| with: | ||
| go-version: ${{ env.GO_VERSION }} | ||
| cache: true | ||
|
|
||
| - name: Run contract tests | ||
| id: contract | ||
| run: go test -v -tags=contract -timeout=5m ./tests/contract/... | ||
|
|
||
| - name: Warn on contract test failure | ||
| if: steps.contract.outcome == 'failure' | ||
| run: echo "::warning::Contract tests failed — golden files may be outdated. Run 'make record-api' to update." | ||
|
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Document this CI contract-test behavior change in contract docs.
This PR changes the contract-test CI flow (non-blocking + warning path). Please update tests/contract/README.md so contributors know expected CI behavior and golden refresh workflow.
Based on learnings: Update tests/contract/README.md when modifying contract testing procedures or golden file validation processes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/test.yml around lines 87 - 107, Update the contract tests
README to document the new CI behavior introduced by the "test-contract" job:
note that contract tests now run non-blocking (continue-on-error) via the go
test command (go test -v -tags=contract -timeout=5m ./tests/contract/...) and
that a "Warn on contract test failure" step will emit a warning rather than
block the PR; add guidance on the golden-file refresh workflow including the
exact command to run locally (make record-api) and what contributors should
expect in CI when golden files are out of date.
Comment on lines
+68
to
+71
| @echo "Recording Gemini embeddings..." | ||
| go run ./cmd/recordapi -provider=gemini -endpoint=embeddings \ | ||
| -model=gemini-embedding-001 \ | ||
| -output=tests/contract/testdata/gemini/embeddings.json |
Contributor
There was a problem hiding this comment.
record-api now hard-fails without Gemini credentials.
Line 68–71 always calls Gemini recording, so make record-api can fail even when users only intend to refresh OpenAI goldens. Gate this step on GEMINI_API_KEY (or split targets).
Proposed fix
- `@echo` "Recording Gemini embeddings..."
- go run ./cmd/recordapi -provider=gemini -endpoint=embeddings \
- -model=gemini-embedding-001 \
- -output=tests/contract/testdata/gemini/embeddings.json
+ `@if` [ -n "$$GEMINI_API_KEY" ]; then \
+ echo "Recording Gemini embeddings..."; \
+ go run ./cmd/recordapi -provider=gemini -endpoint=embeddings \
+ -model=gemini-embedding-001 \
+ -output=tests/contract/testdata/gemini/embeddings.json; \
+ else \
+ echo "Skipping Gemini embeddings (GEMINI_API_KEY not set)"; \
+ fi📝 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.
Suggested change
| @echo "Recording Gemini embeddings..." | |
| go run ./cmd/recordapi -provider=gemini -endpoint=embeddings \ | |
| -model=gemini-embedding-001 \ | |
| -output=tests/contract/testdata/gemini/embeddings.json | |
| `@if` [ -n "$$GEMINI_API_KEY" ]; then \ | |
| echo "Recording Gemini embeddings..."; \ | |
| go run ./cmd/recordapi -provider=gemini -endpoint=embeddings \ | |
| -model=gemini-embedding-001 \ | |
| -output=tests/contract/testdata/gemini/embeddings.json; \ | |
| else \ | |
| echo "Skipping Gemini embeddings (GEMINI_API_KEY not set)"; \ | |
| fi |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Makefile` around lines 68 - 71, The Makefile always runs the Gemini recording
command in the record-api target, which hard-fails without credentials; modify
the Makefile so the Gemini step is only executed when GEMINI_API_KEY is set (or
move the Gemini command into a separate target like record-api-gemini and call
it explicitly). Update the record-api recipe to check the GEMINI_API_KEY
environment variable (using an ifdef/conditional or conditional shell execution)
before running the go run ./cmd/recordapi -provider=gemini -endpoint=embeddings
... command so users can run make record-api without Gemini credentials.
| data: {"id":"chatcmpl-bc4c2b5f-ad1c-4152-831f-a0885de968a3","object":"chat.completion.chunk","created":1769093744,"model":"llama-3.3-70b-versatile","system_fingerprint":"fp_c06d5113ec","choices":[{"index":0,"delta":{},"logprobs":null,"finish_reason":"stop"}],"x_groq":{"id":"req_01kfk38ha1ey6bw3b68mcaeddg","usage":{"queue_time":0.090321431,"prompt_tokens":38,"prompt_time":0.003210785,"completion_tokens":4,"completion_time":0.027554055,"total_tokens":42,"total_time":0.03076484}},"usage":{"queue_time":0.090321431,"prompt_tokens":38,"prompt_time":0.003210785,"completion_tokens":4,"completion_time":0.027554055,"total_tokens":42,"total_time":0.03076484}} | ||
| data: {"id":"chatcmpl-c2c50c48-e13b-4ba8-8f01-f9714db853a0","object":"chat.completion.chunk","created":1772206945,"model":"llama-3.3-70b-versatile","system_fingerprint":"fp_f8b414701e","choices":[{"index":0,"delta":{"content":"!"},"logprobs":null,"finish_reason":null}]} | ||
|
|
||
| data: {"id":"chatcmpl-c2c50c48-e13b-4ba8-8f01-f9714db853a0","object":"chat.completion.chunk","created":1772206945,"model":"llama-3.3-70b-versatile","system_fingerprint":"fp_f8b414701e","choices":[{"index":0,"delta":{},"logprobs":null,"finish_reason":"stop"}],"x_groq":{"id":"req_01kjfw7wstf62a77bs3qb0zysg","usage":{"queue_time":0.048029167,"prompt_tokens":45,"prompt_time":0.001938338,"completion_tokens":5,"completion_time":0.009110938,"total_tokens":50,"total_time":0.011049276}},"usage":{"queue_time":0.048029167,"prompt_tokens":45,"prompt_time":0.001938338,"completion_tokens":5,"completion_time":0.009110938,"total_tokens":50,"total_time":0.011049276}} |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Groq streaming chat completion API usage statistics finish_reason placement
💡 Result:
Expected (OpenAI-compatible) streaming layout (what Groq’s .../openai/v1/chat/completions is aiming to emulate)
If you request token usage in streaming mode with:
{"stream": true, "stream_options": {"include_usage": true}}then the stream is typically:
- Many content chunks:
choices[0].delta.contentcarries text;finish_reasonisnull;usageisnull. - Finish-reason chunk (penultimate): a chunk with
choices[0].delta: {}andchoices[0].finish_reason: "stop" | "length" | ...;usagestillnull. [1] - Usage-only chunk (final): a chunk with
choices: []andusage: {prompt_tokens, completion_tokens, total_tokens}. (Nofinish_reasonhere because there are no choices.) [1] - Then the SSE terminator:
data: [DONE]. [1]
So, when include_usage is enabled, the finish_reason moves to the second-to-last chunk, and the very last chunk is “usage-only” with empty choices. [1]
What to watch for on Groq specifically (finish_reason placement / missing)
Groq community reports show that some Groq models have streamed responses where finish_reason never arrives in a proper final “choices” chunk before [DONE] (e.g., the stream ends with an empty chunk / empty choices and then [DONE]), which breaks clients that assume OpenAI’s “finish_reason chunk before end-of-stream” behavior. [2] A diagnostic example tool for Groq streaming also encodes this expectation: success = finish_reason received before EOF; failure = stream ends without it. [3]
Practical parsing implication: in streaming mode, don’t assume the last chunk has choices[0]; handle (a) a penultimate finish_reason chunk, and (b) an optional final usage-only chunk (choices: []) when include_usage is supported/enabled. [1]
Sources: [1] [2] [3]
🌐 Web query:
Groq OpenAI API compatibility streaming response format
💡 Result:
Groq’s OpenAI-compatible Chat Completions endpoint (POST https://api.groq.com/openai/v1/chat/completions) streams using Server-Sent Events (SSE) when you set "stream": true. Each SSE event is a data: line containing one JSON “chunk”, and the stream ends with data: [DONE]. [1]
What you receive (chunk shape)
The JSON payloads are intended to match OpenAI’s chat completion chunk format (i.e., object: "chat.completion.chunk" with incremental updates in choices[0].delta.*, commonly delta.content). [2]
In Groq’s own examples/SDKs, you read streamed text from:
chunk.choices[0].delta.content[3]
Minimal example of the wire format (typical)
data: {"object":"chat.completion.chunk","choices":[{"index":0,"delta":{"role":"assistant"},"finish_reason":null}]}
data: {"object":"chat.completion.chunk","choices":[{"index":0,"delta":{"content":"Hello"},"finish_reason":null}]}
data: {"object":"chat.completion.chunk","choices":[{"index":0,"delta":{},"finish_reason":"stop"}]}
data: [DONE]
(Structure per OpenAI’s streaming spec; Groq documents SSE + [DONE] termination.) [1][2]
Compatibility gotcha to watch for
At least some Groq models have been reported to omit finish_reason in the final streamed chunk, which can break strict OpenAI-compatible streaming parsers that expect it. [4]
Sources: Groq API Reference (streaming + [DONE]) [1], OpenAI chat completion streaming/chunk format [2], Groq streaming delta access pattern [3], Groq community report on missing finish_reason [4].
Fix usage data placement to match OpenAI-compatible streaming format.
Line 11 incorrectly places both finish_reason="stop" and usage data in the same chunk. According to Groq's OpenAI-compatible streaming API specification, these must be separate: the finish-reason chunk should have choices[0].delta: {} with finish_reason: "stop" and no usage data, followed by a separate final usage-only chunk with choices: [] containing the usage statistics.
This misalignment with the spec will cause contract test failures when testing against actual Groq API responses.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/contract/testdata/groq/chat_completion_stream.txt` at line 11, Split
the single combined chunk into two chunks to match the OpenAI-compatible
streaming spec: keep the existing chunk with choices[0].delta: {} and
finish_reason: "stop" but remove any usage fields from that chunk (remove both
"usage" and "x_groq.usage"), then add a separate final chunk that contains only
the usage information with choices: [] (no deltas or finish_reason) and the
usage/x_groq usage objects; update the test fixture so the first chunk mirrors
the finish-reason-only structure and the second chunk is the usage-only object.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replay tests send requests through the full gateway stack but serve pre-recorded golden files as upstream responses. This catches bugs that E2E tests (fabricated mocks) and contract tests (no gateway code) both miss. Currently exposes three deserialization issues: missing Tools/ToolCalls fields on core types and base64 embedding handling. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/e2e/replay/golden_server_test.go`:
- Around line 55-57: The handler in GoldenFileServer currently ignores errors
from io.ReadAll and r.Body.Close which can lead to empty/partial bodies used for
route matching; update GoldenFileServer.handler to check the error returned by
io.ReadAll(r.Body) and, on error, respond immediately (e.g., http.Error with
appropriate status) and return so matching logic never runs on a bad body, and
likewise check the error from r.Body.Close and log or ignore it explicitly;
ensure the rest of the handler still uses the successfully read body variable
for route matching.
In `@tests/e2e/replay/helpers_test.go`:
- Around line 21-22: The test helpers currently call the package-level
http.Post/http.Get (e.g., the http.Post call in
tests/e2e/replay/helpers_test.go) which uses the default client with no timeout;
replace those calls with a dedicated http.Client that has an explicit Timeout
(e.g., client := &http.Client{Timeout: 10 * time.Second}) and then call
client.Post / client.Get (or create a reusable helper that accepts the client)
and add the necessary time import; update all occurrences (including the other
http.Get/http.Post uses around lines 29-30) so E2E requests cannot hang
indefinitely.
In `@tests/e2e/replay/openai_test.go`:
- Around line 171-189: The test currently accepts either success (200) or
failure (502), hiding regressions; make it deterministic by asserting the
current baseline status only (expect http.StatusBadGateway) and remove the
alternate 200 branch: specifically, update the test around resp handling (the
block that reads into core.EmbeddingResponse via readJSON and checks emb) to
instead assert resp.StatusCode == http.StatusBadGateway and keep the explanatory
message about base64 embedding deserialization failing and EmbeddingData needing
a custom UnmarshalJSON; remove the conditional success-path that inspects emb
and its Data to avoid dual outcomes.
- Around line 118-124: The two assertions in tests/e2e/replay/openai_test.go
(the assert.Equal on choice.FinishReason and assert.NotEmpty on
choice.Message.ToolCalls) are a CI blocker; temporarily gate them by skipping
the test when the Tools/ToolCalls fix isn't present: in the test function wrap
or replace these assertions with a t.Skipf mentioning the tracking issue/PR, or
add a conditional that checks whether core.ChatRequest/core.Message have
Tools/ToolCalls and skip if not, so CI stops failing until the core types are
updated; reference choice, FinishReason, Message.ToolCalls, core.ChatRequest and
core.Message when locating where to add the skip.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
tests/e2e/replay/golden_server_test.gotests/e2e/replay/helpers_test.gotests/e2e/replay/main_test.gotests/e2e/replay/openai_test.go
Comment on lines
+55
to
+57
| func (g *GoldenFileServer) handler(w http.ResponseWriter, r *http.Request) { | ||
| body, _ := io.ReadAll(r.Body) | ||
| _ = r.Body.Close() |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect the actual handler implementation and surrounding context
cat -n tests/e2e/replay/golden_server_test.go | head -80Repository: ENTERPILOT/GOModel
Length of output: 2611
🏁 Script executed:
#!/bin/bash
# Search for other handler implementations in the same test file to check consistency
rg -n --type=go 'func.*handler|io\.ReadAll' tests/e2e/replay/golden_server_test.goRepository: ENTERPILOT/GOModel
Length of output: 175
🏁 Script executed:
#!/bin/bash
# Check if the file has the e2e build tag as mentioned
head -5 tests/e2e/replay/golden_server_test.goRepository: ENTERPILOT/GOModel
Length of output: 102
Handle request body read failures explicitly in the golden handler.
The test mock server ignores io.ReadAll() errors (line 56), which can result in an empty or partial body being used for route matching (line 74). This causes incorrect routing decisions and unreliable test outcomes.
🔧 Suggested patch
func (g *GoldenFileServer) handler(w http.ResponseWriter, r *http.Request) {
- body, _ := io.ReadAll(r.Body)
+ body, err := io.ReadAll(r.Body)
_ = r.Body.Close()
+ if err != nil {
+ http.Error(w, "failed to read request body", http.StatusBadRequest)
+ return
+ }📝 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.
Suggested change
| func (g *GoldenFileServer) handler(w http.ResponseWriter, r *http.Request) { | |
| body, _ := io.ReadAll(r.Body) | |
| _ = r.Body.Close() | |
| func (g *GoldenFileServer) handler(w http.ResponseWriter, r *http.Request) { | |
| body, err := io.ReadAll(r.Body) | |
| _ = r.Body.Close() | |
| if err != nil { | |
| http.Error(w, "failed to read request body", http.StatusBadRequest) | |
| return | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/e2e/replay/golden_server_test.go` around lines 55 - 57, The handler in
GoldenFileServer currently ignores errors from io.ReadAll and r.Body.Close which
can lead to empty/partial bodies used for route matching; update
GoldenFileServer.handler to check the error returned by io.ReadAll(r.Body) and,
on error, respond immediately (e.g., http.Error with appropriate status) and
return so matching logic never runs on a bad body, and likewise check the error
from r.Body.Close and log or ignore it explicitly; ensure the rest of the
handler still uses the successfully read body variable for route matching.
Comment on lines
+21
to
+22
| resp, err := http.Post(gatewayURL+path, "application/json", bytes.NewReader(body)) | ||
| require.NoError(t, err) |
Contributor
There was a problem hiding this comment.
Add explicit HTTP timeouts in request helpers.
Using http.Post / http.Get here relies on the default client (no timeout), so a stalled connection can hang the E2E suite indefinitely.
🔧 Suggested patch
import (
"bytes"
"encoding/json"
"io"
"net/http"
"testing"
+ "time"
"github.com/stretchr/testify/require"
)
+var e2eHTTPClient = &http.Client{Timeout: 10 * time.Second}
+
// sendJSON sends a JSON POST request to the gateway and returns the response.
func sendJSON(t *testing.T, path string, payload any) *http.Response {
t.Helper()
body, err := json.Marshal(payload)
require.NoError(t, err)
- resp, err := http.Post(gatewayURL+path, "application/json", bytes.NewReader(body))
+ req, err := http.NewRequest(http.MethodPost, gatewayURL+path, bytes.NewReader(body))
+ require.NoError(t, err)
+ req.Header.Set("Content-Type", "application/json")
+ resp, err := e2eHTTPClient.Do(req)
require.NoError(t, err)
return resp
}
// sendGET sends a GET request to the gateway and returns the response.
func sendGET(t *testing.T, path string) *http.Response {
t.Helper()
- resp, err := http.Get(gatewayURL + path)
+ req, err := http.NewRequest(http.MethodGet, gatewayURL+path, nil)
+ require.NoError(t, err)
+ resp, err := e2eHTTPClient.Do(req)
require.NoError(t, err)
return resp
}#!/bin/bash
# Verify unbounded default-client calls in replay helpers
rg -n --type=go -C2 'http\.(Post|Get)\(' tests/e2e/replay/helpers_test.goAlso applies to: 29-30
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/e2e/replay/helpers_test.go` around lines 21 - 22, The test helpers
currently call the package-level http.Post/http.Get (e.g., the http.Post call in
tests/e2e/replay/helpers_test.go) which uses the default client with no timeout;
replace those calls with a dedicated http.Client that has an explicit Timeout
(e.g., client := &http.Client{Timeout: 10 * time.Second}) and then call
client.Post / client.Get (or create a reusable helper that accepts the client)
and add the necessary time import; update all occurrences (including the other
http.Get/http.Post uses around lines 29-30) so E2E requests cannot hang
indefinitely.
Comment on lines
+118
to
+124
| assert.Equal(t, "tool_calls", choice.FinishReason, | ||
| "finish_reason should be 'tool_calls' — if 'stop', the tools field was dropped from the request") | ||
|
|
||
| assert.NotEmpty(t, choice.Message.ToolCalls, | ||
| "tool_calls should be present in the response — "+ | ||
| "if empty, core.ChatRequest needs a Tools field and core.Message needs a ToolCalls field") | ||
|
|
Contributor
There was a problem hiding this comment.
This assertion is currently a CI blocker for known unfixed behavior.
The pipeline already fails on this expectation. Either land the corresponding Tools/ToolCalls fix in this PR or gate this as a temporary expected-failure (e.g., t.Skip with issue reference) until the core types are updated.
🔧 Temporary stabilization option
choice := chat.Choices[0]
+ if choice.FinishReason != "tool_calls" || len(choice.Message.ToolCalls) == 0 {
+ t.Skip("Known issue: tools/tool_calls are dropped before/after provider; enable strict assertions after core fix")
+ }
assert.Equal(t, "tool_calls", choice.FinishReason,
"finish_reason should be 'tool_calls' — if 'stop', the tools field was dropped from the request")🧰 Tools
🪛 GitHub Actions: CI
[error] 118-121: TestReplay_OpenAI_ChatWithTools failed: finish_reason should be 'tool_calls' — if 'stop', the tools field was dropped from the request; tool_calls should be present in the response.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/e2e/replay/openai_test.go` around lines 118 - 124, The two assertions
in tests/e2e/replay/openai_test.go (the assert.Equal on choice.FinishReason and
assert.NotEmpty on choice.Message.ToolCalls) are a CI blocker; temporarily gate
them by skipping the test when the Tools/ToolCalls fix isn't present: in the
test function wrap or replace these assertions with a t.Skipf mentioning the
tracking issue/PR, or add a conditional that checks whether
core.ChatRequest/core.Message have Tools/ToolCalls and skip if not, so CI stops
failing until the core types are updated; reference choice, FinishReason,
Message.ToolCalls, core.ChatRequest and core.Message when locating where to add
the skip.
Comment on lines
+171
to
+189
| if resp.StatusCode == http.StatusOK { | ||
| // Bug has been fixed — validate the decoded embedding | ||
| var emb core.EmbeddingResponse | ||
| readJSON(t, resp, &emb) | ||
|
|
||
| assert.Equal(t, "list", emb.Object) | ||
| require.NotEmpty(t, emb.Data) | ||
| assert.NotEmpty(t, emb.Data[0].Embedding, | ||
| "base64 embedding should be decoded into []float64") | ||
| if len(emb.Data[0].Embedding) > 0 { | ||
| assert.Greater(t, len(emb.Data[0].Embedding), 100, | ||
| "decoded embedding vector should have many dimensions") | ||
| } | ||
| } else { | ||
| // Bug exists — the provider fails to unmarshal the base64 string into []float64 | ||
| assert.Equal(t, http.StatusBadGateway, resp.StatusCode, | ||
| "base64 embedding deserialization currently fails with 502 — "+ | ||
| "EmbeddingData needs a custom UnmarshalJSON to handle base64 encoding") | ||
| } |
Contributor
There was a problem hiding this comment.
Make this test deterministic; don't allow both success and failure paths.
Accepting both 200 and 502 masks regressions and weakens signal. Lock it to one expected status for the current baseline, then flip when the fix ships.
🔧 Suggested deterministic assertion (current baseline)
- // Once fixed, change this to http.StatusOK and verify the decoded embedding.
- if resp.StatusCode == http.StatusOK {
- // Bug has been fixed — validate the decoded embedding
- var emb core.EmbeddingResponse
- readJSON(t, resp, &emb)
-
- assert.Equal(t, "list", emb.Object)
- require.NotEmpty(t, emb.Data)
- assert.NotEmpty(t, emb.Data[0].Embedding,
- "base64 embedding should be decoded into []float64")
- if len(emb.Data[0].Embedding) > 0 {
- assert.Greater(t, len(emb.Data[0].Embedding), 100,
- "decoded embedding vector should have many dimensions")
- }
- } else {
- // Bug exists — the provider fails to unmarshal the base64 string into []float64
- assert.Equal(t, http.StatusBadGateway, resp.StatusCode,
- "base64 embedding deserialization currently fails with 502 — "+
- "EmbeddingData needs a custom UnmarshalJSON to handle base64 encoding")
- }
+ require.Equal(t, http.StatusBadGateway, resp.StatusCode,
+ "known issue: base64 embeddings are not decoded yet; switch to StatusOK assertions once fixed")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/e2e/replay/openai_test.go` around lines 171 - 189, The test currently
accepts either success (200) or failure (502), hiding regressions; make it
deterministic by asserting the current baseline status only (expect
http.StatusBadGateway) and remove the alternate 200 branch: specifically, update
the test around resp handling (the block that reads into core.EmbeddingResponse
via readJSON and checks emb) to instead assert resp.StatusCode ==
http.StatusBadGateway and keep the explanatory message about base64 embedding
deserialization failing and EmbeddingData needing a custom UnmarshalJSON; remove
the conditional success-path that inspects emb and its Data to avoid dual
outcomes.
Base automatically changed from
me/gom-47-embedding-endpoints-anthropic-and-openai
to
main
February 28, 2026 10:34
Contributor
Author
|
Alternative PR merged |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Tests
Documentation
Chores