unic-pr-review: false-premise 🔴 critical — diff-only agents assert runtime behavior without tracing data flow

[orioltf]

TL;DR

A re-review run (unic-pr-review v2.1.12, --post) escalated a 🔴 critical finding and a headline "key takeaway" that were factually wrong. The developer pushed back with a deployed-environment payload, and on verification the finding's premise was false. The miss was not random — it traces to a single, fixable mechanism: aspect agents assert runtime behavior from the diff alone, without tracing a value to its producing code, and the orchestration then reads multi-agent agreement on that shared blind spot as independent corroboration.

This issue documents the run, the ground truth, the root-cause analysis, and prioritized recommendations.

---

Environment

• Plugin: unic-pr-review v2.1.12
• Mode: re-review (--post), iteration 2
• Target PR: ADO FZAG/dxp/DXP-Website PR 5672 — fix(42718): use pageProps.locale and reject "default" page_language
• Linked Work Item: Bug 42718 ("404 page: wrong value in page_language")
• Spawn set: code-reviewer, silent-failure-hunter, type-design-analyzer, pr-test-analyzer, comment-analyzer

What the change did (abbreviated)

page_language analytics handling was reworked:

• _app.tsx: AnalyticsProvider pageLanguage prop changed from locale → pageProps.locale
• AnalyticsContext.tsx: removed normalizeLocale(pageLanguage) — value now passed through verbatim
• sendPageViewEvent.ts: added a branch — if pageLanguage === 'default', log + push the event with page_language: undefined, return true
• Tests + Analytics.md updated accordingly

What the review produced

9 fresh findings were posted. The headline takeaway and the top finding:

🔴 Critical — "Genuinely undefined page_language drops the whole page_view instead of firing it … on the 404 page getStaticProps returns props: {} when context.locale is falsy, so pageProps.locale is undefined and the event is silently suppressed — violating TF_10's 'always fire on 404'."

The summary's framing reinforced this: "Multiple agents converged on the same gap." Three agents (code-reviewer, silent-failure-hunter, comment-analyzer) and the test analyzer all circled the same undefined/drop theme.

The pushback and the ground truth

The developer:

1. Deployed the branch to DEV and pasted the actual dataLayer payload for /404: "page_language": "en" — the event fires correctly with a real language.
2. Stated the intended design: emit whenever pageProps.locale is defined (default/en/de/in); when it is genuinely undefined, log and do not track.

Verification against the codebase confirmed the developer:

• fe/src/lib/page-props-factory/plugins/normal-mode.ts:54 — props.locale = context.locale ?? props.site.language (then overridden by the Sitecore context.language at :89-90). So for any Sitecore-served page — including the real 404 (the not-found content page served via the [[...path]].tsx catch-all with notFound: true) — pageProps.locale is always defined.
• fe/src/pages/_app.tsx:280 — <I18nProvider lngDict={dictionary} locale={pageProps.locale}> confirms pageProps.locale is the authoritative server-resolved locale that also drives translations (the developer's "same value" claim).
• The only props: {} / undefined-locale path is the bare static fe/src/pages/404.tsx:37-41 (pure Next.js fallback, no Sitecore data). That page also has no page_id/page_title, so the required-fields guard drops it regardless of page_language — there is no meaningful page_view to send there. Dropping is correct, not a TF_10 violation.
• The 'default' branch already implements the ticket's "language can't be derived → emit with undefined" requirement.

Net: the critical finding's central premise was false. It was conceded on the PR (resolved as by-design). Only one related finding survived as genuinely valid: an inline comment in sendPageViewEvent.ts says "we drop the event" while the code emits it — real comment rot, left open.

Root-cause analysis

diff-only context seeded to aspect agents
        │
        ▼
agents reason about RUNTIME behavior ("on 404, pageProps.locale is undefined")
        │   ← never opened normal-mode.ts:54 to see `?? props.site.language`
        │     (one agent even *named* normal-mode.ts in its notes but didn't trace it)
        ▼
all agents share the SAME blind spot (no upstream data-flow trace)
        │
        ▼
their agreement is read as independent corroboration → confidence inflated → 🔴 critical
        │
        ▼
a human thread that already asked this exact question was dropped (see below)
        │
        ▼
false-premise critical posted + amplified in the "key takeaway"

The defect is not that an agent made one wrong guess — it is that the system has no step that forces a runtime claim to be grounded in the code that produces the value, and no guard against treating correlated agents (same context, same blind spot) as multiple independent votes.

Secondary issues surfaced by the same run

1. A resolved human thread that asked the exact question was dropped. Human thread 63848 (inline, resolveCurrentLocale.ts) asked: "TF_10 marks page_language Required and its 404 example always fires a page_view. When [it] returns undefined, [does it still fire]?" — the precise concern the bot then escalated to critical. Because the thread was status: fixed, human-thread-matcher.mjs produced no annotation/notice. A resolved human thread that maps to a fresh high-severity finding is a strong signal the team already considered and answered it.

2. ado-fetcher emitted a lossy threads summary instead of the full object. The fetcher output contract (agents/ado-fetcher.md, "Emit exactly one JSON object", field "threads": "<THREADS object>") expects the full threads object. In this run the agent returned { "count": 16, "note": "…" } instead of THREADS.value. The orchestrator had to pass rawThreadsJson: null to the Re-review Coordinator and the ADO Writer with a "re-fetch via az devops invoke" note. The writer recovered, but the data-handoff contract was silently broken — a reliability risk for the coordinator's thread classification.

Recommendations (prioritized) + why

P1 — Require runtime claims to be grounded in producing code

Where: aspect-agent system prompts — agents/code-reviewer.md, silent-failure-hunter.md, pr-test-analyzer.md, comment-analyzer.md, type-design-analyzer.md.
What: add an instruction such as — "If a finding asserts a runtime value or state (e.g. 'X is undefined here', 'this path is never reached'), trace that value to its producing code before emitting. You have file tools — use them. If you cannot confirm the producing behavior, cap confidence at ≤60 and tag the finding runtime-unverified."
Why: this is the direct fix for the failure. The agents already had file-read tools; they were anchored by the "Diff to review:" framing and never traced pageProps.locale to normal-mode.ts:54. Grounding the single most dangerous class of finding (runtime assertions) would have prevented the critical.

P2 — Stop treating correlated agreement as corroboration

Where: the summary synthesis / takeaway generation, and review-summary-renderer.mjs language.
What: drop or qualify "Multiple agents converged…". Agreement among agents that share identical diff-only context is one opinion observed N times, not N independent votes. True corroboration should require an independent verification step, not a second reader of the same input.
Why: the convergence framing is what turned a shaky inference into a confident headline. It actively amplifies shared blind spots.

P3 — Surface resolved human threads that map to fresh high-severity findings

Where: scripts/lib/human-thread-matcher.mjs + the notice path in notices.mjs / renderer.
What: when a resolved/fixed human thread matches (by file proximity + topic) a fresh 🔴/🟠 finding, emit a de-escalation notice: "A human thread already discussed this and was resolved — re-confirm before flagging." Do not silently drop it just because it is resolved.
Why: thread 63848 was the single loudest signal that the team had already reasoned through this exact concern. The current matcher only annotates unresolved unmatched threads, so it discarded the most relevant context.

P4 — Enforce the ado-fetcher threads passthrough contract

Where: agents/ado-fetcher.md output step.
What: make explicit that threads must be the full THREADS.value array (not a summarized {count, note}), or move thread-passthrough out of the LLM-emitted JSON into a file/handle the agent cannot compress. Add a validation step in the orchestrator that rejects a non-array/empty threads.value in re-review mode.
Why: the coordinator keys all thread classification on raw threads; a silently summarized field degrades re-review correctness and forced a manual re-fetch workaround in this run.

P5 (optional) — "Verify before escalate" gate for high-severity runtime claims

What: for 🔴/🟠 findings that assert production behavior, offer an explicit verification affordance before they reach the summary headline — check against an existing test, a deployed environment, or the upstream source.
Why: the DEV payload settled this in seconds. A lightweight gate would catch the highest-cost (false-critical) class of error. Lower priority because P1 addresses most of it upstream.

Suggested acceptance signals (for planning)

• A finding that asserts "value X is undefined/never-set at site Y" cannot be emitted at >60 confidence unless the agent has read the code that assigns X.
• The summary contains no language that equates multi-agent agreement with independent verification.
• A resolved human thread overlapping a fresh 🔴/🟠 finding produces a visible de-escalation notice.
• In re-review mode, the orchestrator fails fast (or warns loudly) if threads.value is not a populated array.

Appendix — traceability

• PR: FZAG/dxp/DXP-Website #5672 (re-review iteration 2)
• Conceded threads: 63864 (🔴), 63866, 63871 → resolved by-design; 63868 softened/closed
• Surviving valid finding: 63867 (comment says "drop" but code emits) — left open
• Ground-truth code: normal-mode.ts:54 & :89-90, _app.tsx:280, 404.tsx:37-41
• Human thread missed: 63848