fix(ollama): local /v1 endpoint + Ollama in compose + @ollama BDD - #16
Merged
Conversation
…e + @ollama BDD The ollama provider existed but was never exercised end to end. Two fixes + the test infra to keep it that way: - sources/ollama.py: a LOCAL offer's seller_endpoint must include /v1 (the host appends /chat/completions); discovery still uses the bare base for /api/tags. Without this a local route 404s ("404 page not found"). Cloud already used /api/v1. - compose.yml: an `ollama` sidecar (tiny on purpose — pull a small model after up, e.g. qwen2.5:0.5b), OLLAMA_BASE_URL=http://ollama:11434 so the router discovers it, and a dummy OLLAMA_API_KEY the local server ignores (the provider declares auth_env for Cloud; local needs none but the auth resolver wants the var). - features/ollama_routing.feature + steps: the 4 @ollama scenarios were undefined (StepNotImplementedError). Now: local routing is asserted end to end (routes to provider ollama, real content, $0); the cloud scenarios skip gracefully unless OLLAMA_CLOUD=1 + OLLAMA_API_KEY are set (the @AntSeed skip pattern). Verified: behave features/ollama_routing.feature -> 1 passed, 3 skipped, 0 failed (local qwen2.5:0.5b served by ollama).
|
Warning Review limit reached
More reviews will be available in 50 minutes and 37 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses rolling per-developer review limits. Reviews become available again as older review attempts age out of the rolling limit window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds an Ollama sidecar service to ChangesOllama Routing
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
jmlago
added a commit
that referenced
this pull request
Jun 23, 2026
…ession (#15) * feat(cache): per-session cache affinity — cache_hot field + route_cache + session Agents re-send a large, stable prefix every turn; the provider's prompt-cache discount only lands if the SAME peer keeps serving the conversation. This adds cache-aware routing so a policy can keep an agent's session pinned to the peer that already holds its prefix hot. Form delta (the four natures) Definition: a new observation field cache_hot (Bool) — true for the candidate whose route is the one this session most recently used successfully. Plus route_cache, the host-side per-session memory of that route, and a session id on the request that ties them together. Zero engine change (verified): cache_hot is a HOST-declared extension field via the fields.lua schema{extensions} seam, injected once in LLMRouterHost; the core/ submodule is untouched — signature, ops and goldens unchanged. Route identity stays 100% host-internal; the algebra only ever observes the Bool (same contract as latency_ms/success_rate, engine #14). Locus note: the design first proposed stamping cache_hot in offers_sync, but offers_sync is request-blind (built once per refresh, no session). The correct seam is ctx.request: build_ctx already exposes the whole contract as ctx.request, so the host resolves the session's hot route into contract.cache_hot_route per request and the field getter reconstructs each candidate's route key (exactly as _fold_route_outcome does) and compares — no engine, no offers_sync, no per-source edits. Invariants: /v1 evolves additively (optional session in; nothing removed). The central fold (_fold_route_outcome) folds route_cache alongside reliability/latency on each outcome; no-op without a session. A new/unknown session gets no affinity (default false — no phantom stickiness). Essence (tests): - tests/test_route_cache.py (10): route_cache fold (success-only, per-session), the central-hook integration, and the cache_hot field marking exactly the hot route so a policy scoring it picks it. - features/10_agent_cache.feature (+ steps): end-to-end over /v1 — an agent's session lifts its working route's score, and a brand-new session gets no affinity. Runs on the fixed gpt-5.3-codex-spark family. Goods: +bonum (cache efficiency), no degradation of unum/verum; net-small; no new deps. * cache_hot: serialize the route key once (bridge route_key into Lua) The cache_hot getter re-serialized the route key (provider|family|peer) in Lua — a second source for an identity already defined once by route_reliability.route_key in Python. Format or peer-derivation drift across the Python/Lua boundary would silently lose affinity (cache_hot all false, no error). Bridge the single route_key into Lua as the host_route_key global and call it from the getter, so the serialization has exactly one source. Behaviour unchanged: route_cache tests 10/0; full suite identical A/B (the 3 provider_filter_flow failures are pre-existing — the branch's core/ submodule is at engine #17 and needs #18 for provider_eq; rebase on main). * core: bump to engine #18 (provider_eq) Restores the submodule pin reverted by #13's merge; re-pins to engine main 3d49132 so the provider_eq flow tests pass. Same bump #16 carries.
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
compose.yml (1)
159-159: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winPin the Ollama image to a fixed tag (or digest).
Using
ollama/ollama:latestmakes local/dev behavior non-reproducible and can introduce surprise breakages after upstream pushes.Suggested change
- image: ollama/ollama:latest + image: ollama/ollama:<fixed-version-or-digest>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@compose.yml` at line 159, The Ollama image specification in compose.yml is using the latest tag which results in non-reproducible builds and potential unexpected changes. Replace the image line where it says ollama/ollama:latest with a pinned version by specifying a concrete version tag (such as a specific release number) or a digest hash instead of using the latest tag to ensure consistent and reproducible deployments.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@features/steps/steps.py`:
- Around line 303-310: The step functions step_cloud_auth and
step_cloud_endpoint contain only pass statements and do not perform any
assertions or verifications. Replace the pass statement in step_cloud_auth with
code that extracts and verifies the Authorization header from the context's HTTP
response or request object to ensure it is properly set for Ollama Cloud
authentication. Similarly, replace the pass statement in step_cloud_endpoint
with code that asserts that the actual endpoint URL stored in the context
matches the expected url parameter passed to the function. Both functions should
use appropriate assertion methods to fail the test if the expected values do not
match the actual values.
- Around line 313-316: The assertion in the step_succeeds_local function only
verifies the HTTP status code is 200 but does not prove that the local Ollama
fallback was actually selected. Enhance the assertion to additionally check the
router metadata in the response (such as verifying x_router.provider equals
"ollama") and validate evidence of the local model and endpoint being used,
rather than just checking the status code alone.
- Around line 318-324: The `step_cloud_no_key` function currently
unconditionally skips the test via `context.scenario.skip()`, which means the
cloud authentication error path is never actually exercised. Remove the skip
call and instead implement the actual test steps that set up the conditions
where OLLAMA_CLOUD=1 and OLLAMA_API_KEY is unset, then verify that the expected
authentication error is properly raised and handled.
- Around line 274-277: The step_cost_zero function currently accepts None as a
valid zero cost in its assertion, which masks regressions when the
x_router.cost_usd field is missing from the response. Remove None from the tuple
of acceptable values in the assertion, so only 0 and 0.0 are valid, ensuring
that missing fields will cause the test to fail as expected.
- Around line 247-250: The step_ollama_local function receives a model parameter
from the step definition but ignores it, using the hardcoded OLLAMA_LOCAL_MODEL
constant instead on both the context assignment and the _chat_family call.
Replace both occurrences of OLLAMA_LOCAL_MODEL with the model parameter to
ensure the actual model value from the feature file is used and validated
correctly.
---
Nitpick comments:
In `@compose.yml`:
- Line 159: The Ollama image specification in compose.yml is using the latest
tag which results in non-reproducible builds and potential unexpected changes.
Replace the image line where it says ollama/ollama:latest with a pinned version
by specifying a concrete version tag (such as a specific release number) or a
digest hash instead of using the latest tag to ensure consistent and
reproducible deployments.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 94e74321-744e-44b3-a8e3-5450f206f435
📒 Files selected for processing (5)
compose.ymlcorefeatures/ollama_routing.featurefeatures/steps/steps.pysources/ollama.py
Comment on lines
+247
to
+250
| @given('Ollama is running locally with model "{model}"') | ||
| def step_ollama_local(context, model): | ||
| context.ollama_model = OLLAMA_LOCAL_MODEL | ||
| _chat_family(context, OLLAMA_LOCAL_MODEL) # probe discovery |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use the step parameter instead of hardcoding the local model.
Line 249 ignores the {model} argument, which can let mismatched feature text pass unnoticed.
Suggested fix
def step_ollama_local(context, model):
- context.ollama_model = OLLAMA_LOCAL_MODEL
- _chat_family(context, OLLAMA_LOCAL_MODEL) # probe discovery
+ context.ollama_model = model
+ _chat_family(context, model) # probe discovery🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@features/steps/steps.py` around lines 247 - 250, The step_ollama_local
function receives a model parameter from the step definition but ignores it,
using the hardcoded OLLAMA_LOCAL_MODEL constant instead on both the context
assignment and the _chat_family call. Replace both occurrences of
OLLAMA_LOCAL_MODEL with the model parameter to ensure the actual model value
from the feature file is used and validated correctly.
Comment on lines
+274
to
+277
| @then('the cost is zero') | ||
| def step_cost_zero(context): | ||
| cost = jpath(context.json or {}, "x_router.cost_usd") | ||
| assert cost in (0, 0.0, None), f"expected $0, got {cost!r}" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Cost-zero assertion should fail when cost is missing.
Line 277 accepts None, which can hide regressions where x_router.cost_usd is absent instead of zero.
Suggested tightening
def step_cost_zero(context):
cost = jpath(context.json or {}, "x_router.cost_usd")
- assert cost in (0, 0.0, None), f"expected $0, got {cost!r}"
+ assert cost in (0, 0.0), f"expected $0, got {cost!r}"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@features/steps/steps.py` around lines 274 - 277, The step_cost_zero function
currently accepts None as a valid zero cost in its assertion, which masks
regressions when the x_router.cost_usd field is missing from the response.
Remove None from the tuple of acceptable values in the assertion, so only 0 and
0.0 are valid, ensuring that missing fields will cause the test to fail as
expected.
Comment on lines
+303
to
+310
| @then('the Authorization header is set for Ollama Cloud') | ||
| def step_cloud_auth(context): | ||
| pass | ||
|
|
||
|
|
||
| @then('the endpoint is "{url}"') | ||
| def step_cloud_endpoint(context, url): | ||
| pass |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Cloud verification steps are no-ops.
Line 304 and Line 309 use pass, so the scenario never verifies auth-header or endpoint behavior despite claiming it does.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@features/steps/steps.py` around lines 303 - 310, The step functions
step_cloud_auth and step_cloud_endpoint contain only pass statements and do not
perform any assertions or verifications. Replace the pass statement in
step_cloud_auth with code that extracts and verifies the Authorization header
from the context's HTTP response or request object to ensure it is properly set
for Ollama Cloud authentication. Similarly, replace the pass statement in
step_cloud_endpoint with code that asserts that the actual endpoint URL stored
in the context matches the expected url parameter passed to the function. Both
functions should use appropriate assertion methods to fail the test if the
expected values do not match the actual values.
Comment on lines
+313
to
+316
| @then('the request succeeds from local Ollama') | ||
| def step_succeeds_local(context): | ||
| assert context.resp.status_code == 200, context.resp_text[:200] | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
“Succeeds from local Ollama” is under-asserted.
Line 315 only checks HTTP 200; it does not prove fallback selected local Ollama. Assert router metadata (for example x_router.provider == "ollama" and local model/endpoint evidence) in this step.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@features/steps/steps.py` around lines 313 - 316, The assertion in the
step_succeeds_local function only verifies the HTTP status code is 200 but does
not prove that the local Ollama fallback was actually selected. Enhance the
assertion to additionally check the router metadata in the response (such as
verifying x_router.provider equals "ollama") and validate evidence of the local
model and endpoint being used, rather than just checking the status code alone.
Comment on lines
+318
to
+324
| @given('OLLAMA_CLOUD=1 and OLLAMA_API_KEY is not set') | ||
| def step_cloud_no_key(context): | ||
| # conflicts with the running stack (which carries a key so local works); | ||
| # not reproducible per-scenario here -> skip (the unset-key auth error is | ||
| # covered at the source level). | ||
| context.scenario.skip("cloud-no-key path not reproducible against the shared " | ||
| "running stack") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
No-key auth scenario is untestable in current form.
Line 323 unconditionally skips this path, so the feature’s auth-error contract is never exercised in BDD.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@features/steps/steps.py` around lines 318 - 324, The `step_cloud_no_key`
function currently unconditionally skips the test via `context.scenario.skip()`,
which means the cloud authentication error path is never actually exercised.
Remove the skip call and instead implement the actual test steps that set up the
conditions where OLLAMA_CLOUD=1 and OLLAMA_API_KEY is unset, then verify that
the expected authentication error is properly raised and handled.
# Conflicts: # features/steps/steps.py
#16 makes a local offer's seller_endpoint base + /v1 (the host appends /chat/completions; bare base 404s). The two local-discovery asserts still expected the bare base — align them with the shipped behavior.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The ollama provider existed but was never exercised end to end. Two fixes + the
test infra to keep it that way:
appends /chat/completions); discovery still uses the bare base for /api/tags.
Without this a local route 404s ("404 page not found"). Cloud already used /api/v1.
ollamasidecar (tiny on purpose — pull a small model after up,e.g. qwen2.5:0.5b), OLLAMA_BASE_URL=http://ollama:11434 so the router discovers
it, and a dummy OLLAMA_API_KEY the local server ignores (the provider declares
auth_env for Cloud; local needs none but the auth resolver wants the var).
(StepNotImplementedError). Now: local routing is asserted end to end (routes to
provider ollama, real content, $0); the cloud scenarios skip gracefully unless
OLLAMA_CLOUD=1 + OLLAMA_API_KEY are set (the @AntSeed skip pattern).
Verified: behave features/ollama_routing.feature -> 1 passed, 3 skipped, 0 failed
(local qwen2.5:0.5b served by ollama).
Summary by CodeRabbit
Release Notes
New Features
Tests