[iamsayanmandal]

Fixes #184

Summary

Three related fixes that make the AI/LLM subsystem production-ready and restore a green CI gate on master.

1. LLMClient URL bugs — _call_openai_compat appended /v1/chat/completions unconditionally, causing /v1/v1/chat/completions when OPENAI_BASE_URL already contains /v1. Bare Ollama hosts without http:// crashed urllib. Empty api_key sent a stray Authorization: Bearer header that local servers (vLLM, llama.cpp) reject. Empty choices list returned success=True silently.

2. LLMClient has zero unit tests — added the first-ever test suite (35 tests, fully offline with mocks).

3. CI lint gate broken — ruff check . failed on 4 findings before running any test. Also added missing PackageRecipe.to_dict() which caused ebuild update-index to crash.

Type of Change

• feat — New feature
• fix — Bug fix
• docs — Documentation only
• style — Formatting, no code change
• refactor — Code restructuring without behavior change
• test — Add or fix tests
• build — Build system or dependency changes
• ci — CI/CD pipeline changes
• perf — Performance improvement

Changes

• Add _normalize_openai_url(): strips trailing slash, detects existing /v1, appends correct suffix — prevents /v1/v1/chat/completions double-path
• Add _ensure_scheme(): prepends http:// to bare Ollama hosts like 192.168.1.50:11434
• Add _build_headers(): omits Authorization header when api_key is empty
• Add _parse_openai_response(): returns success=False with clear message when choices is empty
• Honour OLLAMA_HOST, OLLAMA_MODEL, OPENAI_BASE_URL, OPENAI_MODEL, EOS_LLM_* env vars
• Add tests/unit/test_llm_client.py — 35 tests across 9 test classes, zero network calls
• Add PackageRecipe.to_dict() using external YAML key names (package, build) for round-trip correctness
• Fix F811 duplicate import shutil in test_build_dir_resolution.py
• Fix E402 mid-file imports in test_ci_gate.py
• Fix W292 missing newline in test_package_recipe.py
• Fix wrong # type: ignore[attr-defined] → [arg-type] in plugins/__init__.py

Testing

• Unit tests pass (ctest --test-dir build --output-on-failure)
• Integration tests pass
• Manual testing performed
• New tests added for new functionality

ruff check .      →  All checks passed! ✅
pytest tests/ -q  →  715 passed, 1 skipped, 0 failed ✅
                     (master was: 669 passed, 10 failed)

Pre-Submission Checklist

• Code compiles without warnings (-Wall -Wextra -Werror for C)
• All existing tests pass
• New tests added for new functionality
• Documentation updated if API changed
• Commit messages follow (): convention
• Branch is rebased on latest master

Related Issues

Addresses the same LLM URL issues noted in PR #120 (never merged).
Addresses the same lint gate issue noted in PR #122 (never merged).
Addresses the same to_dict() gap as PRs #119 and #124 (never merged).
Consolidated as a single coherent fix to keep review surface small.

Screenshots / Logs

$ ruff check .
All checks passed!

$ pytest tests/ -q
715 passed, 1 skipped in 13.08s

Additional Notes

All 35 new test_llm_client tests run fully offline — no Ollama, no API keys needed in CI.

[srpatcha]

Review — ebuild#127 "fix(eos_ai): LLMClient URL normalization, env var support, and unit test suite"

head: 39779b0 author: iamsayanmandal ci: fail (policy / Policy / Linked Issue)

Verdict: The URL-normalisation, scheme and empty-choices fixes are correct — I exercised all three and they behave as described. Two things block it. The one CI check that ran is red, and the new environment-variable support uses the literal string "llama3" as a sentinel, so a caller who explicitly asks for model="llama3" silently gets a different model. Separately, this PR bundles four independent changes that five other open PRs already cover; the body's rationale for that ("to keep review surface small") is the opposite of its effect.

Findings

#	Severity	File:line	Finding	Recommended fix
1	High	PR body, "Related Issues"	Required check failing. policy / Policy / Linked Issue fails (run 34718125692), and it is the only check that ran — ci.yml produced no run for this branch at all, so the body's 715 passed, 1 skipped has no CI behind it. The policy workflow landed on master in 52e1f94 and is invoked via pull_request_target from embeddedos-org/.github. It fails because "Related Issues" names pull requests (#120, #122, #119, #124), not issues.	Open (or find) a tracking issue and reference it as Closes #NNN. Until then this cannot merge regardless of the code.
2	High	ebuild/eos_ai/llm_integration.py __init__, all three self.model = model if model != "llama3" else ... lines	A default value is used as a sentinel, so an explicit argument is silently discarded. model defaults to "llama3", and each branch treats that exact string as "caller said nothing". Verified: LLMClient(provider="openai", model="llama3", api_key="k").model → 'gpt-4o-mini', and with OPENAI_MODEL=gpt-4o set → 'gpt-4o'. Asking an OpenAI-compatible server for llama3 is an entirely ordinary request — that is what vLLM and llama.cpp, named in this PR's own rationale, serve — and the client quietly sends a different model name. No error, no warning.	Use a real sentinel: model: Optional[str] = None, then self.model = model or os.environ.get("OPENAI_MODEL", "gpt-4o-mini") per branch. The existing tests pass no model, so they are unaffected.
3	Medium	ebuild/eos_ai/llm_integration.py __init__, ollama branch, self.api_key = ""	Same class of defect, different field: the ollama branch hard-assigns "", discarding whatever the caller passed. Verified: LLMClient(provider="ollama", api_key="secret").api_key → ''. Master kept self.api_key = api_key. Ollama behind an authenticating reverse proxy — Open WebUI, or any of the gateway setups people actually deploy — can no longer be reached, and _build_headers() will never emit the Authorization header it was just taught to emit conditionally.	self.api_key = api_key or "". That preserves the "no stray Bearer header by default" behaviour the PR wants while honouring an explicit key.
4	Medium	Whole diff (7 files, +468/−35)	Scope. This PR carries four unrelated changes, each already the subject of another open PR: LLM client fixes (#120, and #135 covers the same success-reporting bug), the four ruff findings (#122, #132), PackageRecipe.to_dict() (#119, #124, #132), and the type: ignore code (#122, #132). The body states this was "consolidated as a single coherent fix to keep review surface small" — but the surface is now 468 lines across four subsystems, and landing it makes five other contributors' PRs conflict or become empty. to_dict() in particular now has four competing implementations in flight; this one and #124's are the only two that preserve install_args.	Split. Keep ebuild/eos_ai/llm_integration.py + tests/unit/test_llm_client.py here — that is the part no other PR does as well — and drop the recipe.py, plugins/__init__.py and three test-file hunks, deferring to #122 and #124. Then reference those PRs rather than re-implementing them.
5	Medium	ebuild/eos_ai/llm_integration.py __init__	Inconsistent precedence within one constructor. For base_url and api_key the explicit argument wins over the environment (base_url or openai_base). For model the environment wins over the explicit argument (finding 2). Two opposite rules, three fields, one function, documented nowhere — the new docstring lists the variables but not who beats whom.	Fixing finding 2 makes all three consistent (argument → env → default). Add that order to the module docstring's "Environment variables" block in one line.
6	Medium	tests/unit/test_llm_client.py, TestEnvVarSupport	The 35 new tests are a genuine improvement on zero, but every constructor call in TestEnvVarSupport omits model, so the sentinel collision in finding 2 is untested in both directions — neither pinned nor caught. test_ollama_model_env_var and test_openai_model_env_var pass because of the sentinel, so they will keep passing after the fix without ever having covered the case that breaks.	Add the failing case: monkeypatch.setenv("OPENAI_MODEL", "gpt-4o"); assert LLMClient(provider="openai", model="llama3", api_key="k").model == "llama3". It fails today and passes after finding 2 is fixed. Add the mirror for api_key and finding 3.
7	Low	ebuild/eos_ai/llm_integration.py _parse_openai_response	An empty content string is now reported as success=False, error="Upstream returned an empty completion.". Distinguishing "no choices" from "empty text" is right, but an empty completion is a legal response — a model that correctly decides there is nothing to say now surfaces as a failure to analyze()'s caller. The choices == [] guard is the one that matters and is unambiguous.	Consider keeping only the not choices branch as an error and returning success=True with empty text for an empty content, or say in the docstring that this client treats an empty completion as a failure by policy. Either is defensible; the current behaviour is just undocumented.
8	Low	PR body, Pre-Submission Checklist	Unsupported claims. "Unit tests pass (ctest --test-dir build --output-on-failure)" is ticked on a repository with no CMake project — ctest cannot have produced that result. "Integration tests pass" is ticked with no integration suite named or shown. The ruff/pytest output blocks below are the real evidence and are fine; these two boxes are not. Per .ai/reviewer.md, an unsupported claim is itself the finding.	Untick both. The pytest tests/ -q block already says what was run.

Architecture conformance

Deviates on tier placement — see the proposal below. §21 assigns ebuild to Tier 1 — Foundation and eAI to Tier 3 — Advanced. ebuild/eos_ai/ is a ten-module AI-assisted hardware-design subsystem — kicad_parser, eagle_parser, component_db, eos_hw_analyzer, eos_config_generator, eos_boot_integrator, eos_project_generator, eos_validator, llm_integration, prompts — living inside the Tier-1 SDK repository. This PR adds 458 lines to it.

§5.1 is not violated: nothing here imports upward, eos_ai imports only stdlib, and eBuild understands the complete graph but is not a runtime dependency still holds because none of this reaches a device. The problem is that the master design has no place for it. It is not eAI as §16.1 defines eAI — that is on-device inference (ONNX/TFLite, accelerators, "the kernel cannot require it"). It is not build orchestration as §9 defines it. It is a developer-time AI assistant, and §21's taxonomy does not name that category.

It also sits against a specific rule. §9.2 lists "No mandatory cloud connection" as an SDK design rule, and §37 defers "Mandatory cloud/fleet integration for core OS users". eos_hw_analyzer.py:392 calls LLMClient.auto(), which probes localhost:11434 and falls back to OPENAI_API_KEY against https://api.openai.com. That is opt-in in practice — auto() degrades to provider="none" and analyze() returns a clean failure — so the rule is honoured in behaviour. But the design does not say it must be, which is why this PR could add outbound-LLM configuration to a Tier-1 repo without any conformance question being raised. I have appended a proposal.

Two things the diff gets right and are worth recording: _normalize_openai_url is idempotent (verified: http://h/v1/chat/completions in → unchanged out), and analyze() retains its broad except Exception wrapper at the call boundary, so the network failure paths in _call_ollama/_call_openai_compat still surface as LLMResponse(success=False) rather than a traceback. No API signature changes: LLMClient.__init__ keeps its parameter list, LLMResponse keeps its fields, and the three new module functions are private by convention. So brief item 8 is satisfied — except for the semantic break in finding 2, which changes what an existing call returns without changing its signature. That is the kind of break silence makes worse; state it in the body if it is kept deliberately.

Proposed changes

In order:

1. Finding 2 and 3 — switch to Optional[str] = None sentinels for model and honour api_key or "" on the ollama branch. Small, mechanical.
2. Finding 6 — add the two regression tests that fail before step 1.
3. Finding 4 — split the PR. Drop ebuild/packages/recipe.py, ebuild/plugins/__init__.py, tests/ebuild/test_build_dir_resolution.py, tests/ebuild/test_package_recipe.py and tests/unit/test_ci_gate.py from this branch.
4. Finding 1 — link an issue so the policy check can go green.
5. Findings 5, 7, 8 — one docstring line, one policy decision, two unticked boxes.

Not checked

• pytest — NOT RUN. pytest is not importable on this host, and the local ebuild clone has a dirty working tree, left untouched per the rules of engagement. None of the 35 new tests were executed by me. 715 passed, 1 skipped is unverified, and CI did not run it either (finding 1).
• mypy — NOT RUN. Not installed. This PR changes # type: ignore[attr-defined] to [arg-type] in ebuild/plugins/__init__.py — a different approach from #122, which replaced it with a cast. Whether [arg-type] alone silences the line is exactly what mypy would answer and I cannot. Unverified either way.
• yamllint — NOT RUN. Not installed; no YAML in the diff.
• No LLM provider was contacted. Every result below comes from constructing LLMClient and calling the pure helpers. Real Ollama, real vLLM, real OpenAI behaviour — including whether _build_headers() actually satisfies llama.cpp, and whether the /v1 normalisation matches every server's routing — is unverified.
• mergeStateStatus is BLOCKED and reviewDecision is REVIEW_REQUIRED; I did not enumerate which branch-protection rules are outstanding beyond the failing policy check.

Verified locally, against a git archive export of head 39779b05:

• ruff 0.16.5 check . → All checks passed! The four master lint findings are cleared on this tree (finding 4's duplication, but it does work).
• _normalize_openai_url: http://h:8000/v1 → http://h:8000/v1/chat/completions; https://api.openai.com → https://api.openai.com/v1/chat/completions; http://h/v1/chat/completions → unchanged. No doubled /v1. Works as claimed.
• _ensure_scheme("192.168.1.50:11434") → http://192.168.1.50:11434. Works as claimed.
• OPENAI_BASE_URL=http://local:8000/v1 → client.base_url == 'http://local:8000/v1'. Works as claimed.
• LLMClient(provider="openai", model="llama3", api_key="k").model → 'gpt-4o-mini'; with OPENAI_MODEL=gpt-4o → 'gpt-4o'. Finding 2.
• LLMClient(provider="ollama", api_key="secret").api_key → ''. Finding 3.

---

Automated architecture review of 39779b051d29 — 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.

[srpatcha]

Thanks for tackling this — the URL normalization, OLLAMA_HOST support and the bare-Bearer fix are all things we need. A few things before this can go in:

1. llm_integration.py:116/123/128 — using "llama3" as the sentinel means an explicit model="llama3" for openai/custom gets replaced (LLMClient(provider="openai", model="llama3", api_key="k").model is gpt-4o-mini). Could you make model default to None and pick the per-provider default from that?
2. 2. llm_integration.py:232 — is_available() still requires api_key, so the keyless local-server path never fires from EosHardwareAnalyzer (it returns early at eos_hw_analyzer.py:641). For custom, bool(self.base_url) would match the new behaviour.
3. 3. Scope — recipe.py:92, plugins/__init__.py:46 and the three lint fixes aren't part of this change and conflict with #122/#132/#124/#119, which already cover them. Could you drop them here and rebase once those land?
4. 4. llm_integration.py:213 — _check_ollama() ignores an explicit base_url; is_available() should probe the URL the client will actually call.
      Note that #120 addresses the same problem set (it fixes 2 and 4, and adds docs), and I've approved it; it would be worth coordinating so we merge one of the two rather than both — if you'd rather, the unique value here (OLLAMA_HOST, the sentinel fix once corrected) could be a small follow-up on top of #120. Also please add the DCO sign-off (git commit -s).

[srpatcha]

Verdict: NEEDS-AUTHOR. About half is superseded by master; the rest needs a rebase onto master's helpers.

Already on master (1a32cb8, 35368ae and later):

• /v1 join: _openai_chat_url.
• Empty choices/content returns success=False: _completed.
• http(s)-only validation: _http_url.
• Probing the configured base_url.
• A 10 MB response cap and credential redaction.
• PackageRecipe.to_dict(), and the ruff/mypy fixes to test_ci_gate.py, test_package_recipe.py, test_build_dir_resolution.py and plugins/__init__.py.

Still new and worth keeping:

• Omit Authorization when there is no API key.
• OLLAMA_HOST/OLLAMA_MODEL/OPENAI_BASE_URL/OPENAI_MODEL env vars.
• Prepending a scheme to a bare OLLAMA_HOST.

Why I did not push: merging master conflicts in 5 files (4 hunks in llm_integration.py). I tried a resolution that keeps master's helpers plus your additions.

• Tests pass (68 passed), but only because test_llm_client.py calls _normalize_openai_url and _parse_openai_response directly. In the merged tree LLMClient never calls either; they are dead code.
• With OLLAMA_HOST=gpu-box:11434, auto() probed http://localhost:11434/api/tags while the client it returned targets gpu-box.

Fixing that means deleting your duplicate helpers and rewriting their tests, which is beyond a small fix. I aborted the merge; the branch is unchanged at 39779b0.

Suggested:

1. Rebase onto master.
2. Drop the recipe, plugins and test-lint changes.
3. Route OLLAMA_HOST through _ensure_scheme then _http_url, and pass it to _check_ollama(base_url) in auto().
4. Drop _normalize_openai_url/_parse_openai_response in favour of master's helpers, and retarget those tests at _openai_chat_url/_completed.
5. Keep _build_headers.

[Aswin-V]

Code review — findings

Reviewed fix/llm-client-url-normalization-and-tests (39779b0). The core fix is sound:

• _normalize_openai_url() is correct and idempotent across the four documented forms (bare host, /v1, /v1/, full endpoint). The /v1/v1 double-append it eliminates was a real bug, as were the bare-Ollama-host crash, the stray Authorization: Bearer header on empty keys, and the silent success=True on empty choices.
• 35 tests, fully offline with mocks — the right way to test this.
• The PackageRecipe.to_dict() addition is independently valuable (it unblocks ebuild update-index, and PR feat: add pinned eCAD hardware authorization foundation #158's validation notes the same gap).

Two notes:

1. Bundling. This PR mixes four concerns (URL normalization, first test suite, ruff gate fixes, to_dict). They're all "restore green CI" so it's defensible, but if any part needs rework, splitting would make review cheaper.
2. Edge case worth a test: a base URL with a non-/v1 API prefix (e.g. http://host/openai/v2) falls through to + "/v1/chat/completions" → .../v2/v1/chat/completions. Probably out of scope, but consider documenting that only the /v1 prefix form is normalized.

Not merging per org policy (reviews are comment-only; the daily loop lands its own equivalents where needed). The normalization approach here is good — no competing implementation planned.

[srpatcha]

@iamsayanmandal, I pushed a731953 to land this, since the branch has been idle since 09-10. Master merged in. Master had already fixed the /v1 doubling, base_url probing and Bearer redaction, so llm_integration.py now starts from master and adds what was still missing: OLLAMA_HOST, OPENAI_BASE_URL, the model env vars, no Authorization header without a key, and the empty-choices failure. All four review points are addressed (see the commit message), and the out-of-scope files are reset. Your 35 tests pass, plus 7 new ones for the review items (42/42); full suite 931 passed; ruff and CI's mypy are clean.

[codecov-commenter]

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!