Let's see if this improves the quality of Copilot's reviews once it's merged.

Would it apply if we asked for Copilot's review here? 😄 (Maybe with some sample code changes if needed)

I'm particularly interested in the point about "NEVER produce a review body". Will it abide that? Is it actually desirable? I mean, if a human would have reasons to leave a top-level review comment, why wouldn't the same apply to an AI agent?

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: mikachan <[email protected]>
Co-authored-by: aduth <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

Would it apply if we asked for Copilot's review here?

I don't think so, I think Copilot works off what is in trunk. From the docs here:

When reviewing a pull request, Copilot uses the custom instructions in the base branch of the pull request. For example, if your pull request seeks to merge my-feature-branch into main, Copilot will use the custom instructions in main.

Hmm, I wonder if we could open a PR using this branch as the base branch? 🤔

Is it actually desirable? I mean, if a human would have reasons to leave a top-level review comment, why wouldn't the same apply to an AI agent?

In the Copilot reviews I've seen, the top-level review comment usually restates what the PR is doing rather than leaving a useful review comment. Maybe we could soften the rule to something like: "Only write a review body when the finding spans multiple files and can't be attached to a specific line."?

I opened another branch using this PR as the base, at #78643, and requested a review from Copilot. The changes included a deliberately-flawed change to math.js.

The suppression rules around style/edges/tests appear to be working:

• short variable naming (w / h)
• theoretical edge cases (null rect, NaN, negative dimensions)
• missing tests for the new functions
• duplicating right - left between the two new helpers

(I'm not highly opinionated about any of these rules by the way, this was all based on the sample of Copilot reviews I looked at.)

But it still left a top-level comment restating what the PR is doing 😅 I don't think this is the most important rule, but it's frustrating that it just ignored it. I think we'd be best tweaking it to attempt to stop it from restating what the PR is doing: "Keep any review body to a single sentence stating only what was reviewed, not a restatement of the diff. Put all findings in inline comments." What do you think?

I think we'd be best tweaking it to attempt to stop it from restating what the PR is doing: "Keep any review body to a single sentence stating only what was reviewed, not a restatement of the diff. Put all findings in inline comments." What do you think?

I kinda liked what you had suggested previously with "the finding spans multiple files and can't be attached to a specific line." Though I agree in general about trying to discourage it from restating the diff and preferring inline comments where possible.

I'm a little hesitant about a couple of the specific suggestions you flagged:

• Discouraging tests: What do we mean by "out of scope", and will the AI agent understand that? In general I would think we should still value test coverage.
• Discouraging refactors: Given AI's propensity toward accumulation of code, I would think something like suggesting refactoring of duplicate code would be something we'd want to invite.

I'm not sure what the context behind these was if there's some examples of it behaving poorly around these suggestions. Do you have some examples you could share? I feel like these are the exact kinds of problems that unregulated AI contributions would contribute toward reducing code quality in ways that I'd be happy for Copilot to catch at review time.

I kinda liked what you had suggested previously with "the finding spans multiple files and can't be attached to a specific line." Though I agree in general about trying to discourage it from restating the diff and preferring inline comments where possible.

Yeah I agree, I think I like that best so far. I've updated that line to: "Keep the top-level review body empty unless a finding genuinely spans multiple files and can't be attached to a specific line. Put findings in inline comments wherever possible, and never use the review body to restate what the PR is doing." in 7c40097.

Discouraging tests: What do we mean by "out of scope", and will the AI agent understand that? In general I would think we should still value test coverage.

I'm not sure there were any strong examples for the tests. Maybe this one. I tried to look for reviews where there was pushback from either the PR author or any other reviewers, but even with pushback it doesn't always mean we don't want Copilot to never make similar suggestions.

Discouraging refactors: Given AI's propensity toward accumulation of code, I would think something like suggesting refactoring of duplicate code would be something we'd want to invite.

Here's an example mentioning duplicated code. But I agree that refactoring duplicate code is something we'd likely want to invite.

I've removed both of these rules for now.

Thank you for reviewing 🙇

Thanks @aduth! Let's see if it helps.

I think it's a great idea to have tailored instructions for Copilot reviews to make them more relevant and actionable.

It looks like we can get even more specific by adding path-specific instructions files, e.g. block-editor.instructions.md. So that might be worth trying out too, but I'm also very conscious of not adding too many AI-directive text files 😅

It looks like we can get even more specific by adding path-specific instructions files, e.g. block-editor.instructions.md. So that might be worth trying out too, but I'm also very conscious of not adding too many AI-directive text files 😅

I agree! A few ideas come to mind:

• With the workflows listed in #77209, there's a lot of overlap in the skill content and Copilot review guidance for "Code review of a design system contribution" (e.g. review guidance for code contributions within the packages/ui path)
• Review guidance for test files specifically to catch common issues. One that came up recently was effective teardown of content created during E2E tests to avoid conflicts between tests (#78503 is similar)