andygrove opened a new pull request, #6483: URL: https://github.com/apache/datafusion-comet/pull/6483
## Which issue does this PR close? No issue. This is a change to the PR review skill, and the rationale is below. ## Rationale for this change `main` needs a single approval to merge. When a reviewer finds a correctness bug or a performance regression and posts it as a comment or as a **Comment** review, nothing stops the PR from merging over it. Any other committer's approval still satisfies branch protection, including one given before the review, and a PR with auto-merge armed can merge on its own. A **Request changes** review from someone with write access does block the merge until the same reviewer approves or someone dismisses the review ([GitHub docs](https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/approving-a-pull-request-with-required-reviews)). `review-comet-pr` never said which review state to use. It produced findings and suggested comments only. Run against a PR that already had an approval and armed auto-merge, the current skill found the performance regression and said it had to be fixed before merge, but recommended nothing that would actually hold the merge. It even offered a tracking issue as an alternative. ## What changes are included in this PR? All changes are in `.ai/skills/review-comet-pr/SKILL.md`. - A new "Request Changes for Correctness and Performance Regressions" section defines the two kinds of finding that must block the merge. The first is a correctness problem the PR introduces, which includes answering where Spark raises an error and reporting `Compatible()` over a known divergence. The second is a performance regression. A tracking issue for a later fix does not clear either one. The section says these reviews are submitted as **Request changes** and explains why. It also notes that requesting changes commits the reviewer to the re-review, because a later **Comment** review leaves the block in place. - The output format gains a **Review State** item: **Request changes**, **Approve** on a re-review whose blocking findings are all addressed, or **Comment** otherwise. - When the user tells the agent to post the review, it posts it in that state with `gh pr review --request-changes`. Before this change the skill declined to post even when asked, so the review state was left to whoever posted it. The area skills defer to `review-comet-pr` for the review bar and output format, so they need no change. ## How are these changes tested? I ran the skill before and after the change against synthetic review scenarios where the investigation was already done, and compared the review state it recommended. | Scenario | Before | After | | --- | --- | --- | | Wrong answer vs Spark reported as `Compatible()`, PR already approved, release cut the next day | no review state | Request changes (2 of 2 runs) | | 9% TPC-H regression in a crash fix, approved, auto-merge armed | no review state, offered a tracking issue instead | Request changes (2 of 2) | | Config wording and duplicated code only | no review state | Comment | | Re-review after the author fixed both blocking findings | not run | Approve, or dismiss the earlier review | | Refactor that moves code with a pre-existing Spark divergence | not run | Comment, plus a tracking issue for the bug (2 of 2) | | "Post the review for me" on the first scenario | declined to post | `gh pr review --request-changes` | `prettier --check` passes on the file. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
