fix(eai): don't report LLM success when the call failed

[vudathayajushi]

Summary

ebuild analyze --llm prints "LLM analysis complete" even when the model call fails or returns nothing. The configs still come from the rule engine, so the log is just wrong.

While I was in there I also fixed a couple of things that make --llm miss the server you actually configured:

• Ollama availability always hit localhost:11434
• EOS_LLM_URL values that already end in /v1 became /v1/v1/chat/completions
• file:// URLs were passed straight to urllib.request.urlopen

I did not touch index sync / PackageRecipe.to_dict or the ruff findings. Those are separate.

Type of Change

• fix - Bug fix
• test - Add or fix tests
• docs - Documentation only

Changes

• analyze --llm warns and keeps the rule-engine profile if the LLM call didn't actually succeed
• Probe the Ollama URL on the client, not a hardcoded localhost
• Join OpenAI-compatible /v1 once
• Reject non-http(s) endpoints, cap the response at 10 MB, redact bearer tokens in error text
• Tests in tests/ebuild/test_llm_integration.py (no network)
• Docs: --llm-provider / --llm-model are not real flags; --file is required for files

Testing

• Unit tests pass (ctest --test-dir build --output-on-failure) — no C changes
• New tests added for new functionality

python -m pytest tests/ebuild/test_llm_integration.py tests/ebuild/test_eos_ai.py -v
# 57 passed

python -m ruff check ebuild/eos_ai/llm_integration.py ebuild/eos_ai/eos_hw_analyzer.py ebuild/cli/commands.py tests/ebuild/test_llm_integration.py
# All checks passed

python -m mypy ebuild/eos_ai/llm_integration.py ebuild/eos_ai/eos_hw_analyzer.py --ignore-missing-imports --no-strict-optional
# Success: no issues found

tests/ebuild + tests/unit is 699 passed, 3 skipped, 9 failed. The 9 are the existing PackageRecipe.to_dict crash in index_sync.py. I didn't change that file.

I also temporarily put the old /v1 join back; three of the new tests failed with /v1/v1/chat/completions. Restoring the fix made them pass.

Pre-Submission Checklist

• New tests added for new functionality
• Documentation updated if API changed
• Commit messages follow the <type>(<scope>): <description> convention
• This PR links a real issue in this repository with a closing keyword

Related Issue

Fixes #134

Additional Notes

#127 also works on LLMClient URL joining. This PR is specifically the --llm success lie and the localhost availability probe; I kept it to that path on purpose.

[srpatcha]

Review — ebuild#135 "fix(eai): don't report LLM success when the call failed"

head: 1a32cb8 author: vudathayajushi ci: pass (policy / Policy / Linked Issue)

Verdict: Real fixes for a real lie, with the strongest evidence trail I have seen in this repo this run — every number in the body reproduces here, including the deliberate revert checks. The scheme guard, the response cap and the bearer redaction are the right instincts and they are tested rather than asserted. One design consequence is not addressed: the llm_failed:/llm_analyzed: marker the CLI now branches on is also written into the generated board.yaml, which makes a generated board definition carry a network-call outcome.

Findings

#	Severity	File:line	Finding	Recommended fix
1	Medium	ebuild/eos_ai/eos_hw_analyzer.py:654 → ebuild/eos_ai/eos_config_generator.py:76	The status marker is carried in profile.features, and HardwareProfile.features is serialized straight into the generated board definition (eos_config_generator.py:76, generate_board_yaml). Verified by running it: ebuild analyze --llm against a provider that returns HTTP 401 writes board.yaml containing features:\n - llm_failed:openai. Two consequences. (a) A generated board definition now records whether an outbound HTTP call succeeded — §23.2 lists "Board definitions — versioned board schema and compatibility constraints" as a compatibility contract, and llm_failed:openai is not a board feature. (b) The same ebuild analyze --llm invocation produces a different board.yaml depending on network luck, against §9.2 "Reproducible lockfiles/manifests for production builds". llm_analyzed: already had this shape on master (eos_hw_analyzer.py:661), so the wart is pre-existing — but this PR adds the second marker and, at commands.py:1847, makes the marker load-bearing, which is what turns a cosmetic leak into a contract the code depends on.	Keep the signal out of the artifact. Smallest version: have analyze_with_llm return the outcome (-> tuple[HardwareProfile, bool], or set a non-serialized profile.llm_status) and branch on that at commands.py:1847. If you would rather not touch the signature in this PR, filter llm_ markers in generate_board_yaml: "features": [f for f in profile.features if not f.startswith("llm_")]. Either way the CLI keeps its honesty fix and board.yaml stops depending on the network.
2	Low	ebuild/eos_ai/llm_integration.py, get_provider_info(); printed at ebuild/cli/commands.py:1837	get_provider_info() returns self.base_url verbatim for the ollama and custom branches, and commands.py:1837 logs it on every --llm run. This PR added credential redaction for error text (_error_text, _KEY_QUERY_RE) but not here, so an EOS_LLM_URL of https://llm.internal/v1?api_key=sk-live-… — or one carrying userinfo, https://user:tok@llm.internal — is printed to the console and into any captured CI log. .ai/tooling.md, CLI conventions: "Never print a secret, a key, or a device credential." The PR's own test test_provider_info_covers_each_kind asserts the OpenAI branch does not leak sk-live-secret, which is exactly the branch that never prints a URL; the two branches that do print one are untested for this.	Reuse the redaction you already wrote: run base_url through _KEY_QUERY_RE and strip parsed.username/parsed.password before formatting. Then extend test_provider_info_covers_each_kind with a custom client whose base_url carries ?api_key= and one carrying userinfo.
3	Low	TASKS.md:45	T-006 collides with open PR #133, which adds its own, different T-006 — Preserve source extensions in Ninja object paths. origin/master stops at T-005, so both branches allocated the next ID independently and whichever merges second produces two T-006 rows in one table. Your T-007 and T-008 are also at risk if another branch is sitting on them.	Renumber to the next free ID at rebase time, or agree with #133 who takes T-006. Nothing else in the PR is affected.
4	Low	ebuild/eos_ai/llm_integration.py, _check_ollama	Dead handler. _http_url(url) is already called in its own try that returns False on ValueError, so by the time the second try runs the URL is known to be http(s) and urlopen cannot raise the ValueError ("unknown url type") that the newly widened except (..., ValueError) was added to catch. Harmless, but it reads as though a path is guarded that cannot occur.	Drop ValueError from the second except, or fold the two try blocks into one. Not worth a round trip on its own.

Evidence check

Per .ai/reviewer.md every PASS should point at command output. This body does, and it holds up. Reproduced against a git archive export of head 1a32cb87 in a clean venv:

Claim in the PR	Result here
"pytest tests/ebuild/test_llm_integration.py tests/ebuild/test_eos_ai.py -v # 57 passed"	57 passed in 0.39s.
"ruff check … # All checks passed"	All checks passed on all four named files.
"I also temporarily put the old /v1 join back; three of the new tests failed"	3 failed, 30 passed on reverting _call_openai_compat to f"{self.base_url}/v1/chat/completions". The composition differs slightly from the body's description — the _openai_chat_url parametrised cases keep passing because the helper itself is untouched by that revert; the three that fail are test_analyze_posts_to_the_joined_url, test_file_url_does_not_reach_urlopen and test_ftp_url_is_rejected. The count and the conclusion are right.
"the 9 are the existing PackageRecipe.to_dict crash in index_sync.py"	Confirmed independently while reviewing #133: those 9 failures reproduce on plain master and are what #124 fixes. Correctly scoped out here, and correctly recorded as T-008.

The test file is the part worth keeping. test_file_url_is_not_available_and_does_not_open and test_file_url_does_not_reach_urlopen assert urlopen.assert_not_called() rather than asserting on a return value, which is the only form of that test that actually proves the SSRF path is closed. test_error_text_does_not_echo_a_bearer_token asserts both the absence of the secret and the presence of [redacted], so a regression that drops the redaction entirely cannot pass it. test_empty_response_is_a_failure is the test that makes the headline fix stick.

Architecture conformance

Conforms, with one naming observation and one gap I have written up as a proposal.

• §21 / Tier 1. ebuild is Tier 1 — Foundation. ebuild/eos_ai/ is a developer-side hardware-analysis helper inside the SDK, not the Tier-3 eAI inference runtime, so §5.1's "the kernel must not depend on eAI" is not engaged and neither is the tier rule: nothing in this diff is imported by anything below eBuild. §5.1's "eBuild understands the complete graph but is not a runtime dependency" holds — this code runs at development time and ships in no image.
• §9.2, "No mandatory cloud connection." Conforms, and the PR strengthens it: --llm is opt-in, the failure path keeps the rule-engine profile, and the command still exits 0 (commands.py:1847-1852). That is the correct reading of the clause — the cloud must be optional, not absent.
• Naming, §37 / Appendix C. The master design lists "Overlapping AI product names and research brands" as a thing to defer and directs consolidation under eAI. A module named ebuild/eos_ai/ sitting next to a Tier-3 product named eAI, with this PR titled fix(eai): …, is exactly that overlap. It is pre-existing and not this PR's to fix, so it is not a finding — but the PR title does make an eBuild-internal change look like an eAI change.
• Brief item 8 (API/wire compatibility). _check_ollama changed from a no-arg @staticmethod to one taking base_url: Optional[str] = None. Source-compatible for every existing caller, and the PR's own tests call it both ways. LLMResponse and LLMClient.__init__ are unchanged. Nothing to migrate, so the body's silence is correct.
• MEMORY.md records the http-vs-https decision with the rejected alternative, which is what that file is for and is the reason I did not raise "why is plain HTTP allowed here when the index fetcher requires HTTPS" as a finding — it is answered, with a reason that is correct (stock Ollama listens on http://localhost:11434).

Proposed changes

1. Take the marker out of the serialized profile (finding 1). This is the only one I would hold the PR for.
2. Redact base_url in get_provider_info() using the redaction already in the file (finding 2).
3. Renumber T-006 (finding 3).
4. Optional: collapse the double try in _check_ollama (finding 4).

No architecture proposal is appended. The design-level gap this PR touches — §9.2 says the cloud connection must not be mandatory but never says what a developer's design data may be sent to once they opt in — is already open as a proposal dated 2026-09-10 in .ai/autoreview/proposals/2026-09.md, triggered by ebuild#120. This PR is a second instance of the same gap, not a new one. Finding 1 is not a design gap: §9.2 already requires reproducible manifests; the code simply does not meet it.

Not checked

• No network call was ever made. Every test here mocks urllib.request.urlopen. I did not exercise a real Ollama, a real OpenAI-compatible endpoint, or a real 401 — so the redaction, the 10 MB cap and the /v1 join are verified against the code's behaviour, not against a live server's.
• mypy — NOT RUN. Not installed on this host. The body's mypy … # Success: no issues found is unverified.
• Full suite — NOT RUN in this review. I ran the two modules the body names (57 tests). The "699 passed, 3 skipped, 9 failed" claim for tests/ebuild + tests/unit is unverified here, though I did independently reproduce the 9 to_dict failures on master while reviewing #133, which is the part of it that matters.
• Windows/macOS — NOT RUN. test_file_url_does_not_reach_urlopen uses file:///C:/secrets/key.json, which urlparse handles identically on any platform, so I do not expect divergence — but I did not run it there.
• _read_limited against a real streamed response — NOT RUN. The oversize test uses a MagicMock whose read(n) ignores n and returns the whole body. A real http.client.HTTPResponse.read(limit + 1) returns at most limit + 1 bytes, so the check is correct, but the test would also pass against an implementation that read the whole body first. Worth an integration-level test eventually; not a blocker.
• Coverage — NOT RUN. The "99.46% on llm_integration.py" claim in TASKS.md was not reproduced; I did not run pytest-cov.
• I did not audit whether anything else in the tree reads profile.features expecting only hardware features. grep found two consumers (eos_config_generator.py:76, commands.py:1847) and I checked both; I did not sweep templates or downstream repos.

---

Automated architecture review of 1a32cb87e9d9 — 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.