rollup: June 2026#176
Merged
Merged
Conversation
…p alive after a panic
…er pool fails to build
…endpoints unhealthy
…reference backends
…passthrough routing
… stream on interleaved deltas
…ing models keep their reasoning
…nthropic surface, with profiling
…st-only allocations, not startup noise
…coming-parse allocations
…thropic-further-work
…ening a tool block when id and name repeat
…t so alias changes invalidate it
… reasoning plus content plus tool_calls chunk
…de-code-e2e skill, hot-path tuning and review fixes
… final usage chunks aren't dropped
…ed panic returns 502 not a 200 sse error
… nil-safety for interface-typed pools
…ated SSE keeps flushing
…am omits usage, with real backend usage still winning
…n include_usage backend
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (16)
🚧 Files skipped from review as they are similar to previous changes (10)
WalkthroughThis PR adds a live Claude Code E2E skill, expands Anthropic translation for reasoning, token counting, models, and streaming behaviour, refines passthrough and panic handling in handlers, and hardens several runtime, pooling, shutdown, logging, and utility paths with broader tests. ChangesAnthropic translation and runtime hardening
Sequence Diagram(s)sequenceDiagram
participant Client
participant TranslationHandler
participant AnthropicTranslator
participant Backend
Client->>TranslationHandler: Anthropic request
TranslationHandler->>TranslationHandler: detect features / choose path
alt Passthrough
TranslationHandler->>Backend: native Anthropic messages request
Backend-->>TranslationHandler: Anthropic-compatible response
else Translation
TranslationHandler->>AnthropicTranslator: transform request
TranslationHandler->>Backend: OpenAI request
Backend-->>TranslationHandler: OpenAI response or stream
TranslationHandler->>AnthropicTranslator: transform response
end
TranslationHandler-->>Client: Anthropic response or SSE error event
Estimated code review effort🎯 5 (Critical) | ⏱️ ~110 minutes Possibly related PRs
Suggested labels
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
🧹 Nitpick comments (1)
internal/adapter/health/checker_test.go (1)
796-811: ⚡ Quick winReplace fixed sleep with deadline polling to reduce CI flakiness.
Line 797 uses a fixed delay; on a busy runner the second tick may not occur in time, causing intermittent false failures. Poll for
calls >= 2with a bounded timeout instead.Proposed refactor
- // Wait long enough for two ticks (the first panics, the second must succeed). - time.Sleep(120 * time.Millisecond) + // Wait until two ticks are observed or timeout. + deadline := time.Now().Add(500 * time.Millisecond) + for time.Now().Before(deadline) { + panicRepo.mu.Lock() + calls := panicRepo.calls + panicRepo.mu.Unlock() + if calls >= 2 { + break + } + time.Sleep(5 * time.Millisecond) + } // isRunning must still be true - the loop survived the panic. if !checker.isRunning.Load() { t.Error("isRunning is false after tick panic; loop likely died") }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/adapter/health/checker_test.go` around lines 796 - 811, In checker_test.go within the panic-recovery test around checker.isRunning and panicRepo.calls, replace the fixed time.Sleep-based wait with bounded deadline polling until panicRepo.calls reaches at least 2 or the timeout expires, then assert both the loop survived and the second tick occurred; keep the existing verification logic but make the wait condition-driven instead of time-based.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.claude/skills/claude-code-e2e/SKILL.md:
- Line 168: The fitness preflight check at line 168 immediately exits when the
OLLAMA endpoint is unreachable, but it should first check if the default
endpoint was used and no custom endpoint flag was provided. If the default
endpoint is unreachable, invoke AskUserQuestion to prompt the user for an
alternative endpoint, then retry the curl check against the new endpoint before
exiting. Only exit with failure if the retry also fails. Update the if-block
that checks the curl command against OLLAMA to implement this conditional retry
logic, distinguishing between the default endpoint scenario (which requires user
interaction) and custom endpoint scenarios (which should fail immediately).
In `@internal/adapter/health/checker.go`:
- Around line 152-164: The health check tick work is creating a timeout context
from context.Background() on line 162, which detaches it from shutdown
cancellation and prevents in-flight checks from being cancelled when the service
stops. Replace context.Background() with the loop context that is passed to this
goroutine (which should be available in the closure and is cancelled during
shutdown). This ensures that when the main loop context is cancelled during
service shutdown, any in-flight performHealthChecks calls will also be cancelled
rather than continuing to completion.
In `@internal/adapter/proxy/olla/service_retry.go`:
- Around line 141-144: The buffer pool acquisition failure at the beginning of
the function returns an error before calling s.RecordFailure(...), which
bypasses normal failure accounting and error shaping for the streaming path.
Move the s.RecordFailure(...) call to occur before the early return when poolErr
is not nil, ensuring that buffer-pool acquisition failures are recorded and
shaped consistently with other failures in the streaming flow.
In `@internal/adapter/proxy/olla/service.go`:
- Around line 377-379: Replace the em-dashes in the affected Go comments with
plain punctuation to match repository documentation style and the no-em-dash
guideline. Update the comments in internal/adapter/proxy/olla/service.go at the
nil-guard and tick-recovery locations,
internal/app/middleware/logging_gate_test.go at the request-ID and non-proxy
info-level comments, internal/app/services/http_bind_test.go at the bind-path
comment, and pkg/eventbus/eventbus_worker.go at the shutdown race explanation so
they read naturally with standard punctuation only.
In `@internal/adapter/proxy/sherpa/service_retry.go`:
- Around line 130-133: In the stream-buffer acquisition failure branch inside
the sherpa retry flow, make sure this early return is tracked the same way as
the other request failures by recording it with s.RecordFailure(...) before
returning the wrapped error. Update the failure path in the same function that
calls s.bufferPool.Get() so the buffer-unavailable case uses the same accounting
as the rest of the retry logic.
In `@internal/adapter/stats/collector_test.go`:
- Around line 405-407: The test generates only a bounded keyspace of unique IP
identifiers due to the modulo operations in the string concatenation (26 letters
× 10 digits = 260 max combinations), which means the collector may never
actually reach the MaxUniqueRateLimitedIPs cap if that limit is set higher. Fix
the IP generation in the loop iterating over MaxUniqueRateLimitedIPs * 2 in the
recordRateLimitedIP calls to use unbounded unique values (such as converting the
counter i directly to a string) instead of modulo-based arithmetic, ensuring the
test definitively exercises the capping behavior regardless of the
MaxUniqueRateLimitedIPs value. This fix applies to both instances: the one
around line 405-407 and the one around line 456-459.
In `@internal/adapter/stats/collector.go`:
- Around line 287-292: Update the rate-limited IP tracking logic in the
collector so that existing entries always refresh their timestamp, even when the
map has reached MaxUniqueRateLimitedIPs; only skip inserting a brand-new
clientIP when at capacity. In the code block guarded by c.securityMu.Lock() that
updates c.uniqueRateLimitedIPs, check whether clientIP already exists and always
assign now for existing keys, while keeping the hard-cap guard only for new keys
so active offenders do not age out incorrectly.
In `@internal/adapter/translator/anthropic/reasoning_test.go`:
- Line 176: The inline comment on the line containing "content_block_stop" uses
an em dash (—) which violates the repository's documentation style rule
requiring Australian English with no em-dashes. Replace the em dash in the
comment "// index 0 — thinking closed before text opens" with a standard hyphen
or double hyphen to conform to the coding guidelines.
In `@internal/adapter/translator/anthropic/streaming_bench_test.go`:
- Around line 230-232: The benchmark loop in processStreamLine is reusing
mutable benchmark state and ignoring returned errors, which can distort timing
and hide failures. Update the benchmark around tr.processStreamLine to create
fresh per-iteration state/record inputs (or reset them each iteration) and
explicitly check the returned error so the benchmark stops on processing
failures. Keep the fix localized to the benchmark in streaming_bench_test.go and
preserve the existing benchmark setup outside the measured loop.
- Line 151: The comment "It is not wired into any production path — delete after
measurements are taken." contains an em dash which violates the repository style
guide that prohibits em-dashes in comments and documentation. Replace the em
dash character (—) between "path" and "delete" with a regular hyphen (-) or
reword the sentence to use standard punctuation that complies with Australian
English style guidelines.
In `@internal/adapter/translator/anthropic/streaming_parse_test.go`:
- Around line 20-21: The test comment at the mockOpenAIStream call in
streaming_parse_test.go contains an em dash (—) which violates the repository's
style guidelines requiring Australian English with no em-dashes in comments.
Replace the em dash with appropriate punctuation that conforms to the project's
coding standards, such as restructuring the comment to use a hyphen, semicolon,
or simply removing the problematic punctuation while maintaining clarity.
In `@internal/adapter/translator/anthropic/streaming_test.go`:
- Around line 457-460: The comment block in the
TestStreamingInterleavedToolStartsAndArgs test contains em dashes which violate
the repository's coding standards that specify no em-dashes in comments for Go
files. Replace the em dashes in the comment with repository-standard punctuation
such as hyphens, semicolons, or other appropriate punctuation marks while
maintaining clarity and readability of the explanation about the
single-active-block invariant and tool argument handling behavior.
In `@internal/adapter/translator/anthropic/streaming.go`:
- Around line 17-20: New comments in openAIChunk use em dash punctuation, which
violates the repository comment style. Update the affected comment text in
streaming.go to replace the em dash with a comma or hyphen while keeping the
wording and meaning intact, and ensure any other newly added comment in this
file follows the same Australian English documentation rule with no em dashes.
In `@internal/adapter/translator/anthropic/token_count.go`:
- Around line 70-73: Standardise the changed Go comments to comply with the
repository writing rule by replacing all em dashes with commas or parentheses
across the affected comment blocks. Update the anchor in
internal/adapter/translator/anthropic/token_count.go#L70-L73 and the sibling
comments in internal/adapter/translator/anthropic/token_count_test.go#L425-L427,
internal/adapter/translator/anthropic/translator_test.go#L177-L185,
internal/app/handlers/handler_translator_models.go#L182-L184, and
internal/app/handlers/handler_translator_models_test.go#L103-L104; only adjust
the comment wording, keeping the intent and meaning the same.
In `@internal/app/handlers/handler_translation.go`:
- Around line 966-974: Move the success response in handler_translation.go’s
token-count path so `w.WriteHeader(http.StatusOK)` is only called after
`translator.TokenCountSerializer.SerialiseCountTokens` succeeds; if `serErr` is
returned, log and exit without sending 200. Update the logic around
`serialiser.SerialiseCountTokens` and the `w.Header().Set`/`WriteHeader`
sequence so failures never commit an OK status before the body is known to be
valid.
In `@internal/app/handlers/handler_translator_models_cache_test.go`:
- Around line 280-287: Refactor the doGetModels helper function to return
([]byte, error) instead of calling t.Fatalf. Replace the if rr.Code !=
http.StatusOK check with an error return, and remove the t.Fatalf call. Update
the parent test goroutine at line 139 where doGetModels is invoked to handle the
returned error properly by asserting the error is nil within the test goroutine
context, ensuring all test assertions happen on the main test goroutine rather
than worker goroutines.
In `@internal/app/middleware/logging_gate_test.go`:
- Around line 116-145: The benchmark function
BenchmarkEnhancedLogging_ProxyPath_InfoLevel measures middleware logging
behavior rather than balancer or engine performance, which violates the
repository's benchmark policy that requires benchmarks in _test.go files to
focus on balancer and engine performance. Remove this benchmark function
entirely from the logging_gate_test.go file, including its documentation comment
and the entire function body from BenchmarkEnhancedLogging_ProxyPath_InfoLevel
through its closing brace.
In `@internal/app/middleware/logging.go`:
- Around line 142-146: The comment block in logging.go needs to conform to the
repository’s Go comment style by removing the em dash punctuation. Update the
affected comment near the record emission gate in the logging middleware to use
Australian English wording without em dashes, keeping the intent the same and
preserving the existing explanation around the Debug/Info logging path.
In `@internal/app/services/http_bind_test.go`:
- Around line 16-46: Update the HTTP service tests so they exercise the real
startup bind path instead of only calling net.Listen; in
TestHTTPService_Start_DuplicatePortReturnsError and the sibling test site,
invoke HTTPService.Start or a small helper extracted from its bind logic so
regressions in startup binding are covered. Keep NewHTTPService only as setup,
and ensure the test asserts the error returned by the actual service bind path
rather than the standalone listener behavior.
In `@pkg/format/format_test.go`:
- Around line 23-27: Replace the em-dashes with regular hyphens or dashes in the
test comments to maintain consistency with Go coding guidelines that prohibit
em-dashes in comments and documentation. Specifically, in the comments above the
test cases at lines 23 and 27 in the format_test.go file, change the em-dash
characters (—) to regular hyphens (-) or spaces as appropriate to maintain
readability while adhering to the Australian English punctuation style required
for Go files.
---
Nitpick comments:
In `@internal/adapter/health/checker_test.go`:
- Around line 796-811: In checker_test.go within the panic-recovery test around
checker.isRunning and panicRepo.calls, replace the fixed time.Sleep-based wait
with bounded deadline polling until panicRepo.calls reaches at least 2 or the
timeout expires, then assert both the loop survived and the second tick
occurred; keep the existing verification logic but make the wait
condition-driven instead of time-based.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2d518a53-06e2-40d0-9ee7-d1a20599d8a2
📒 Files selected for processing (71)
.claude/skills/claude-code-e2e/SKILL.md.claude/skills/claude-code-e2e/config.regression.yaml.tmpl.claude/skills/claude-code-e2e/tasks/totalsize.md.claude/skills/claude-code-e2e/tasks/totalsize.prompt.txt.claude/skills/olla-validate/areas/anthropic.md.claude/skills/olla-validate/areas/limits-failures.md.claude/skills/olla-validate/areas/openai-api.md.gitignoredocs/content/api-reference/anthropic.mddocs/content/concepts/api-translation.mddocs/content/integrations/api-translation/anthropic.mdinternal/adapter/health/checker.gointernal/adapter/health/checker_test.gointernal/adapter/inspector/body_inspector.gointernal/adapter/proxy/benchmark_refactor_test.gointernal/adapter/proxy/core/retry.gointernal/adapter/proxy/core/retry_safety_test.gointernal/adapter/proxy/olla/service.gointernal/adapter/proxy/olla/service_concurrency_test.gointernal/adapter/proxy/olla/service_leak_test.gointernal/adapter/proxy/olla/service_retry.gointernal/adapter/proxy/sherpa/service_retry.gointernal/adapter/stats/collector.gointernal/adapter/stats/collector_test.gointernal/adapter/translator/anthropic/constants.gointernal/adapter/translator/anthropic/extended_features_test.gointernal/adapter/translator/anthropic/integration_test.gointernal/adapter/translator/anthropic/passthrough_test.gointernal/adapter/translator/anthropic/path_translation_test.gointernal/adapter/translator/anthropic/reasoning_test.gointernal/adapter/translator/anthropic/request.gointernal/adapter/translator/anthropic/request_test.gointernal/adapter/translator/anthropic/response.gointernal/adapter/translator/anthropic/response_test.gointernal/adapter/translator/anthropic/security_test.gointernal/adapter/translator/anthropic/streaming.gointernal/adapter/translator/anthropic/streaming_bench_test.gointernal/adapter/translator/anthropic/streaming_parse_test.gointernal/adapter/translator/anthropic/streaming_test.gointernal/adapter/translator/anthropic/streaming_test_helpers.gointernal/adapter/translator/anthropic/token_count.gointernal/adapter/translator/anthropic/token_count_test.gointernal/adapter/translator/anthropic/translator.gointernal/adapter/translator/anthropic/translator_test.gointernal/adapter/translator/anthropic/types.gointernal/adapter/translator/types.gointernal/adapter/unifier/lifecycle_unifier.gointernal/app/handlers/application.gointernal/app/handlers/handler_security_status_codes_test.gointernal/app/handlers/handler_translation.gointernal/app/handlers/handler_translation_passthrough_test.gointernal/app/handlers/handler_translation_test.gointernal/app/handlers/handler_translator_models.gointernal/app/handlers/handler_translator_models_cache_test.gointernal/app/handlers/handler_translator_models_test.gointernal/app/middleware/logging.gointernal/app/middleware/logging_gate_test.gointernal/app/services/http.gointernal/app/services/http_bind_test.gointernal/app/services/proxy.gointernal/app/services/proxy_cleanup_test.gointernal/core/constants/context.gointernal/core/domain/profile_config.gointernal/util/request.gopkg/eventbus/eventbus_worker.gopkg/eventbus/eventbus_worker_test.gopkg/format/format.gopkg/format/format_test.gopkg/pool/lite_pool.gopkg/pool/lite_pool_test.gotest/cmd/ollamock/streaming.go
|
|
||
| ```bash | ||
| # 1) endpoint reachable; if not and no --ollama-endpoint was given, ASK the user. | ||
| if ! curl -sf --connect-timeout 5 "${OLLAMA}/api/version" >/dev/null; then |
There was a problem hiding this comment.
ASK the user is invoked only if Ollama is unreachable and no flag given.
Line 169 says "AskUserQuestion for an endpoint, retry once" in a comment, but the code calls exit 1 immediately without prompting. According to the logic at line 48, if the default endpoint is unreachable and no --ollama-endpoint was given, the skill should invoke AskUserQuestion and retry, not immediately exit.
The fitness preflight currently exits hard on endpoint unreachability. The documented behaviour (lines 48–50) says: "If that is not reachable and no flag was given, ask the user (AskUserQuestion) for an endpoint rather than failing silently." This contract is not honoured in the code.
Suggested fix approach
The fitness preflight at line 168 should check if the default endpoint was used; if so, invoke AskUserQuestion and retry before failing. Something like:
if ! curl -sf --connect-timeout 5 "${OLLAMA}/api/version" >/dev/null; then
if [ "$OLLAMA" = "http://localhost:11434" ]; then
# Default endpoint failed; ask the user for an alternative
read -p "Ollama not reachable at ${OLLAMA}. Enter Ollama endpoint URL: " OLLAMA
if ! curl -sf --connect-timeout 5 "${OLLAMA}/api/version" >/dev/null; then
echo "FAIL: Ollama not reachable at ${OLLAMA} (retried after user input)"
exit 1
fi
else
echo "FAIL: Ollama not reachable at ${OLLAMA}"
exit 1
fi
fiAlternatively, if AskUserQuestion is a skill framework invocation, use that instead of read -p.
🧰 Tools
🪛 SkillSpector (2.1.1)
[warning] 180: [E1] External Transmission: Data is being sent to an external URL. This could be legitimate telemetry or data exfiltration. Manual review is recommended.
Remediation: Verify the destination URL is trusted and necessary. Remove or replace with documented APIs. Ensure no secrets, tokens, or PII are transmitted.
(Data Exfiltration (E1))
[warning] 189: [E1] External Transmission: Data is being sent to an external URL. This could be legitimate telemetry or data exfiltration. Manual review is recommended.
Remediation: Verify the destination URL is trusted and necessary. Remove or replace with documented APIs. Ensure no secrets, tokens, or PII are transmitted.
(Data Exfiltration (E1))
[warning] 204: [E1] External Transmission: Data is being sent to an external URL. This could be legitimate telemetry or data exfiltration. Manual review is recommended.
Remediation: Verify the destination URL is trusted and necessary. Remove or replace with documented APIs. Ensure no secrets, tokens, or PII are transmitted.
(Data Exfiltration (E1))
[warning] 310: [E1] External Transmission: Data is being sent to an external URL. This could be legitimate telemetry or data exfiltration. Manual review is recommended.
Remediation: Verify the destination URL is trusted and necessary. Remove or replace with documented APIs. Ensure no secrets, tokens, or PII are transmitted.
(Data Exfiltration (E1))
[warning] 368: [E1] External Transmission: Data is being sent to an external URL. This could be legitimate telemetry or data exfiltration. Manual review is recommended.
Remediation: Verify the destination URL is trusted and necessary. Remove or replace with documented APIs. Ensure no secrets, tokens, or PII are transmitted.
(Data Exfiltration (E1))
[warning] 562: [E1] External Transmission: Data is being sent to an external URL. This could be legitimate telemetry or data exfiltration. Manual review is recommended.
Remediation: Verify the destination URL is trusted and necessary. Remove or replace with documented APIs. Ensure no secrets, tokens, or PII are transmitted.
(Data Exfiltration (E1))
[error] 144: [TM1] Tool Parameter Abuse: Tool parameters are crafted to achieve unintended or unsafe behavior. Parameter abuse can bypass intended safety checks (e.g. shell=True, --force, dangerous glob patterns).
Remediation: Validate all tool parameters against an allowlist. Reject dangerous parameter values (shell=True, --force, -rf /) and use safe defaults.
(Tool Misuse (TM1))
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.claude/skills/claude-code-e2e/SKILL.md at line 168, The fitness preflight
check at line 168 immediately exits when the OLLAMA endpoint is unreachable, but
it should first check if the default endpoint was used and no custom endpoint
flag was provided. If the default endpoint is unreachable, invoke
AskUserQuestion to prompt the user for an alternative endpoint, then retry the
curl check against the new endpoint before exiting. Only exit with failure if
the retry also fails. Update the if-block that checks the curl command against
OLLAMA to implement this conditional retry logic, distinguishing between the
default endpoint scenario (which requires user interaction) and custom endpoint
scenarios (which should fail immediately).
Comment on lines
+377
to
+379
| // Guard against a nil current pointer — only reachable if UpdateConfig is | ||
| // called on a zero-value Service (e.g. in tests) before NewService stores | ||
| // the initial configuration. |
There was a problem hiding this comment.
Replace em-dashes in changed Go comments to match repository documentation standards.
Use plain punctuation in these comment lines to keep style consistent and avoid violating the repo standard.
internal/adapter/proxy/olla/service.go#L377-L379: replace the em-dash in the nil-guard comment.internal/adapter/proxy/olla/service.go#L405-L407: replace the em-dash in the tick-recovery comment.internal/app/middleware/logging_gate_test.go#L66-L66: replace the em-dash in the request-ID comment.internal/app/middleware/logging_gate_test.go#L99-L99: replace the em-dash in the non-proxy info-level comment.internal/app/services/http_bind_test.go#L30-L30: replace the em-dash in the bind-path comment.pkg/eventbus/eventbus_worker.go#L82-L83: replace the em-dash in the shutdown race explanation.
As per coding guidelines, "**/*.{go,md,yaml,yml}: Use Australian English for comments and documentation. No em-dashes".
📍 Affects 4 files
internal/adapter/proxy/olla/service.go#L377-L379(this comment)internal/adapter/proxy/olla/service.go#L405-L407internal/app/middleware/logging_gate_test.go#L66-L66internal/app/middleware/logging_gate_test.go#L99-L99internal/app/services/http_bind_test.go#L30-L30pkg/eventbus/eventbus_worker.go#L82-L83
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/adapter/proxy/olla/service.go` around lines 377 - 379, Replace the
em-dashes in the affected Go comments with plain punctuation to match repository
documentation style and the no-em-dash guideline. Update the comments in
internal/adapter/proxy/olla/service.go at the nil-guard and tick-recovery
locations, internal/app/middleware/logging_gate_test.go at the request-ID and
non-proxy info-level comments, internal/app/services/http_bind_test.go at the
bind-path comment, and pkg/eventbus/eventbus_worker.go at the shutdown race
explanation so they read naturally with standard punctuation only.
Source: Coding guidelines
Comment on lines
+116
to
+145
| // BenchmarkEnhancedLogging_ProxyPath_InfoLevel measures the hot-path overhead | ||
| // of EnhancedLoggingMiddleware at the default info level for proxy requests. | ||
| // At info level all Debug records are suppressed, so formatBytes, the []any field | ||
| // slices, and fmt.Sprintf must be skipped entirely. | ||
| // | ||
| // Run with: | ||
| // | ||
| // go test -bench=BenchmarkEnhancedLogging_ProxyPath_InfoLevel -benchmem ./internal/app/middleware/ | ||
| func BenchmarkEnhancedLogging_ProxyPath_InfoLevel(b *testing.B) { | ||
| ch := &countingHandler{minLevel: slog.LevelInfo} | ||
| prev := slog.Default() | ||
| slog.SetDefault(slog.New(ch)) | ||
| b.Cleanup(func() { slog.SetDefault(prev) }) | ||
|
|
||
| mockLogger := &mockStyledLogger{} | ||
| inner := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| w.WriteHeader(http.StatusOK) | ||
| }) | ||
|
|
||
| mw := EnhancedLoggingMiddleware(mockLogger)(inner) | ||
|
|
||
| b.ResetTimer() | ||
| b.ReportAllocs() | ||
|
|
||
| for range b.N { | ||
| req := httptest.NewRequest(http.MethodPost, "/olla/ollama/api/chat", nil) | ||
| rr := httptest.NewRecorder() | ||
| mw.ServeHTTP(rr, req) | ||
| } | ||
| } |
There was a problem hiding this comment.
Benchmark scope does not match repository benchmark policy.
This benchmark measures middleware logging behaviour, not balancer or engine performance, so it does not align with the benchmark scope required for _test.go files.
As per coding guidelines, "**/*_test.go: ... benchmark tests must measure balancer and engine performance".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/app/middleware/logging_gate_test.go` around lines 116 - 145, The
benchmark function BenchmarkEnhancedLogging_ProxyPath_InfoLevel measures
middleware logging behavior rather than balancer or engine performance, which
violates the repository's benchmark policy that requires benchmarks in _test.go
files to focus on balancer and engine performance. Remove this benchmark
function entirely from the logging_gate_test.go file, including its
documentation comment and the entire function body from
BenchmarkEnhancedLogging_ProxyPath_InfoLevel through its closing brace.
Source: Coding guidelines
Comment on lines
+142
to
+146
| // Gate field construction on whether the record will actually be emitted. | ||
| // On the proxy hot path at the default info level, Debug records are discarded | ||
| // by the handler — building the []any slice and calling formatBytes 2x per | ||
| // request is pure waste. Non-proxy requests log at Info, so they only pay the | ||
| // cost when info-level logging is actually enabled. |
There was a problem hiding this comment.
Replace em dash punctuation in changed Go comments.
This comment block uses em dash punctuation, which violates the repository comment style rule.
As per coding guidelines, **/*.{go,md,yaml,yml}: "Use Australian English for comments and documentation. No em-dashes".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/app/middleware/logging.go` around lines 142 - 146, The comment block
in logging.go needs to conform to the repository’s Go comment style by removing
the em dash punctuation. Update the affected comment near the record emission
gate in the logging middleware to use Australian English wording without em
dashes, keeping the intent the same and preserving the existing explanation
around the Debug/Info logging path.
Source: Coding guidelines
Comment on lines
+16
to
+46
| func TestHTTPService_Start_DuplicatePortReturnsError(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| // Grab an ephemeral port on localhost so the test is portable. | ||
| anchor, err := net.Listen("tcp", "127.0.0.1:0") | ||
| if err != nil { | ||
| t.Fatalf("failed to acquire anchor listener: %v", err) | ||
| } | ||
| defer anchor.Close() | ||
|
|
||
| port := anchor.Addr().(*net.TCPAddr).Port | ||
|
|
||
| // We cannot call svc.Start() directly here because it initialises full | ||
| // application dependencies. Exercise the bind path directly — this mirrors | ||
| // exactly what Start() does internally after the setup phase. | ||
| _ = NewHTTPService( | ||
| &config.ServerConfig{ | ||
| Host: "127.0.0.1", | ||
| Port: port, | ||
| }, | ||
| &config.Config{Server: config.ServerConfig{Host: "127.0.0.1", Port: port}}, | ||
| newTestLogger(), | ||
| ) | ||
|
|
||
| ln, bindErr := net.Listen("tcp", anchor.Addr().String()) | ||
| if bindErr == nil { | ||
| ln.Close() | ||
| t.Fatal("expected bind to fail while anchor holds the port, but it succeeded") | ||
| } | ||
| // bindErr is non-nil — the synchronous bind correctly surfaced the error. | ||
| } |
There was a problem hiding this comment.
These tests do not exercise HTTPService.Start, so they can miss real regressions.
Both cases only validate net.Listen behaviour. They should hit the actual Start bind path (or a helper extracted from it) so failures in service startup logic are caught.
As per coding guidelines, "**/*_test.go: Unit tests must isolate functionality".
Also applies to: 50-58
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/app/services/http_bind_test.go` around lines 16 - 46, Update the
HTTP service tests so they exercise the real startup bind path instead of only
calling net.Listen; in TestHTTPService_Start_DuplicatePortReturnsError and the
sibling test site, invoke HTTPService.Start or a small helper extracted from its
bind logic so regressions in startup binding are covered. Keep NewHTTPService
only as setup, and ensure the test asserts the error returned by the actual
service bind path rather than the standalone listener behavior.
Source: Coding guidelines
…eturns an error, not an empty 200
…tdown cancels in-flight checks
…the streaming benchmark per iteration
…he other retry failure paths
thushan
added
enhancement
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.
June 2026 fixes
Hardened streaming translation for Anthropic/OpenAI-compatible flows:
usagechunks withchoices: []output_tokenswhen upstream usage is absentImproved translated SSE reliability:
Flush/Unwrapon wrapped response writers so translated streams flush correctlyHardened proxy and lifecycle concurrency:
Improved pool safety:
litepool.Getreturn errors instead of panickingOptimised hot paths:
Improved Anthropic compatibility:
/v1/modelsshape for Anthropic clientsmessage_startImproved server/security behaviour:
Added broad test coverage for:
Summary by CodeRabbit
New Features
Bug Fixes