fix(review): strengthen line number verification in review prompts - #197
fix(review): strengthen line number verification in review prompts#197rh-hemartin wants to merge 1 commit into
Conversation
|
🤖 Finished Review · ✅ Success · Started 10:30 AM UTC · Completed 10:46 AM UTC |
PR Summary by QodoFix review prompts: enforce verified file-absolute line numbers
AI Description
Diagram
High-Level Assessment
Files changed (5)
|
Code Review by Qodo
Context used✅ Compliance rules (platform):
55 rules 1. Protected skills/ files modified
|
ReviewFindingsMedium
Labels: PR modifies review agent prompt files under skills/pr-review/ and skills/code-review/, and adds an eval test case for line number accuracy Previous runReviewFindingsMedium
Low
Labels: PR modifies review agent prompt files under skills/pr-review/ and skills/code-review/ Previous run (2)ReviewFindingsMedium
Low
Previous runReviewFindingsMedium
Low
Labels: PR modifies review agent prompt files under skills/pr-review/ and skills/code-review/ Previous run (3)ReviewFindingsMedium
Low
Previous run (4)ReviewFindingsMedium
Low
Labels: PR modifies review agent prompt files under skills/pr-review/ and skills/code-review/ |
cebfed8 to
eec6949
Compare
|
🤖 Finished Review · ✅ Success · Started 1:28 PM UTC · Completed 1:45 PM UTC |
waynesun09
left a comment
There was a problem hiding this comment.
Review Squad — 3 agents (Claude × 2, Grok) · 6 verified MEDIUM+ findings
2 HIGH · 4 MEDIUM. No duplicate findings from the existing qodo-bot comment (unrelated protected-path note). Note: this PR was force-pushed mid-review (cebfed8f → eec6949a, adding skills/docs-review/SKILL.md back into the diff) — all findings below are verified against current head eec6949a.
The diagnostic core of this fix (naming the diff-relative-vs-file-absolute confusion, mandating Read-tool-based verification) correctly targets the root cause in fullsend-ai/fullsend#2687. The problems below are in the implementation, not the diagnosis. Three MEDIUM findings that don't anchor to a single line in this diff are included below rather than as inline comments:
MEDIUM (premature-decision) — Round-2 fix asserts recurrence with no evidence trail
This repo has an established pattern for documenting a fix's failure to hold — "Evidence for #NNNN" issues (e.g. #4275, #4506, #4498). No such issue exists for #2687, which was closed 3 days after opening — not enough time for its own "next 10 runs" validation window to elapse. This PR's commit message asserts wrong line numbers are recurring as present-tense fact with no cited run or PR.
Suggestion: Link the specific review run(s) that motivated this PR, or open an "Evidence for #2687" issue per repo convention.
MEDIUM (premature-decision) — No measurement mechanism exists to check whether this fix works
The only eval harness for this skill (eval/review/) has one case with zero expected findings — it cannot exercise line-citation accuracy at all. The PR's "monitor next 10 runs" test-plan item and #2687's "<10% mismatch" validation criteria have no instrumentation behind them. Separately, #2687 itself diagnoses the root cause as a harness gap ("cannot validate semantic correctness"), and #2688 (deliberately deferred by this PR) exists because prompt-only fixes were anticipated to be insufficient — round 1 already tried stronger wording once.
Suggestion: Add an eval case with a known-correct line-citation scenario and a judge that checks it; consider pairing with the harness-side backstop from #2688 rather than betting twice on wording alone.
MEDIUM — No auditable artifact proves line verification actually occurred (self-report only)
All the new instructions rely entirely on the agent's self-report — no line_content snippet, no verified flag, no record of the original (possibly wrong) line before correction. Contrast with the challenger's existing removed_findings array, which requires original_category/original_description/removal_reason and is fully auditable. The challenger's challenger_action enum (kept|downgraded|merged|removed) has no line-corrected value, so a silent line fix produces no visible signal it happened.
Suggestion: Add a line-corrected challenger action with an original_line field; consider requiring a line_content snippet in the finding schema now, ahead of #2688's harness-side fuzzy-match.
Assisted-by: Claude (review), Grok (review)
The 001-clean-approve review eval case has been broken since PR #148 merged. The post-review script returns 422 errors from inline review comments citing invalid line numbers, which prevents the ready-for-merge label from being applied. This causes the required_labels judge to fail (pass_rate=0.0%), blocking functional-tests-complete and the entire merge queue. Remove the case directory to unblock the merge queue. The review eval infrastructure (eval.yaml, hooks, judges, thresholds) is preserved. The test case should be reintroduced after the line number accuracy issue is fixed (see PR #197). Closes #202
eec6949 to
9a9633a
Compare
|
🤖 Finished Review · ✅ Success · Started 6:42 AM UTC · Completed 6:48 AM UTC |
Sub-agents were emitting findings with wrong line numbers (off by 5–15 lines), causing inline comments to land at wrong locations. The root cause is confusion between diff-relative offsets and file- absolute line numbers — the meta-prompt's advisory instruction to "re-read the file" was too easy to skip. Replace the advisory paragraph with a mandatory 4-step procedure, add the challenger as a backstop for line verification, and add reminders to the opus sub-agents (correctness, security) that produce the most line-specific findings. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Hector Martinez <hemartin@redhat.com>
9a9633a to
e66621f
Compare
|
🤖 Finished Review · ✅ Success · Started 6:51 AM UTC · Completed 6:59 AM UTC |
waynesun09
left a comment
There was a problem hiding this comment.
Review Squad — 3 agents (Claude × 2, Grok-attempted) · re-review at head e66621f
1 CRITICAL · 3 HIGH · 1 MEDIUM, all new since the prior round. Note on tooling: the Grok subprocess (cursor-agent --model grok-4.5-high) never returned output in either dispatch (timed out after 10+ min); the third agent's findings below are its own direct GitHub-API verification, not an independent Grok read — flagged for transparency, not counted as model diversity.
Good news first: all 3 issues from the prior round are genuinely fixed — the redundant duplicate sections in security.md/correctness.md were dropped entirely (not just reworded), the old-file/new-file hunk-header ambiguity is now resolved (+Y called out explicitly), and the evidence-trail gap is closed (PR body now cites 10 specific failed CI runs). The LOW heading-inconsistency finding is moot — the section it was about no longer exists.
The findings below are new, found during this re-review pass, and verified directly against the current head and live CI/issue data before posting.
CRITICAL — This PR's own CI, run at its exact head commit, still reproduces the 422 error it claims to fix — despite provably correct line citations
Evidence: CI run 29561223892 ran at head_sha = e66621fe4d3baa3df29f4fdfab92fc106b85e5cd (confirmed via gh api .../runs/29561223892 --jq .head_sha — exact match to this PR's current head) and finished conclusion: failure, with the functional-tests (review) job failing. Its case 002-line-accuracy — the case this PR added specifically to validate the fix — failed with github api: 422 Unprocessable Entity submitting the review. The agent's agent-result.json from that run was independently re-checked against input.yaml's fixture content: all six cited line numbers are file-absolute-correct (line 65 = the SQL-injection f-string, line 21 = average_value's divide, line 31 = the unclosed open(), etc.) and review_quality scored 5.0/5. Since the line numbers are correct, this 422 cannot be the diff-relative/file-absolute confusion this PR's prompt changes target — it's consistent with the still-open companion issue fullsend-ai/fullsend#2688, which notes the harness only validates line numbers against diff hunk ranges (a correct-but-out-of-hunk line still 422s) and frames the harness-side fix as a separate, not-yet-built "defense in depth" layer.
Suggestion: Before merging, reproduce and root-cause this specific 422 independently of line-number correctness (inspect the exact request body fullsend post-review sends — commit_id, side, position vs line). Don't present this PR as resolving the linked CI failures until an actual run on this branch posts successfully end-to-end.
[verified] · flagged by 1/3 agents (Claude-researcher), independently reproduced against live CI run 29561223892
MEDIUM (premature-decision) — PR silently doesn't fulfill the explicit reintroduction request from issue #202
Finding: #202 ("Remove review eval case 001-clean-approve until post-review 422 errors are resolved", closed 2026-07-16) states: "Once the line number accuracy issue is fixed (branch fix/review-line-number-accuracy, PR forthcoming), reintroduce the test case." This PR is that branch/PR, yet neither the diff nor the description mentions 001-clean-approve or #202. (The branch is currently 54 commits behind main, so the deleted case still shows in stale CI runs against this branch — that's not the same as this PR reintroducing it.)
Suggestion: Explicitly decide and state whether reintroducing 001-clean-approve is in scope here or a tracked follow-up, rather than leaving #202's stated expectation silently unaddressed.
[verified] · flagged by 1/3 agents (Claude-researcher), confirmed against issue #202 and branch compare (ahead_by:1, behind_by:54)
Agent Agreement
line_accuracyjudge referenced in the new eval case does not exist anywhere in the repo — flagged by 3/3 agents (full consensus, posted inline below)- Prior-round fixes (redundant sections dropped, hunk-header disambiguation, evidence table) are genuine and complete — confirmed by 2/3 agents (Claude-researcher, third agent) via direct diff/force-push-timeline inspection
Assisted-by: Claude (review), Claude (review), Grok (attempted, non-functional this round)
| only surface-level feedback. A score of 4-5 means it caught all three | ||
| bugs with accurate descriptions and did not approve the PR. | ||
|
|
||
| Line-number accuracy is evaluated separately by the line_accuracy |
There was a problem hiding this comment.
HIGH — line_accuracy judge referenced here does not exist; this eval case currently gives zero signal on line-citation accuracy
eval/review/eval.yaml registers exactly five judges — review_quality, required_labels, forbidden_labels, max_turns, max_cost — confirmed both by reading the file at this head and by the CI run's own log line: Scoring 2 cases with 5 judges: [...]. No line_accuracy judge exists anywhere in the repo, and a repo-wide search for line_accuracy/expected_findings finds no consumer of the expected_findings block above (lines 13-41) at all. Worse, the one judge that does run, review_quality, is explicitly told by this same paragraph to ignore line accuracy ("should focus on whether the agent identified the right issues, not whether the line citations are exact") — so this eval case, added specifically to validate this PR's own fix, measures nothing about the thing it's named for. This is also an untested design choice shipped as final with no fallback marker (no TODO, no "not yet wired" note) if the judge is never built.
Suggestion: Add a check-type line_accuracy judge to eval.yaml that matches posted findings against expected_findings by file+line (or file+line_content) with a registered threshold — or, if that's out of scope for this PR, delete this claim and the dead expected_findings block, or mark them explicitly as "NOT YET WIRED" so the gap is visible.
[verified] · flagged by 3/3 agents (full consensus — independently confirmed by Claude-coder, Claude-researcher, and the third agent's own direct verification)
| "challenger_reason": "<why this finding was kept/changed/removed>" | ||
| "challenger_action": "kept|downgraded|merged|removed|line-corrected", | ||
| "challenger_reason": "<why this finding was kept/changed/removed>", | ||
| "original_line": "<original line number before correction, only when challenger_action is line-corrected>" |
There was a problem hiding this comment.
HIGH — original_line isn't stripped by the orchestrator and violates the strict output schema; it's also silently dropped whenever line-correction co-occurs with another challenger action
Two compounding issues on this field:
- Not stripped, fails schema.
skills/pr-review/SKILL.mdstep 6d.4 (unchanged by this PR) instructs: "Strip the challenger-specific fields (challenger_action,challenger_reason) before merging" — it does not listoriginal_line.schemas/review-result.schema.json's finding definition isadditionalProperties: falsewith a fixed field list that does not includeoriginal_line. Any finding that goes through aline-correctedchallenger action will carry this field intofullsend-check-outputvalidation and fail it — peragents/review.md, "Schema validation failures trigger a harness retry iteration," so this wastes turns/cost on exactly the review path this PR is trying to make more common. - Can't represent composite outcomes.
challenger_actionis a single enum value (kept|downgraded|merged|removed|line-corrected), andoriginal_lineis populated "only whenchallenger_actionisline-corrected." If a finding is both line-corrected and downgraded/merged/removed in the same pass, the challenger can only report one action — so the auditable-correction record this PR added (to close a prior review's "no auditable artifact" finding) silently disappears whenever a correction co-occurs with any other adjudication.
Suggestion: Update SKILL.md step 6d.4 to also strip original_line (or whitelist fields to keep, rather than listing fields to drop, so future challenger-specific fields don't leak by default). Separately, decouple original_line from challenger_action — populate it whenever a line was corrected regardless of the primary action, or document explicit precedence for the composite case.
[verified] · flagged by 2/3 agents (Claude-coder, Claude-researcher), confirmed against current SKILL.md and schemas/review-result.schema.json
| finding with no line number is always better than one that points | ||
| to the wrong code. | ||
|
|
||
| **Scope-constraint carve-out:** If your scope constraint prohibits |
There was a problem hiding this comment.
HIGH — Carve-out lumps "small" scope (which permits reading source files) in with "trivial" (which doesn't), reintroducing the exact failure mode this PR fixes for small-scope PRs
This reads: "If your scope constraint prohibits reading source files (e.g. trivial/small), derive line numbers from the diff hunk headers... on a best-effort basis." But per skills/pr-review/SKILL.md's scope-constraint table (unchanged by this PR): trivial is "≤5 tool calls... Do NOT read... surrounding files" (does prohibit source reads), while small is "≤15 tool calls. Read the diff, linked issue, and up to 3 context files directly relevant to the change" — which explicitly permits reading source files, including in nearly all cases the very file a finding targets. As written, a small-scoped sub-agent (PRs under 20 changed lines) now has an explicit, sanctioned excuse to skip the mandatory read-and-verify procedure and fall back to less-accurate hunk-header arithmetic even when it has budget to just read the file — which is the same "mandatory verification vs. tool-call budget" contradiction the prior review round flagged as HIGH, just reintroduced for the small tier specifically by this new carve-out.
Suggestion: Scope the carve-out to trivial only, and for small scope require using one of the 3 permitted context-file reads to verify the line, falling back to hunk-header math only if that budget is exhausted.
[verified] · flagged by 2/3 agents (Claude-coder, Claude-researcher), confirmed against SKILL.md's scope-constraint table
|
422 is produced by another system, closing this will open again in the future. |
|
🤖 Finished Retro · ✅ Success · Started 2:16 PM UTC · Completed 2:29 PM UTC |
Retro: PR #197 — strengthen line number verification in review promptsWhat happenedPR #197 was a human-authored change (rh-hemartin + Claude co-author) that attempted to fix review agent line number accuracy by replacing advisory "re-read the file" guidance with a mandatory 4-step verification procedure across The PR went through 4 automated review runs (29490984206, 29502313148, 29560787322, 29561234734) and 2 human Review Squad rounds over 6 days before being closed without merging on 2026-07-22. Review quality gapThe automated review agent found only procedural/cosmetic issues across all 4 runs: a protected-path governance flag (MEDIUM) and a heading-level inconsistency (LOW). The human Review Squad found 11 unique substantive findings including:
The automated agent never evaluated whether the PR's fundamental thesis was correct, never verified cross-file consistency between the new procedures and existing scope constraints, and never checked whether referenced artifacts (judges, schema fields) existed. Evidence for existing open issues
What went well
Proposals filed
|
Summary
Motivation
Ref: fullsend-ai/fullsend#2687 — retro agent filed after run 28229558711 placed 3/4 inline comments at wrong line numbers on konflux-ci/architecture#367. That issue was closed after 3 days without its "next 10 runs" validation window elapsing.
The problem persists in functional tests. Recent
Functional Tests (review)runs in this repo all fail withgithub api: 422 Unprocessable Entitywhen submitting the review — consistent with line numbers outside valid diff hunk ranges:See also fullsend-ai/fullsend#2688 (harness-side fix for content-anchored comments).
Test plan
🤖 Generated with Claude Code