Review orchestrator should accept challenger findings when backed by verifiable config evidence

[fullsend-ai-retro]

What happened

On PR #501, a Renovate bot dependency update PR, the review agent's third run (workflow 27614150563) flagged 5 MEDIUM-severity stale image references in .tekton/integration/ test files. A challenger sub-agent investigated and correctly determined these files were managed by a separate Renovate update group (evidenced by the repo's renovate.json config). The challenger removed all findings. However, the orchestrator classified the challenger's result as a [sub-agent-failure] and fell back to the champion's pre-challenger finding set, escalating the PR to requires-manual-review. The stale references were resolved by a subsequent Renovate rebase 4 days later, at which point the fourth review run cleared the PR.

What could go better

The challenger sub-agent had the correct analysis backed by verifiable evidence (the renovate.json configuration file), but the orchestrator treated its disagreement with the champion as a failure rather than a valid correction. This caused an unnecessary escalation that blocked the PR for 4 days until the next rebase resolved the issue naturally. The distinction between 'challenger failed' (errored, timed out) and 'challenger disagrees with evidence' (returned valid findings that differ from champion) appears to be missing from the orchestration logic. Confidence: HIGH — the logs explicitly show the challenger provided correct evidence that was discarded. Related issues #1795 and #1774 are adjacent but neither addresses this specific gap: #1795 is about qualifying verdicts when sub-agents fail (model unavailable), not about trusting correct challenger analysis; #1774 is about verifying functional impact before flagging multi-site changes, not about the champion-challenger resolution process.

Proposed change

In the review agent's orchestration logic (likely in the review agent definition or the champion-challenger resolution code), distinguish between two challenger outcomes: (1) challenger failure (error, timeout, invalid output) — fall back to champion findings as today, and (2) challenger disagreement with evidence (valid output that reduces/removes findings, backed by verifiable references like config files) — accept the challenger's findings or at minimum downgrade the champion's severity. A simple heuristic: if the challenger's output references specific config files (e.g., renovate.json, .github/dependabot.yml) that explain why flagged files are out of scope for the PR, treat the challenger's analysis as authoritative for those files.

Validation criteria

On the next 3 Renovate/bot dependency PRs in repos with grouped update configs, the review agent should not escalate to requires-manual-review for stale references in files managed by a different update group. The [sub-agent-failure] info finding should not appear when the challenger returns valid structured output with config-backed evidence.

---

Generated by retro agent from konflux-ci/konflux-test-tasks#501

[fullsend-ai-triage]

🤖 Finished Triage · ✅ Success · Started 8:39 AM UTC · Completed 8:46 AM UTC
Commit: 4e21a60 · View workflow run →

[fullsend-ai-triage]

Triage Summary

This issue identifies a gap in the review orchestrator's champion-challenger resolution logic (step 6d in the pr-review skill). The current heuristic treats any empty adjudicated_findings result as a challenger failure, but this doesn't account for cases where the challenger legitimately removes all findings with evidence-backed reasoning.

Problem

The blanket fallback rule — "empty adjudicated_findings + non-empty pre-challenger set = failure" — conflates two distinct scenarios:

1. Actual failure: parsing error, context truncation, or malformed output
2. Valid analysis: challenger correctly determines all findings are false positives, backed by verifiable config evidence (e.g., renovate.json)

In the reported incident, the challenger correctly identified that flagged stale image references were managed by a separate Renovate update group, but its analysis was discarded, causing a 4-day unnecessary escalation.

Recommended Fix

Modify the empty-result fallback in skills/pr-review/SKILL.md (and scaffold copy) to inspect the removed_findings array before deciding. If every removal includes a substantive removal_reason referencing specific config files or verifiable evidence, accept the challenger's result. Only fall back when removed_findings is absent, empty, or contains unsupported reasoning.

Related Issues

• Review orchestrator should qualify verdict when change-relevant sub-agents fail #1795 — Qualifying verdicts when sub-agents fail (model unavailability). Adjacent but different: that issue is about prevention of failures, this is about correctly handling valid challenger disagreement.
• Review agent: verify functional impact before flagging incomplete multi-site changes #1774 — Verifying functional impact before flagging multi-site changes. Also adjacent: addresses completeness-check false positives, not the champion-challenger resolution process.

Proposed Test

Scenario 1 (evidence-backed removal accepted):
  Given: challenger returns empty adjudicated_findings
  And: removed_findings contains entries with config-file-backed removal_reasons
  Then: orchestrator accepts challenger result (no sub-agent-failure, no fallback)

Scenario 2 (empty removal treated as failure):
  Given: challenger returns empty adjudicated_findings
  And: removed_findings is empty or absent
  Then: orchestrator falls back to champion findings (existing behavior)

Scenario 3 (partial removal):
  Given: challenger removes some findings with evidence, keeps others
  Then: orchestrator uses adjudicated_findings as-is

---

Labels: Review agent orchestration feature request for champion-challenger resolution logic, medium priority given workaround exists (natural resolution via rebase).