Conversation
save binary so we can go back
fix ANSI code in output logs fix pathing issues fix missing log files fix config resolution errors fix missing test results
|
Warning Rate limit exceeded@thushan has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 11 minutes and 52 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (41)
WalkthroughThis change introduces a new "profile" setting for proxy streaming behaviour, allowing configuration between "auto", "streaming", and "standard" modes. The logic for detecting and handling streaming responses is refactored and expanded, with new code and tests for content-type-based streaming detection. Proxy configuration structures are updated, and comprehensive automated and manual test scripts are added for all engine/profile combinations. Documentation and example configurations are updated to reflect the new options and terminology. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Proxy
participant Upstream
Client->>Proxy: HTTP request
Proxy->>Upstream: Forward request
Upstream-->>Proxy: HTTP response (with Content-Type)
Proxy->>Proxy: Determine streaming mode (profile + content-type)
alt Profile is "streaming" or streaming content-type
Proxy-->>Client: Stream response with flushes
else Profile is "standard" or binary content-type
Proxy-->>Client: Send response as standard HTTP (buffered)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes ✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
🔭 Outside diff range comments (1)
test/scripts/cases/view-test-results.sh (1)
1-194: Fix file line endings to resolve carriage return errors.The static analysis tools detect carriage return characters throughout the entire file, indicating Windows-style line endings (CRLF) instead of Unix-style (LF). This will cause issues on Unix systems.
Run this script to fix the line endings:
#!/bin/bash # Description: Fix line endings in the shell script tr -d '\r' < test/scripts/cases/view-test-results.sh > test/scripts/cases/view-test-results.sh.tmp mv test/scripts/cases/view-test-results.sh.tmp test/scripts/cases/view-test-results.sh
🧹 Nitpick comments (8)
internal/core/constants/content.go (1)
4-5: Consider consolidating duplicate JSON content type constants.Both
DefaultContentTypeJSONandContentTypeJSONhave identical values. This duplication could lead to confusion about their intended usage. Consider either:
- Using a single constant if they serve the same purpose
- Adding comments to clarify their different use cases
- Removing one if redundant
const ( - DefaultContentTypeJSON = "application/json" ContentTypeJSON = "application/json" ContentTypeText = "text/plain" ContentTypeHeader = "Content-Type" )test/scripts/streaming/README.md (1)
135-136: Consider using en dashes for numerical ranges.The static analysis tool correctly identified that numerical ranges should use en dashes (–) instead of hyphens (-) for improved typography.
-**Chunk Count**: -- 1-2 chunks: Definitely standard mode -- 3-10 chunks: Possibly standard mode or very short response +**Chunk Count**: +- 1–2 chunks: Definitely standard mode +- 3–10 chunks: Possibly standard mode or very short responseinternal/adapter/proxy/core/streaming.go (1)
67-86: Consider adding more comprehensive binary type coverage.The binary type detection is good but could benefit from additional common types like WebP images, AVIF, and newer document formats.
var binaryPrefixes = []string{ "image/", "video/", "audio/", "application/pdf", "application/zip", "application/gzip", "application/x-tar", "application/x-rar", "application/x-7z", "font/", "model/", // 3D models, CAD files + "application/wasm", // WebAssembly } var binaryTypes = []string{ "application/octet-stream", "application/vnd.ms-excel", "application/vnd.openxmlformats-officedocument", "application/msword", "application/vnd.ms-powerpoint", + "application/vnd.sqlite3", + "application/x-executable", }test/scripts/streaming/test-streaming-detection.py (1)
366-366: Remove unnecessary f-string prefix.The f-string has no placeholders, so the
fprefix is unnecessary.- self.print_color(GREEN, f" Mode: STANDARD (as expected with stream:false)") + self.print_color(GREEN, " Mode: STANDARD (as expected with stream:false)")internal/adapter/proxy/sherpa/service_streaming.go (1)
225-225: Clean up informal comment for professionalism.- // client has gone gone, but we'll finish the current chunk, chunky stuff! + // client has disconnected, but we'll finish the current chunktest/scripts/cases/_streaming_tests.sh (1)
68-76: SC2155: avoid masking$?with command substitutionUsing
local current_dir=$(pwd)overwrites$?before you can test the
previous command. Declare first, assign next.- local current_dir=$(pwd) + local current_dir + current_dir=$(pwd)internal/adapter/proxy/core/streaming_test.go (1)
225-225: Consider using a typed context key instead of string literalUsing string literals as context keys can lead to collisions. Consider defining a typed key.
Add a typed context key:
// Add at package level type contextKey string const streamContextKey contextKey = "stream" // Then use it in tests: ctx = context.WithValue(ctx, streamContextKey, tt.ctxStream)Also applies to: 337-337, 537-537
internal/adapter/proxy/proxy_streaming_profiles_test.go (1)
220-240: Consider extracting proxy creation logic to reduce duplicationThe proxy creation logic is duplicated across multiple test functions.
Create a helper function:
func createProxyForSuite(suite ProxyTestSuite, profile string, discovery ports.DiscoveryService, selector domain.EndpointSelector, collector ports.StatsCollector) ports.ProxyService { if suite.Name() == "Sherpa" { config := &sherpa.Configuration{ Profile: profile, ResponseTimeout: 5 * time.Second, ReadTimeout: 5 * time.Second, StreamBufferSize: 8192, } return suite.CreateProxy(discovery, selector, config, collector) } config := &olla.Configuration{ Profile: profile, ResponseTimeout: 5 * time.Second, ReadTimeout: 5 * time.Second, StreamBufferSize: 8192, MaxIdleConns: 10, IdleConnTimeout: 90 * time.Second, MaxConnsPerHost: 5, } return suite.CreateProxy(discovery, selector, config, collector) }Also applies to: 312-331, 513-532
test/scripts/cases/_olla.sh
Outdated
| local current_dir=$(pwd) | ||
| cd "$PROJECT_ROOT" | ||
|
|
||
| # Get short hash | ||
| local git_hash=$(git rev-parse --short HEAD 2>/dev/null || echo "unknown") | ||
|
|
||
| # Check if working directory is clean | ||
| if [[ -n "$(git status --porcelain 2>/dev/null)" ]]; then | ||
| echo "olla-${git_hash}-wip" | ||
| else | ||
| echo "olla-${git_hash}" | ||
| fi | ||
|
|
||
| cd "$current_dir" |
There was a problem hiding this comment.
Add error handling for directory changes and variable declarations
The function has several issues that could lead to silent failures:
cdcommands don't check for failure- Variable declaration combined with assignment masks return values
Apply this fix:
get_git_version() {
- local current_dir=$(pwd)
- cd "$PROJECT_ROOT"
+ local current_dir
+ current_dir=$(pwd) || return 1
+ cd "$PROJECT_ROOT" || return 1
# Get short hash
- local git_hash=$(git rev-parse --short HEAD 2>/dev/null || echo "unknown")
+ local git_hash
+ git_hash=$(git rev-parse --short HEAD 2>/dev/null || echo "unknown")
# Check if working directory is clean
if [[ -n "$(git status --porcelain 2>/dev/null)" ]]; then
echo "olla-${git_hash}-wip"
else
echo "olla-${git_hash}"
fi
- cd "$current_dir"
+ cd "$current_dir" || return 1
}📝 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.
| local current_dir=$(pwd) | |
| cd "$PROJECT_ROOT" | |
| # Get short hash | |
| local git_hash=$(git rev-parse --short HEAD 2>/dev/null || echo "unknown") | |
| # Check if working directory is clean | |
| if [[ -n "$(git status --porcelain 2>/dev/null)" ]]; then | |
| echo "olla-${git_hash}-wip" | |
| else | |
| echo "olla-${git_hash}" | |
| fi | |
| cd "$current_dir" | |
| get_git_version() { | |
| local current_dir | |
| current_dir=$(pwd) || return 1 | |
| cd "$PROJECT_ROOT" || return 1 | |
| # Get short hash | |
| local git_hash | |
| git_hash=$(git rev-parse --short HEAD 2>/dev/null || echo "unknown") | |
| # Check if working directory is clean | |
| if [[ -n "$(git status --porcelain 2>/dev/null)" ]]; then | |
| echo "olla-${git_hash}-wip" | |
| else | |
| echo "olla-${git_hash}" | |
| fi | |
| cd "$current_dir" || return 1 | |
| } |
🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 10-10: Declare and assign separately to avoid masking return values.
(SC2155)
[error] 10-10: Literal carriage return. Run script through tr -d '\r' .
(SC1017)
[warning] 11-11: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
[error] 11-11: Literal carriage return. Run script through tr -d '\r' .
(SC1017)
[error] 12-12: Literal carriage return. Run script through tr -d '\r' .
(SC1017)
[error] 13-13: Literal carriage return. Run script through tr -d '\r' .
(SC1017)
[warning] 14-14: Declare and assign separately to avoid masking return values.
(SC2155)
[error] 14-14: Literal carriage return. Run script through tr -d '\r' .
(SC1017)
[error] 15-15: Literal carriage return. Run script through tr -d '\r' .
(SC1017)
[error] 16-16: Literal carriage return. Run script through tr -d '\r' .
(SC1017)
[error] 17-17: Literal carriage return. Run script through tr -d '\r' .
(SC1017)
[error] 18-18: Literal carriage return. Run script through tr -d '\r' .
(SC1017)
[error] 19-19: Literal carriage return. Run script through tr -d '\r' .
(SC1017)
[error] 20-20: Literal carriage return. Run script through tr -d '\r' .
(SC1017)
[error] 21-21: Literal carriage return. Run script through tr -d '\r' .
(SC1017)
[error] 22-22: Literal carriage return. Run script through tr -d '\r' .
(SC1017)
[warning] 23-23: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
[error] 23-23: Literal carriage return. Run script through tr -d '\r' .
(SC1017)
🤖 Prompt for AI Agents
In test/scripts/cases/_olla.sh around lines 10 to 23, the script changes
directories using cd without checking if the command succeeded, which can cause
silent failures. Also, variable declarations combined with assignments can mask
errors. To fix this, add checks after each cd command to verify it succeeded,
and separate variable declarations from assignments where needed to properly
handle errors and return values.
test/scripts/cases/_olla.sh
Outdated
| local current_dir=$(pwd) | ||
|
|
||
| # Change to project root | ||
| cd "$PROJECT_ROOT" |
There was a problem hiding this comment.
Fix directory navigation error handling in build_olla
Multiple cd commands don't check for failure, which could lead to builds in wrong directories.
Apply these fixes:
- local current_dir=$(pwd)
+ local current_dir
+ current_dir=$(pwd) || return 1
# Change to project root
- cd "$PROJECT_ROOT"
+ cd "$PROJECT_ROOT" || return 1
# ... rest of function ...
- cd "$current_dir"
+ cd "$current_dir" || return 1
return 1
fiAlso applies to: 52-52, 58-58, 62-62, 68-68
🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 31-31: Declare and assign separately to avoid masking return values.
(SC2155)
[error] 31-31: Literal carriage return. Run script through tr -d '\r' .
(SC1017)
[error] 32-32: Literal carriage return. Run script through tr -d '\r' .
(SC1017)
[error] 33-33: Literal carriage return. Run script through tr -d '\r' .
(SC1017)
[warning] 34-34: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
[error] 34-34: Literal carriage return. Run script through tr -d '\r' .
(SC1017)
🤖 Prompt for AI Agents
In test/scripts/cases/_olla.sh around lines 31 to 34 and also at lines 52, 58,
62, and 68, the cd commands do not check for failure, risking running commands
in the wrong directory. Update each cd command to verify success by adding error
handling, such as checking the exit status immediately after cd and exiting or
handling the error if cd fails. This ensures the script stops or handles errors
properly if directory changes fail.
| # Function to create test configuration for engine and profile | ||
| create_engine_profile_config() { | ||
| local engine=$1 | ||
| local profile=$2 | ||
|
|
||
| print_color "$CYAN" "Creating config: engine=$engine, profile=$profile, host=localhost, port=$TEST_PORT" | ||
|
|
||
| # Use the create_test_config function from _olla.sh with yq modifications | ||
| if command_exists yq; then | ||
| create_test_config "$BASE_CONFIG" ".proxy.engine = \"$engine\" | .proxy.profile = \"$profile\" | .server.host = \"localhost\" | .server.port = $TEST_PORT" | ||
| else | ||
| # Use awk to modify YAML | ||
| print_color "$YELLOW" "Using awk to modify YAML configuration" | ||
| cp "$BASE_CONFIG" "$PROJECT_ROOT/$TEST_CONFIG" | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
TEST_CONFIG is never initialised – results in empty path and broken cp
Functions from _olla.sh expect a global $TEST_CONFIG pointing to the
generated YAML. Because it is unset, commands such as
cp "$PROJECT_ROOT/$TEST_CONFIG" …
expand to cp /project/root/ … and fail.
# Configuration
BASE_CONFIG=""
TEST_RESULTS_DIR=""
TEST_PORT=""
+# Name of the per-run generated config file
+TEST_CONFIG="config.test.yaml"📝 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.
| # Function to create test configuration for engine and profile | |
| create_engine_profile_config() { | |
| local engine=$1 | |
| local profile=$2 | |
| print_color "$CYAN" "Creating config: engine=$engine, profile=$profile, host=localhost, port=$TEST_PORT" | |
| # Use the create_test_config function from _olla.sh with yq modifications | |
| if command_exists yq; then | |
| create_test_config "$BASE_CONFIG" ".proxy.engine = \"$engine\" | .proxy.profile = \"$profile\" | .server.host = \"localhost\" | .server.port = $TEST_PORT" | |
| else | |
| # Use awk to modify YAML | |
| print_color "$YELLOW" "Using awk to modify YAML configuration" | |
| cp "$BASE_CONFIG" "$PROJECT_ROOT/$TEST_CONFIG" | |
| # Configuration | |
| BASE_CONFIG="" | |
| TEST_RESULTS_DIR="" | |
| TEST_PORT="" | |
| # Name of the per-run generated config file | |
| TEST_CONFIG="config.test.yaml" |
🧰 Tools
🪛 Shellcheck (0.10.0)
[error] 90-90: Literal carriage return. Run script through tr -d '\r' .
(SC1017)
[error] 91-91: Literal carriage return. Run script through tr -d '\r' .
(SC1017)
[error] 92-92: Literal carriage return. Run script through tr -d '\r' .
(SC1017)
[error] 93-93: Literal carriage return. Run script through tr -d '\r' .
(SC1017)
[error] 94-94: Literal carriage return. Run script through tr -d '\r' .
(SC1017)
[error] 95-95: Literal carriage return. Run script through tr -d '\r' .
(SC1017)
[error] 96-96: Literal carriage return. Run script through tr -d '\r' .
(SC1017)
[error] 97-97: Literal carriage return. Run script through tr -d '\r' .
(SC1017)
[error] 98-98: Literal carriage return. Run script through tr -d '\r' .
(SC1017)
[error] 99-99: Literal carriage return. Run script through tr -d '\r' .
(SC1017)
[error] 100-100: Literal carriage return. Run script through tr -d '\r' .
(SC1017)
[error] 101-101: Literal carriage return. Run script through tr -d '\r' .
(SC1017)
[error] 102-102: Literal carriage return. Run script through tr -d '\r' .
(SC1017)
[error] 103-103: Literal carriage return. Run script through tr -d '\r' .
(SC1017)
[error] 104-104: Literal carriage return. Run script through tr -d '\r' .
(SC1017)
🤖 Prompt for AI Agents
In test/scripts/cases/test-proxy-engine-profiles.sh around lines 90 to 104, the
variable TEST_CONFIG is never initialized before being used, causing commands
like cp "$PROJECT_ROOT/$TEST_CONFIG" to fail due to an empty path. To fix this,
initialize TEST_CONFIG with a valid filename or path for the generated YAML
configuration before it is used in the function create_engine_profile_config,
ensuring all references to TEST_CONFIG point to a proper file location.
| for engine in "${PROXY_ENGINES[@]}"; do | ||
| for profile in "${PROXY_PROFILES[@]}"; do | ||
| echo "" | ||
| echo "$engine/$profile:" | ||
| analyze_streaming_results "$TEST_RESULTS_DIR/$engine-$profile" | sed 's/^/ /' | ||
| done |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Incorrect path passed to analyze_streaming_results – always prints “None found”
Logs are written to $test_case_dir/logs/, but the summary passes the parent
directory, so the analyser looks for
$engine-$profile/detection.log which does not exist.
- echo "$engine/$profile:"
- analyze_streaming_results "$TEST_RESULTS_DIR/$engine-$profile" | sed 's/^/ /'
+ echo "$engine/$profile:"
+ analyze_streaming_results "$TEST_RESULTS_DIR/$engine-$profile/logs" | sed 's/^/ /'📝 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.
| for engine in "${PROXY_ENGINES[@]}"; do | |
| for profile in "${PROXY_PROFILES[@]}"; do | |
| echo "" | |
| echo "$engine/$profile:" | |
| analyze_streaming_results "$TEST_RESULTS_DIR/$engine-$profile" | sed 's/^/ /' | |
| done | |
| for engine in "${PROXY_ENGINES[@]}"; do | |
| for profile in "${PROXY_PROFILES[@]}"; do | |
| echo "" | |
| echo "$engine/$profile:" | |
| analyze_streaming_results "$TEST_RESULTS_DIR/$engine-$profile/logs" | sed 's/^/ /' | |
| done |
🧰 Tools
🪛 Shellcheck (0.10.0)
[error] 310-310: Literal carriage return. Run script through tr -d '\r' .
(SC1017)
[error] 311-311: Literal carriage return. Run script through tr -d '\r' .
(SC1017)
[error] 312-312: Literal carriage return. Run script through tr -d '\r' .
(SC1017)
[error] 313-313: Literal carriage return. Run script through tr -d '\r' .
(SC1017)
[error] 314-314: Literal carriage return. Run script through tr -d '\r' .
(SC1017)
[error] 315-315: Literal carriage return. Run script through tr -d '\r' .
(SC1017)
🤖 Prompt for AI Agents
In test/scripts/cases/test-proxy-engine-profiles.sh around lines 310 to 315, the
path passed to analyze_streaming_results is incorrect because it uses
"$TEST_RESULTS_DIR/$engine-$profile" instead of the logs directory where
detection.log is located. Update the argument to point to
"$TEST_RESULTS_DIR/$engine-$profile/logs" so the analyzer can find the
detection.log file correctly.
| return 1 | ||
| fi | ||
|
|
||
| local latest=$(ls -dt "$results_base"/test-results-* 2>/dev/null | head -1) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Separate variable declaration and assignment to avoid masking return values.
Combining local declaration with command assignment can hide command failures.
Apply these changes:
- local latest=$(ls -dt "$results_base"/test-results-* 2>/dev/null | head -1)
+ local latest
+ latest=$(ls -dt "$results_base"/test-results-* 2>/dev/null | head -1)- local results_dir=$(find_latest_results)
+ local results_dir
+ results_dir=$(find_latest_results)Also applies to: 33-33
🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 20-20: Declare and assign separately to avoid masking return values.
(SC2155)
[error] 20-20: Literal carriage return. Run script through tr -d '\r' .
(SC1017)
🤖 Prompt for AI Agents
In test/scripts/cases/view-test-results.sh at lines 20 and 33, separate the
variable declaration and assignment for the 'latest' variable. First declare
'local latest' on its own line, then assign the command output to 'latest' on
the next line. This avoids masking the return value of the command substitution
and improves error detection.
|
|
||
| if not tester.fetch_models(): | ||
| self.print_color(RED, "No models found!") | ||
| tester.print_color(RED, "No models found!") |
There was a problem hiding this comment.
Fix incorrect f-string usage.
The static analysis tool correctly identified an f-string without placeholders.
- tester.print_color(WHITE, f"Configuration:")
+ tester.print_color(WHITE, "Configuration:")Also apply the same fix to the duplicate in run_main():
- runner.print_color(WHITE, f"Configuration:")
+ runner.print_color(WHITE, "Configuration:")Also applies to: 428-428, 460-460, 545-545
🤖 Prompt for AI Agents
In test/scripts/streaming/test-streaming-responses.py at lines 424, 428, 460,
and 545, the code uses f-strings without any placeholders, which is incorrect.
Replace these f-strings with regular string literals by removing the leading 'f'
to fix the syntax. Apply this change to all mentioned lines including the
duplicate in the run_main() function.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
internal/adapter/proxy/olla/service_leak_test.go (1)
257-364: Comprehensive service lifecycle test with robust mocks.The test effectively verifies that the cleanup goroutine starts and stops properly. The mock implementations are comprehensive and follow testing best practices.
Consider making the goroutine count check more resilient to implementation changes:
- if currentGoroutines <= initialGoroutines { - t.Error("Cleanup goroutine not started") - } + if currentGoroutines < initialGoroutines + 1 { + t.Error("Cleanup goroutine not started") + }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal/adapter/proxy/olla/config.go(1 hunks)internal/adapter/proxy/olla/service_leak_test.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
internal/{app,adapter}/**/*.go
📄 CodeRabbit Inference Engine (CLAUDE.md)
Endpoints should be exposed at
/internal/healthand/internal/status.
Files:
internal/adapter/proxy/olla/config.gointernal/adapter/proxy/olla/service_leak_test.go
**/*_test.go
📄 CodeRabbit Inference Engine (CLAUDE.md)
Unit tests should test individual components in isolation.
Files:
internal/adapter/proxy/olla/service_leak_test.go
🧠 Learnings (7)
📚 Learning: applies to {proxy_sherpa.go,proxy_olla.go} : proxy implementations should be in `proxy_sherpa.go` an...
Learnt from: CR
PR: thushan/olla#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-27T12:59:29.787Z
Learning: Applies to {proxy_sherpa.go,proxy_olla.go} : Proxy implementations should be in `proxy_sherpa.go` and `proxy_olla.go`.
Applied to files:
internal/adapter/proxy/olla/config.gointernal/adapter/proxy/olla/service_leak_test.go
📚 Learning: applies to internal/adapter/proxy/*.go : expose the following response headers: `x-olla-endpoint`, `...
Learnt from: CR
PR: thushan/olla#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-27T12:59:29.788Z
Learning: Applies to internal/adapter/proxy/*.go : Expose the following response headers: `X-Olla-Endpoint`, `X-Olla-Model`, `X-Olla-Backend-Type`, `X-Olla-Request-ID`, `X-Olla-Response-Time`.
Applied to files:
internal/adapter/proxy/olla/config.gointernal/adapter/proxy/olla/service_leak_test.go
📚 Learning: applies to internal/adapter/proxy/*_test.go : shared proxy tests should ensure compatibility between...
Learnt from: CR
PR: thushan/olla#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-27T12:59:29.787Z
Learning: Applies to internal/adapter/proxy/*_test.go : Shared proxy tests should ensure compatibility between both proxy engines.
Applied to files:
internal/adapter/proxy/olla/config.gointernal/adapter/proxy/olla/service_leak_test.go
📚 Learning: applies to internal/adapter/proxy/*_test.go : integration tests should test the full request flow th...
Learnt from: CR
PR: thushan/olla#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-27T12:59:29.787Z
Learning: Applies to internal/adapter/proxy/*_test.go : Integration tests should test the full request flow through the proxy.
Applied to files:
internal/adapter/proxy/olla/service_leak_test.go
📚 Learning: applies to internal/adapter/proxy/*_test.go : benchmark tests should measure performance of critical...
Learnt from: CR
PR: thushan/olla#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-27T12:59:29.787Z
Learning: Applies to internal/adapter/proxy/*_test.go : Benchmark tests should measure performance of critical paths, proxy engine comparisons, connection pooling efficiency, and circuit breaker behavior.
Applied to files:
internal/adapter/proxy/olla/service_leak_test.go
📚 Learning: applies to **/*_test.go : unit tests should test individual components in isolation....
Learnt from: CR
PR: thushan/olla#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-27T12:59:29.787Z
Learning: Applies to **/*_test.go : Unit tests should test individual components in isolation.
Applied to files:
internal/adapter/proxy/olla/service_leak_test.go
📚 Learning: applies to internal/{app,adapter}/**/*.go : endpoints should be exposed at `/internal/health` and `/...
Learnt from: CR
PR: thushan/olla#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-27T12:59:29.788Z
Learning: Applies to internal/{app,adapter}/**/*.go : Endpoints should be exposed at `/internal/health` and `/internal/status`.
Applied to files:
internal/adapter/proxy/olla/service_leak_test.go
🧬 Code Graph Analysis (1)
internal/adapter/proxy/olla/config.go (2)
internal/core/constants/config.go (1)
ConfigurationProxyProfileAuto(4-4)internal/core/constants/endpoint.go (1)
DefaultOllaProxyPathPrefix(5-5)
🔇 Additional comments (10)
internal/adapter/proxy/olla/config.go (5)
1-7: LGTM!Package declaration and imports are appropriate for the proxy configuration functionality.
9-23: Well-structured configuration.The Configuration struct appropriately separates general proxy settings from Olla-specific advanced connection pooling parameters. The Profile field correctly supports the new proxy profile feature.
25-30: LGTM!The GetProxyProfile method correctly defaults to "auto" profile when unset, aligning with the PR objectives.
32-37: LGTM!The GetProxyPrefix method correctly uses the default Olla proxy path prefix from constants.
53-55: Verify intentional lack of default value.Unlike other getter methods, GetResponseTimeout doesn't provide a default value when the field is zero. Ensure this is intentional behaviour rather than an oversight.
internal/adapter/proxy/olla/service_leak_test.go (5)
1-17: LGTM!Package declaration and imports are appropriate for comprehensive leak testing, including runtime monitoring and concurrent data structures.
19-82: Comprehensive endpoint pool cleanup test.This test effectively verifies that endpoint pools are properly cleaned up to prevent memory leaks. The methodology of creating pools, marking them as old, and verifying cleanup is sound.
84-135: LGTM!The circuit breaker cleanup test effectively verifies that inactive circuit breakers are properly cleaned up to prevent memory leaks.
137-181: Effective goroutine leak detection test.This test properly verifies that cleanup goroutines exit cleanly without leaking. The allowance for small variance in goroutine count is reasonable and realistic.
183-255: Thorough context pool reset verification.Both context pool tests effectively verify that requestContext and errorContext objects are properly reset when returned to pools, preventing data leakage between requests.
| func (c *Configuration) GetConnectionTimeout() time.Duration { | ||
| if c.ConnectionTimeout == 0 { | ||
| return DefaultTimeout | ||
| } | ||
| return c.ConnectionTimeout | ||
| } |
There was a problem hiding this comment.
Undefined constant DefaultTimeout.
The method references DefaultTimeout which is not defined in this file or imported from the constants package.
Verify that this constant is defined and available, or add it to the constants package:
#!/bin/bash
# Search for DefaultTimeout definition in the codebase
rg -A 2 "DefaultTimeout.*=" --type go🤖 Prompt for AI Agents
In internal/adapter/proxy/olla/config.go around lines 39 to 44, the constant
DefaultTimeout used in GetConnectionTimeout is undefined. Verify if
DefaultTimeout is defined in a constants package or elsewhere in the codebase.
If it exists, import the package where it is defined. If it does not exist,
define DefaultTimeout with an appropriate duration value in this file or in a
shared constants package and then use it here.
| func (c *Configuration) GetConnectionKeepAlive() time.Duration { | ||
| if c.ConnectionKeepAlive == 0 { | ||
| return DefaultKeepAlive | ||
| } | ||
| return c.ConnectionKeepAlive | ||
| } |
There was a problem hiding this comment.
Undefined constant DefaultKeepAlive.
The method references DefaultKeepAlive which is not defined in this file or imported from the constants package.
🤖 Prompt for AI Agents
In internal/adapter/proxy/olla/config.go around lines 46 to 51, the constant
DefaultKeepAlive is used but not defined or imported. Define DefaultKeepAlive in
this file with an appropriate time.Duration value or import it from the correct
constants package if it exists elsewhere. Ensure the constant is accessible in
this scope to avoid undefined identifier errors.
| func (c *Configuration) GetReadTimeout() time.Duration { | ||
| if c.ReadTimeout == 0 { | ||
| return DefaultReadTimeout | ||
| } | ||
| return c.ReadTimeout | ||
| } | ||
|
|
||
| func (c *Configuration) GetStreamBufferSize() int { | ||
| if c.StreamBufferSize == 0 { | ||
| return DefaultStreamBufferSize | ||
| } | ||
| return c.StreamBufferSize | ||
| } |
There was a problem hiding this comment.
Undefined constants DefaultReadTimeout and DefaultStreamBufferSize.
Both methods reference constants that are not defined in this file or imported from the constants package:
DefaultReadTimeout(line 59)DefaultStreamBufferSize(line 66)
These constants need to be defined and made available, likely in the constants package.
🤖 Prompt for AI Agents
In internal/adapter/proxy/olla/config.go around lines 57 to 69, the constants
DefaultReadTimeout and DefaultStreamBufferSize are used but not defined or
imported. Define these constants with appropriate default values in the
constants package and import that package into this file. Then replace the
undefined constants with the imported ones to ensure the methods return valid
default values.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (11)
test/scripts/cases/view-test-results.sh (2)
20-21: Separate declaration from assignment to avoid masking failures
Same ShellCheck SC2155 warning as raised before – the command substitution’s exit status is lost.
33-34: Repeat of SC2155 – split theresults_dirassignment
Identical to the earlier comment; please fix here as well.test/scripts/cases/_common.sh (1)
122-124: SC2155 – split creation ofresults_dir
This was already highlighted in previous rounds – the issue remains.test/scripts/cases/_streaming_tests.sh (1)
12-14:cdwithout failure check can derail the test run
Existing feedback still applies: add|| return(orexit) so the script aborts if the path is wrong.test/scripts/cases/test-proxy-engine-profiles.sh (2)
6-8: Re-enableset -e– silent failures break CI reliability
Commenting it out undermines the entire suite; earlier advice still stands.
314-315: Incorrect path passed toanalyze_streaming_results
You’re analysing$engine-$profileinstead of itslogssub-dir, so the helper never finds the log files. Same bug as previously reported.test/scripts/cases/_olla.sh (5)
8-24:get_git_versionstill masks errors and ignores failedcd– identical issue previously flagged
The combined declaration/assignment (local current_dir=$(pwd)) and uncheckedcd "$PROJECT_ROOT"continue to trigger SC2155/SC2164. Please split the assignment and guard the directory change, as suggested in the earlier review.
26-71:build_ollaretains the same uncheckedcdand masked assignments
All remarks in the earlier “Fix directory navigation error handling in build_olla” comment remain unresolved.
110-154: Duplicateportdeclaration and unchecked directory changes remain
The duplicatedlocal port(Line 122 vs 137) and missing|| return 1aftercdwere already highlighted in earlier feedback; they are still present.
169-176: Combined declaration/assignment forhealth_responsestill masks curl failure
Same SC2155 issue previously reported.
221-223:response/http_codemasking issue unchanged
The variable-masking and error-checking advice from the previous review remains unaddressed here as well.
🧹 Nitpick comments (3)
test/scripts/cases/view-test-results.sh (1)
78-87: Multiple SC2155 instances – declaresizefirst, then assign
Botholla.logand per-test log handling reuse the pattern that hides failures. Refactor once and reuse a helper if possible.- local size=$(wc -l < "$test_dir/logs/olla.log" 2>/dev/null || echo "0") + local size + size=$(wc -l < "$test_dir/logs/olla.log" 2>/dev/null || echo "0")(and the same for lines 86-87)
test/scripts/cases/_streaming_tests.sh (1)
12-13: SC2155 across allcurrent_dircaptures
Declare the variable, then assign, to preserve the real exit status ofpwd.- local current_dir=$(pwd) + local current_dir + current_dir=$(pwd)Apply to every function.
Also applies to: 40-41, 68-69
test/scripts/cases/_olla.sh (1)
1-4: Consider enabling strict-mode flags for more robust scriptsAdding
set -euo pipefail(and optionallyIFS=$'\n\t') right after the she-bang will cause the script to abort on an unset variable, non-zero exit of any simple command, or failed pipeline – preventing a large class of silent failures during CI runs.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
.gitattributes(1 hunks)test/scripts/cases/_common.sh(1 hunks)test/scripts/cases/_olla.sh(1 hunks)test/scripts/cases/_streaming_tests.sh(1 hunks)test/scripts/cases/debug-test.sh(1 hunks)test/scripts/cases/test-environment.sh(1 hunks)test/scripts/cases/test-proxy-engine-profiles.sh(1 hunks)test/scripts/cases/test-simple.sh(1 hunks)test/scripts/cases/view-test-results.sh(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- test/scripts/cases/test-simple.sh
- .gitattributes
- test/scripts/cases/test-environment.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- test/scripts/cases/debug-test.sh
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: applies to internal/adapter/proxy/*_test.go : shared proxy tests should ensure compatibility between...
Learnt from: CR
PR: thushan/olla#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-27T12:59:29.787Z
Learning: Applies to internal/adapter/proxy/*_test.go : Shared proxy tests should ensure compatibility between both proxy engines.
Applied to files:
test/scripts/cases/test-proxy-engine-profiles.sh
📚 Learning: applies to {proxy_sherpa.go,proxy_olla.go} : proxy implementations should be in `proxy_sherpa.go` an...
Learnt from: CR
PR: thushan/olla#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-27T12:59:29.787Z
Learning: Applies to {proxy_sherpa.go,proxy_olla.go} : Proxy implementations should be in `proxy_sherpa.go` and `proxy_olla.go`.
Applied to files:
test/scripts/cases/test-proxy-engine-profiles.sh
📚 Learning: applies to test/scripts/logic/test-model-routing.sh : test routing and headers using `/test/scripts/...
Learnt from: CR
PR: thushan/olla#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-27T12:59:29.787Z
Learning: Applies to test/scripts/logic/test-model-routing.sh : Test routing and headers using `/test/scripts/logic/test-model-routing.sh`.
Applied to files:
test/scripts/cases/test-proxy-engine-profiles.shtest/scripts/cases/_olla.sh
📚 Learning: applies to test/scripts/security/*.sh : security tests should validate rate limiting and size restri...
Learnt from: CR
PR: thushan/olla#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-27T12:59:29.787Z
Learning: Applies to test/scripts/security/*.sh : Security tests should validate rate limiting and size restrictions (see `/test/scripts/security/`).
Applied to files:
test/scripts/cases/test-proxy-engine-profiles.shtest/scripts/cases/_common.shtest/scripts/cases/_olla.shtest/scripts/cases/_streaming_tests.sh
🧬 Code Graph Analysis (2)
test/scripts/cases/test-proxy-engine-profiles.sh (4)
test/scripts/cases/_common.sh (8)
print_color(23-31)command_exists(113-115)print_section(42-46)print_header(34-39)deactivate_venv(104-110)check_venv(49-101)create_results_dir(118-125)get_random_port(175-179)test/scripts/cases/_olla.sh (7)
create_test_config(74-108)start_olla(111-208)check_phi_models(279-301)stop_olla(229-252)cleanup_olla(304-311)build_olla(27-71)get_git_version(9-24)test/scripts/cases/_streaming_tests.sh (3)
run_all_streaming_tests(90-120)check_streaming_test_passed(163-178)analyze_streaming_results(123-160)test/scripts/cases/view-test-results.sh (1)
main(29-110)
test/scripts/cases/_olla.sh (1)
test/scripts/cases/_common.sh (5)
print_color(23-31)command_exists(113-115)is_port_available(134-144)wait_for_condition(147-165)strip_ansi_codes(182-194)
🪛 Shellcheck (0.10.0)
test/scripts/cases/test-proxy-engine-profiles.sh
[warning] 152-152: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 153-153: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 154-154: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 193-193: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 241-241: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 338-338: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 363-363: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 419-419: Declare and assign separately to avoid masking return values.
(SC2155)
test/scripts/cases/_common.sh
[warning] 9-9: BLUE appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 11-11: CYAN appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 19-19: TEST_CONFIG appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 20-20: OLLA_BINARY appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 122-122: Declare and assign separately to avoid masking return values.
(SC2155)
test/scripts/cases/_olla.sh
[warning] 10-10: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 11-11: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
[warning] 14-14: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 23-23: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
[warning] 31-31: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 34-34: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
[warning] 52-52: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
[warning] 58-58: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
[warning] 62-62: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
[warning] 68-68: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
[warning] 118-118: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 119-119: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
[warning] 125-125: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
[warning] 132-132: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
[warning] 149-149: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 169-169: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 179-179: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
[warning] 205-205: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
[warning] 221-221: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 222-222: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 257-257: Declare and assign separately to avoid masking return values.
(SC2155)
test/scripts/cases/_streaming_tests.sh
[warning] 12-12: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 13-13: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
[warning] 20-20: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
[warning] 28-28: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
[warning] 40-40: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 41-41: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
[warning] 48-48: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
[warning] 56-56: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
[warning] 68-68: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 69-69: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
[warning] 76-76: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
[warning] 84-84: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
test/scripts/cases/view-test-results.sh
[warning] 20-20: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 33-33: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 78-78: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 86-86: Declare and assign separately to avoid masking return values.
(SC2155)
| if [[ ! -f "$base_config" ]]; then | ||
| print_color "$RED" "ERROR: Base configuration not found: $base_config" | ||
| return 1 | ||
| fi | ||
|
|
||
| print_color "$CYAN" "Creating test configuration..." | ||
|
|
||
| # Ensure config directory exists | ||
| mkdir -p "$PROJECT_ROOT/config" | ||
|
|
||
| # Use yq if available for reliable YAML manipulation | ||
| if command_exists yq; then | ||
| eval "yq eval '$modifications' \"$base_config\"" > "$PROJECT_ROOT/$TEST_CONFIG" | ||
| else | ||
| # Fallback to sed (less reliable but works for simple cases) | ||
| cp "$base_config" "$PROJECT_ROOT/$TEST_CONFIG" | ||
|
|
||
| # Apply modifications using sed | ||
| # This is a simplified approach - real implementation would need better parsing | ||
| print_color "$YELLOW" "Warning: Using sed for YAML manipulation (install yq for better reliability)" | ||
| eval "$modifications" | ||
| fi |
There was a problem hiding this comment.
Avoid eval – command-injection risk when creating the test config
Both code paths funnel the user-supplied $modifications string through eval, which executes arbitrary shell code. A malicious test case (or merely an unescaped quote in a legitimate modification) can execute commands with the script’s privileges.
- eval "yq eval '$modifications' \"$base_config\"" > "$PROJECT_ROOT/$TEST_CONFIG"
+ # Pass the argument unchanged – yq parses the expression itself.
+ yq eval "$modifications" "$base_config" > "$PROJECT_ROOT/$TEST_CONFIG"For the sed fallback, write the sed program to a temporary file and invoke sed -f instead of eval "$modifications".
Treating $modifications as data rather than code eliminates this vector.
🤖 Prompt for AI Agents
In test/scripts/cases/_olla.sh around lines 78 to 99, the use of eval with the
user-supplied $modifications string poses a command injection risk. To fix this,
avoid using eval entirely. For the sed fallback path, write the $modifications
content to a temporary file and then run sed with the -f option pointing to that
file. For the yq path, replace eval with a direct yq command invocation that
safely passes $modifications as an argument or script without eval. This treats
$modifications as data, not code, eliminating the injection vulnerability.
1 participant
This PR adds a new feature to each proxy to enable
streamingorstandardbased on use-case, or the default ofautomode where Olla tries to determine the best method based on circumstances.The added configurable proxy profiles control HTTP response flushing behavior for the most optimal streaming scenario vs buffering of LLM responses (standard way).
Three Profiles
auto(default) - Intelligent content-type detectionstandard- Never flushes responses (until buffer length reached)streaming- Always flushes responses and optimised to be fast (low latency from LLM backend to client)For most scenarios, leaving it as
autoseems to be the best in our testing.Technical Implementation
Detection Logic (AutoDetectStreamingMode):
- Checks known streaming content types
- Respects client preference via context (stream=true)
- Buffers binary content to prevent corruption & broken packets
- Defaults to streaming for text-based responses
Key Benefits
Extensive testing
This PR also includes extensive testing via scripts (see
test/scripts/casesandtest/scripts/streaming) built by Claude (especially the all runners) that test various aspects and combinations to ensure streaming vs buffering.Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
Chores
.gitignoreto cover new test and configuration files..gitattributesfor consistent line endings and file type handling.