Skip to content

feat(review-pr): emit and validate GitHub suggestion blocks#24

Merged
Sayt-0 merged 2 commits into
mainfrom
feat/review-pr-suggestion-blocks
Jul 1, 2026
Merged

feat(review-pr): emit and validate GitHub suggestion blocks#24
Sayt-0 merged 2 commits into
mainfrom
feat/review-pr-suggestion-blocks

Conversation

@Sayt-0

@Sayt-0 Sayt-0 commented Jun 29, 2026

Copy link
Copy Markdown
Member

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.

Comment thread src/validate-suggestions/index.ts Fixed
@Sayt-0 Sayt-0 force-pushed the feat/review-pr-suggestion-blocks branch from 14b9447 to 1f88080 Compare June 29, 2026 09:47
@Sayt-0 Sayt-0 requested a review from derekmisler June 29, 2026 09:49

@docker-agent docker-agent left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assessment: 🟡 NEEDS ATTENTION

}
if (close === -1) {
blocks.push({ start: i, end: lines.length - 1, closed: false });
break; // unclosed fence swallows the remainder; nothing left to scan

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[medium] findSuggestionBlocks stops scanning after the first unclosed block, silently dropping any closed suggestion blocks that follow it

When the body contains a closed suggestion block followed by an unclosed one (e.g. [closed][unclosed][…remaining…]), the outer loop breaks at line 261 as soon as the unclosed block is detected and sets end: lines.length - 1. This causes stripBlocks to treat everything from the unclosed opener to the end of the body as part of the unclosed block — including the content of any legitimate, properly-closed suggestion blocks that happen to come after it.

Trigger: a comment body shaped like:

```suggestion
const a = 1;

Some prose.

Suggested change
break; // unclosed fence swallows the remainder; nothing left to scan
const b = 2;
no closing fence here

The first block is valid and closed; the second is unclosed. findSuggestionBlocks records block 1 as {start:0, end:2, closed:true} and, when the inner loop finds no closer for block 2, pushes {start:4, end:N-1, closed:false} and breaks. The caller never sees block 1 as separate from block 2's range, so stripBlocks strips block 1's content as part of the unclosed range.

Impact: a valid, well-formed suggestion block after an unclosed one is silently dropped from the posted comment. The author's one-click suggestion is lost without any warning in result.issues.

Suggestion: instead of breaking out of the outer loop entirely, record the unclosed block and continue scanning from i + 1 (or from the index of the second opener when the inner loop broke on it). If "swallow to end of document" semantics are intentional, add a test for [closed][unclosed][closed] to document and assert that behaviour explicitly.

Add one-click GitHub suggestion blocks to PR reviews, plus a validator that
sanitizes them before posting. GitHub rejects an entire review (HTTP 422) when
any one inline suggestion anchors to an invalid line, so the validator strips
malformed suggestions while keeping the prose finding, ensuring one bad
suggestion cannot lose the whole review.

Emission: the agent embeds a `suggestion` fenced block when a finding has an
exact fix for contiguous changed lines (review-pr/agents/pr-review.yaml,
refs/posting-format.md).

Validation (src/validate-suggestions): parse the diff's addressable right-side
lines and strip any suggestion whose anchor GitHub would reject (wrong side,
out of range, invalid multi-line range, unclosed fence). Built as a standalone
tsup entry and run before the gh api post.

Robustness, covered by unit tests:
- decode the "+++" header paths git delimits (trailing tab for names with a
  space, C-quoting for non-ASCII) so suggestions on those files are not dropped
- require a closing fence at least as long as the opener, and treat a second
  opener before any closer as the first block being unclosed
- fail closed when a comment has no integer line anchor
- redirect the agent diff fallbacks into pr.diff so the validator reads the
  same diff the agent used
@Sayt-0 Sayt-0 force-pushed the feat/review-pr-suggestion-blocks branch from 1f88080 to 57364f3 Compare June 29, 2026 10:06
@Sayt-0 Sayt-0 enabled auto-merge (squash) June 29, 2026 10:14
Resolve conflict in review-pr/agents/pr-review.yaml: keep the new suggestion-block emission instruction from this branch and adopt main's configurable inline-threshold (T) wording for the lower-confidence summary bullet (from #23).

@derekmisler derekmisler left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice work overall. the validation logic is solid and the test coverage is thorough. one non-blocking thing to consider: the node /tmp/validate-suggestions.js call in posting-format.md has no guard for when the file is absent (the staging step only emits a warning and skips the copy). if dist/validate-suggestions.js is ever missing, the node call exits non-zero and could abort the shell before the gh api post runs. an easy fix: [ -f /tmp/validate-suggestions.js ] && node /tmp/validate-suggestions.js /tmp/review_comments.json pr.diff || true. ci is green, approving!

@Sayt-0 Sayt-0 merged commit 7934968 into main Jul 1, 2026
8 checks passed
@Sayt-0 Sayt-0 deleted the feat/review-pr-suggestion-blocks branch July 1, 2026 18:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants