fix(eos_ai): normalize LLM URLs, improve error resilience, and add unit test suite

[sapandeep31]

Summary

This PR addresses critical URL path handling bugs, error response edge cases, scheme normalization, and provider configuration gaps in the embedded AI subsystem's LLMClient (ebuild/eos_ai/llm_integration.py), and introduces a dedicated unit test suite (tests/unit/test_eos_ai_llm.py) with 34 comprehensive tests (~96% branch/statement coverage).

Prior to this PR, LLMClient had zero unit tests in the repository, and several failure modes impacted developers integrating local/cloud LLMs into hardware analysis pipelines.

Standards & Research Alignment

Our implementation was designed and cross-verified against official ecosystem documentation and standards:

1. OpenAI API & SDK Conventions (OpenAI API Docs, openai-python):
   • The canonical base URL route is https://api.openai.com/v1, and the chat completions resource is /chat/completions.
   • In existing tools (e.g. LangChain, LiteLLM, vLLM), developers configure base URLs interchangeably with or without /v1 (e.g., http://localhost:8000/v1 vs https://api.openai.com) and with or without trailing slashes. Naive string concatenation previously resulted in duplicate path segments (/v1/v1/chat/completions) or double slashes (//).
   • Added support for standard OPENAI_BASE_URL and OPENAI_MODEL environment variables.
2. Fail-Safe Completion Parsing:
   • When an upstream API returns empty choices ({"choices": []}) or empty completion text, LLMResponse(success=False, error="Upstream returned no completion choices") is returned. This guarantees fail-safe behavior and prevents downstream consumers (EosHardwareAnalyzer.analyze_with_llm) from falsely inflating hardware profile confidence or stamping unverified llm_analyzed metadata.
3. Ollama API Specifications (Ollama REST API Docs):
   • Model presence is verified via GET /api/tags, and synchronous completions require POST /api/generate with "stream": false.
   • Added support for standard OLLAMA_HOST and OLLAMA_MODEL environment variables.
   • Scheme normalization via _ensure_scheme automatically prepends http:// to schemeless host strings (e.g., 192.168.1.50:11434), supporting both ambient env vars and explicit base_url arguments.
4. Headless & Local Inference Runners (vLLM, llama.cpp, LocalAI):
   • Self-hosted model endpoints typically operate unauthenticated. Requiring a mandatory api_key for custom providers prevented developers from using local servers. The Authorization: Bearer header is now strictly omitted when api_key is empty or absent.
5. Resilient Response Parsing & Exception Narrowing:
   • Structured urllib.error.HTTPError decoding: extracts nested error.message from JSON error bodies across HTTP 401, 429, and 500 status codes with clean fallback to raw status text.
   • Narrowed decoding exception handler to (OSError, UnicodeDecodeError, AttributeError).

Changes

1. URL Normalization & Scheme Handling:
   • _normalize_openai_url: Safely strips trailing slashes and resolves endpoints whether the user passes a root domain (https://api.openai.com), a versioned path (http://localhost:8000/v1/), or a full resource URL (http://localhost:8000/v1/chat/completions).
   • _normalize_ollama_url: Normalizes Ollama base URLs to /api/generate.
   • _ensure_scheme: Guarantees scheme presence (http://) on Ollama URLs, whether provided via OLLAMA_HOST or explicit base_url.
   • _check_ollama: Resolves target /api/tags on the configured base_url (or OLLAMA_HOST) instead of hardcoding localhost.
2. Fail-Safe Completions & Error Extraction:
   • Empty choices / empty text return LLMResponse(success=False, error="Upstream returned no completion choices").
   • Extracted reusable _error_message(payload) helper used across both HTTP error decoding and OpenAI error-payload inspection.
   • Guarded HTTPError body reading with (OSError, UnicodeDecodeError, AttributeError).
3. Documentation:
   • Updated docs/ai-input-formats.md with current auto-detection priority order and a reference table for all 8 environment variables (OLLAMA_HOST, OLLAMA_MODEL, OPENAI_API_KEY, OPENAI_BASE_URL, OPENAI_MODEL, EOS_LLM_URL, EOS_LLM_API_KEY, EOS_LLM_MODEL).
4. Comprehensive Unit Test Suite (tests/unit/test_eos_ai_llm.py):
   • 34 unit tests covering initialization, URL normalization edge cases, scheme prefixing, provider auto-detection priority, is_available() matrix, Ollama payload construction, OpenAI chat completions parsing, error decoding, standard environment variables, and EosHardwareAnalyzer.analyze_with_llm enrichment and regression checks.
   • Added @pytest.fixture(autouse=True) with monkeypatch.delenv(..., raising=False) ensuring clean test isolation from ambient developer environment variables.
   • Negative testing ("the one check that matters") verified.

Test Plan

# Run new unit test suite with coverage
.venv/bin/pytest --cov=ebuild.eos_ai.llm_integration tests/unit/test_eos_ai_llm.py -v
# 34 passed in 0.14s (95.98% coverage on llm_integration.py)

# Run existing AI tests
.venv/bin/pytest tests/ebuild/test_eos_ai.py -v
# 24 passed in 0.03s

# Run importability tests
.venv/bin/pytest tests/unit/test_sources_are_importable.py -v
# 61 passed in 2.96s

# Lint and style check with project gate tool (ruff)
.venv/bin/ruff check ebuild/eos_ai/llm_integration.py tests/unit/test_eos_ai_llm.py
# All checks passed!

Negative testing ("the one check that matters"):

• Bypassed empty choices guard (if False and (not choices or not content):); observed test_empty_choices_handled_safely_without_index_error fail with assert True is False and test_analyzer_unchanged_when_llm_returns_empty_choices fail with assert 0.9 == 0.8 (demonstrating false confidence inflation caught).
• Restored code and confirmed all 34 tests pass cleanly.

[srpatcha]

Review — ebuild#120 "fix(eos_ai): normalize LLM URLs, improve error resilience, and add unit test suite"

head: b7b7b15 author: sapandeep31 ci: none reported

Verdict: Genuinely good work — the URL normalisation, the optional Authorization header and 546 lines of first-ever tests for LLMClient all land correctly — but one of the "resilience" changes converts a path that used to fail safe into one that silently reports success and inflates a hardware profile's confidence, and the new test suite pins that behaviour rather than catching it.

Findings

#	Severity	File:line	Finding	Recommended fix
1	High	ebuild/eos_ai/llm_integration.py (_call_openai_compat, empty-choices guard) + tests/unit/test_eos_ai_llm.py::test_empty_choices_handled_safely_without_index_error	A response of {"choices": []} now yields LLMResponse(text="", success=True). Before this diff the same body raised IndexError on body.get("choices", [{}])[0], which analyze()'s except Exception turned into success=False — the profile was left alone. Now it passes the if not response.success: return profile gate at ebuild/eos_ai/eos_hw_analyzer.py:647, so analyze_with_llm() runs its keyword scan over an empty string, finds nothing, and still appends llm_analyzed:<provider> (:661) and does profile.confidence = min(profile.confidence + 0.1, 1.0) (:662). An upstream API that answered nothing raises the reported confidence of a hardware profile by 0.1 and stamps it as LLM-analysed. That is a fail-safe path turned fail-silent, in the very function the PR describes as hardening, and the new test asserts resp.success is True so a future reader will take it as intended.	Treat "no usable content" as a failure, not an empty success: after computing content, if not choices or not content: return LLMResponse(text="", model=self.model, provider=self.provider, success=False, error="Upstream returned no completion choices"). Then change the test to assert resp.success is False and add one asserting analyze_with_llm() leaves confidence at its input value. The IndexError guard is still worth keeping — the point is what it reports, not that it no longer crashes.
2	Medium	tests/unit/test_eos_ai_llm.py — test_default_init, test_auto_detect_prefers_ollama_if_online, test_auto_detect_falls_back_to_openai_if_ollama_offline, test_is_available_logic	The suite reads the ambient environment for four variables this PR itself introduces. test_default_init asserts base_url == "http://localhost:11434" while __init__ now consults OLLAMA_HOST. test_auto_detect_prefers_ollama_if_online asserts model == "llama3" while auto() now consults OLLAMA_MODEL; the OpenAI equivalent asserts "gpt-4o-mini" against OPENAI_MODEL; test_is_available_logic's LLMClient(provider="openai", ...) depends on OPENAI_BASE_URL being unset. Any developer who has OLLAMA_HOST exported — exactly the users this PR is written for — gets red tests on an unmodified checkout. test_auto_detect_falls_back_to_custom_url already does monkeypatch.delenv("OPENAI_API_KEY", raising=False), so the technique is known; it is just applied to one variable out of six.	Add a module-scoped autouse fixture: @pytest.fixture(autouse=True) that monkeypatch.delenvs OLLAMA_HOST, OLLAMA_MODEL, OPENAI_BASE_URL, OPENAI_MODEL, OPENAI_API_KEY, EOS_LLM_URL, EOS_LLM_API_KEY, EOS_LLM_MODEL with raising=False. Tests that want a variable then set it explicitly, as test_ollama_respects_ollama_host_env_var already does.
3	Medium	ebuild/eos_ai/llm_integration.py (__init__, ollama branch) and (auto(), ollama branch)	Scheme normalisation is applied to OLLAMA_HOST but not to an explicitly passed base_url: default_url gets the http:// prefix, then self.base_url = (base_url or default_url).rstrip("/") discards default_url entirely when a caller supplies one. So LLMClient(provider="ollama", base_url="192.168.1.50:11434") — the same string that works as OLLAMA_HOST, per test_ollama_respects_ollama_host_env_var — builds 192.168.1.50:11434/api/generate and urllib.request raises ValueError: unknown url type, which surfaces through the generic except Exception as an opaque message rather than "missing scheme". The identical five-line prefix block is also written twice, in __init__ and in auto() — duplication the diff introduces (brief §10), and the reason the two paths could drift apart in the first place.	Extract it once and apply it after resolution, not before: @staticmethod def _ensure_scheme(url: str) -> str: return url if not url or url.startswith(("http://", "https://")) else f"http://{url}", then self.base_url = self._ensure_scheme(base_url or default_url).rstrip("/") and the same call in auto(). Add a test for the explicit-base_url-without-scheme case.
4	Medium	docs/ai-input-formats.md:103-107	Behaviour changed and the document describing it is now wrong. Line 104 says Ollama "checks http://localhost:11434" — it now checks OLLAMA_HOST first. Line 106 says Custom "uses EOS_LLM_API_KEY + EOS_LLM_URL + EOS_LLM_MODEL" — EOS_LLM_API_KEY is no longer required, which is one of the PR's headline changes. OPENAI_BASE_URL, OLLAMA_MODEL and OPENAI_MODEL are new and undocumented anywhere. Per brief §11 and the project's own rule, a change that makes existing documentation wrong is not finished.	Update the auto-detection list at docs/ai-input-formats.md:103-107 to the new precedence, and add a short table of the eight environment variables the client now reads.
5	Medium	PR CI	No checks ran. gh pr checks 120 reports none on fix/eos-ai-llm-resilience-and-tests, the bundle's checks.txt is empty, and the PR is BLOCKED. Everything in the Test Plan is the author's local run; nothing is independently reproduced. The Test Plan is unusually well-specified for this org — commands, counts, and a stated negative-control experiment — so this is about CI not having executed, not about the claims being hollow.	Re-trigger the workflow. .github/workflows/ci.yml runs ruff check . and the pytest suite, which covers items 1-4 of the Test Plan.
6	Low	ebuild/eos_ai/llm_integration.py (HTTPError handler, except Exception: pass)	Bare except Exception: pass around the error-body read. .ai/reviewer.md lists a swallowed exception as a finding. It is bounded — the code falls through to f"HTTP {e.code}: {e.reason}" — but it will also silently absorb a bug in the parsing block above it, including the str(inner) calls.	Narrow it to except (OSError, UnicodeDecodeError, AttributeError): pass, which is the set that can actually arise from e.read().decode() on a closed or non-text body.
7	Low	ebuild/eos_ai/llm_integration.py (_call_openai_compat)	Two small redundancies in the new code: the isinstance(body, dict) and in the error-payload check is dead — the guard three lines above already returned for non-dict bodies; and the expression inner.get("message", str(inner)) if isinstance(inner, dict) else str(inner) is written twice, once here and once in the HTTPError handler.	Drop the redundant isinstance, and lift the message extraction into a @staticmethod _error_message(payload) -> str used by both sites.
8	Low	PR body, "Test Plan"	The lint step ran .venv/bin/flake8 --ignore=E501,E731,E741,F403,F405,F541,F841 .... This repo's gate is ruff check . (.github/workflows/ci.yml:58), configured in pyproject.toml:34-51; flake8 appears only in the weekly job with different arguments (.github/workflows/weekly.yml:31). Re-typing the ignore list onto a different tool proves that tool's opinion, not the gate's. I ran the right one: ruff check with the project's select/ignore over both changed files passes with no diagnostics, so there is no actual lint defect here — the claim is just not evidence for the check that will run.	Quote ruff check . in the Test Plan instead.

Checked and clear: exception ordering in analyze() is correct (HTTPError before URLError, of which it is a subclass, and a bare TimeoutError after both). asdict-style serialisation is not involved. The pytest.mark.ebuild marker is registered in pytest.ini:16, so --strict-markers (pytest.ini:28) will not reject the new file. The optional-Authorization change is right and test_custom_endpoint_omits_bearer_header_when_api_key_empty asserts the header's absence rather than its emptiness, which is the stronger check.

Architecture conformance

Conforms, with one boundary question worth recording rather than blocking on.

Master design §9.2 sets the SDK rule that matters here — "No mandatory cloud connection." This code satisfies it: EosHardwareAnalyzer.analyze_with_llm() returns the profile unchanged when is_available() is false (eos_hw_analyzer.py:641-642), the rule engine works with provider="none", and §19's "Cloud services must remain optional" is respected. Widening is_available() so a custom provider needs only a base_url (no API key) moves toward §9.2, not away — it is what lets a self-hosted vLLM or llama.cpp endpoint work without an account. §21 tier placement is unchanged: this is Tier 1 ebuild, and nothing here imports from a higher tier — the LLM is reached over HTTP, not by depending on the Tier 3 eAI repository, so §5.1's dependency direction holds. eBuild remains a developer-time tool and not a runtime dependency (§5.1), since none of this is compiled into firmware.

The boundary question: ebuild/eos_ai/ is a third AI-named surface in the org alongside the eAI repository (§21 Tier 3) and eosllm, while Appendix C directs the foundation to "consolidate overlapping AI names under eAI" and §16.1 to "expose eAI Vision, eAI Audio, eAI LLM and eAI Tiny as subproducts rather than unrelated top-level brands". The master design describes eAI as an on-device inference platform and says nothing at all about LLM-assisted developer tooling that runs on the workstation and calls a third-party API — which is what this module is. Under §21.1 it does not warrant its own repository (one consumer, no independent release lifecycle), so keeping it inside ebuild is the right call today; the naming is what collides. Recorded as a proposal in .ai/autoreview/proposals/2026-09.md rather than held against this PR.

Proposed changes

Smallest sequence, in order:

1. Make empty choices a failure (finding 1) and flip the two assertions in test_empty_choices_handled_safely_without_index_error. Add the analyze_with_llm() confidence-unchanged test — this is the one that would have caught it.
2. Add the autouse environment-clearing fixture (finding 2). Do this before 3, so the new test in 3 is not itself environment-dependent.
3. Extract _ensure_scheme(), call it in both __init__ and auto(), and add the explicit-base_url-without-scheme test (finding 3).
4. Update docs/ai-input-formats.md:103-107 and add the environment-variable table (finding 4).
5. Sweep findings 6 and 7 — three lines total.
6. Re-run ruff check . and python3 -m pytest tests/ -q, and replace the Test Plan's flake8 line with the ruff invocation.

Not checked

• Nothing in this repository was executed by this review beyond ruff. ebuild has a dirty working tree (4 files) and this pipeline leaves such repos untouched, so pytest tests/ was NOT RUN. The ruff check cited in finding 8 was run against copies of the two changed files extracted from head b7b7b15 into a temporary directory, with this repo's select/ignore passed on the command line — that is not the same as ruff check . over the whole tree.
• The "32 passed, 96.60% coverage" claim is unverified. No CI ran and I did not run pytest. The commands are plausible and specific, but the numbers are the author's, not observed here.
• The stated negative-control experiment is unverified — "intentionally altered _normalize_openai_url … observed 3 tests failing" is exactly the right thing to have done and exactly the kind of claim that leaves no artifact. Taken at face value, not confirmed.
• Finding 1's failure sequence is traced through the code (llm_integration.py → eos_hw_analyzer.py:641-662), not observed in a run.
• No live endpoint of any kind was contacted: the Ollama /api/tags probe, real OpenAI 401/429 bodies, and vLLM's actual response shape are all untested here and mocked in the suite.
• Whether any other caller of LLMClient outside eos_hw_analyzer.py depends on the old success=True-on-empty behaviour. ebuild/ was searched for .analyze(; other repositories were not.
• Thread-safety and concurrent use of LLMClient, and the behaviour of the timeout parameter against a slow-but-responding server. Neither is exercised.

---

Automated architecture review of b7b7b1529772 — scheduled, model claude-opus-5, checked against the EmbeddedOS Master Design v2.0. Advisory only: this reviewer never approves, requests changes, or merges. Reply here to discuss or push back — a wrong finding is a bug worth reporting.

[sapandeep31]

Thank you for the thorough and constructive architectural review! All 8 findings have been addressed in commit 047c111:

1. Fail-Safe on Empty Choices (Finding 1):

   • In _call_openai_compat: When not choices or not content, LLMClient now returns LLMResponse(text="", model=self.model, provider=self.provider, success=False, error="Upstream returned no completion choices").
   • Updated assertions in test_empty_choices_handled_safely_without_index_error to verify resp.success is False and resp.error == "Upstream returned no completion choices".
   • Added test_analyzer_unchanged_when_llm_returns_empty_choices in TestHardwareAnalyzerIntegrationWithLLM verifying that EosHardwareAnalyzer.analyze_with_llm leaves profile.confidence completely unchanged (0.8) and does not stamp llm_analyzed when upstream returns empty choices.
   • Performed negative control verification: temporarily bypassing this check resulted in AssertionError: assert 0.9 == 0.8 with features=['llm_analyzed:openai'], proving the regression was caught and eliminated.

2. Ambient Environment Isolation (Finding 2):

   • Added @pytest.fixture(autouse=True) in tests/unit/test_eos_ai_llm.py that strips all 8 ambient LLM variables (OLLAMA_HOST, OLLAMA_MODEL, OPENAI_BASE_URL, OPENAI_MODEL, OPENAI_API_KEY, EOS_LLM_URL, EOS_LLM_API_KEY, EOS_LLM_MODEL) using monkeypatch.delenv(..., raising=False). Tests now run consistently on clean or customized checkouts alike.

3. Scheme Normalization (Finding 3):

   • Extracted @staticmethod def _ensure_scheme(url: str) -> str to prepend http:// to schemeless URLs.
   • Applied _ensure_scheme() to the resolved URL in __init__ for provider="ollama" (so explicit base_url="192.168.1.50:11434" works seamlessly) and in auto().
   • Added unit test test_init_normalizes_scheme_for_explicit_base_url_without_scheme.

4. Documentation (Finding 4):

   • Updated docs/ai-input-formats.md with the updated auto-detection priority order.
   • Added a Markdown reference table documenting all 8 environment variables, their defaults, providers, and descriptions.

5. CI Execution (Finding 5):

   • Pushed commit 047c111 to the PR branch. Upstream GitHub Actions workflows (CI — ebuild, CodeQL, Simulation Test) are now queued in action_required status awaiting maintainer runner approval for outside fork contributors.

6. Narrow Exception Handling (Finding 6):

   • Replaced bare except Exception: pass in the HTTPError body reader with except (OSError, UnicodeDecodeError, AttributeError): pass.

7. Code Redundancies & Deduplication (Finding 7):

   • Extracted @staticmethod def _error_message(payload: Any) -> str and reused it across both HTTPError response parsing and _call_openai_compat JSON error payloads.
   • Removed the redundant isinstance(body, dict) check in _call_openai_compat.

8. Test Plan Tooling (Finding 8):

   • Updated the PR description Test Plan to reference .venv/bin/ruff check ebuild/eos_ai/llm_integration.py tests/unit/test_eos_ai_llm.py (matching the repo CI linter gate).
   • Test suite now has 34 passing tests with 95.98% statement/branch coverage.