Skip to content

Commit 7934968

Browse files
authored
feat(review-pr): emit and validate GitHub suggestion blocks (#24)
## Summary Adds GitHub one-click suggestion blocks to PR reviews, plus a validator that sanitizes them before posting. GitHub rejects an entire review (HTTP 422) if any one inline suggestion anchors to an invalid line, so the validator strips malformed suggestions while keeping the prose finding, so one bad suggestion cannot lose the whole review. ## What changed | Area | Change | | --- | --- | | Emission | The agent embeds a `suggestion` fenced block for findings with an exact fix on contiguous changed lines (`review-pr/agents/pr-review.yaml`, `refs/posting-format.md`) | | Validation | New `src/validate-suggestions` module parses addressable right-side diff lines and strips suggestions GitHub would reject | | Build and wiring | `tsup` entry plus `action.yml` staging; the validator runs before the `gh api` post; agent diff fallbacks redirect into `pr.diff` | ## Robustness (covered by unit tests) | Case | Handling | | --- | --- | | Path with a space (git trailing tab in the `+++` header) | tab stripped so the key matches `comment.path`; suggestion kept | | Non-ASCII path (git C-quoting) | header decoded back to UTF-8; suggestion kept | | Closing fence shorter than the opener | closer must be at least as long; longer blocks are not truncated | | Second opener before any closer | first block treated as unclosed and stripped, not merged | | Comment with no integer line anchor | fails closed (stripped) instead of 422-ing the review | | Prefetch failure | agent diff fallbacks write `pr.diff` so the validator reads the same diff | ## Validation | Check | Result | | --- | --- | | Unit tests | 649 passed (35 in `validate-suggestions`) | | Build (`tsup`) | pass | | `biome ci`, `tsc --noEmit` | pass | | `actionlint` | pass | Each robustness case was additionally reproduced against the implementation and confirmed handled via an independent adversarial verification pass.
1 parent 7938006 commit 7934968

8 files changed

Lines changed: 1104 additions & 6 deletions

File tree

review-pr/README.md

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -307,6 +307,33 @@ confidence: strong (92/100)
307307
<!-- docker-agent-review -->
308308
```
309309

310+
When a finding has a small, exact fix, the comment also carries a GitHub
311+
[suggestion block](https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/incorporating-feedback-in-your-pull-request)
312+
with the precise replacement code, so it can be applied in one click:
313+
314+
````markdown
315+
**[medium] Timeout is never applied**
316+
317+
`DefaultConfig()` returns a zero `Timeout`; set it before use.
318+
319+
```suggestion
320+
cfg := DefaultConfig()
321+
cfg.Timeout = 30 * time.Second
322+
```
323+
324+
confidence: moderate (68/100)
325+
326+
<!-- docker-agent-review -->
327+
````
328+
329+
GitHub is strict about the lines a suggestion can attach to: a suggestion
330+
anchored outside the diff, spanning more than one hunk, or on a deleted line
331+
makes the whole review fail (HTTP 422). Before posting, the agent runs a
332+
validator that checks every suggestion's line range against the diff and strips
333+
any malformed block (keeping the prose finding), so one bad suggestion can never
334+
lose the whole review. The validator is implemented and unit-tested in
335+
[`src/validate-suggestions/`](../src/validate-suggestions/validate-suggestions.ts).
336+
310337
When no issues are found:
311338

312339
```markdown

review-pr/action.yml

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -746,7 +746,7 @@ runs:
746746
echo ""
747747
echo "Execute the review pipeline:"
748748
echo ""
749-
echo "1. **Gather**: Read the pre-fetched \`pr.diff\` file. If missing, run \`gh pr diff https://github.com/${REPO}/pull/${PR_NUMBER}\` (use the full URL, not just the number)"
749+
echo "1. **Gather**: Read the pre-fetched \`pr.diff\` file. If missing, run \`gh pr diff https://github.com/${REPO}/pull/${PR_NUMBER} > pr.diff\` (use the full URL, not just the number) so the validator reads the same diff"
750750
echo '2. **Draft**: Delegate to `drafter` agent to generate bug hypotheses'
751751
echo '3. **Verify**: For each hypothesis, delegate to `verifier` agent'
752752
echo '4. **Post**: Aggregate findings and post review via `gh api`'
@@ -779,6 +779,13 @@ runs:
779779
run: |
780780
mkdir -p /tmp/refs
781781
cp "$ACTION_PATH"/agents/refs/*.md /tmp/refs/
782+
# Stage the suggestion-block validator where the agent can run it before
783+
# posting (the agent's working dir is the consumer repo, not the action).
784+
if [ -f "$ACTION_PATH/../dist/validate-suggestions.js" ]; then
785+
cp "$ACTION_PATH/../dist/validate-suggestions.js" /tmp/validate-suggestions.js
786+
else
787+
echo "::warning::validate-suggestions.js not found in dist — suggestion validation will be skipped"
788+
fi
782789
783790
# ========================================
784791
# RUN REVIEW using root docker-agent-action

review-pr/agents/pr-review.yaml

Lines changed: 34 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -75,12 +75,15 @@ agents:
7575
skip every step that reads from `REFS_DIR`. Never look in `/tmp/`.
7676
1. Get the diff and context:
7777
- **GitHub posting mode** (GITHUB_ACTIONS=true): The prompt contains a PR URL.
78-
a. Get the diff using this priority order (stop at the first one that works):
79-
1. Check if `pr.diff` exists in the working directory (pre-fetched by the CI workflow)
80-
2. `gh pr diff <URL>` using the full PR URL (e.g., `https://github.com/owner/repo/pull/123`).
78+
a. Get the diff into the file `pr.diff` using this priority order (stop at the
79+
first one that works). The drafter and the suggestion validator both read
80+
`pr.diff`, so a fallback MUST redirect into it, not just print to stdout —
81+
otherwise the validator sees no diff and strips every suggestion:
82+
1. Check if `pr.diff` already exists in the working directory (pre-fetched by the CI workflow)
83+
2. `gh pr diff <URL> > pr.diff` using the full PR URL (e.g., `https://github.com/owner/repo/pull/123`).
8184
NEVER use just the number — `gh pr diff <number>` fails in detached HEAD checkouts.
82-
3. `git diff $(git merge-base origin/main HEAD)...HEAD` — the repo is checked out with
83-
full history, so this always works as a last resort.
85+
3. `git diff $(git merge-base origin/main HEAD)...HEAD > pr.diff` — the repo is checked out
86+
with full history, so this always works as a last resort.
8487
Do NOT try `curl`, `gh repo clone`, or any other method. The three options above are sufficient.
8588
After obtaining the diff, log which method succeeded:
8689
```bash
@@ -224,6 +227,8 @@ agents:
224227
- **Inline comments** — every finding whose disposition is `inline`. Each comment uses
225228
the finding's `issue` (one-line summary), `details` (full explanation), `severity`,
226229
`category`, `file`, `line`, and a confidence label, e.g. `confidence: moderate (68/100)`.
230+
When the fix is a small, exact, contiguous line replacement, also embed a GitHub
231+
suggestion block with the exact replacement code (see "Suggestion blocks" below).
227232
- **Lower-confidence summary** — non-forced findings scoring below the inline threshold T
228233
(plus any pushed past the comment cap), listed under "Lower-confidence findings (not
229234
posted inline)" with their scores. Never silently drop these.
@@ -412,6 +417,29 @@ agents:
412417
in the body text. Each finding becomes an inline comment with `<!-- docker-agent-review -->`
413418
marker on its own line. Do NOT include the marker in console mode.
414419
420+
## Suggestion blocks (GitHub posting mode)
421+
422+
When an inline finding has a small, exact fix that replaces one or more contiguous
423+
changed lines, embed a GitHub suggestion block in the comment body containing the
424+
EXACT replacement code (not a description) so the author can apply it in one click:
425+
426+
````
427+
```suggestion
428+
<exact replacement for the anchored line(s)>
429+
```
430+
````
431+
432+
Before writing the block, read the current line(s) with `read_file`/`grep -n` and
433+
reproduce their indentation exactly — the block replaces the whole anchored range.
434+
Anchor on the RIGHT side only (added `+`/context ` ` lines); NEVER put a suggestion on
435+
a deleted line (`side: "LEFT"`). For a multi-line fix set `start_line`/`start_side`
436+
(`start_line < line`, same hunk). Emit a block ONLY for a clean mechanical drop-in;
437+
otherwise keep the suggestion as prose. See `/tmp/refs/posting-format.md` for the exact
438+
field layout. GitHub is strict: a suggestion anchored outside the diff or on a deleted
439+
line makes the WHOLE review fail (422). The mandatory validation step in the posting
440+
template (`node /tmp/validate-suggestions.js /tmp/review_comments.json pr.diff`) strips
441+
any malformed suggestion block before posting — run it; do not skip it.
442+
415443
## Domain-Specific Review Output
416444
417445
When drafter findings reference a domain-specific guide (identifiable by
@@ -919,3 +947,4 @@ permissions:
919947
- shell:cmd=gh *
920948
- shell:cmd=git *
921949
- shell:cmd=grep *
950+
- shell:cmd=node *

review-pr/agents/refs/posting-format.md

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -66,6 +66,21 @@ jq --arg path "$file_path" --argjson line "$old_line_number" --arg side "LEFT" \
6666
/tmp/review_comments.json > /tmp/review_comments.tmp \
6767
&& mv /tmp/review_comments.tmp /tmp/review_comments.json
6868

69+
# For a MULTI-LINE suggestion (replacing lines start..end within one hunk), add
70+
# start_line and start_side. start_line < line, both on the RIGHT side:
71+
jq --arg path "$file_path" --argjson start "$start_line_number" --argjson line "$end_line_number" \
72+
--rawfile body /tmp/comment_body.md \
73+
'. += [{path: $path, start_line: $start, start_side: "RIGHT", line: $line, side: "RIGHT", body: $body}]' \
74+
/tmp/review_comments.json > /tmp/review_comments.tmp \
75+
&& mv /tmp/review_comments.tmp /tmp/review_comments.json
76+
77+
# Validate & sanitize suggestion blocks BEFORE posting. GitHub rejects the
78+
# ENTIRE review (HTTP 422) if any one suggestion anchors outside the diff or to a
79+
# deleted line, so this strips malformed suggestion blocks (keeping the prose
80+
# finding) so one bad suggestion can't lose the whole review. Safe to run even
81+
# when there are no suggestions. `pr.diff` is the pre-fetched diff in the workdir.
82+
node /tmp/validate-suggestions.js /tmp/review_comments.json pr.diff
83+
6984
# Defensive: remove any comments with empty bodies before posting
7085
jq '[.[] | select(.body | length > 0)]' /tmp/review_comments.json > /tmp/review_comments.tmp \
7186
&& mv /tmp/review_comments.tmp /tmp/review_comments.json
@@ -83,6 +98,47 @@ jq -n \
8398
The `<!-- docker-agent-review -->` marker MUST be on its own line, separated by a blank line
8499
from the content. Do NOT include it in console output mode.
85100

101+
# Suggestion Blocks (actionable fixes)
102+
103+
When an in-scope finding has a small, exact fix that REPLACES one or more contiguous
104+
changed lines, include a GitHub suggestion block in the comment body so the author can
105+
apply it in one click. Put the EXACT replacement code in the block — the verbatim lines
106+
that should replace the anchored range, never a description of the change:
107+
108+
````markdown
109+
**[medium] One-line issue summary**
110+
111+
Why this is wrong and what the fix does.
112+
113+
```suggestion
114+
cfg := DefaultConfig()
115+
cfg.Timeout = 30 * time.Second
116+
```
117+
118+
confidence: moderate (68/100)
119+
120+
<!-- docker-agent-review -->
121+
````
122+
123+
Because the comment body is written via a quoted heredoc (`<< 'EOF'`), the backticks and
124+
indentation inside the block are preserved verbatim — no extra escaping is needed.
125+
126+
Rules GitHub enforces (a violation makes the ENTIRE review fail with HTTP 422):
127+
- **Right side only.** A suggestion replaces right-side content, so anchor it on an added
128+
(`+`) or context (` `) line. NEVER attach a suggestion to a deleted line (`side: "LEFT"`).
129+
- **The anchor is the replaced range.** A single-line suggestion uses `line`; a multi-line
130+
suggestion uses `start_line`..`line` with `start_line < line` and `start_side: "RIGHT"`,
131+
and the whole range MUST stay inside ONE diff hunk.
132+
- **Match the real code.** Read the current line(s) with `read_file`/`grep -n` first and
133+
reproduce the existing indentation exactly — the block replaces the entire line range.
134+
- **One block per comment, fence closed.** Open with ` ```suggestion ` and close with ` ``` `.
135+
- **Only when it is a clean drop-in.** If the fix needs prose, edits elsewhere, or spans
136+
non-contiguous lines, describe it in prose instead — do not force a suggestion block.
137+
138+
The validator (`node /tmp/validate-suggestions.js …`, run before posting above) strips any
139+
suggestion block whose anchor breaks these rules and keeps the prose finding, but emit valid
140+
suggestions in the first place so the actionable fix survives.
141+
86142
# Comment Scope (REQUIRED)
87143

88144
Each comment must address a problem this PR **introduces** — one that would not exist if

0 commit comments

Comments
 (0)