fix: verify Ollama model discovery after pull - #6075
Closed
HOYALIM wants to merge 2 commits into
Closed
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (3)
💤 Files with no reviewable changes (2)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughAdds a post-pull verification step in Ollama model preparation that polls model discovery with exponential backoff and matches model references with tag normalization, updates ChangesOllama post-pull model registration check
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant prepareOllamaModel
participant pullOllamaModel
participant waitForPulledOllamaModel
participant getOllamaModelOptions
prepareOllamaModel->>pullOllamaModel: pull(model)
pullOllamaModel-->>prepareOllamaModel: pull success
prepareOllamaModel->>waitForPulledOllamaModel: verify registration(model)
loop retry with exponential backoff (up to 8 attempts)
waitForPulledOllamaModel->>getOllamaModelOptions: poll installed models
getOllamaModelOptions-->>waitForPulledOllamaModel: model list
end
alt model listed (ref match)
waitForPulledOllamaModel-->>prepareOllamaModel: true
prepareOllamaModel-->>prepareOllamaModel: ok: true
else not listed after retries
waitForPulledOllamaModel-->>prepareOllamaModel: false
prepareOllamaModel-->>prepareOllamaModel: ok: false, "not listed yet"
end
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/onboard-ollama-pull-registration.test.ts (1)
60-70: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate fake
ollamabinary script across both tests.The fake
ollamapull-stub script (lines 60-70 and 167-177) is byte-for-byte identical between the two test cases, whilewriteAlwaysOkCurlwas already extracted as a shared helper for the analogouscurlstub. Extracting an equivalentwriteAlwaysOkOllama(fakeBin, pullLog)helper would keep the file consistent and reduce duplication.♻️ Suggested helper extraction
+function writeAlwaysOkOllama(fakeBin: string, pullLog: string) { + fs.writeFileSync( + path.join(fakeBin, "ollama"), + `#!/usr/bin/env bash +if [ "$1" = "pull" ]; then + echo "$2" >> ${JSON.stringify(pullLog)} + exit 0 +fi +exit 0 +`, + { mode: 0o755 }, + ); +}Then replace both inline
fs.writeFileSync(path.join(fakeBin, "ollama"), ...)blocks withwriteAlwaysOkOllama(fakeBin, pullLog);.Also applies to: 167-177
🤖 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 `@test/onboard-ollama-pull-registration.test.ts` around lines 60 - 70, The fake ollama pull stub is duplicated in both test cases, so extract it into a shared helper alongside writeAlwaysOkCurl. Add a writeAlwaysOkOllama(fakeBin, pullLog) helper that writes the same executable ollama script used by both tests, then replace each inline fs.writeFileSync(path.join(fakeBin, "ollama"), ...) block with the helper call to keep the test file consistent and remove duplication.
🤖 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.
Nitpick comments:
In `@test/onboard-ollama-pull-registration.test.ts`:
- Around line 60-70: The fake ollama pull stub is duplicated in both test cases,
so extract it into a shared helper alongside writeAlwaysOkCurl. Add a
writeAlwaysOkOllama(fakeBin, pullLog) helper that writes the same executable
ollama script used by both tests, then replace each inline
fs.writeFileSync(path.join(fakeBin, "ollama"), ...) block with the helper call
to keep the test file consistent and remove duplication.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 9a9fc8e1-ade9-4aaa-900e-e56cd3ac785d
📒 Files selected for processing (4)
ci/test-file-size-budget.jsonsrc/lib/inference/ollama/proxy.tstest/onboard-ollama-pull-registration.test.tstest/onboard-selection.test.ts
HOYALIM
force-pushed
the
codex/issue-6038-ollama-empty-onboard
branch
from
June 30, 2026 23:54
4b2a913 to
b425d5a
Compare
5 tasks
wscurran
added
area: local-models
Contributor
|
✨ Thanks for the fix. Verifying Ollama model discovery after pull prevents silent broken routes during onboarding. Related open issues: |
HOYALIM
force-pushed
the
codex/issue-6038-ollama-empty-onboard
branch
from
July 6, 2026 14:53
b425d5a to
78ecae5
Compare
Signed-off-by: Ho Lim <subhoya@gmail.com>
HOYALIM
force-pushed
the
codex/issue-6038-ollama-empty-onboard
branch
from
July 6, 2026 15:00
78ecae5 to
ba2211b
Compare
cv
requested changes
Jul 7, 2026
cv
left a comment
cv
left a comment
Collaborator
There was a problem hiding this comment.
Please rebase or port the narrow post-pull discovery fix onto current main. This branch conflicts, and its stale 253-line test reshuffle would overwrite newer onboarding test-performance refactors from #6276 and #6336. Preserve those mainline changes and submit only the behavior needed for #6038; CI and advisor must then run.
14 tasks
Collaborator
|
Current-main replacement opened as #6481. It preserves Ho Lim as the signed commit author and ports only the #6038 behavior: bounded post-pull discovery, a clear retry path when the model never appears, focused wait tests, and net-zero updates to the existing onboarding fixtures. The stale 253-line test reshuffle and conflicts with current test-performance work are not carried forward. Local replacement-head validation: build/typecheck, 7 proxy tests, all 64 onboarding-selection tests, Biome, and the test-size guard passed. I will close this conflicted branch once #6481's exact-head CI settles. |
Port the verified contributor change onto current main without the stale test split. Add delayed-discovery, retry, and implicit latest-tag coverage. Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Collaborator
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Collaborator
|
Superseded for v0.0.78 by #6481, which preserves Ho Lim's original fix and attribution while adding bounded discovery polling, canonical implicit- |
cv
added a commit
that referenced
this pull request
Jul 8, 2026
<!-- markdownlint-disable MD041 --> ## Summary Confirms that Ollama exposes a newly pulled model before onboarding accepts the route. This is a current-main replacement for #6075 that preserves Ho Lim's original authorship without carrying the conflicted test reshuffle. ## Related Issue Fixes #6038. ## Changes - Poll Ollama model discovery with bounded exponential backoff after a successful pull. - Return to model selection with a clear error when the daemon never lists the pulled model. - Treat omitted model tags as Ollama's canonical `:latest` form across pull authorization and post-pull discovery. - Keep model-reference, pull-readiness, and bounded discovery logic in a focused typed module.\n- Remove the legacy `@ts-nocheck` from `proxy.ts` while shrinking that hotspot below its main-branch line count. - Add deterministic wait success/exhaustion tests plus integrated delayed-registration and zero-exit/unlisted reprompt coverage. - Update existing successful-pull onboarding fixtures without growing the legacy test file. ## Type of Change - [x] Code change (bug fix) - [ ] Code change with doc updates - [ ] Doc only ## Quality Gates - [x] Tests added or updated for changed behavior - [x] Docs not applicable — this corrects existing onboarding behavior and error handling - [x] Sensitive paths changed — local Ollama onboarding and inference preparation. - [x] No secrets, API keys, or credentials committed ## Notes The registration-wait status and post-pull failure message make an existing onboarding contract explicit. The current Ollama docs already describe pull, load, and validation, so this reliability fix adds no new command, option, or workflow. ## Verification - [x] `npm run build:cli` - [x] `npm run typecheck:cli` - [x] `model-discovery.test.ts` + `proxy.test.ts` — 18/18 passed - [x] `test/onboard-selection.test.ts` — 64/64 passed - [x] Biome format and lint on all touched files - [x] `proxy.ts` shrank from 972 to 969 lines and now passes CLI type-checking without `@ts-nocheck`\n- [x] `npm run test-conditionals:scan -- --top 25`\n- [x] `npm run test-size:check` - [x] All commits are signed and the original contributor remains the first commit author --- Signed-off-by: Ho Lim <subhoya@gmail.com> Signed-off-by: cjagwani <cjagwani@nvidia.com> Signed-off-by: Carlos Villela <cvillela@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Enhanced Ollama model recognition to handle implied or differently formatted tags (e.g., `latest`) during selection and onboarding. * Added a post-pull discovery retry flow that waits for a newly pulled model to become available before proceeding. * **Bug Fixes** * Fixed cases where onboarding could continue before the model appeared in Ollama, leading to “model not found” and inconsistent recovery behavior. * **Tests** * Expanded coverage for pull completion, discovery retries, and deterministic onboarding scenarios. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Ho Lim <subhoya@gmail.com> Signed-off-by: cjagwani <cjagwani@nvidia.com> Signed-off-by: Carlos Villela <cvillela@nvidia.com> Co-authored-by: Ho Lim <subhoya@gmail.com> Co-authored-by: Carlos Villela <cvillela@nvidia.com>
Hadar301
pushed a commit
to Hadar301/NemoClaw-OpenShift
that referenced
this pull request
Jul 12, 2026
<!-- markdownlint-disable MD041 --> ## Summary Confirms that Ollama exposes a newly pulled model before onboarding accepts the route. This is a current-main replacement for NVIDIA#6075 that preserves Ho Lim's original authorship without carrying the conflicted test reshuffle. ## Related Issue Fixes NVIDIA#6038. ## Changes - Poll Ollama model discovery with bounded exponential backoff after a successful pull. - Return to model selection with a clear error when the daemon never lists the pulled model. - Treat omitted model tags as Ollama's canonical `:latest` form across pull authorization and post-pull discovery. - Keep model-reference, pull-readiness, and bounded discovery logic in a focused typed module.\n- Remove the legacy `@ts-nocheck` from `proxy.ts` while shrinking that hotspot below its main-branch line count. - Add deterministic wait success/exhaustion tests plus integrated delayed-registration and zero-exit/unlisted reprompt coverage. - Update existing successful-pull onboarding fixtures without growing the legacy test file. ## Type of Change - [x] Code change (bug fix) - [ ] Code change with doc updates - [ ] Doc only ## Quality Gates - [x] Tests added or updated for changed behavior - [x] Docs not applicable — this corrects existing onboarding behavior and error handling - [x] Sensitive paths changed — local Ollama onboarding and inference preparation. - [x] No secrets, API keys, or credentials committed ## Notes The registration-wait status and post-pull failure message make an existing onboarding contract explicit. The current Ollama docs already describe pull, load, and validation, so this reliability fix adds no new command, option, or workflow. ## Verification - [x] `npm run build:cli` - [x] `npm run typecheck:cli` - [x] `model-discovery.test.ts` + `proxy.test.ts` — 18/18 passed - [x] `test/onboard-selection.test.ts` — 64/64 passed - [x] Biome format and lint on all touched files - [x] `proxy.ts` shrank from 972 to 969 lines and now passes CLI type-checking without `@ts-nocheck`\n- [x] `npm run test-conditionals:scan -- --top 25`\n- [x] `npm run test-size:check` - [x] All commits are signed and the original contributor remains the first commit author --- Signed-off-by: Ho Lim <subhoya@gmail.com> Signed-off-by: cjagwani <cjagwani@nvidia.com> Signed-off-by: Carlos Villela <cvillela@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Enhanced Ollama model recognition to handle implied or differently formatted tags (e.g., `latest`) during selection and onboarding. * Added a post-pull discovery retry flow that waits for a newly pulled model to become available before proceeding. * **Bug Fixes** * Fixed cases where onboarding could continue before the model appeared in Ollama, leading to “model not found” and inconsistent recovery behavior. * **Tests** * Expanded coverage for pull completion, discovery retries, and deterministic onboarding scenarios. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Ho Lim <subhoya@gmail.com> Signed-off-by: cjagwani <cjagwani@nvidia.com> Signed-off-by: Carlos Villela <cvillela@nvidia.com> Co-authored-by: Ho Lim <subhoya@gmail.com> Co-authored-by: Carlos Villela <cvillela@nvidia.com>
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
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.
Summary
Makes Ollama onboarding confirm that a just-pulled model is visible in local discovery before continuing, so a zero-exit pull that has not registered yet does not silently produce a broken route.
The current-main salvage also treats an omitted model tag as
:latestand preserves the newer onboarding test structure.Related Issue
Closes #6038
Changes
:latestlisting.Type of Change
Quality Gates
Verification
Verifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run check:diffpassed when hooks were skipped or unavailablenpx vitest run --project cli src/lib/inference/ollama/proxy.test.tspassed 12/12;npx vitest run --project integration test/onboard-selection.test.ts -t "Ollama"passed 23/23 with 41 unrelated tests skipped;npm run typecheck:cli, Biome, test-size, and conditional scans passed.npm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result:npm run docsbuilds without warnings (doc changes only)Signed-off-by: Ho Lim subhoya@gmail.com
Summary by CodeRabbit
New Features
Bug Fixes