internal/adapter/converter/lmstudio_converter.go (1)

68-75: Normalise model type for LM Studio responses

helper.GetModelType("llm") may return metadata-driven values like "chat" or "completion". If LM Studio expects coarse-grained types (e.g., "llm", "vlm", "embeddings"), returning "chat" could be incompatible.

Recommend normalising here to ensure predictable output for LM Studio clients.

Would you confirm LM Studio’s accepted values for the type field? If it only accepts "llm", "vlm", "embeddings", consider this minimal normalisation:

-	modelType := helper.GetModelType("llm")
+	modelType := helper.GetModelType("llm")
+	// Normalise finer-grained types to LM Studio's expected categories.
+	if modelType == "chat" || modelType == "completion" {
+		modelType = "llm"
+	}
+	if modelType == "embedding" {
+		modelType = "embeddings"
+	}

internal/adapter/converter/base_converter.go (4)

7-17: Doc comments should explain “why”, and use Australian English

The implementation is fine. Per guidelines, prefer comments that justify decisions rather than restate what the code already shows. A small wording tweak improves clarity.

Apply:

-// BaseConverter provides common functionality for all converters
+// BaseConverter centralises provider‑agnostic logic so individual converters stay thin and consistent,
+// reducing duplication and the risk of divergent behaviour.
 
-// NewBaseConverter creates a new base converter
+// NewBaseConverter records the provider type so helper lookups are properly provider‑scoped.

---

29-38: Endpoint lookup by NativeName is reasonable; minor note

Matching providerName to NativeName mirrors how aliases map to platform names. If some providers use EndpointName in routing, consider a future extension to check both. No change required now.

---

40-57: Metadata extractors: consider json.Number and string numerics

Current int extraction covers int and float64. Some JSON paths may use json.Number or numeric strings depending on deserialisation options. Supporting these would make the helpers more robust.

+import (
+	"github.com/thushan/olla/internal/core/domain"
+	"encoding/json"
+)
@@
 func (b *BaseConverter) ExtractMetadataInt(metadata map[string]interface{}, key string) int {
 	if val, ok := metadata[key].(int); ok {
 		return val
 	}
 	if val, ok := metadata[key].(float64); ok {
 		return int(val)
 	}
+	if val, ok := metadata[key].(json.Number); ok {
+		if i64, err := val.Int64(); err == nil {
+			return int(i64)
+		}
+	}
+	if s, ok := metadata[key].(string); ok {
+		if f, err := strconv.ParseFloat(s, 64); err == nil {
+			return int(f)
+		}
+	}
 	return 0
 }

Note: this adds encoding/json and strconv imports.

---

88-105: Type determination: verify metadata semantics across providers

Returning metadata["type"] verbatim (e.g., "chat") gives flexibility, but downstream formatters (e.g., LM Studio) may need canonical categories. Consider centralising a normalisation step here, but be aware it will affect tests asserting "chat".

Would you like a NormaliseModelType() helper that maps:

• "chat", "completion", "llm" → "llm"
• "vision", "vlm" → "vlm"
• "embedding", "embeddings" → "embeddings"

If so, I can propose a patch and update affected tests.

internal/adapter/converter/openai_converter.go (2)

54-62: Prefer provider‑specific alias over “first alias”; fix typo

Using the first alias is pragmatic but can be ambiguous. Since this converter now embeds BaseConverter, prefer its alias resolution first, then fall back to the first alias for backwards compatibility. Also fix “alas” → “alias”.

 func (c *OpenAIConverter) convertModel(model *domain.UnifiedModel) OpenAIModelData {
-	// OLLA-85: [Unification] Models with different digests fail to unify correctly.
-	// we need to use first alas as ID for routing compatibility
-	// to make sure the returned model ID can be used for requests
+	// OLLA-85: [Unification] Models with different digests fail to unify correctly.
+	// Use the provider‑specific alias when present to avoid routing ambiguity;
+	// fall back to the first alias to preserve historical compatibility.
 	modelID := model.ID
-	if len(model.Aliases) > 0 {
-		modelID = model.Aliases[0].Name
-	}
+	if c.BaseConverter != nil {
+		if helper := c.BaseConverter.NewConversionHelper(model); helper != nil && helper.Alias != "" {
+			modelID = helper.Alias
+		} else if len(model.Aliases) > 0 {
+			modelID = model.Aliases[0].Name
+		}
+	} else if len(model.Aliases) > 0 {
+		modelID = model.Aliases[0].Name
+	}

---

63-69: Created timestamp source: consider stable time for deterministic outputs

Using time.Now().Unix() makes responses time‑variant, which can complicate caching and test determinism. If model.LastSeen is meaningful, prefer that when set, falling back to time.Now().

-	return OpenAIModelData{
-		ID:      modelID,
-		Object:  "model",
-		Created: time.Now().Unix(),
-		OwnedBy: "olla",
-	}
+	created := time.Now().Unix()
+	if !model.LastSeen.IsZero() {
+		created = model.LastSeen.Unix()
+	}
+	return OpenAIModelData{
+		ID:      modelID,
+		Object:  "model",
+		Created: created,
+		OwnedBy: "olla",
+	}

internal/adapter/converter/unified_converter.go (2)

82-85: Fix typo in comment: “alias”

Minor spelling issue.

-// we need to use first alas as ID for routing compatibility
+// we need to use the first alias as ID for routing compatibility

---

183-194: Use the embedded BaseConverter instead of creating a new one

A new base with empty provider type is unnecessary and slightly obscures intent. Using c.BaseConverter keeps behaviour consistent and future‑proof.

-	// Use base converter to determine model type
-	base := NewBaseConverter("")
-	modelType := base.DetermineModelType(model, "")
+	// Use the embedded base converter to determine model type (provider‑agnostic here)
+	modelType := c.BaseConverter.DetermineModelType(model, "")

internal/adapter/converter/base_converter_test.go (2)

149-182: Metadata extractor tests are solid; consider adding json.Number case

Great coverage. If you adopt json.Number/string support in the extractor, add corresponding tests.

---

266-334: Model type determination tests are valuable; watch for future normalisation

If you later normalise metadata types (e.g., mapping "chat" to "llm"), update these expectations accordingly. For now, behaviour is consistent with the current design.

internal/adapter/proxy/sherpa/service_streaming.go (1)

80-86: Treat non‑positive read timeouts as “use default”

Defensive tweak: if a negative duration slips through config, we should still fall back to the default. Micro change, clearer semantics.

Apply this diff:

 func (s *Service) getReadTimeout() time.Duration {
   readTimeout := s.configuration.GetReadTimeout()
-  if readTimeout == 0 {
+  if readTimeout <= 0 {
     readTimeout = config.DefaultReadTimeout
   }
   return readTimeout
 }

internal/adapter/proxy/sherpa/service_leak_test.go (1)

150-220: Unify leak tests across Sherpa and Olla engines

Per the repository guideline to maintain a shared proxy test suite running against both Sherpa and Olla, consider table‑driving these leak tests to execute for both engines behind a common interface. This ensures parity and prevents regressions diverging between engines.

I can sketch a small test harness that instantiates each engine and runs the same scenarios if helpful.

internal/adapter/proxy/sherpa/service_retry_test.go (2)

81-167: Reduce duplication by extracting a small test config helper

The three tests duplicate identical configuration blocks. A tiny helper reduces repetition and keeps changes local if defaults move again.

You can add this helper (outside the changed hunks) and use it in all three tests:

// test helper (near top of file)
func newTestConfig() *Configuration {
	c := &Configuration{}
	c.ProxyPrefix = "/olla/"
	c.ConnectionTimeout = time.Second
	c.ConnectionKeepAlive = time.Second
	c.ResponseTimeout = time.Second
	c.ReadTimeout = time.Second
	c.StreamBufferSize = 8192
	return c
}

Then replace each block with:

config := newTestConfig()

Also applies to: 220-295, 296-387

---

81-167: Consider executing retry tests against both engines

To uphold the shared‑suite guideline, table‑drive these tests to run with Sherpa and Olla services where feasible (construct via a common factory or interface). This helps catch engine‑specific regressions early.

internal/adapter/proxy/proxy_sherpa_test.go (5)

146-148: Avoid incidental flakiness in disconnect test by relaxing read timeout

A 200ms ReadTimeout is close to the test’s 100ms chunk cadence and 250ms cancel, introducing a second failure mode (read timeout vs client cancel). Bump it so cancellation is the primary trigger.

-				config.ReadTimeout = 200 * time.Millisecond
+				config.ReadTimeout = 2 * time.Second

---

158-160: Mirror timeout relaxation for Olla case

Same rationale as for Sherpa to keep the test deterministic.

-				config.ReadTimeout = 200 * time.Millisecond
+				config.ReadTimeout = 2 * time.Second

---

261-263: Buffer pooling test asserts only through side‑effects

The test relies on “no failures across multiple requests” as a proxy for pooling correctness. Consider minimally asserting response sizes or using b.ReportAllocs in a benchmark variant to validate that allocations remain stable with the chosen StreamBufferSize.

---

163-166: Use the canonical context key in proxy tests

The dynamic tc.proxyPrefix and tc.proxyConfigPrefix keys injected in tests aren’t read by the proxy implementations (which exclusively use constants.ContextRoutePrefixKey). To keep tests aligned with production behaviour and avoid brittle overrides, update or remove these uses.

• internal/adapter/proxy/proxy_path_stripping_test.go:164
• internal/adapter/proxy/proxy_path_stripping_test.go:321

Suggested diff:

-   // dynamic key that proxy impls ignore
-   ctx := context.WithValue(req.Context(), tc.proxyPrefix, tc.contextPrefix)
+   // use the stable key recognised by the proxy implementation
+   ctx := context.WithValue(req.Context(), constants.ContextRoutePrefixKey, tc.contextPrefix)

-   // dynamic key that proxy impls ignore
-   ctx := context.WithValue(req.Context(), tc.proxyConfigPrefix, tc.handlerPrefix)
+   ctx := context.WithValue(req.Context(), constants.ContextRoutePrefixKey, tc.handlerPrefix)

---

27-30: Extract a shared Sherpa test config builder to reduce duplication

The explicit field assignments for ResponseTimeout, ReadTimeout and StreamBufferSize recur throughout your Sherpa proxy tests (e.g. proxy_sherpa_test.go:27–30, as well as in service_retry_test.go, service_leak_test.go, etc.). Defining a small helper function will localise these defaults, keep tests DRY, and make future changes to the defaults impossible to miss.

It’s safe to introduce a builder because the Sherpa service always uses the getters—and falls back to its own defaults only when you haven’t explicitly set a field. For example:

• In service_streaming.go, getReadTimeout() calls s.configuration.GetReadTimeout() and only if that yields zero does it use config.DefaultReadTimeout.
• In service.go, both the buffer and cloned configs are initialised via configuration.GetStreamBufferSize() and config.GetReadTimeout()/GetResponseTimeout().

Implementing a test-only helper won’t affect production behaviour, and will clean up over a dozen lines of boilerplate across your test suite.

Suggested diff:

+// newSherpaTestConfig returns a Configuration pre-populated with the
+// proxy tests’ standard timeouts and buffer size, allowing optional
+// overrides.
+func newSherpaTestConfig(overrides func(*sherpa.Configuration)) *sherpa.Configuration {
+    c := &sherpa.Configuration{
+        ResponseTimeout: 30 * time.Second,
+        ReadTimeout:     10 * time.Second,
+        StreamBufferSize: 1024,
+    }
+    if overrides != nil {
+        overrides(c)
+    }
+    return c
+}

Then, in your tests:

-   config := &sherpa.Configuration{}
-   config.ResponseTimeout = 30 * time.Second
-   config.ReadTimeout = 10 * time.Second
-   config.StreamBufferSize = 1024
+   config := newSherpaTestConfig(nil)

This keeps all defaults in one place, and any test needing non-standard values can simply pass an override function.

internal/adapter/converter/vllm_converter_test.go (1)

210-210: Decouple tests from unexported VLLMConverter helpers

Your tests in internal/adapter/converter/vllm_converter_test.go directly cast the result of NewVLLMConverter() to *VLLMConverter and invoke two unexported methods:

• Line ≈210 — converter.determineOwner(…)
• Line ≈248 — converter.findVLLMNativeName(…)

This tight coupling makes the tests fragile to any future renames, embedding changes or constructor tweaks. Consider one of the following lightweight approaches to decouple:

• Export thin wrappers on BaseConverter
– e.g. add func (b *BaseConverter) DetermineOwnerFromID(id string) string and
func (b *BaseConverter) FindProviderAliasName(model *domain.UnifiedModel) (string, bool)
– Then in tests call converter := NewVLLMConverter(); converter.DetermineOwnerFromID(...) without a type assertion.

• Introduce a package-private test shim in a _test.go file
– e.g. define func DetermineOwner(c ports.ModelResponseConverter, id string) string that performs the cast internally
– Keeps your tests free of repeated .(*VLLMConverter) assertions.

• Or simply verify equivalent behaviour via the exported ConvertToFormat surface
– Assert that the OwnedBy field of the resulting VLLMModelResponse matches expectations, covering both helper functions indirectly.

These changes will make your suite more maintainable and resilient to refactors.

internal/adapter/proxy/proxy_path_stripping_test.go (4)

140-147: Config construction: suggest a tiny factory to avoid repetition

You set ProxyPrefix in multiple places; a helper reduces duplication and makes intent clearer.

-						sherpaConfig := &sherpa.Configuration{}
-						sherpaConfig.ProxyPrefix = tc.proxyPrefix
+						sherpaConfig := &sherpa.Configuration{ }
+						sherpaConfig.ProxyPrefix = tc.proxyPrefix
 						config = sherpaConfig

Optionally:

+func newSherpaConfigWithPrefix(prefix string) *sherpa.Configuration {
+	c := &sherpa.Configuration{}
+	c.ProxyPrefix = prefix
+	return c
+}

---

216-218: Same for Sherpa in double‑strip prevention test

Apply the same small factory for consistency and readability.

-	sherpaConfig := &sherpa.Configuration{}
-	sherpaConfig.ProxyPrefix = "/olla"
+	sherpaConfig := newSherpaConfigWithPrefix("/olla")

---

305-307: Prefer helper for Olla configuration as well

Keeps both implementations aligned and reduces test boilerplate.

-			config := &olla.Configuration{}
-			config.ProxyPrefix = tc.proxyConfigPrefix
+			config := &olla.Configuration{}
+			config.ProxyPrefix = tc.proxyConfigPrefix

Or via helper:

+func newOllaConfigWithPrefix(prefix string) *olla.Configuration {
+	c := &olla.Configuration{}
+	c.ProxyPrefix = prefix
+	return c
+}

---

163-166: Use the stable context key constant in proxy_path_stripping_test.go

The test at lines 163–166 currently injects the route prefix into the request context using tc.proxyPrefix as the key, but:

• All production code and other proxy tests (e.g. proxy_routing_test.go, config.go) consistently use constants.ContextRoutePrefixKey as the context key.
• Mixing in tc.proxyPrefix risks the test silently passing without actually exercising any real lookup in the proxy implementation.

To align this test with the rest of the codebase, either:

• Update the block to use the constant key, or
• Remove it entirely to reflect that proxy implementations defensively ignore context-based overrides.

Suggested optional refactor:

-                   if tc.contextPrefix != "" {
-                       ctx := context.WithValue(req.Context(), tc.proxyPrefix, tc.contextPrefix)
-                       req = req.WithContext(ctx)
-                   }
+                   // Context-based route‐prefix overrides are ignored by proxy implementations.
+                   // To test/document this, you could inject it with the stable key:
+                   // ctx := context.WithValue(req.Context(), constants.ContextRoutePrefixKey, tc.contextPrefix)
+                   // req = req.WithContext(ctx)

internal/adapter/proxy/benchmark_comparison_test.go (1)

58-61: Reduce duplication with small config builder helpers

These two setup blocks are identical to patterns repeated across the suite. Consider local helpers to keep benchmarks focused and reduce future drift when defaults evolve.

Example replacement within the changed ranges:

-	config := &sherpa.Configuration{}
-	config.ResponseTimeout = 30 * time.Second
-	config.ReadTimeout = 10 * time.Second
-	config.StreamBufferSize = 8192
+	config := newSherpaCfg(30*time.Second, 10*time.Second, 8192)

-	config := &olla.Configuration{}
-	config.ResponseTimeout = 30 * time.Second
-	config.ReadTimeout = 10 * time.Second
-	config.StreamBufferSize = 8192
-	config.MaxIdleConns = 200
-	config.IdleConnTimeout = 90 * time.Second
-	config.MaxConnsPerHost = 50
+	config := newOllaCfg(30*time.Second, 10*time.Second, 8192, 200, 90*time.Second, 50)

Add these helpers near the top of the file:

func newSherpaCfg(resp, read time.Duration, buf int) *sherpa.Configuration {
	c := &sherpa.Configuration{}
	c.ResponseTimeout = resp
	c.ReadTimeout = read
	c.StreamBufferSize = buf
	return c
}
func newOllaCfg(resp, read time.Duration, buf int, maxIdle int, idleTO time.Duration, maxPerHost int) *olla.Configuration {
	c := &olla.Configuration{}
	c.ResponseTimeout = resp
	c.ReadTimeout = read
	c.StreamBufferSize = buf
	c.MaxIdleConns = maxIdle
	c.IdleConnTimeout = idleTO
	c.MaxConnsPerHost = maxPerHost
	return c
}

Also applies to: 71-77

internal/adapter/proxy/proxy_streaming_profiles_test.go (2)

222-226: DRY up repeated config initialisation across suites

The Sherpa/Olla config assembly is copy-pasted six times here. Factor into suite-aware helpers (e.g., makeConfig(profile string)) to improve readability and keep behaviour aligned if defaults change.

Illustrative replacement within the changed blocks:

- config := &sherpa.Configuration{}
- config.Profile = tt.profile
- config.ResponseTimeout = 5 * time.Second
- config.ReadTimeout = 5 * time.Second
- config.StreamBufferSize = 8192
+ config := newSherpaCfg(tt.profile, 5*time.Second, 5*time.Second, 8192)

- config := &olla.Configuration{}
- config.Profile = tt.profile
- config.ResponseTimeout = 5 * time.Second
- config.ReadTimeout = 5 * time.Second
- config.StreamBufferSize = 8192
- config.MaxIdleConns = 10
- config.IdleConnTimeout = 90 * time.Second
- config.MaxConnsPerHost = 5
+ config := newOllaCfg(tt.profile, 5*time.Second, 5*time.Second, 8192, 10, 90*time.Second, 5)

Add minimal helpers once in this file:

func newSherpaCfg(profile string, resp, read time.Duration, buf int) *sherpa.Configuration {
	c := &sherpa.Configuration{}
	c.Profile, c.ResponseTimeout, c.ReadTimeout, c.StreamBufferSize = profile, resp, read, buf
	return c
}
func newOllaCfg(profile string, resp, read time.Duration, buf, maxIdle, maxPerHost int, idleTO time.Duration) *olla.Configuration {
	c := &olla.Configuration{}
	c.Profile, c.ResponseTimeout, c.ReadTimeout, c.StreamBufferSize = profile, resp, read, buf
	c.MaxIdleConns, c.MaxConnsPerHost, c.IdleConnTimeout = maxIdle, maxPerHost, idleTO
	return c
}

Also applies to: 229-236, 311-316, 318-326, 510-515, 517-525

---

192-275: Parallelise subtests carefully or leave as-is to avoid loop variable capture traps

If you plan to add t.Parallel() to these subtests, copy the loop variable (tt := tt) inside the loop before t.Run; otherwise parallel execution will race on tt. As written (serial), it’s fine.

internal/adapter/proxy/proxy_olla_test.go (2)

24-31: Consolidate Olla configuration setup and assert UpdateConfig semantics

• Config initialisation is repeated across the file; introduce a small helper to create a tuned Olla config for tests to reduce duplication and drift.
• In TestOllaProxyService_UpdateConfig, newConfig does not set pooling fields; if UpdateConfig replaces the whole struct, these may be reset to zero and then defaulted. Either set them explicitly in newConfig or assert that previous values are preserved by the update path.

Minimal changes within the changed ranges:

- config := &olla.Configuration{}
- config.ResponseTimeout = 30 * time.Second
- config.ReadTimeout = 10 * time.Second
- config.StreamBufferSize = 1024
- config.MaxIdleConns = 200
- config.IdleConnTimeout = 90 * time.Second
- config.MaxConnsPerHost = 50
+ config := newOllaTestCfg(30*time.Second, 10*time.Second, 1024, 200, 90*time.Second, 50)

And in UpdateConfig:

- newConfig := &olla.Configuration{}
- newConfig.ResponseTimeout = 20 * time.Second
- newConfig.ReadTimeout = 10 * time.Second
- newConfig.StreamBufferSize = 8192
+ newConfig := &olla.Configuration{}
+ newConfig.ResponseTimeout = 20 * time.Second
+ newConfig.ReadTimeout = 10 * time.Second
+ newConfig.StreamBufferSize = 8192
+ // Preserve pooling where not under test to avoid accidental regression from zeroing.
+ newConfig.MaxIdleConns = initialConfig.MaxIdleConns
+ newConfig.IdleConnTimeout = initialConfig.IdleConnTimeout
+ newConfig.MaxConnsPerHost = initialConfig.MaxConnsPerHost

Add once in this file:

func newOllaTestCfg(resp, read time.Duration, buf, maxIdle int, idleTO time.Duration, maxPerHost int) *olla.Configuration {
	c := &olla.Configuration{}
	c.ResponseTimeout, c.ReadTimeout, c.StreamBufferSize = resp, read, buf
	c.MaxIdleConns, c.IdleConnTimeout, c.MaxConnsPerHost = maxIdle, idleTO, maxPerHost
	return c
}

Also applies to: 51-57, 117-123, 199-205, 257-263, 478-485, 493-496, 529-535

---

449-469: Avoid magic numbers for defaults; assert against config constants

In TestOllaProxyService_ConfigDefaults you’re asserting specific numeric defaults (e.g., 64*1024, 90s). Prefer asserting against the actual default constants to keep tests resilient to tuning changes.

Example:

import proxycfg "github.com/thushan/olla/internal/adapter/proxy/config"

if config.StreamBufferSize != proxycfg.OllaDefaultStreamBufferSize { ... }
if config.MaxConnsPerHost != proxycfg.OllaDefaultMaxConnsPerHost { ... }
if config.IdleConnTimeout != proxycfg.OllaDefaultIdleConnTimeout { ... }

internal/adapter/proxy/proxy_integration_test.go (2)

174-178: Config initialisation repetition; introduce shared builders to keep parity tests tidy

Each test case re-assembles Sherpa and Olla configs with the same fields. Consolidate into small builder helpers to avoid drift and emphasise parity logic.

Illustrative within changed ranges:

- sherpaConfig := &sherpa.Configuration{}
- sherpaConfig.ResponseTimeout = 30 * time.Second
- sherpaConfig.ReadTimeout = 10 * time.Second
- sherpaConfig.StreamBufferSize = 8192
+ sherpaConfig := newSherpaCfg(30*time.Second, 10*time.Second, 8192)

- ollaConfig := &olla.Configuration{}
- ollaConfig.ResponseTimeout = 30 * time.Second
- ollaConfig.ReadTimeout = 10 * time.Second
- ollaConfig.StreamBufferSize = 8192
- ollaConfig.MaxIdleConns = 200
- ollaConfig.IdleConnTimeout = 90 * time.Second
- ollaConfig.MaxConnsPerHost = 50
+ ollaConfig := newOllaCfg(30*time.Second, 10*time.Second, 8192, 200, 90*time.Second, 50)

Add once in this file:

func newSherpaCfg(resp, read time.Duration, buf int) *sherpa.Configuration {
	c := &sherpa.Configuration{}
	c.ResponseTimeout, c.ReadTimeout, c.StreamBufferSize = resp, read, buf
	return c
}
func newOllaCfg(resp, read time.Duration, buf, maxIdle int, idleTO time.Duration, maxPerHost int) *olla.Configuration {
	c := &olla.Configuration{}
	c.ResponseTimeout, c.ReadTimeout, c.StreamBufferSize = resp, read, buf
	c.MaxIdleConns, c.IdleConnTimeout, c.MaxConnsPerHost = maxIdle, idleTO, maxPerHost
	return c
}

Also applies to: 179-186, 201-205, 206-213, 242-246, 247-254, 417-423, 462-465, 589-594, 595-603

---

498-571: Performance test average latency maths is acceptable, but consider reporting p95

Dividing wall-clock by request count under concurrency gives a coarse average; adding a simple p95 sample (tracking per-request durations) would better capture tail behaviour without much complexity. Optional.

internal/adapter/proxy/olla/config.go (1)

4-5: LGTM: Embed OllaConfig; add compile-time interface assertion for safety

Embedding config.OllaConfig keeps the surface unified. Consider adding an interface assertion to catch drift at compile time.

 package olla

 import (
   "github.com/thushan/olla/internal/adapter/proxy/config"
+  "github.com/thushan/olla/internal/core/ports"
 )

 // Configuration holds Olla proxy settings
 type Configuration struct {
   config.OllaConfig
 }
+
+// Ensure Configuration satisfies the proxy configuration interface
+var _ ports.ProxyConfiguration = (*Configuration)(nil)

Also applies to: 7-10

internal/adapter/proxy/sherpa/config.go (1)

8-11: Prefer embedding config.SherpaConfig (not BaseProxyConfig) for intent + forward‑compatibility

Sherpa now has its own wrapper type in the shared package. Embedding that keeps semantics self‑documenting and protects you if Sherpa‑specific fields are added later.

Apply:

-// Configuration holds Sherpa proxy settings
+// Configuration holds Sherpa proxy settings. We embed config.SherpaConfig
+// (rather than the bare BaseProxyConfig) so future Sherpa‑specific fields
+// and defaults flow through without touching this package.
 type Configuration struct {
-	config.BaseProxyConfig
+	config.SherpaConfig
 }

internal/adapter/proxy/config/unified.go (6)

27-40: Clarify zero‑value semantics in the interface doc

Documenting how zero values are interpreted helps implementers avoid surprises and keeps behaviour consistent across proxies.

Apply:

-// ProxyConfig defines the interface for all proxy configurations
+// ProxyConfig defines the interface for all proxy configurations.
+// Zero‑value semantics:
+//  - Durations of 0 adopt package defaults unless otherwise noted.
+//  - ResponseTimeout of 0 means “no response timeout”.
+//  - StreamBufferSize of 0 adopts the proxy‑specific default.
 type ProxyConfig interface {

---

61-67: Avoid product‑specific defaults in Base; push “/olla/” into OllaConfig

Baking “/olla/” into the base ties all proxy implementations to Olla’s path. Prefer a neutral base default and override in OllaConfig.

Suggested changes:

 func (c *BaseProxyConfig) GetProxyPrefix() string {
   if c.ProxyPrefix == "" {
-    return "/olla/"
+    return "/"
   }
   return c.ProxyPrefix
 }

And add an Olla override:

+// GetProxyPrefix returns the proxy prefix, defaulting to "/olla/" for Olla.
+func (c *OllaConfig) GetProxyPrefix() string {
+  if c.ProxyPrefix == "" {
+    return "/olla/"
+  }
+  return c.ProxyPrefix
+}

---

53-59: Call out that implementations may override profile defaults

Make the override pattern explicit to avoid confusion when packages (like Sherpa) diverge from “auto”.

Apply:

-// GetProxyProfile returns the proxy profile, defaulting to "auto" if not set
+// GetProxyProfile returns the proxy profile, defaulting to "auto" if not set.
+// Implementations may override this to reflect typical usage (e.g. Sherpa
+// defaults to "streaming" to optimise interactive behaviour).
 func (c *BaseProxyConfig) GetProxyProfile() string {

---

106-109: Implement real validation (non‑negative durations, sensible buffer size)

Returning nil hides misconfiguration that can lead to surprising runtime behaviour.

Apply:

+import (
+  "time"
+  "github.com/thushan/olla/internal/core/constants"
+  "fmt"
+)
@@
 func (c *BaseProxyConfig) Validate() error {
-  return nil
+  if c.StreamBufferSize < 0 {
+    return fmt.Errorf("stream buffer size must be >= 0 (got %d)", c.StreamBufferSize)
+  }
+  if c.ConnectionTimeout < 0 || c.ConnectionKeepAlive < 0 || c.ResponseTimeout < 0 || c.ReadTimeout < 0 {
+    return fmt.Errorf("timeouts must be >= 0 (got ct=%v, ka=%v, rt=%v, read=%v)",
+      c.ConnectionTimeout, c.ConnectionKeepAlive, c.ResponseTimeout, c.ReadTimeout)
+  }
+  // Optional: require a leading slash for prefixes to avoid path bugs.
+  if c.ProxyPrefix != "" && c.ProxyPrefix[0] != '/' {
+    return fmt.Errorf("proxy prefix must start with '/', got %q", c.ProxyPrefix)
+  }
+  return nil
 }

---

9-25: Tighten comments and prefer Australian English (“behaviour”)

Minor doc polish to explain why these defaults exist and match the project’s style guide.

Apply:

-// Default values for proxy settings
+// Default values for proxy settings — chosen to provide sensible out‑of‑the‑box behaviour.
@@
-  // Olla-specific defaults for high-performance
+  // Olla‑specific defaults tuned for high‑throughput streaming workloads
@@
-  // Olla uses 30s timeouts for faster failure detection in AI workloads
+  // Olla uses 30 s timeouts for faster failure detection in AI workloads

---

111-115: Dead code: remove unused SherpaConfig

A repository-wide search shows SherpaConfig is only ever declared in
internal/adapter/proxy/config/unified.go and never referenced elsewhere.
To reduce cognitive overhead, you can safely delete this dead type.

• File: internal/adapter/proxy/config/unified.go (lines 111–115)
• Remove the block:

- // SherpaConfig extends BaseProxyConfig with Sherpa-specific configuration
- type SherpaConfig struct {
-     BaseProxyConfig
- }

• If you meant to have a distinct wrapper for Sherpa configuration, consider merging it with the existing Sherpa config type (e.g. in pkg/sherpa/config) instead of keeping two parallel definitions.

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

• MCP integration is disabled by default for public repositories
• Jira integration is disabled by default for public repositories
• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

Learnt from: CR
PR: thushan/olla#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-21T11:42:29.496Z
Learning: Applies to internal/adapter/proxy/**/*_test.go : Maintain a shared test suite that validates both Sherpa and Olla proxy engines

Learnt from: CR
PR: thushan/olla#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-21T11:42:29.496Z
Learning: Applies to internal/app/handlers/handler_proxy.go : Serve proxy routes under the /olla/ prefix

internal/adapter/proxy/olla/service.go (1)

686-709: Also accept Olla-specific getters in UpdateConfig

Currently we only copy Olla-specific fields when the incoming config is exactly *olla.Configuration. Mirror the factory’s optional-getters path so unified configs carrying these values flow through.

Apply this diff:

 	// we try to get Olla-specific fields from incoming config if it's an *olla.Configuration
-	if ollaConfig, ok := config.(*Configuration); ok && ollaConfig != nil {
+	if oc, ok := config.(interface {
+		GetMaxIdleConns() int
+		GetIdleConnTimeout() time.Duration
+		GetMaxConnsPerHost() int
+	}); ok {
+		newConfig.MaxIdleConns = oc.GetMaxIdleConns()
+		newConfig.IdleConnTimeout = oc.GetIdleConnTimeout()
+		newConfig.MaxConnsPerHost = oc.GetMaxConnsPerHost()
+	} else if ollaConfig, ok := config.(*Configuration); ok && ollaConfig != nil {
 		newConfig.MaxIdleConns = ollaConfig.MaxIdleConns
 		newConfig.IdleConnTimeout = ollaConfig.IdleConnTimeout
 		newConfig.MaxConnsPerHost = ollaConfig.MaxConnsPerHost
 	} else {

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Learnt from: CR
PR: thushan/olla#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-23T08:30:20.348Z
Learning: Applies to internal/adapter/proxy/**/service.go : Proxy services must include response headers: `X-Olla-Endpoint`, `X-Olla-Model`, `X-Olla-Backend-Type`, `X-Olla-Request-ID`, `X-Olla-Response-Time`

Learnt from: CR
PR: thushan/olla#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-23T08:30:20.348Z
Learning: Applies to internal/app/handlers/handler_proxy.go : All proxied routes must use the `/olla/` URL prefix