LLM - Enrichen summary and intent/rules with the actual product metadata in the HTML document if it exists for greater precision.#4199
Conversation
…ata in the HTML document if it exists for greater precision.
AmirF194
left a comment
There was a problem hiding this comment.
This reads well and the no-metadata path is handled carefully at every layer: compute_llm_enrichment returns '' when the HTML is falsy, the plugin yields nothing, or the block would exceed budget; get_fetched_html() returning False is guarded with or ''; and both prompt builders gate on if metadata:. I like that over-budget enrichment is dropped whole rather than truncated, so the feature cannot turn a previously-passing call into an over-size failure, and that the extractor reuses the stdlib html.parser path (robust to unclosed <script> and invalid JSON, both tested). Folding metadata into the summary and evaluate_change cache keys is the right call too, since a changed fact cannot then serve a stale summary. On injection: the JSON-LD values are page-controlled and appended verbatim, but the diff and current snapshot in the same prompt are also page-derived, so it is the same trust boundary, not a new one.
The blocking thing, probably already on your radar given the WIP head commit: CI is red for a real reason, not environmental. All four failures are in tests/test_llm_all_changes_diff.py. The PR adds metadata=_llm_metadata to the summarise_change(...) call in blueprint/ui/diff.py, but the four fake_summarise mocks (lines 58, 96, 128, 165) are defined as (watch, datastore, diff, current_snapshot=None) and do not accept metadata, so Python raises TypeError: unexpected keyword argument 'metadata' at call binding. The two direct tests then hit the endpoint's except Exception and return 500 (assert 500 == 200), and the two cache tests never reach the counter (assert 0 == 2). Adding metadata=None (or **kwargs) to each mock fixes it, no secrets needed since the LLM is mocked.
Two optional thoughts: in worker.py, compute_llm_enrichment runs for every LLM-enabled changed watch before the if _llm_intent: and summary checks, so a watch with neither an intent nor a summary prompt still parses its full HTML on every change for a result nobody reads. Gating the compute on "intent or summary configured" would avoid that. And the summary size guard measures diff + meta, but the payload also carries current_snapshot, which stays unbounded (pre-existing, just adjacent).
On coverage: the extractor unit tests are strong and would fail without the change (present/absent/adversarial, including 50 products, dangling scripts, invalid JSON). Two gaps if you want them: compute_llm_enrichment itself has no direct test, so its over-budget drop branch is unverified, and no test drives non-empty metadata end to end through diff.py/worker.py (the existing integration tests save text history rather than {timestamp}.html.br, so get_fetched_html returns False and metadata stays '' even once the mocks are fixed). One integration test that stores raw HTML with JSON-LD and asserts the block reaches the prompt would cover the actual feature.
closes #4195