Skip to content

feat: post a run's new inline comments in one review - #83

Merged
brentvatne merged 6 commits into
mainfrom
tsapeta/inline-single-review
Oct 22, 2026
Merged

brentvatne merged 6 commits into
mainfrom
tsapeta/inline-single-review

Conversation

@tsapeta

@tsapeta tsapeta commented Oct 20, 2026 •

Copy link
Copy Markdown
Contributor

Why

Each new inline comment goes out as its own POST /pulls/{n}/comments, and GitHub turns every such comment into its own review. On expo/expo#51900 a run with three findings put three empty "expo-bot reviewed" entries on the PR, and watchers got three notifications.

How

The inline sync now posts all of a run's new threads in one POST /pulls/{n}/reviews with event: "COMMENT" and the reviewed head as commit_id. GitHub requires a body for a COMMENT review, so it carries a short fixed line that points at the summary comment. The review response doesn't list its comments, so the sync reads every page of GET /pulls/{n}/reviews/{id}/comments (inline.maxComments has no upper bound) and maps each one to its finding through the inline marker. Patches, stubs and deletes stay per comment, since they edit existing threads and create no review. It still never approves or requests changes, which is what ROADMAP §1 planned.

GitHub rejects the whole review with a 422 if it refuses one comment, and then creates nothing, so only a 422 falls back to one comment per thread, which is today's behavior. A 5xx or a dropped connection can come after GitHub created the review, so those aren't retried: the threads count as failed, the findings render in full in the main comment, and the next run creates whatever is missing. A rate limit still aborts the rest of the sync. A review that was created but came back without a usable id is not posted again, since that would duplicate every comment; those findings render in full in the main comment until the next run links them.

LLP 0008 said the reporter never touches the review-thread or reviews APIs, which hasn't been true since inline comments landed. It now has an "Inline Threads" section and says the reporter never approves or requests changes. The README and the templates/config.jsonc note said the first run sends one notification per inline comment; both now describe the single review.

Test Plan

bun test (801 pass), bun run typecheck, bun run lint, bun run fmt:check and bun run llp:check all pass. The new tests run the sync against a fake gh on PATH: one review with every new comment and the right payload, the fallback to single comments on a 422, no retry after a 502 or a rate limit, no repost when the review has no id or its comments can't be read, and links for a review with more than one page of comments. I haven't run it against GitHub yet.

Follow-up commit (8c86a3e)

From a multi-model review of this PR:

  • Why a POST failed now comes from GitHub's response body. gh api prints only the top-level message (Validation Failed (HTTP 422)) to stderr; errors[] goes to stdout. Both inline POSTs now run with check: false, and ghApiFailure keeps the body's distinct messages, each capped.
  • GitHub's spam throttle stops the sync. Its 422 says "submitted too quickly", and it now counts as a rate limit. That aborts the sync on the review POST and inside the one-by-one fallback, which before kept posting every remaining comment and then ran the stubs and deletes.
  • A pending review on the posting account no longer triggers the fallback. GitHub allows one pending review per account per PR. The new threads fail with a message saying to submit or delete it; stubs and deletes still run.
  • Reading back the review's comments goes page by page. A failed page keeps the links read before it. A rate limit there now stops the remaining stubs and deletes.
  • New tests:
    • the review payload's comment bodies;
    • a run that creates threads and deletes a retired one;
    • a read-back rate limit, and a failed later page;
    • pending-review and spam 422s;
    • a rate limit while patching, which skips the review;
    • spam during the fallback;
    • ghApiFailure itself.
  • Docs. README, the config template and LLP 0008 now say one review per scope in per-scope mode, and that the review body stays after its threads are deleted (GitHub can't delete a submitted review). The GitHub and gh claims that haven't been checked against a live PR are tagged [inferred].

bun test (810 pass), typecheck, lint, fmt:check and llp:check pass. Still not run against GitHub.

Follow-up commit (dd543a7): empty review body, observed GitHub behavior

This replaces the "GitHub requires a body" sentence in How. A probe against a private scratch PR showed:

  • No body is needed. GitHub accepts a COMMENT review with comments and an empty or missing body, despite its docs. The review is now posted with body: "". A submitted review can't be deleted, so the fixed body used to stay on the PR after its threads were torn down; now nothing does.
  • The batch is all or nothing. A review with one valid and one unanchorable line came back 422 Line could not be resolved, and neither comment was created.
  • gh splits the error. stderr is only gh: Unprocessable Entity (HTTP 422); the errors[] messages are only on stdout, which is why ghApiFailure reads it.
  • A pending review blocks both POSTs. The review POST and a single-comment POST both fail with "…one pending review per pull request"; the comment doesn't join the pending review.

LLP 0008 now tags these [observed]. The spam throttle's text is still from reports, not reproduced. bun test (810 pass), typecheck, lint, fmt:check and llp:check pass.

Each new inline comment went out as its own `POST /pulls/{n}/comments`, and
GitHub makes every such comment its own review. A run with three findings
put three "reviewed" entries and three notifications on the PR.

The inline sync now posts all of a run's new threads in one
`POST /pulls/{n}/reviews` with `event: "COMMENT"`, the reviewed head as
`commit_id`, and a short fixed body, which GitHub requires for a COMMENT
review. The response doesn't list the comments, so the sync reads the
review's comments and maps each one to its finding by the inline marker.
Patches, stubs and deletes stay per comment and create no review.

GitHub rejects the whole review when it refuses one comment, so any failure
other than a rate limit falls back to one comment per thread. A review that
was created but returned no usable id is not posted again; its findings
render in full in the main comment until the next run links them.
@github-actions

github-actions Bot commented Oct 20, 2026 •

Copy link
Copy Markdown

🤖 AI code review

Decision: Ready for human review

Overall PR risk: Medium. This change modifies inline comment posting to use one COMMENT review instead of individual POSTs, affecting only repositories with inline.enabled. The new behavior adds a dependency on GitHub's empty review body acceptance and correct 422/rate-limit classification to trigger the fallback. Most plausible breakage: edge cases in failure classification cause fallback to per-comment posting for one run, or the empty review body stops working after a GitHub API change. Blast radius is contained to inline-enabled repos, the main comment still renders unlinked findings in full, and rollback restores prior behavior directly.

🔗 Review setup:

  • This PR changes code the reviewer prompts cite (src, src/tests, templates, templates/config.jsonc). Check that the guidance quoting it is still correct.

No findings.


This review is advisory — it never blocks a merge and never auto-approves.

…y page of the review

A review POST that failed with a 5xx or a dropped connection fell back to
posting each comment, but GitHub can have created the review anyway, so the
fallback could post every thread twice. Only a 422, which means GitHub
created nothing, now falls back. Any other failure counts the threads as
failed and does not retry; the main comment renders those findings in full,
and the next run creates the threads that are still missing.

`inline.maxComments` has no upper bound, so the read of the new review's
comments now goes through every page, like the other review-comment reads.
@tsapeta
tsapeta marked this pull request as ready for review October 20, 2026 22:28
@tsapeta
tsapeta requested a review from brentvatne October 20, 2026 22:29
- Both inline POSTs run with check: false and throw ghApiFailure, which keeps
  the response body's distinct messages (each capped) next to gh's one-line
  stderr, since gh prints errors[] only to stdout.
- The spam throttle ("submitted too quickly") counts as a rate limit, so it
  aborts the sync for the review and inside the one-by-one fallback. A
  pending review on the posting account fails the new threads without a
  fallback; stubs and deletes still run.
- Reading back the review's comments goes page by page: a failed page keeps
  the links before it, and a rate limit there stops the remaining mutations.
- Tests: payload bodies, a mixed create/delete run, read-back rate limit and
  partial pages, pending-review and spam 422s, a rate limit while patching,
  spam during the fallback, and ghApiFailure.
- README, template and LLP 0008: one review per scope in per-scope mode, the
  review body stays after its threads are deleted, and GitHub and gh
  behavior that hasn't been checked live is tagged [inferred].
…bserved behavior

A probe against a private scratch PR showed that GitHub accepts a COMMENT
review with comments and an empty or missing body, despite its docs. A
submitted review can't be deleted, so the fixed body stayed on the PR after
its threads were torn down; an empty one leaves nothing.

The same probe confirmed that one unanchorable line makes GitHub refuse the
whole review and create nothing, that gh prints errors[] only to stdout, and
what both POSTs return while the account has a pending review. LLP 0008 now
tags those [observed].
@brentvatne
brentvatne merged commit 763fc70 into main Oct 22, 2026
3 checks passed
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.

2 participants