Ollama/hailo-ollama installs ignore target_remote: remote-node model installs pull onto the controller - #2463
Conversation
The ollama installer never received target_remote from install_app, so remote-worker installs pulled models onto the controller's localhost daemon. Add resolve_ollama_url(target_remote, backend_id) following the resolve_rkllama_url convention: local targets use the existing defaults (_default_host() for ollama, localhost:7836 for hailo-ollama with TAOS_HAILO_OLLAMA_PORT override), remote targets build http://<target_remote>:<port> (11434 for ollama, 7836 for hailo-ollama). Pass the resolved host through get_installer instead of the previous hardcoded localhost:7836. Includes red-then-green regression tests through the real get_installer path and a changelog fragment.
|
ⓘ Qodo reviews are paused because your trial has ended. Ask your workspace admin to add credits to resume reviews. Manage billing |
|
Warning Review limit reached
Next review available in: 16 minutes Limit details: You’ve used all 2 included reviews currently available under your plan. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?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 reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughOllama installation now resolves endpoints for local and remote targets. Standard Ollama and ChangesOllama remote target support
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant StoreInstall
participant resolve_ollama_url
participant OllamaInstaller
StoreInstall->>resolve_ollama_url: target_remote and backend_id
resolve_ollama_url-->>StoreInstall: resolved endpoint
StoreInstall->>OllamaInstaller: construct installer with endpoint
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
|
|
|
nemotron-super review VERDICT: Blocking issues found.
Automated first-pass review by the nemotron-super lane. The lead still reviews before merge. |
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| if not raw: | ||
| return _DEFAULT_HAILO_OLLAMA_PORT | ||
| try: | ||
| return int(raw) |
There was a problem hiding this comment.
WARNING: _hailo_ollama_port() accepts invalid port values without range validation
The function only catches ValueError when converting the env var to int, but does not validate the port is within the valid range (1–65535). A misconfigured TAOS_HAILO_OLLAMA_PORT=0 or a very large value would be silently accepted, producing an invalid port that causes connection failures at runtime.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| return _DEFAULT_HAILO_OLLAMA_PORT | ||
|
|
||
|
|
||
| def resolve_ollama_url(target_remote: str | None, backend_id: str = "ollama") -> str: |
There was a problem hiding this comment.
SUGGESTION: Docstring misleadingly claims all local targets use "localhost"
The docstring states "the controller's own daemon (localhost)", but for the plain ollama backend the function returns _default_host(), which can return a non-localhost URL when OLLAMA_HOST is set. The docstring should clarify that only hailo-ollama hardcodes localhost, while plain ollama honours OLLAMA_HOST.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| port = _hailo_ollama_port() | ||
| if not target_remote or target_remote == "local": | ||
| return f"http://localhost:{port}" | ||
| return f"http://{target_remote}:{port}" |
There was a problem hiding this comment.
SUGGESTION: target_remote is not sanitized before being interpolated into the URL
On lines 77 and 81, target_remote is used directly in an f-string to build a URL without validating or sanitizing it. A value containing special characters (e.g. spaces, slashes, or ://) would produce a malformed URL. Consider validating that target_remote matches an expected hostname/IP pattern, or at minimum rejecting strings containing ://, /, or whitespace.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: 3 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (1 file)
Fix these issues in Kilo Cloud Previous Review Summaries (2 snapshots, latest commit de1030c)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit de1030c)Status: No Issues Found | Recommendation: Merge Files Reviewed (0 files)No code changed since the previous review (latest commit is an empty CI merge-ref refresh). Previous findings remain on unchanged lines and are not re-reported here. Previous review (commit 1429425)Status: 3 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (3 files)
Reviewed by step-3.7-flash · Input: 122.8K · Output: 23.4K · Cached: 603.4K |
|
Reviewed vs card tsk-mlogef + nemotron/Kilo. APPROVED with dispositions; doc-gate satisfied via body trailer; one class card cut. Contract: delivered. Red proof (was missing from the body — measured by me, now appended there): dev's wiring + this PR's tests = both remote-target tests FAIL (install captured nemotron BLOCKING (target_remote URL injection) + Kilo S77 — CONFIRMED as a real hole but PRE-EXISTING CLASS, not introduced here: Kilo W53 (port range validation) — declined: non-integer override warns and falls back, matching the install script's own Merge chain: on green (incl. bot-review-gate CR review), merge + close tsk-mlogef. |
|
@coderabbitai full review |
|
|
@coderabbitai full review |
✅ Action performedFull review finished. |
Empty commit: re-mints the PR merge ref so deleted-symbols-gate re-runs against current dev (its red was the two-snapshot false positive fixed by #2464) and doc-gate re-runs past the Docs-Reviewed body trailer.
Docs-Reviewed: bug fix inside existing install endpoint and ollama installer; no route surface or installer-flow change; user-facing behavior recorded in changelog.d/tsk-mlogef-remote-target-ollama.md
|
@coderabbitai full review |
|
|
@coderabbitai full review |
|
1 participant
CARD TITLE (intent, not commit subject): Ollama/hailo-ollama installs ignore target_remote: remote-node model installs pull onto the controller
Autonomous build of board card tsk-mlogef.
The ollama installer never received target_remote from install_app, so
remote-worker installs pulled models onto the controller's localhost daemon.
Add resolve_ollama_url(target_remote, backend_id) following the
resolve_rkllama_url convention: local targets use the existing defaults
(_default_host() for ollama, localhost:7836 for hailo-ollama with
TAOS_HAILO_OLLAMA_PORT override), remote targets build
http://<target_remote>: (11434 for ollama, 7836 for hailo-ollama).
Pass the resolved host through get_installer instead of the previous
hardcoded localhost:7836. Includes red-then-green regression tests through
the real get_installer path and a changelog fragment.
Files:
changelog.d/tsk-mlogef-remote-target-ollama.md | 2 +
tests/test_routes_store_install.py | 163 +++++++++++++++++++++++++
tinyagentos/installers/ollama_installer.py | 43 ++++++-
tinyagentos/routes/store_install.py | 3 +-
4 files changed, 209 insertions(+), 2 deletions(-)
Red proof (measured by the lead on the branch, 2026-08-17)
Behavioral red at the wiring layer — dev's
store_install.py+ this PR's tests:(the 2 passes are the local-target controls — no behavior change locally). Green: 61/61 on the branch.
Docs-Reviewed: routes/installer changes route installs to the node the UI already lets the user select; no end-user doc describes daemon-URL selection. docs/design/hailo-llm-backend.md §B stays accurate — TAOS_HAILO_OLLAMA_PORT is pre-existing (install-hailo.sh:74) and this PR makes the client honour what §B already promises. README install docs are backend-agnostic and unaffected.
Summary by CodeRabbit
New Features
Bug Fixes