Repository navigation
Conversation
GitHub rejects the whole review when one inline comment lands outside the diff. The fallback then posted each comment as a COMMENT review and the summary as an issue comment, which dropped APPROVE/REQUEST_CHANGES and fired an issue_comment event that cancelled the run under the per-PR concurrency group. Findings outside the diff now go into the review body, the fallback keeps the event in one review (then COMMENT, then an issue comment as last resort), and only comments that pass the job's trigger join the shared group.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review
Summary
Rewrites the review-posting path: findings are pre-filtered against each file's patch so out-of-diff findings go into the review body instead of getting the whole createReview rejected, posting logic is extracted into a tested submitReview fallback ladder, and the workflow's concurrency group is made run-unique for comments that don't themselves trigger a review (so bot/self/non-trigger comments can no longer cancel an in-flight review).
Risk
MEDIUM — touches the core review-posting path and the concurrency semantics of every issue_comment, but the new units are well covered (11/11 tests pass), degrade gracefully at each step, and I traced the GHA expression/precedence, folded-scalar YAML, and group IDs for every event type — they hold.
Issues
- 🟡 nit
src/submit-review.ts:31— the doc comment says the issue-comment fallback "fires an issue_comment event that can cancel this run", but this same PR's concurrency change makes that impossible (bot comments areassociation: NONE, so they get the run-unique suffix and their run is rejected by the jobifanyway) — the rationale is stale at merge time; the real costs left are the dropped verdict and the wasted run. - 🟡 nit
.github/workflows/ai-review.yml:18— the PR adds a second copy of the identical condition inREADME.md:62with a different constant (@berget reviewvs@berget), and the repo's own workflowifonly accepts@berget reviewwhilesrc/parse.tsand the README treat bare@bergetas a review trigger — drift between the three copies will bite whoever edits one of them; consider extracting the phrase difference per file or a comment here like the README one. - 🟡 nit
src/diff-lines.ts:34—splitByDiffmaps onlyf.filename; findings that cite a pre-rename path (GitHub also returnsprevious_filenamefor renames) silently fall to the review body instead of inline — graceful, so low priority.
Verified non-issues: the folded-scalar ${{ ... }}${{ ... }} fold inserts whitespace only inside the second expression's delimiters (no stray spaces in group IDs); for pull_request/workflow_dispatch events the suffix evaluates to '' so groups are unchanged; qualifying @berget review comments still share ai-review-<PR#> and cancel in-flight runs as intended; listChangedFiles uses github.context.repo, which is the same object as ctx.repo; commentableLines correctly skips -/\-rows, handles function-context hunk headers and multi-hunk numbering; the SHA marker survives every fallback path since headSha is always set there; reviewShaMarker extraction is unaffected by the extra body section.
Suggestions
.github/workflows/ai-review.yml— add the same "keep in step with the job'sif" note the README got, and state the actual trigger phrase (@berget review) next to it.src/submit-review.ts:31— update the comment to say the strayissue_commentrun is skipped by the jobif(verdict dropped, wasted run) rather than that it can cancel this run.
Architecture
Clean extraction of imperative fallback orchestration into pure, DI-shaped modules (submit-review.ts, diff-lines.ts) with honest unit tests — this push replaces the fragile "reject-then-post-N-individual-reviews" approach with deterministic pre-filtering, which is the right structure.
Code quality score
8.5/10 — readable, small pure functions with typed interfaces and thorough fallback-ladder tests; only dings are the duplicated trigger-phrase constant across YAML/README and the one stale doc comment.
Inline findings
Berget AI (berget/zai-org/GLM-5.3-Flash) | PR #15
| * to one review carrying the same event, with every finding in the body. A | ||
| * refused verdict ("Can not approve your own pull request") falls back to a | ||
| * COMMENT review. An issue comment is the last resort only: it drops the | ||
| * verdict, and it fires an issue_comment event that can cancel this run. |
There was a problem hiding this comment.
🟡 nit — Stale rationale: after this PR's concurrency change, the bot's own issue comment can no longer cancel this run (NONE association → run-unique group, and the job if rejects it); the remaining costs are the dropped verdict and the wasted run.
| group: ai-review-${{ github.event.pull_request.number || github.event.issue.number || github.run_id }} | ||
| group: >- | ||
| ai-review-${{ github.event.pull_request.number || github.event.issue.number || github.run_id }}${{ | ||
| github.event_name == 'issue_comment' && !(contains(github.event.comment.body, '@berget review') && |
| group: ai-review-${{ github.event.pull_request.number || github.event.issue.number || github.run_id }} | ||
| group: >- | ||
| ai-review-${{ github.event.pull_request.number || github.event.issue.number || github.run_id }}${{ | ||
| github.event_name == 'issue_comment' && !(contains(github.event.comment.body, '@berget') && |
| export function splitByDiff( | ||
| findings: Finding[], | ||
| files: ChangedFile[], | ||
| ): { inDiff: Finding[]; outside: Finding[] } { |
There was a problem hiding this comment.
🟡 nit — Findings citing a renamed file's previous_filename (also returned by listFiles) fall to the review body instead of staying inline; harmless but consider also mapping previous_filename.
| }: { body: string; event: ReviewEvent; comments: ReviewComment[]; fallbackBody: string }, | ||
| ): Promise<ReviewOutcome> { | ||
| try { | ||
| await api.createReview({ body, event, ...(comments.length > 0 ? { comments } : {}) }); |
There was a problem hiding this comment.
✅ good — Clean verdict-preserving fallback ladder; covered by 11 passing unit tests including every rejection tier.
| */ | ||
| export function commentableLines(patch: string | undefined): Set<number> { | ||
| const lines = new Set<number>(); | ||
| let next = 0; |
There was a problem hiding this comment.
✅ good — Correct hunk parsing: function-context headers handled, '-' rows and '\ No newline' markers skipped, multi-hunk numbering reset per header.
There was a problem hiding this comment.
Follow-up review
New changes: 1 commit (7786ff7), docs-only — rewords the fallback-cost comment in src/submit-review.ts and adds a "keep in step with the job's if" note above the workflow concurrency block. No behavior change.
Previous findings:
- ✅ nit #1 (
src/submit-review.ts:31stale fallback comment) — fixed: now scopes cancellation to "repos still on the old README concurrency group", which I verified against the pre-116d09f README (c86cfc2documented a plainai-review-<PR#>group, where a skipped bot run indeed cancels the in-flight review) — accurate. - 🟡 nit #2 (trigger-phrase drift,
ai-review.yml:18) — partially fixed: the YAML got the keep-in-step note, but it doesn't name the actual phrase and the underlying README (@berget, lines 62/76) vs workflow (@berget review) drift is untouched — README is unchanged in this commit. - ⏳ nit #3 (
src/diff-lines.ts:34ignoresprevious_filename) — still present, file untouched.
New findings: None — both hunks are comments, and I verified the new comment's claims (old-README group behavior; the YAML note matches the job if).
Done right in response to feedback: ✅ both the comment reword and the workflow note landed as suggested (minus the phrase callout).
Code quality score
8.5/10 — unchanged; docs-only commit resolves the one stale-comment ding.
Inline findings
Berget AI (berget/zai-org/GLM-5.3-Flash) | PR #15
| * to one review carrying the same event, with every finding in the body. A | ||
| * refused verdict ("Can not approve your own pull request") falls back to a | ||
| * COMMENT review. An issue comment is the last resort only: it drops the | ||
| * verdict, and in repos still on the old README concurrency group its |
There was a problem hiding this comment.
✅ good — Fixed: fallback-cost comment now matches actual behavior — cancellation only affects repos still on the old (pre-run-unique) README concurrency group.
| @@ -12,8 +12,14 @@ permissions: | |||
| pull-requests: write | |||
| issues: write | |||
|
|
|||
|
Looks good! Good find. |
Problem
When one inline finding points at a line outside the PR diff, GitHub rejects the whole review (
422 Line could not be resolved). The fallback then posted each finding as its ownCOMMENTreview and the summary as an issue comment. Two things broke:APPROVE/REQUEST_CHANGESnever reached GitHub, so a review with a blocker only left comments.issue_comment, which joined the README's recommendedai-review-<PR>concurrency group and cancelled the run during cleanup. The check went red even though the review was posted.Seen on berget-ai/guardrails-md#24:
Review event: APPROVE; 422 onREADME.md:81andadapters/opencode.test.ts:76(not in the diff); 2 empty COMMENT reviews + summary comment; run cancelled by the bot's own comment@bergetThe same comment-cancels-review pattern shows on berget-ai/guardrails-md#23.
Changes
src/diff-lines.ts): findings are checked against the RIGHT-side lines ofpulls.listFilespatches. Findings outside the diff go into the review body under "Findings not posted inline" instead of failing the review. Replaying the four findings above against that PR's patches splits them exactly as GitHub's 422 did.src/submit-review.ts): if a review is still rejected, retry as one review with the same event and every finding in the body, then as aCOMMENTreview (for a refused verdict such as approving your own PR), and only then as an issue comment.@bergetfrom an owner, member or collaborator) shares the PR's group. Every other comment, including the bot's own "skipped" and "no new commits" comments that mention@berget, gets a group of its own and cannot cancel a review in progress. Repos that copied the README workflow need to update theirconcurrencyblock.