Conversation
Signed-off-by: Derek Misler <derek.misler@docker.com>
|
⏭️ Review skipped — another review for this PR is already in progress (started 154s ago). Only one review runs at a time to avoid duplicate comments. The in-progress review will post its results when complete. |
| found on disk, use diff hunk headers instead. Never read the same file more than twice. | ||
| **Deleted lines**: If a finding is on a deleted line (`-` in the diff), the line | ||
| does not exist in the new file. Use `side: "LEFT"` with the old-file line number |
There was a problem hiding this comment.
[MEDIUM] No algorithm provided for computing old-file line numbers for deleted lines
The new guidance instructs verifiers to use side: "LEFT" with the old-file line number (from the -X,Y side of the hunk header) when a finding is on a deleted line, and explicitly says not to use grep. However, there is no corresponding explanation of how to derive that old-file line number from the hunk header. The existing new-file line-number algorithm (walk from +Z in the @@ -X,Y +Z,W @@ header, counting +/ lines) is already documented elsewhere, but no analogous -X-side algorithm is given for the deleted-lines case.
Without a deterministic procedure, agents may guess or use the wrong counter, producing misplaced GitHub review comments or 422 errors from the API.
Suggested addition: Add a brief note like: "To find the old-file line number, start from X in the hunk header and count only - and (context) lines down to the target deleted line."
⚠️ Verification was inconclusive (verifier returned empty response) — the drafter's analysis is surfaced for author evaluation.
| && mv /tmp/review_comments.tmp /tmp/review_comments.json | ||
|
|
||
| # For deleted lines (- in diff), add side: LEFT with the OLD file line number: | ||
| jq --arg path "$file_path" --argjson line "$old_line_number" --arg side "LEFT" \ |
There was a problem hiding this comment.
[MEDIUM] $old_line_number shell variable is undefined/unexplained in the jq snippet
The new jq snippet for deleted-line comments uses --argjson line "$old_line_number", but $old_line_number is never defined or set anywhere in the surrounding script context. The added/context-line snippet uses $line_number, which is at least consistent with other variables in the template.
If an agent or human copies this template without knowing to substitute a real integer, jq will receive an empty string for --argjson, causing a jq parse error or producing null for the line field — which the GitHub API will reject with a 422.
Suggestion: Either define old_line_number explicitly in the snippet (e.g., old_line_number=45 # from -X,Y side of hunk header) or add a comment explaining that this is a placeholder to be replaced with the actual old-file line number derived from the hunk header.
⚠️ Verification was inconclusive (verifier returned empty response) — the drafter's analysis is surfaced for author evaluation.
Related Issues
Closes: https://github.com/docker/gordon/issues/437
Summary
The drafter agent was generating unnecessarily verbose hypotheses due to using
sonnet-thinkingmodel. This PR switches tosonnetand adds guardrails to prevent the verifier from reading temporary diff chunk files and clarifies how deleted lines should be handled in comments.Changes
sonnet-thinkingtosonnetto reduce verbose output while maintaining analysis qualityread_multiple_filesfrom verifier toolset and added explicit instruction to only read source files, never/tmp/— prevents unnecessary diff chunk file readsside: "LEFT"with old-file line numbers when findings are on deleted lines (-in diff), since deleted lines don't exist in the new working treejqexamples for deleted line handlingTip
Comment
/reviewto trigger the PR Reviewer agent for automated feedback.Comment
/describeto generate a PR description.