fix(onboard): reject malformed Ollama model entries

[apurvvkumaria]

Summary

The Windows-host Ollama probe introduced in #9482 accepted a models array even when its entries were null, primitive values, or nested arrays. The shared response validator now requires every model entry to be a non-null, non-array object while preserving valid empty model lists.

Related Issue

Refs #9348. Follow-up to #9482.

Changes

• Harden isValidOllamaTagsResponseBody so malformed model entries fail closed.
• Add public onboarding-boundary regression coverage for null, primitive, and array entries.

This change adds no abstraction, configuration, fallback, migration, or compatibility path. It tightens the response contract already used by the Windows-host reachability probe.

Type of Change

• Code change (feature, bug fix, or refactor)
• Code change with doc updates
• Doc only (prose changes, no code sample modifications)
• Doc only (includes code sample changes)

Quality Gates

• Tests added or updated for changed behavior
• Existing tests cover changed behavior — justification:
• Tests not applicable — justification:
• Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging)
• Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: security review passes because the change only tightens untrusted response parsing; it adds no credential, authentication, command, filesystem, permission, dependency, or network-destination behavior.
• Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue:

DGX Station Hardware Evidence

Not applicable. scripts/prepare-dgx-station-host.sh is unchanged.

Verification

• PR description includes a Signed-off-by: line and every commit appears as Verified in GitHub
• Normal pre-commit, commit-msg, and pre-push hooks passed, or npm run validate:pr passed after refreshing origin/main when hooks were skipped or unavailable
• Targeted behavior tests pass for the current change set, or tests are marked not applicable above — npx vitest run --project cli src/lib/onboard/provider-host-state.test.ts src/lib/inference/local.test.ts: 110 passed; npx vitest run --project integration test/growth-guardrails.test.ts: 32 passed; npm run typecheck:cli: passed.
• Applicable broad gate passed — the focused consumer and shared-validator suites cover this bounded two-file response-validation change.
• Quality Gates section completed with required justifications or waivers
• No secrets, API keys, or credentials committed
• npm run docs builds without warnings (doc changes only)
• Doc pages follow the style guide (doc changes only)
• New doc pages include SPDX header and frontmatter (new pages only)

---

Signed-off-by: Apurv Kumaria akumaria@nvidia.com

Summary by CodeRabbit

• Bug Fixes

  • Improved validation of local Ollama and vLLM responses.
  • Malformed responses—including HTML, null values, numbers, arrays, error objects, and invalid model entries—are now rejected.
  • Prevented incorrect detection of Ollama availability while preserving the local Ollama installation option.

• Tests

  • Expanded regression coverage for malformed responses from Windows-hosted Ollama installations, backend health checks, and authentication proxies.

[apurvvkumaria]

Documentation Writer Review: PASS

No documentation change is needed. This follow-up tightens validation of malformed /api/tags response entries without changing supported configuration, commands, diagnostics, or the behavior of valid Ollama responses.

No documentation blocker found.

[apurvvkumaria]

Security Review: PASS

I reviewed the complete two-file change.

• The change tightens parsing of an untrusted Ollama response. A models entry must now be a non-null, non-array object; malformed entries fail closed.
• The network destination, request construction, timeout, command execution, authentication, credential handling, filesystem behavior, permissions, and dependencies are unchanged.
• Empty model lists remain valid, so a healthy Ollama service without downloaded models is still detected correctly.
• The public onboarding-boundary regression test covers null, primitive, and array entries. The focused suites pass all 110 tests, the growth suite passes all 32 tests, and the CLI type-check passes.

No security blocker found.

[coderabbitai]

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: d5fc6d69-423a-4ab7-83ad-88bc0bd4ec7b

📥 Commits

Reviewing files that changed from the base of the PR and between f0967c6 and 338ffb0.

📒 Files selected for processing (1)

• src/lib/inference/local.test.ts

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

---

📝 Walkthrough

Walkthrough

The local response parser now uses isObjectRecord to reject malformed Ollama and vLLM response structures. Tests cover invalid response bodies for local health probes and Windows-host detection.

Changes

Ollama validation

Layer / File(s)	Summary
Validate local model responses src/lib/inference/local.ts, ci/source-architecture-budget.json	Ollama tag and model inventory parsing now requires object records for response roots and model entries. Architecture budgets account for the updated helper usage.
Cover malformed response bodies src/lib/inference/local.test.ts, src/lib/onboard/provider-host-state.test.ts	Parameterized tests cover HTML, error objects, null, numeric, array, and nested-array responses. Invalid responses remain unhealthy, Windows-host Ollama remains unreachable, and the local installation option remains selected.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: ⚪ Minimal · up to 338ff

The change rejects malformed Ollama model entries while preserving valid empty lists, with no actionable merge-blocking risk remaining after normal checks and review.

Possibly related PRs

• NVIDIA/NemoClaw#9483: Extends the same local response validation in src/lib/inference/local.ts.
• NVIDIA/NemoClaw#9482: Updates related Ollama response validation and Windows-host regression tests.

Suggested labels: provider: ollama, area: inference, area: local-models

Suggested reviewers: cv, laitingsheng, ericksoa

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main change: rejecting malformed Ollama model entries during onboarding.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
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.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch codex/fix-ollama-tags-model-entry-validation

---

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

[jyaunches]

LOC Reduction / Codebase Simplicity Review

Why changes are requested

The validator change belongs in the existing Ollama response boundary, but the commit under review adds avoidable duplicate code in both changed files.

• src/lib/onboard/provider-host-state.test.ts adds a 21-line table test that repeats the complete fixture and the same three assertions from the immediately preceding non-Ollama-body test. Only the captured response body differs.
• src/lib/inference/local.ts spells out the same non-null object and non-array check for the response and each model entry. src/lib/core/json-types.ts already provides isObjectRecord for this boundary check.

Refactor direction

• Convert the existing non-Ollama-body test into one it.each table. Include the HTML body, null entry, primitive entry, and nested-array entry. Keep one buildDeps block and one assertion block.
• Import isObjectRecord and use it for the parsed response and each models entry. Use the same helper for the adjacent modelInventory entry guard if that guard changes.
• Keep isValidOllamaTagsResponseBody as the shared Ollama health-shape owner. PR #9483 already has a separate request to reuse this validator with the existing inventory and matcher path, so this PR does not need a second inventory abstraction.

Expected result

The same malformed responses fail closed, valid empty model lists remain accepted, and the test adds roughly six lines instead of 21. The production change can be neutral or negative instead of adding two lines.

[github-actions]

PR Review Advisor — No blocking findings reported

Advisor assessment: No blocking advisor findings reported
Next action: No advisor follow-up needed.
Findings: 0 blockers · 0 warnings · 0 suggestions

Model lanes

• GPT-5.6 Terra (primary): Completed · medium confidence · 0 blockers · 0 warnings · 0 suggestions
• Nemotron 3 Ultra (second opinion): Completed · high confidence · 0 blockers · 0 warnings · 0 suggestions
• Model comparison: normalized findings match; normalized terminology decisions differ; normalized E2E selections differ; severity counts match.

3 additional E2E selections from the second opinion

Advisory only. The primary lane did not select these E2E jobs or targets.

• ollama-auth-proxy: The completed second-opinion lane identified E2E coverage that the primary lane omitted.
• onboard-repair: The completed second-opinion lane identified E2E coverage that the primary lane omitted.
• onboard-resume: The completed second-opinion lane identified E2E coverage that the primary lane omitted.

Second-opinion terminology and E2E selections are advisory. Live E2E does not run automatically for pull requests.

2 semantic terminology decisions

Terminology decisions are advisory. They affect the assessment only when a separate finding identifies concrete semantic impact.

• established — model entry at src/lib/inference/local.test.ts:787: Keep `model entry` for one models-array element.
• established — Ollama health at src/lib/inference/local.ts:361: Keep `Ollama health` for the probe classification.

E2E guidance

Advisory only. A maintainer can dispatch the default E2E suite for the commit under review.

Recommended E2E: inference-routing

Manual-only E2E: cloud-onboard, network-policy
The manual PR workflow does not run these selectors for the commit under review. Run them from reviewed code on main.

Workflow run details

This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge.

[github-code-quality]

Code Coverage Overview

Languages: TypeScript

TypeScript / code-coverage/plugin

The overall coverage in commit 338ffb0 in the codex/fix-ollama-tag... branch remains at 96%, unchanged from commit 5ab38cf in the main branch.

TypeScript / code-coverage/cli

The overall coverage in commit 338ffb0 in the codex/fix-ollama-tag... branch remains at 82%, unchanged from commit 7afe395 in the main branch.

Show a code coverage summary of the most impacted files.

File	main 7afe395	codex/fix-ollama-tag... 338ffb0	+/-
src/lib/onboard...nflict-guard.ts	93%	88%	-5%
src/lib/onboard...ce-lifecycle.ts	93%	89%	-4%
src/lib/onboard...hell-version.ts	87%	85%	-2%
src/lib/onboard...uild-context.ts	86%	86%	0%
src/lib/onboard...ider-profile.ts	100%	100%	0%
src/lib/onboard...-transaction.ts	94%	94%	0%
src/lib/onboard...eway-service.ts	83%	84%	+1%
src/lib/inference/local.ts	80%	81%	+1%
src/lib/actions...aged-profile.ts	84%	88%	+4%
src/lib/actions...er-lifecycle.ts	85%	94%	+9%

---

_{Updated August 19, 2026 04:37 UTC}

[apurvvkumaria]

Addressed the test-duplication portion of the change request in the current branch revision. One table now owns the HTML, null-entry, primitive-entry, and nested-array cases with one fixture and one assertion block.

The requested direct isObjectRecord import is blocked by the enforced source architecture budget. That import makes src/lib/core/json-types.ts fan-in 37 (limit 36) and src/lib/inference/local.ts fan-out 22 (limit 21). I reverted the import and did not raise either limit; the repository architecture gate passes again.

Validation:

• affected provider-host suite: 19 passed
• growth guardrails: 32 passed
• repository checks, including source architecture: passed
• normal commit and push hooks: passed
• GitHub commit verification: valid

Maintainer decision needed for the remaining request: accept the existing local boundary check in this PR, or approve a separately scoped dependency refactor that provides a sanctioned shared path.

Partially addressed by 1958ade: the duplicate malformed-body test setup was consolidated, but the canonical production record predicate remains unresolved. Superseded by the refreshed review at the current head.

[jyaunches]

LOC Reduction / Codebase Simplicity Review

What this update resolved

The duplicate test setup is resolved at 1958adea0cf36174a13f111d3d247d7f27a603ed. The HTML response and the three malformed model-entry bodies now share one it.each table, one dependency fixture, and one assertion block. The exact response removes 18 net test lines.

Why changes are still requested

The production half of the prior finding is unchanged. src/lib/inference/local.ts:365-372 still implements the same object-record predicate twice: once for the parsed response and once for each models entry. The repository already owns that boundary rule as isObjectRecord in src/lib/core/json-types.ts.

This leaves the production diff at +10/-8 even though the validation can reuse the canonical predicate directly. Keeping a second spelling makes null/array handling easier to drift from the adjacent JSON boundaries and adds lines without a new contract.

Refactor direction

Import isObjectRecord from ../core/json-types and express the validator with the existing owner:

return (
  isObjectRecord(parsed) &&
  Array.isArray(parsed.models) &&
  parsed.models.every(isObjectRecord)
);

Keep isValidOllamaTagsResponseBody as the shared Ollama health-shape boundary. No new inventory helper or abstraction is needed.

Expected result

The same malformed entries fail closed and valid empty model lists remain accepted, while the production response becomes neutral or negative and the repository has one definition of an object record.

[prekshivyas]

Addressed the remaining requested change and the current CI failure through f0967c694.

Requested canonical boundary (a3e1cce19):

• imported isObjectRecord directly from core/json-types
• expressed /api/tags validation as the requested object-record + models-array + every-entry-object contract
• reused the same owner for the adjacent modelInventory response and entry guards
• recorded the real dependency increase explicitly in the architecture ratchet (json-types fan-in 37, local.ts fan-out 22); no re-export or hidden dependency edge
• production follow-up is net -6 lines

CI repair (f0967c694):

• merged current main because failed shard 10 was the stale-base Dockerfile fixture failure (Expected a serializable SandboxMessagingPlan), independent of the Ollama validator

Validation on the exact pushed head:

• Ollama/provider-host plus Dockerfile suites: 3 files / 147 tests passed
• npm run checks:repository passed, including source architecture
• npm run typecheck passed
• npm run validate:pr passed
• both new commits are signed, DCO-signed-off, and GitHub Verified

Fresh exact-head checks are starting. The changes-requested review remains intact for reviewer re-evaluation.

Resolved at f0967c6. The validator now imports and reuses the canonical isObjectRecord predicate. The consolidated malformed-response table remains in place.

[jyaunches]

LOC Reduction / Codebase Simplicity Review

What this update resolved

Commit f0967c694792f9de0a8b865949df6416ef12aa9e resolves the remaining simplicity request.

src/lib/inference/local.ts now imports isObjectRecord from core/json-types. The Ollama validator uses it for the parsed response and every models entry. The adjacent modelInventory path also reuses the same predicate.

The consolidated it.each test remains unchanged. One fixture and one assertion block cover the HTML response and all malformed model-entry cases.

The production file is now net negative relative to the PR base. The architecture-budget changes record the direct dependency without adding a wrapper or hidden re-export.

Scope of this resolution

This comment resolves only the LOC-reduction and codebase-simplicity blocker. It is not an approval and does not assess other review requirements.

[apurvvkumaria]

Documentation Writer Review: PASS

No documentation change is required. The PR tightens validation of malformed Ollama responses and reuses an existing internal JSON boundary. It adds no user command, configuration, supported integration, or operator action.

Security Review: PASS

No findings. The parser now requires the response and each model entry to be plain object records, rejects malformed JSON and invalid entries, and keeps the existing empty-array behavior. It adds no credential handling, authorization path, dependency, remote execution, cryptography, security-header change, or new system privilege. Error handling remains fail closed. The changed 19-test suite, the full 79-test installer file that failed in CI, and the source architecture check pass on the current branch revision.

[udsy19]

Outside contributor here, not a maintainer — offering evidence rather than a decision. I read this diff against main and re-ran the predicate by hand; everything below is what I could check myself.

What holds up

I executed the pre-change and post-change versions of isValidOllamaTagsResponseBody side by side against each body in the new table at src/lib/onboard/provider-host-state.test.ts:264-269, plus two controls:

body	before	after
<html>captive portal</html>	rejected	rejected
{"models":[null]}	accepted	rejected
{"models":[1]}	accepted	rejected
{"models":[[]]}	accepted	rejected
{"models":[]}	accepted	accepted
{"models":[{"name":"llama3:latest"}]}	accepted	accepted

Three of the four table rows genuinely flip, so the added cases cannot pass without the production change, and the empty-model-list case stays valid. The HTML row was already rejected before — that is the case the table grew out of, so no surprise there.

I also re-derived the import graph that ci/source-architecture-budget.json measures, because the diff edits a budget entry for src/lib/core/json-types.ts, a file the PR does not otherwise touch. On the base commit that file's fan-in is 36 and src/lib/inference/local.ts's fan-out is 21 — both equal to the checked-in values — and the diff adds exactly one runtime import edge between them. scripts/checks/source-architecture.mts raises a metric-ratchet violation when a maxByFile entry sits above the measured value, so both +1 edits look required rather than incidental.

To be clear about the limits: I did not run the repository test suite, type-check, or build. The table above is a hand-executed comparison of the two versions of the predicate only.

One thing worth catching: this overlaps #9483

#9483 rewrites the same block of src/lib/inference/local.ts. A local test-merge of the two heads with git merge-tree conflicts in that file, so whichever lands first, the other needs a rebase over the same lines rather than a clean merge.

The two also draw the boundary in different places. Executed against both heads:

{"models":[{}]}            -> valid here, inconclusive under #9483
{"models":[{"name":""}]}   -> valid here, inconclusive under #9483

That is a real judgement call about how much of an /api/tags body has to be well-formed before the daemon counts as live, and it seems worth settling deliberately rather than by merge order, because this validator gates three call sites, not one:

• src/lib/inference/local.ts:829 — the Ollama auth-proxy subprobe
• src/lib/inference/local.ts:955 — the local backend probe
• src/lib/onboard/provider-host-state.ts:203 — the Windows-host reachability probe

The summary describes the Windows-host probe, which is the case the new tests cover; the other two consumers are easy to miss when reading the change.

Smaller note

After this change the file carries two different answers for a partly-malformed list. isValidOllamaTagsResponseBody at src/lib/inference/local.ts:363 is all-or-nothing (models.every(isObjectRecord)), while modelInventory at :373 still skips bad entries one at a time. For ollama-local the skip path is now unreachable — :955 rejects the body first — though it still matters for the data branch used by vLLM. Not a defect, just an asymmetry a later reader could trip over.

Thanks for the change either way — the tightening itself reads correctly to me, and the consolidated table is easy to follow.

[prekshivyas]

Addressed the remaining advisor coverage warning in signed, GitHub-Verified commit 338ffb0a1:

• converted the direct Ollama backend invalid-body test into a four-case table covering HTML, null, primitive, and nested-array model entries
• applied the same four-case contract to the auth-proxy path
• retained exact unhealthy labels and /api/tags diagnostics for both paths

Validation on the pushed commit:

• src/lib/inference/local.test.ts: 97/97 passed
• repository checks and source architecture passed
• TypeScript passed
• full validate:pr hooks passed, including source-shape and growth guardrails

Fresh exact-head CI is now running.

[prekshivyas]

Approved exact head 338ffb0. The shared Ollama tags boundary rejects null, primitive, array, and malformed object entries while preserving valid empty inventories; the direct backend and auth-proxy probes now cover the same malformed shapes. All six commits are verified, review threads are resolved, local focused/repository/type/full validation passed, and all current CI, advisory, CodeRabbit, security, and managed-runtime checks are green.