tests: Prepared and executed the testing strategy#46
Merged
SantiagoDePolonia merged 10 commits intomainfrom Jan 26, 2026
Merged
tests: Prepared and executed the testing strategy#46SantiagoDePolonia merged 10 commits intomainfrom
SantiagoDePolonia merged 10 commits intomainfrom
Conversation
Contributor
📝 WalkthroughWalkthroughAdds contract and integration testing infrastructure (golden fixtures, recordapi CLI), testcontainers-backed integration tests with DB assertion helpers, CI/Makefile updates to run new test layers, many provider-specific fixtures/tests, and threads provider through usage extraction call sites. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Integration Test
participant TC as Testcontainers
participant PG as PostgreSQL
participant Mongo as MongoDB
participant App as Test App
participant Mock as MockLLMServer
participant DBAssert as dbassert
Test->>TC: start Postgres container
TC-->>PG: provision & expose connection (URL)
Test->>TC: start Mongo container
TC-->>Mongo: provision & expose connection (URL)
Test->>App: initialize app with DB configs (Postgres/Mongo)
App->>PG: open pool / connect
App->>Mongo: open DB
Test->>App: POST /v1/chat/completions (via TestProvider)
App->>Mock: forward request to MockLLMServer
Mock-->>App: mock response (streaming or JSON)
App->>PG: insert AuditLog / Usage entries
App->>Mongo: optionally insert AuditLog / Usage entries
App-->>Test: 200 OK
Test->>App: FlushAndClose()
App->>PG: ensure writes persisted
App->>Mongo: ensure writes persisted
Test->>DBAssert: QueryAuditLogsByRequestID(requestID)
DBAssert->>PG: SELECT rows
PG-->>DBAssert: rows -> AuditLogEntry[]
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
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 |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 26
🤖 Fix all issues with AI agents
In @.github/workflows/test.yml:
- Around line 56-75: Add a top-level permissions block to the workflow to
restrict token scopes to the minimum required (e.g., permissions: contents:
read) so actions like actions/checkout and Codecov only have read access; update
the .github/workflows/test.yml file by inserting a workflow-level permissions:
contents: read entry above the jobs (affecting jobs such as integration and
build) to replace default broad permissions.
In `@cmd/recordapi/main.go`:
- Around line 188-215: The code currently writes response bodies (including
error responses) to files; update the handling after reading resp.Body in
main.go to abort on non-2xx responses: check resp.StatusCode (e.g., if
resp.StatusCode < 200 || resp.StatusCode >= 300) and print the response body (or
a descriptive error including resp.Status and the body) to stderr, then exit
with non-zero status instead of proceeding to writeOutput or writeStreamOutput;
preserve existing formatting for streaming vs non-streaming paths
(writeStreamOutput and writeOutput) but only call them when the status guard
passes.
- Around line 63-149: The endpointConfigs map uses untyped requestBody
(map[string]interface{}) which bypasses compile-time validation; replace these
maps with strongly typed request structs (e.g., define types ChatRequest,
ChatStreamRequest, ModelsRequest or a shared Request struct) and update usages
in main where requestBody is read/modified (including the model override logic)
to build and marshal these structs instead of mutating map entries; also update
adjustForAnthropic to accept and return the concrete request type(s) and modify
any response/metadata parsing code noted around lines 226-235 and 238-259 to use
typed response structs so all payloads are compile-time checked.
- Around line 190-205: The code currently calls io.ReadAll(resp.Body) for all
responses then checks if *endpoint has "_stream", which buffers the entire
stream into memory; change the streaming path to stream directly from resp.Body
to disk instead of ReadAll. Concretely, stop using io.ReadAll for stream
endpoints: for the "_stream" branch call a function that accepts io.Reader
(e.g., update writeStreamOutput to take an io.Reader and filename) and stream
with io.Copy(file, resp.Body), handling errors and closing the file and
resp.Body appropriately; leave non-stream responses to use io.ReadAll as before.
Also update the other occurrence that handles streaming (the block around
writeStreamOutput at lines ~271-274) to use the new reader-based streaming
approach.
- Around line 23-95: The configs use a single endpoint path and a hardcoded
model ("gpt-4o-mini") causing failures for non-OpenAI providers; update
providerConfigs and endpointConfigs so each provider key (e.g., "anthropic") has
its own default path and default model (Anthropic -> path "/v1/messages" and
Claude model, Groq -> Llama model, xai -> Grok model, Gemini -> proper Gemini
model id) or enforce that the CLI `-model` flag must be supplied for any
provider other than "openai"; modify the code that reads
providerConfigs/endpointConfigs to select per-provider path and default model
accordingly and keep using the Authorization header for Gemini.
In `@docs/FUTURE_ARCHITECTURE.md`:
- Around line 1-123: Add a top-level H1 heading ("Future Architecture") at the
top of FUTURE_ARCHITECTURE.md and change the opening fenced code block to
include a language identifier (e.g., ```text) so the file satisfies MD041 and
MD040; specifically update the initial lines around the existing triple-backtick
block and ensure the closing triple-backtick remains unchanged.
In `@docs/TESTING_STRATEGY.md`:
- Around line 5-18: The markdown contains unlabeled fenced code blocks (the
ASCII diagram block, the tests/ tree block, and the tests/contract/testdata/
tree block) which trigger MD040; update each opening triple-backtick fence to
include a language identifier such as text (e.g., ```text) so the diagram and
file-tree fenced blocks are labeled; ensure you change the three unlabeled
fences that wrap the ASCII diagram, the tests/ tree listing, and the
tests/contract/testdata/ listing to use ```text consistently.
In `@go.mod`:
- Around line 30-37: Update the vulnerable containerd and Docker module versions
in go.mod: bump github.com/containerd/containerd from v1.7.18 to at least
v1.7.29 (or later in the 1.7.x series) to address the listed HIGH-severity
advisories, and review/upgrade github.com/docker/docker (currently
v27.1.1+incompatible) to a patched release if available; then run `go get` / `go
mod tidy` and run tests to ensure compatibility (look for references to
github.com/containerd/containerd and github.com/docker/docker to validate no
breaking API changes).
In `@tests/contract/anthropic_test.go`:
- Around line 211-221: The local flag hasThinking is assigned while iterating
resp.Content (checking block.Type == "thinking" and asserting block.Thinking)
but is never used beyond being assigned to `_`; either remove the hasThinking
variable and the trailing `_ = hasThinking` line to avoid the unused variable,
or use it meaningfully — e.g., after the loop inspect hasThinking and call
t.Logf or t.Skipf (or assert) to record or conditionally skip the test when a
"thinking" block is/n't present; refer to resp.Content, block.Type and
block.Thinking to locate and update the loop in the test.
In `@tests/contract/gemini_test.go`:
- Around line 142-149: The function containsIgnorePrefix misleads: it strips a
"models/" prefix then checks for a prefix match, not a contains; update the code
to reflect intended semantics—either (A) rename the function to
hasPrefixIgnoreModels or startsWithIgnorePrefix and keep the current logic in
containsIgnorePrefix (update all call sites to the new name), or (B) implement
true contains behavior by using strings.Contains on the stripped string (replace
the current prefix check with strings.Contains(s, substr)) in the
containsIgnorePrefix function; reference the function containsIgnorePrefix when
making the change and update any callers accordingly.
In `@tests/contract/groq_test.go`:
- Around line 79-87: Replace the manual length check and slice comparison in the
loop that sets hasLlama over modelIDs with strings.HasPrefix for clarity and
idiomatic style: in the loop inside the test (the block using hasLlama and
iterating over modelIDs) call strings.HasPrefix(id, "llama") instead of checking
len(id) >= 5 && id[:5] == "llama", and add the necessary import of the strings
package to the test file.
In `@tests/contract/main_test.go`:
- Around line 46-53: The helper goldenFileExists currently swallows all os.Stat
errors and returns false; update goldenFileExists(t *testing.T, path string) to
treat missing files (os.IsNotExist(err)) as false but log any other unexpected
filesystem errors using t.Logf or t.Errorf including the fullPath and the error,
so callers can diagnose permission/IO issues while still returning false for
non-existent files; keep the function name goldenFileExists and variable
fullPath/testdataDir to locate the change.
In `@tests/contract/openai_test.go`:
- Around line 119-130: Replace the manual prefix slicing in the "ModelType" test
with strings.HasPrefix for clarity: in the t.Run("ModelType") block, remove the
length check and prefix variable and instead use strings.HasPrefix(resp.Model,
"o1") || strings.HasPrefix(resp.Model, "o3") to set isReasoningModel; add the
strings import if not present and keep the existing assert.True on
isReasoningModel.
In `@tests/contract/README.md`:
- Around line 5-44: The Markdown code fence containing the directory tree in
tests/contract/README.md is missing a language identifier (MD040); edit the
fenced block that starts with the tree (the block showing "testdata/" and its
subfolders) and add a language tag such as text (i.e., replace ``` with ```text)
so markdownlint is satisfied.
In `@tests/contract/testdata/anthropic/messages.json`:
- Around line 1-18: The JSON fixture uses 4-space indentation causing
inconsistency with other Anthropic fixtures; reformat this fixture so it uses
2-space indentation (match the style of messages_extended_thinking.json and
messages_multimodal.json) — update the file containing the object with "model":
"claude-sonnet-4-20250514" and "id": "msg_013LBDy1nvGiRgxiRAWseYRS" to use
2-space indentation (or run the project JSON formatter/linter) without changing
any keys or values.
In `@tests/contract/testdata/openai/chat_completion_stream.txt`:
- Around line 1-13: Remove the non-spec `obfuscation` property from the
streaming chunk test entries in chat_completion_stream.txt (the JSON objects
that start with data: {"id":"chatcmpl-..."}), or if it is intentionally
synthetic, explicitly mark the file or each chunk as synthetic in a
comment/header. Locate the JSON chunks containing "obfuscation": and delete that
key/value from each chunk (or replace with a clear synthetic marker), leaving
only the OpenAI-spec fields such as id, object, created, model,
system_fingerprint, service_tier, and choices.
In `@tests/contract/xai_test.go`:
- Around line 76-81: Replace the manual prefix check in the loop over modelIDs
(the block that inspects variable id and sets hasGrok) with strings.HasPrefix to
avoid slicing errors: add an import "strings" and change the condition "if
len(id) >= 4 && id[:4] == \"grok\"" to use strings.HasPrefix(id, "grok") so the
loop in the test (the code that sets hasGrok) is safer and clearer.
In `@tests/integration/auditlog_test.go`:
- Around line 140-143: Add an integration assertion that the /health endpoint is
excluded from audit logs when OnlyModelInteractions is true by issuing a GET to
fixture.ServerURL + "/health" with a unique request ID, closing the response
body, then querying audit logs via dbassert.QueryAuditLogsByRequestID (using the
same request ID and fixture.PgPool) and asserting the result is empty; place
this check near the existing OnlyModelInteractions test (after making the health
request and before or after FlushAndClose/re-setup) so the test verifies zero
entries are recorded for that request ID.
- Around line 69-110: TestAuditLog_CapturesAllFields_MongoDB is missing the
parity assertions present in the PostgreSQL test; after retrieving entry :=
entries[0] (and after dbassert.AssertAuditLogFieldCompleteness), add calls to
dbassert.AssertAuditLogDurationPositive(t, entry), dbassert.AssertNoErrorType(t,
entry), dbassert.AssertAuditLogHasData(t, entry),
dbassert.AssertAuditLogHasBody(t, entry, true, true) and
dbassert.AssertAuditLogHasHeaders(t, entry, true, true) so the MongoDB test
checks duration, error type, and that bodies/headers are logged just like the
Postgres test.
In `@tests/integration/dbassert/auditlog.go`:
- Around line 105-113: Add a symmetric MongoDB cleanup function named
ClearAuditLogsMongo that mirrors ClearAuditLogs: define func
ClearAuditLogsMongo(t *testing.T, col *mongo.Collection) (or accept a
*mongo.Client plus DB/collection name), call t.Helper(), create a
context.WithTimeout of 10*time.Second and defer cancel, call col.DeleteMany(ctx,
bson.M{}) to remove all documents, and assert require.NoError(t, err, "failed to
clear audit logs") so tests cleanup Mongo audit logs the same way as PostgreSQL.
In `@tests/integration/dbassert/fields.go`:
- Around line 36-45: The ExpectedAuditLog struct's Stream field should be
changed from a plain bool to a pointer (*bool) so tests can assert true, false,
or unset; update the struct definition (ExpectedAuditLog) to use Stream *bool
and adjust any tests and helpers that construct or compare ExpectedAuditLog
instances (and the similar occurrences referenced around the other occurrence at
lines ~102-104) to pass &true/&false or nil and to dereference safely when
comparing to actual values.
In `@tests/integration/helpers_test.go`:
- Around line 60-62: The HTTP client in helpers_test.go is created without a
timeout (client := &http.Client{}), which can hang CI; modify the client
creation to set Timeout: 10 * time.Second (import the time package) so requests
in the test use a 10s deadline consistent with other tests (see setup_test.go
pattern), then use that client for client.Do(req).
In `@tests/integration/main_test.go`:
- Around line 43-75: The deferred cancelFunc() in TestMain will not run because
os.Exit is used; change to call cancelFunc() explicitly before any os.Exit and
before/after cleanup as appropriate: remove the defer cancelFunc() and invoke
cancelFunc() just prior to both exit paths (the error exit after container setup
and the final exit after tests) so the context is always cancelled; reference
TestMain, cancelFunc, cleanup, m.Run and the os.Exit calls when making the
edits.
In `@tests/integration/setup_test.go`:
- Around line 381-390: The StreamChatCompletion stub in TestProvider calls
ChatCompletion then returns an error, causing an unnecessary real request;
change StreamChatCompletion to avoid calling TestProvider.ChatCompletion when
streaming is unimplemented and immediately return the streaming-not-implemented
error (or implement SSE conversion) instead—locate the method named
StreamChatCompletion on TestProvider and remove the call to ChatCompletion so it
just returns the error (or replace with a proper SSE ReadCloser implementation
if you want streaming behavior).
- Around line 102-111: The goroutine calling application.Start currently
discards startup errors which leads to opaque health-check timeouts; modify the
test to create an error channel, have the goroutine call application.Start(addr)
and send any returned error into that channel, then in the main test wait
(select) on either an error from that channel or the successful
waitForServer(serverURL+"/health") result so startup failures are reported
immediately; reference application.Start, the anonymous goroutine, the error
channel, waitForServer, and serverURL in your changes.
In `@tests/integration/usage_test.go`:
- Around line 219-220: The test hardcodes the mock LLM response ID
"chatcmpl-test123" in the assertion (entries[0].ProviderID), which is brittle;
either reference the mock's canonical constant or add a comment pointing to
where that value is defined. Fix by exposing the mock response ID as a shared
constant (e.g., MockLLMResponseID or MockProviderResponseID) in the mock
implementation and replace the literal in the test's assert with that constant,
or add a one-line comment above the assertion referencing the mock symbol that
defines "chatcmpl-test123".
Comment on lines
+190
to
+205
| // Read response body | ||
| body, err := io.ReadAll(resp.Body) | ||
| if err != nil { | ||
| fmt.Fprintf(os.Stderr, "Error reading response: %v\n", err) | ||
| os.Exit(1) | ||
| } | ||
|
|
||
| // Handle streaming responses differently | ||
| if strings.HasSuffix(*endpoint, "_stream") { | ||
| if err := writeStreamOutput(*output, body); err != nil { | ||
| fmt.Fprintf(os.Stderr, "Error writing output: %v\n", err) | ||
| os.Exit(1) | ||
| } | ||
| fmt.Printf("Streaming response saved to %s\n", *output) | ||
| return | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Stream SSE responses directly to disk instead of ReadAll.
For streaming endpoints, buffering the entire response defeats the purpose and can spike memory. Pipe resp.Body to file when the endpoint is a stream.
♻️ Proposed refactor (stream to file)
- // Read response body
- body, err := io.ReadAll(resp.Body)
- if err != nil {
- fmt.Fprintf(os.Stderr, "Error reading response: %v\n", err)
- os.Exit(1)
- }
-
// Handle streaming responses differently
if strings.HasSuffix(*endpoint, "_stream") {
- if err := writeStreamOutput(*output, body); err != nil {
+ if err := writeStreamOutput(*output, resp.Body); err != nil {
fmt.Fprintf(os.Stderr, "Error writing output: %v\n", err)
os.Exit(1)
}
fmt.Printf("Streaming response saved to %s\n", *output)
return
}
+
+ // Read response body
+ body, err := io.ReadAll(resp.Body)
+ if err != nil {
+ fmt.Fprintf(os.Stderr, "Error reading response: %v\n", err)
+ os.Exit(1)
+ }
@@
-func writeStreamOutput(path string, data []byte) error {
- // For streaming responses, save as-is (SSE format)
- return writeOutput(path, data)
+func writeStreamOutput(path string, r io.Reader) error {
+ dir := filepath.Dir(path)
+ if err := os.MkdirAll(dir, 0755); err != nil {
+ return fmt.Errorf("failed to create directory: %w", err)
+ }
+ f, err := os.Create(path)
+ if err != nil {
+ return err
+ }
+ defer func() { _ = f.Close() }()
+ _, err = io.Copy(f, r)
+ return err
}Also applies to: 271-274
🤖 Prompt for AI Agents
In `@cmd/recordapi/main.go` around lines 190 - 205, The code currently calls
io.ReadAll(resp.Body) for all responses then checks if *endpoint has "_stream",
which buffers the entire stream into memory; change the streaming path to stream
directly from resp.Body to disk instead of ReadAll. Concretely, stop using
io.ReadAll for stream endpoints: for the "_stream" branch call a function that
accepts io.Reader (e.g., update writeStreamOutput to take an io.Reader and
filename) and stream with io.Copy(file, resp.Body), handling errors and closing
the file and resp.Body appropriately; leave non-stream responses to use
io.ReadAll as before. Also update the other occurrence that handles streaming
(the block around writeStreamOutput at lines ~271-274) to use the new
reader-based streaming approach.
Comment on lines
+60
to
+62
| client := &http.Client{} | ||
| resp, err := client.Do(req) | ||
| require.NoError(t, err, "failed to send request") |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check if the file exists and view the relevant lines
ls -la tests/integration/helpers_test.go 2>/dev/null && echo "=== File exists ===" || echo "File not found"
# View lines around 60-62 to verify the code
if [ -f tests/integration/helpers_test.go ]; then
echo "=== Lines 50-70 ===" && sed -n '50,70p' tests/integration/helpers_test.go | cat -n
fiRepository: ENTERPILOT/GOModel
Length of output: 922
🏁 Script executed:
# Check for http.Client instances without timeouts in integration tests
echo "=== Searching for http.Client patterns in integration tests ===" && \
rg -n --type=go 'http\.Client\{' tests/integration/ -A 1Repository: ENTERPILOT/GOModel
Length of output: 539
🏁 Script executed:
# Also check the imports to see if time package is already imported
echo "=== Check imports in helpers_test.go ===" && \
head -20 tests/integration/helpers_test.go | cat -nRepository: ENTERPILOT/GOModel
Length of output: 463
Add an HTTP client timeout to prevent hung integration tests.
The client at line 60 lacks a timeout and can stall CI if the server hangs. Other clients in setup_test.go use timeouts (2 * time.Second and 30 * time.Second), so the pattern should be applied consistently. Import the time package and set Timeout: 10 * time.Second on the client.
🛠️ Proposed fix
import (
"bytes"
"context"
"encoding/json"
"fmt"
"io"
"net/http"
"testing"
+ "time"
"github.com/stretchr/testify/require"
@@
- client := &http.Client{}
+ client := &http.Client{Timeout: 10 * time.Second}📝 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
| client := &http.Client{} | |
| resp, err := client.Do(req) | |
| require.NoError(t, err, "failed to send request") | |
| client := &http.Client{Timeout: 10 * time.Second} | |
| resp, err := client.Do(req) | |
| require.NoError(t, err, "failed to send request") |
🤖 Prompt for AI Agents
In `@tests/integration/helpers_test.go` around lines 60 - 62, The HTTP client in
helpers_test.go is created without a timeout (client := &http.Client{}), which
can hang CI; modify the client creation to set Timeout: 10 * time.Second (import
the time package) so requests in the test use a 10s deadline consistent with
other tests (see setup_test.go pattern), then use that client for
client.Do(req).
The usage tracking integration tests were failing because: 1. Provider field was empty - extractors read from response object which doesn't include provider info from upstream APIs 2. Endpoint format mismatch - tests expected short names but handlers pass full HTTP paths Changes: - Add provider parameter to ExtractFromChatResponse and ExtractFromResponsesResponse functions - Update handlers to pass providerType to extractors - Update test expectations to use full endpoint paths - Fix MongoDB timestamp parsing in test helpers (bson.DateTime) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/usage/extractor_test.go (1)
175-199: Missing assertions forProviderandModelfields.Unlike
TestExtractFromChatResponse, this test doesn't verify thatentry.Providerandentry.Modelare correctly populated from the input parameters and response. Consider adding assertions to ensure provider propagation is validated.♻️ Suggested additional assertions
if entry.TotalTokens != tt.wantTotal { t.Errorf("TotalTokens = %d, want %d", entry.TotalTokens, tt.wantTotal) } + if entry.Provider != tt.provider { + t.Errorf("Provider = %s, want %s", entry.Provider, tt.provider) + } + if tt.resp != nil && entry.Model != tt.resp.Model { + t.Errorf("Model = %s, want %s", entry.Model, tt.resp.Model) + } })
♻️ Duplicate comments (2)
tests/integration/usage_test.go (1)
220-221: Hardcoded provider response ID may be brittle.The assertion expects
"chatcmpl-test123"as the provider ID. Consider extracting this as a shared constant or adding a comment referencing where this value is defined in the mock.tests/integration/dbassert/auditlog.go (1)
105-113: Consider addingClearAuditLogsMongofor symmetry.There's
ClearAuditLogsfor PostgreSQL but no MongoDB equivalent. If MongoDB integration tests need cleanup, a corresponding function would maintain consistency.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@tests/contract/gemini_test.go`:
- Around line 67-72: Add a check that each returned model has a non-empty
OwnedBy field to match other provider tests: inside the loop iterating resp.Data
(the same loop that asserts model.ID and model.Object) add an assertion that
model.OwnedBy is not empty (e.g., assert.NotEmpty(t, model.OwnedBy, "model %d:
owned_by should not be empty", i)) so the Gemini contract test validates
ownership consistency alongside resp.Data and model.ID/model.Object checks.
In `@tests/contract/README.md`:
- Around line 164-178: The multimodal example in the curl snippet (model
"gpt-4o-mini") references an external image URL via the "image_url" object which
creates a brittle external dependency; replace that external URL with either a
local test asset served from your test harness (add a small image under testdata
and point "image_url.url" to the local test server) or embed a base64 data URL
("image_url.url":"data:image/png;base64,...") in the README example, and update
any recorded golden/contract examples that reference the old Google logo so
tests no longer depend on external hosting.
- Around line 66-83: Update the Gemini recordapi command in the README: replace
the invalid model identifier "gemini-2.5-flash-preview-09-2025" with the correct
model name "gemini-2.5-flash" in the gemini -endpoint=chat invocation so the
command that generates tests/contract/testdata/gemini/chat_completion.json uses
the valid model.
♻️ Duplicate comments (2)
docs/FUTURE_ARCHITECTURE.md (1)
3-3: Add a language identifier to the fenced block (MD040).Unlabeled fenced block still triggers markdownlint.
♻️ Proposed fix
-``` +```text @@ -``` +```docs/TESTING_STRATEGY.md (1)
22-46: Add language identifiers to the remaining fenced blocks (MD040).The two tree/diagram fences are still unlabeled.
♻️ Proposed fix
-``` +```text @@ -``` +``` @@ -``` +```text @@ -``` +```Also applies to: 135-158
Add workflow-level permissions block to limit GITHUB_TOKEN scope to contents:read, following the principle of least privilege. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@tests/integration/helpers_test.go`:
- Around line 163-193: The test helper forwardStreamingChatRequest (and sibling
forwardStreamingResponsesRequest) return an io.ReadCloser but currently lack
documentation that the caller must close the returned body; add a short comment
line above each function (e.g., "// caller must close") to match the production
provider docs and make the ownership requirement explicit for callers using
forwardStreamingChatRequest and forwardStreamingResponsesRequest.
In `@tests/integration/setup_test.go`:
- Around line 72-132: SetupTestServer currently returns a TestServerFixture but
does not register automatic cleanup, so mockLLM and the context cancel may leak
if a test exits early; update SetupTestServer to call t.Cleanup and ensure it
invokes the fixture.Shutdown (or FlushAndClose plus mock shutdown/cancel) to
stop the application, close the mockLLM (from NewMockLLMServer), and call
cancelFunc; reference TestServerFixture, SetupTestServer, Shutdown (or
FlushAndClose), mockLLM, and cancelFunc to locate where to register t.Cleanup
and run the full teardown sequence.
♻️ Duplicate comments (5)
tests/integration/helpers_test.go (1)
45-65: Add a timeout to the HTTP client. A zero-timeout client can hang integration tests if the server stalls.🛠️ Proposed fix
import ( "bytes" "context" "encoding/json" "fmt" "io" "net/http" "testing" + "time" "github.com/stretchr/testify/require" @@ - client := &http.Client{} + client := &http.Client{Timeout: 10 * time.Second} resp, err := client.Do(req)tests/integration/usage_test.go (1)
199-223: Avoid hardcoding mock provider IDs. This is brittle if the mock changes; prefer a shared constant or a comment pointing to the mock definition.tests/integration/auditlog_test.go (2)
70-111: Add MongoDB parity assertions. Mirror the duration, error-type, and body/header checks present in the PostgreSQL test to keep coverage consistent.
113-144: Verify health endpoint exclusion. Add an explicit/healthrequest with a unique request ID and assert zero audit entries whenOnlyModelInteractionsis true.tests/integration/setup_test.go (1)
104-113: Surface application.Start errors instead of swallowing them. This avoids opaque health-check timeouts.🛠️ Proposed fix
// Start server in background serverURL := fmt.Sprintf("http://127.0.0.1:%d", port) + startErr := make(chan error, 1) go func() { addr := fmt.Sprintf("127.0.0.1:%d", port) - _ = application.Start(addr) + startErr <- application.Start(addr) }() @@ // Wait for server to be healthy err = waitForServer(serverURL + "/health") require.NoError(t, err, "server failed to become healthy") + select { + case err := <-startErr: + require.NoError(t, err, "server failed to start") + default: + }
Deferred functions don't run when os.Exit is called. Move cancelFunc() from defer to explicit calls before both exit paths to ensure the context is always cancelled. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
This was referenced Feb 27, 2026
This was referenced Mar 7, 2026
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
✏️ Tip: You can customize this high-level summary in your review settings.