fix(brand-profile): hold entity-validation failures as unverified, not fail-open (#3268, re-opens #361) - #366
mikkeline-elleby wants to merge 6 commits into
Conversation
…main + content The QID->article guard added in #359 cannot catch a QID that was itself resolved to the wrong same-named entity (e.g. capella.edu -> Q12970, the star Capella), so products/sub_brands/competitors still get contaminated even though the guard passes (wikipedia_verified: true). Add a validation gate right after the QID is established in extractProducts: - deterministic pass when the entity's official website (P856) registrable-domain matches the brand's site domain; - otherwise an LLM check of the entity's description against the brand's domain + industry. On no match, drop the QID so neither the Wikidata SPARQL query nor the Wikipedia fallback runs (no data rather than wrong data). Fails open on infrastructure errors so it never regresses a brand it merely could not verify. Introduced by: N/A Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Adds a registrableDomain case whose input fails new URL() even after the https:// prefix (space in the authority), exercising the catch-and-fall-through to bare-hostname handling (wikipedia.js:253-254). Closes the codecov/patch gap. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…t fail-open
Re-opens the Class B wrong-QID entity-validation gate and changes its fail-open
posture. validateEntityMatchesBrand now returns a three-state
{status: verified|mismatch|unverified} instead of a boolean match:
- verified = authoritative P856 official-website domain match (only status that publishes)
- mismatch = definitive LLM "no" -> drop the QID (entity-brand-mismatch)
- unverified = no domain match + (LLM "yes" | no verifier | infra error) -> non-publishable,
held for review. The LLM may only downgrade, never auto-pass-to-publish
(it reads attacker-editable third-party text), so even an LLM "yes" holds.
Infra errors (Wikidata/LLM) previously failed open to trusted; they are now held and
flagged (failOpen), with an alarmable fail-open-rate metric emitted as structured log
lines. The LLM leg is flag-gated via BRAND_PROFILE_ENTITY_VALIDATION_LLM (defaults on).
Validation now also runs whenever a domain is present (industry optional), closing a gap
where a missing industry skipped the check and let contamination through.
Re-opens #361 (closed without a stated reason; author nsdere to confirm). Closes #360.
Refs adobe-rnd/llmo-data-retrieval-service#3268, adobe-rnd/llmo-data-retrieval-service#3200.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Critical validation gaps can still publish incorrect entities or contaminate competitor output.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Open (4)
What changed in this PR
Summary
Adds three-state Wikidata entity validation and fail-open holding for brand-profile product extraction. Several critical gaps remain in ambiguity handling, domain parsing, and competitor protection.
Changes:
- Added P856/LLM entity validation with structured observability.
- Withheld unverified product and Wikipedia data.
- Added extensive validation and regression tests.
Issues
Critical
wikipedia.js:373: Any response beginning withnis treated as a definitive mismatch; ambiguous responses such as “not sure” should remain unverified.wikipedia.js:232-236: The incomplete suffix list can make unrelated.org.ukdomains normalize identically and falsely publish a wrong QID.index.js:258-262: Competitor inference consumes the unvalidated Wikipedia summary before product validation, so wrong-entity competitor contamination remains possible.
This PR should not be merged until these are fixed.
Major
BRAND_PROFILE_ENTITY_VALIDATION_LLMis not documented inREADME.md.
Suggested Tests
- Add ambiguous LLM responses such as
not sureandno idea. - Add unsupported public-suffix cases.
- Add orchestration tests confirming invalid entities cannot influence competitor inference.
| File | Description |
|---|---|
src/agents/brand-profile/services/wikipedia.js |
Adds domain and entity validation logic. |
src/agents/brand-profile/services/product-extractor.js |
Gates Wikidata/Wikipedia product extraction. |
src/agents/brand-profile/index.js |
Passes validation context and feature flag. |
test/agents/brand-profile/services/wikipedia.test.js |
Tests domain and validation outcomes. |
test/agents/brand-profile/services/product-extractor.test.js |
Tests product withholding and validation paths. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…gate
- Parse LLM answer as an EXACT yes/no; hedged replies ("not sure", "no idea")
now hold as unverified instead of being discarded as a definitive mismatch.
- Harden registrableDomain's two-level-suffix set (org.uk/ac.uk/gov.uk, *.au,
*.jp, *.nz, *.in, *.za, *.br, ...) to avoid false P856 domain matches now that
it is the sole publish-authoritative signal; note PSL as the robust follow-up.
- Document BRAND_PROFILE_ENTITY_VALIDATION_LLM in the README env table.
Refs #366 review.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
This PR will trigger a patch release when merged. |
|
Thanks for the review — addressed in aeda5c5:
|
…t products Hoists the Class-B entity validation to run once (Phase 3.5) right after the Wikidata article is resolved, before competitor inference, persona inference and product extraction. A wrong same-named QID's Wikipedia summary/text is now withheld from every consumer (previously it still reached inferCompetitors, and the sitemap path skipped validation entirely). - New productService.validateEntity(...) owns the check (shared gpt + LLM flag). - index.js gates brandWiki (nulls wikidataId/fullText/summary) when not verified and emits the fail-open metric here; the verdict is passed into extractProducts, which reuses it (no second LLM call) via the new ctx.entityValidation. - extractProducts keeps its own validation for standalone/direct callers. Addresses review feedback on #366. Refs adobe-rnd/llmo-data-retrieval-service#3268. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…-resolution - Cover the new branches flagged by codecov/patch: productService.validateEntity (P856 verify, no LLM), extractProducts' precomputed-verdict reuse path, and the index.js fail-open metric branch. - Fix: when a precomputed entityValidation verdict is passed, extractProducts no longer name-searches a replacement QID (that search is itself a contamination vector) — it respects index.js's decision. Refs #366. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Re-opens the Class B wrong-QID entity-validation gate from #361 (closed without a stated reason) and adds the fail-open posture change required by adobe-rnd/llmo-data-retrieval-service#3268.
Background
The brand-profile agent can resolve a brand to the wrong same-named Wikidata entity and inject that other company's products (Behr paint → Iberian ham; Capella University → the star). #359 fixed Class A (right QID, wrong article); this closes Class B (the QID itself is wrong), which #359's article-guard can't catch.
What changed
validateEntityMatchesBrandnow returns a three-state outcome instead of a booleanmatch:verifiedmismatchentity-brand-mismatch(as #361)unverifiedmatch: true(silently trusted); they now returnunverifiedwithfailOpen: true, so a transient blip during a 1,000+ job regen can't silently re-poison sites.entity_validation_outcomeper call +entity_validation_fail_openwarn) — matching this repo's log-based observability (no metrics SDK). A CloudWatch filter can chart fail-open / total.BRAND_PROFILE_ENTITY_VALIDATION_LLM=falsedisables the LLM leg (non-matches then just hold asunverified); defaults on (Damian approved the LLM cost).domainis present (industry optional) — previously a missing industry skipped the check entirely and let contamination through.Per #3268 ("the LLM may only flag/downgrade, never auto-pass-to-publish — it reads attacker-editable third-party text"), an LLM "yes" no longer publishes — it lands in
unverified(held). Net effect: the only path to publish is the deterministic P856 domain match, which raises the "hold for review" volume. The held cohort is worked through by the sibling batch runner (#3271) / consumer gate (#3270). Flagging so it's a conscious call.@nsdere — could you confirm why #361 was closed?
The record shows no stated reason. Understanding is it was paused, not a defect. This PR re-opens that work; a one-line confirmation would let us merge with confidence.
Tests
Full suite green (444 passing locally via mocha). New/updated cases: P856 verifies; LLM-"no" → mismatch/discard; LLM-"yes" → unverified; garbled answer → unverified (never discard on ambiguity); infra error → unverified +
failOpen+ products withheld; LLM-flag-off → no validation call + hold; Behr/Capella regression.Re-opens #361. Closes #360. Refs adobe-rnd/llmo-data-retrieval-service#3268, adobe-rnd/llmo-data-retrieval-service#3200.
🤖 Generated with Claude Code