Skip to content

refactor(inference): remove duplicate Ollama probe coverage - #10848

Merged
ericksoa merged 1 commit into
mainfrom
fix/ollama-review-followup-10741
Sep 2, 2026
Merged

refactor(inference): remove duplicate Ollama probe coverage#10848
ericksoa merged 1 commit into
mainfrom
fix/ollama-review-followup-10741

Conversation

@ericksoa

@ericksoa ericksoa commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Outcome

Windows-host Ollama curl probes now have one transport-translation owner, and the E2E support suite no longer duplicates the focused inference transport test.

Reason

A post-merge review of #10741 identified one redundant pre-translation before the shared capture wrapper and one duplicate helper-output test. Both added maintenance surface without distinct behavior coverage.

Changes

  • Pass the validated raw curl command to createOllamaApiCaptureEx, which owns Docker Desktop translation and credential isolation.
  • Remove the duplicate E2E support assertion; retain the focused Windows transport boundary suite.

Verification

  • Windows-host transport suite: 20 tests passed.
  • Local inference suite: 97 tests passed.
  • E2E fixture client suite: 75 tests passed.
  • npm run typecheck:cli passed.
  • Repository hooks passed formatting, lint, repository checks, secret scan, source-shape budget, and growth guardrails.
  • The public diff and PR text contain no private bug identifiers or non-public third-party names.

Review notes

This is a narrow follow-up to #10741. It does not change the supported host allowlist, Docker credential isolation, cleanup behavior, or public command surface.


Signed-off-by: Aaron Erickson aerickson@nvidia.com

Summary by CodeRabbit

  • Bug Fixes
    • Improved local Ollama connectivity probing across Docker host environments.
    • Preserved existing probe behavior and result handling while improving command execution compatibility.

@coderabbitai

coderabbitai Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: abe0d3cb-87e7-4e2d-8f96-5867d500481f

📥 Commits

Reviewing files that changed from the base of the PR and between 9ad536b and f628a77.

📒 Files selected for processing (2)
  • src/lib/inference/local.ts
  • test/e2e/support/e2e-clients.test.ts
💤 Files with no reviewable changes (1)
  • test/e2e/support/e2e-clients.test.ts

Included review availability: Your plan provides up to 12 included reviews per hour; 9 remain after this review.


📝 Walkthrough

Walkthrough

The local Ollama probe now passes a direct curl command to createOllamaApiCaptureEx, which handles host-specific Docker translation. Unused E2E imports and the related Windows-host image test are removed.

Changes

Ollama probe transport

Layer / File(s) Summary
Direct curl probe and test cleanup
src/lib/inference/local.ts, test/e2e/support/e2e-clients.test.ts
The probe delegates host translation to createOllamaApiCaptureEx. The E2E test file removes unused imports and the digest-pinned curl image assertion.

Estimated code review effort: 1 (Trivial) | ~2 minutes

Merge Risk: ⚪ Minimal · up to f628a

This localized refactor centralizes Windows-host transport translation while preserving validation, credential isolation, and cleanup behavior. No actionable merge-blocking risk remains after normal checks and review.

Suggested reviewers: dongni-yang, laitingsheng, sandl99

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 1 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly describes the primary change: removing duplicate Ollama probe coverage through a focused refactor.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/ollama-review-followup-10741

Comment @coderabbitai help to get the list of available commands.

@github-code-quality

github-code-quality Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Code Coverage Overview

Languages: TypeScript

TypeScript / code-coverage/plugin

The overall line coverage in commit f628a77 in the fix/ollama-review-fo... branch remains at 96%, unchanged from commit 97c8816 in the main branch.

TypeScript / code-coverage/cli

The overall line coverage in commit f628a77 in the fix/ollama-review-fo... branch remains at 84%, unchanged from commit 95c0a60 in the main branch.

Show a line coverage summary of the most impacted files.
File main 95c0a60 fix/ollama-review-fo... f628a77 +/-
src/lib/inferen...a/model-size.ts 100% 83% -17%
src/lib/inferen...odel-prompts.ts 98% 91% -7%
src/lib/inferen...ollama/proxy.ts 44% 42% -2%
src/lib/inference/nim.ts 76% 74% -2%
src/lib/onboard...al-inference.ts 90% 90% 0%
src/lib/policy/index.ts 71% 72% +1%
src/lib/inference/local.ts 81% 85% +4%
src/lib/inferen...er-lifecycle.ts 75% 79% +4%
src/lib/inferen...er-lifecycle.ts 69% 77% +8%
src/lib/inferen...-materialize.ts 77% 90% +13%

Updated September 02, 2026 03:50 UTC

@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

PR Review Advisor finished for commit f628a77. Include the Advisor findings in the complete PR feedback collection. Verify and group valid findings before repair.

All previous runs

@ericksoa
ericksoa merged commit 0fda7f4 into main Sep 2, 2026
108 of 120 checks passed
@ericksoa
ericksoa deleted the fix/ollama-review-followup-10741 branch September 2, 2026 03:52
@wscurran wscurran added the refactor PR restructures code without intended behavior change label Sep 9, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

refactor PR restructures code without intended behavior change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants