fix: Alternative method of resolving profile paths #90
Conversation
WalkthroughDiscovery URL selection was made profile-aware: OpenAI-compatible profiles avoid treating common model paths ( Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested labels
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (1)**/*.go📄 CodeRabbit inference engine (CLAUDE.md)
Files:
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (4)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
internal/adapter/discovery/http_client.go(2 hunks)internal/adapter/discovery/http_client_test.go(1 hunks)internal/adapter/discovery/repository.go(2 hunks)internal/adapter/discovery/repository_test.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.go: Use Australian English for comments and documentation
Comment on why rather than what in code
Files:
internal/adapter/discovery/repository_test.gointernal/adapter/discovery/http_client.gointernal/adapter/discovery/http_client_test.gointernal/adapter/discovery/repository.go
🧬 Code graph analysis (3)
internal/adapter/discovery/repository_test.go (2)
internal/adapter/discovery/repository.go (1)
NewStaticEndpointRepository(26-36)internal/config/types.go (1)
EndpointConfig(100-111)
internal/adapter/discovery/http_client.go (2)
internal/core/domain/profile.go (1)
ProfileOpenAICompatible(10-10)internal/adapter/discovery/errors.go (1)
NewDiscoveryError(33-42)
internal/adapter/discovery/http_client_test.go (3)
internal/core/domain/profile.go (1)
ProfileOpenAICompatible(10-10)internal/adapter/discovery/http_client.go (1)
NewHTTPModelDiscoveryClientWithDefaults(54-71)internal/logger/fatal.go (1)
Fatalf(14-17)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test (1.24)
🔇 Additional comments (1)
internal/adapter/discovery/repository_test.go (1)
172-232: joinURLPath and nested-path repository tests give solid coverageThe new tests exercise the important edge cases for slash handling and the nested Docker-style path scenario the helper is meant to fix, and they validate the repository wiring matches that behaviour. No issues from my side here.
Also applies to: 234-280
| // TestOpenAICompatibleModelURLBehavior verifies special handling for OpenAI-compatible | ||
| // profiles where /v1/models override is ignored to preserve existing configurations. | ||
| func TestOpenAICompatibleModelURLBehavior(t *testing.T) { | ||
| tests := []struct { | ||
| name string | ||
| modelURLConfig string // What user configures | ||
| expectProfileDefault bool // Whether we expect the profile default to be used | ||
| expectedPath string // Expected discovery path | ||
| }{ | ||
| { | ||
| name: "OpenAI-compatible with /v1/models uses profile default", | ||
| modelURLConfig: "/v1/models", | ||
| expectProfileDefault: true, | ||
| expectedPath: "/v1/models", | ||
| }, | ||
| { | ||
| name: "OpenAI-compatible with custom path uses override", | ||
| modelURLConfig: "/api/v2/models", // Path, will be combined with server URL | ||
| expectProfileDefault: false, | ||
| expectedPath: "/api/v2/models", | ||
| }, | ||
| { | ||
| name: "OpenAI-compatible with /models ignored (Docker/OpenWebUI case)", | ||
| modelURLConfig: "/models", // Often returns HTML, not JSON | ||
| expectProfileDefault: true, | ||
| expectedPath: "/v1/models", | ||
| }, | ||
| { | ||
| name: "OpenAI-compatible with empty model_url uses profile default", | ||
| modelURLConfig: "", | ||
| expectProfileDefault: true, | ||
| expectedPath: "/v1/models", | ||
| }, | ||
| } | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| var requestedPath string | ||
|
|
||
| server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| requestedPath = r.URL.Path | ||
| t.Logf("Server received request at path: %s", r.URL.Path) | ||
|
|
||
| // Only respond to the expected path | ||
| if r.URL.Path == tt.expectedPath { | ||
| w.WriteHeader(http.StatusOK) | ||
| w.Write([]byte(`{"object": "list", "data": [{"id": "test-model", "object": "model"}]}`)) | ||
| return | ||
| } | ||
|
|
||
| w.WriteHeader(http.StatusNotFound) | ||
| })) | ||
| defer server.Close() | ||
|
|
||
| // For the override test, we need to build the full URL first | ||
| modelURLString := tt.modelURLConfig | ||
| if tt.modelURLConfig == "/api/v2/models" { | ||
| modelURLString = server.URL + tt.modelURLConfig | ||
| } | ||
|
|
||
| // Create endpoint with configured model URL | ||
| endpoint := createTestEndpointWithModelURL(server.URL, domain.ProfileOpenAICompatible, modelURLString) | ||
|
|
||
| client := NewHTTPModelDiscoveryClientWithDefaults(createTestProfileFactory(t), createTestLogger()) | ||
|
|
||
| models, err := client.DiscoverModels(context.Background(), endpoint) | ||
|
|
||
| if err != nil { | ||
| t.Fatalf("Unexpected error: %v", err) | ||
| } | ||
|
|
||
| if requestedPath != tt.expectedPath { | ||
| t.Errorf("Expected path %q to be used, but server received request at %q", tt.expectedPath, requestedPath) | ||
| } | ||
|
|
||
| if len(models) == 0 { | ||
| t.Errorf("Expected models to be discovered, got 0") | ||
| } | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
OpenAI-compatible tests bypass the repository and don’t reflect ModelURLString as produced in production
The new TestOpenAICompatibleModelURLBehavior nicely captures the intended semantics for /v1/models vs /models vs custom paths, but it constructs endpoints via createTestEndpointWithModelURL, so:
ModelURLStringis a bare path in these tests (e.g."/v1/models"), whereasStaticEndpointRepository.LoadFromConfignow always produces a fully-qualified URL (e.g."http://host/engines/llama.cpp/v1/models").- Combined with the equality checks in
discoverWithProfile, config-driven OpenAI-compatible endpoints will not hit the/v1/models//modelsspecial-cases the tests are asserting, even though the comments imply they should.
To keep the tests aligned with real-world behaviour, consider either:
- Building the test endpoints via
LoadFromConfigsoModelURLStringmatches production, and then adjusting the client logic accordingly (e.g. by inspecting the parsed.Path), or - Relaxing the checks in the client to look at the path component/suffix rather than the entire
ModelURLString.
Also, the expectProfileDefault field in the test table is currently unused; if you do not plan to assert on it, you can drop it to reduce noise.
🤖 Prompt for AI Agents
internal/adapter/discovery/http_client_test.go lines 812-892: the test
constructs endpoints with bare paths (e.g. "/v1/models") but production
StaticEndpointRepository.LoadFromConfig always produces fully-qualified
ModelURLString, so the test doesn't reflect real behaviour and the
expectProfileDefault field is unused; update the test to either (A) build
endpoints using LoadFromConfig (or the same codepath) so ModelURLString is a
full URL (server.URL + path) and then assert behaviour by parsing/inspecting the
URL.Path, or (B) keep the test helper but change the client logic under test to
make its OpenAI-compatible checks based on the parsed URL path/suffix
(r.URL.Path) instead of exact ModelURLString equality; also remove
expectProfileDefault from the table (or add assertions using it) to eliminate
the unused field.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
internal/adapter/registry/profile/configurable_profile.go (1)
54-56: Verify that ModelDiscoveryPath is never an absolute URL.The method now uses
util.JoinURLPath, which will incorrectly handle absolute URLs inp.config.API.ModelDiscoveryPath. If any profile configurations specify absolute URLs for model discovery (e.g., pointing to a different host), this change would break that functionality.Run this script to check if any profile configurations use absolute URLs for ModelDiscoveryPath:
#!/bin/bash # Description: Check for absolute URLs in ModelDiscoveryPath across profile configs # Search for model_discovery_path with http:// or https:// rg -n --type=yaml -A1 'model_discovery_path.*https?://' # Also check for any YAML files that might define profiles fd -e yaml -e yml | xargs grep -l "model_discovery_path"internal/adapter/discovery/repository.go (1)
144-161: Critical: JoinURLPath breaks absolute URLs in endpoint configurations.While the change fixes nested path handling (e.g.,
/engines/llama.cpp/), it introduces a regression for configurations that specify absolute URLs incfg.HealthCheckURLorcfg.ModelURLfields.Example failure scenario:
endpoints: - url: http://localhost:8080/api/ health_check_url: http://healthcheck.internal/status # Absolute URL model_url: http://models.internal/v1/models # Absolute URLThis would produce invalid URLs:
http://localhost:8080/api/http://healthcheck.internal/status❌http://localhost:8080/api/http://models.internal/v1/models❌This breaks legitimate proxy and load-balancer configurations where health checks and model discovery might be routed to different services.
The fix should be applied to
util.JoinURLPathas suggested in the review ofinternal/util/url.goto detect and preserve absolute URLs before concatenation.
🧹 Nitpick comments (1)
internal/util/url_test.go (1)
49-53: Remove duplicate test case.This test case is identical to the one at lines 13-17 ("base with trailing slash, path with leading slash"). The duplicate provides no additional coverage and should be removed.
Apply this diff:
{ name: "empty path", baseURL: "http://localhost:11434", path: "", expected: "http://localhost:11434", }, - { - name: "Docker nested path case", - baseURL: "http://localhost:12434/engines/llama.cpp/", - path: "/v1/models", - expected: "http://localhost:12434/engines/llama.cpp/v1/models", - }, }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
internal/adapter/discovery/repository.go(2 hunks)internal/adapter/discovery/repository_test.go(1 hunks)internal/adapter/registry/profile/configurable_profile.go(2 hunks)internal/util/url.go(1 hunks)internal/util/url_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/adapter/discovery/repository_test.go
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.go: Use Australian English for comments and documentation
Comment on why rather than what in code
Files:
internal/adapter/registry/profile/configurable_profile.gointernal/util/url.gointernal/util/url_test.gointernal/adapter/discovery/repository.go
🧠 Learnings (1)
📚 Learning: 2025-12-09T11:21:09.222Z
Learnt from: CR
Repo: thushan/olla PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-09T11:21:09.222Z
Learning: Applies to internal/adapter/proxy/olla/service.go : Implement Olla proxy engine in `internal/adapter/proxy/olla/service.go`
Applied to files:
internal/adapter/registry/profile/configurable_profile.gointernal/adapter/discovery/repository.go
🧬 Code graph analysis (3)
internal/adapter/registry/profile/configurable_profile.go (1)
internal/util/url.go (1)
JoinURLPath(10-29)
internal/util/url_test.go (1)
internal/util/url.go (1)
JoinURLPath(10-29)
internal/adapter/discovery/repository.go (1)
internal/util/url.go (1)
JoinURLPath(10-29)
🔇 Additional comments (1)
internal/util/url.go (1)
1-10: LGTM! Documentation clearly explains the rationale.The comment effectively explains why string concatenation is preferred over
url.ResolveReference()for this use case, with a concrete example demonstrating the RFC 3986 path replacement behaviour.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/util/url_test.go (1)
5-125: Comprehensive test coverage, with a few suggested additions.The test suite covers the key scenarios well and addresses the previously flagged concern about absolute URL handling. Consider adding these edge cases:
- Invalid base URL fallback: Verify the function returns
pathOrURLwhenbaseURLis malformed.- Protocol-relative URLs: Test paths like
"//cdn.example.com/assets"to document behaviour.{ name: "base URL with fragment", baseURL: "http://localhost:8080/api#section", pathOrURL: "/models", expected: "http://localhost:8080/api/models#section", }, + { + name: "invalid base URL returns path", + baseURL: "://invalid-url", + pathOrURL: "/v1/models", + expected: "/v1/models", + }, }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
internal/adapter/discovery/repository.go(2 hunks)internal/adapter/registry/profile/configurable_profile.go(2 hunks)internal/util/url.go(1 hunks)internal/util/url_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/adapter/registry/profile/configurable_profile.go
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.go: Use Australian English for comments and documentation
Comment on why rather than what in code
Files:
internal/util/url_test.gointernal/util/url.gointernal/adapter/discovery/repository.go
🧠 Learnings (1)
📚 Learning: 2025-12-09T11:21:09.222Z
Learnt from: CR
Repo: thushan/olla PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-09T11:21:09.222Z
Learning: Applies to internal/adapter/proxy/olla/service.go : Implement Olla proxy engine in `internal/adapter/proxy/olla/service.go`
Applied to files:
internal/adapter/discovery/repository.go
🧬 Code graph analysis (2)
internal/util/url_test.go (1)
internal/util/url.go (1)
ResolveURLPath(21-44)
internal/adapter/discovery/repository.go (1)
internal/util/url.go (1)
ResolveURLPath(21-44)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test (1.24)
🔇 Additional comments (2)
internal/adapter/discovery/repository.go (2)
144-162: Good refactor using the centralised URL resolution utility.This correctly addresses the previously flagged issue where absolute URLs in config would be incorrectly concatenated. The new approach using
util.ResolveURLPathhandles both relative paths (preserving base URL prefix) and absolute URLs (returning as-is).
164-182: LGTM! Endpoint construction with precomputed URL strings.Storing both the parsed
*url.URLobjects and their string representations is practical for downstream consumers that need either form. The field assignments are consistent and complete.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal/util/url.go(1 hunks)internal/util/url_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/util/url_test.go
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.go: Use Australian English for comments and documentation
Comment on why rather than what in code
Files:
internal/util/url.go
🪛 GitHub Actions: CI
internal/util/url.go
[error] 18-18: go fmt formatting check failed. Detected formatting changes in internal/util/url.go after running 'make fmt'. Please run 'make fmt' to fix formatting. Command: 'make fmt && git diff --exit-code'
🪛 GitHub Check: lint
internal/util/url.go
[failure] 20-20:
File is not properly formatted (goimports)
🔇 Additional comments (2)
internal/util/url.go (2)
21-32: Excellent documentation that follows coding guidelines.The documentation correctly explains why the function exists and why it uses
path.Join()instead ofurl.ResolveReference(), providing important context about RFC 3986 behaviour. It also properly uses Australian English spelling as per the coding guidelines.
33-57: Solid implementation with appropriate error handling.The function correctly:
- Returns early for empty inputs (lines 34-39)
- Detects absolute URLs using
IsAbs()which is more robust than scheme checking alone (lines 41-44)- Falls back gracefully when base URL parsing fails (lines 46-51)
- Preserves base path prefixes when joining relative paths using
path.Join()(lines 53-56)The inline comments properly explain why the code behaves as it does and use Australian English spelling ("normalise"), which aligns with the coding guidelines. This implementation successfully addresses the concerns raised in previous reviews about handling absolute URLs.
This PR is still experimental to fix pathing issues post the
model_urlfix to ensure we use themodel_urlspecified in the endpoint config and only fall back to profile if it's not available (see #88).Summary by CodeRabbit
Bug Fixes
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.