[infra] Add a guide for AI-assisted PR implementation descriptions - #1073
[infra] Add a guide for AI-assisted PR implementation descriptions#1073weiqingy wants to merge 2 commits into
Conversation
Add contribution-guides/ai-assisted-pr.md, defining the Implementation Description a non-trivial, AI-assisted code change owes its reviewer: when the format applies, the two-stage review it feeds, how the seven fields fold into the pull request template, which detail belongs in a collapsed block, and what not to write. Point at it from AGENTS.md, which also gains its first reference to .github/CONTRIBUTING.md, and add one Implementation Description heading to the pull request template for the three fields with no existing home. Exclude contribution-guides/* in tools/.rat-excludes. The directory is new and no existing rule covers it; without the entry the license check fails with one unknown license. Generated-by: Claude Code 2.1.251 (Claude Opus 5)
wenjin272
left a comment
There was a problem hiding this comment.
Thank you for putting this guide together and for grounding it in the experiments from #952 and #965. The direction looks useful and should make AI-assisted changes easier to review. I left four comments about keeping the applicability consistent, avoiding a naming collision in the template, preserving coverage for mixed PRs, and clarifying where the canonical description lives. Thanks again for the thoughtful work on this.
|
|
||
| ### Implementation Description | ||
|
|
||
| <!-- For non-trivial code changes: interaction decisions, behavioral contracts, and failure behavior. See `contribution-guides/ai-assisted-pr.md`. Leave empty for a change whose diff is already prose, such as documentation, comments, or site content. --> |
There was a problem hiding this comment.
The guide and AGENTS.md scope this format to PRs whose implementation is largely AI-assisted, but this template comment applies it to every non-trivial code change. The heading itself also remains visible when left empty, so non-applicable PRs would still render a blank section. Could we keep the scope consistent here and ask authors to remove the whole section when it does not apply? If the intent is instead to cover every non-trivial code change, the guide and AGENTS.md should be broadened to match.
There was a problem hiding this comment.
Yes, narrowed. The comment now reads:
<!-- For a non-trivial code change whose implementation is largely AI-assisted: interaction decisions, behavioral contracts, and failure behavior. See `contribution-guides/ai-assisted-pr.md`. Remove this heading and this comment otherwise. -->
The guide says the same rule from the other side now. A prose diff uses the template as it stands and removes the heading. This PR is a prose diff, so I removed the section from its own body too.
Your point sent me looking, and the same broad wording was in two more spots. The guide's When this applies opened with Non-trivial code changes, and AGENTS.md said a non-trivial code change owes its reviewer. I narrowed both. Was that the scope you had in mind, or were you thinking of the template alone?
| | `Tests` | the contracts-to-tests table | | ||
| | `API` | compatibility impact | | ||
|
|
||
| Three of the seven fields have no existing home, and one new `### |
There was a problem hiding this comment.
Implementation Description currently names both the complete seven-field description distributed across the PR body and this template subsection that carries only three fields. That makes it easy for an author or checking agent to treat the subsection as the complete description. Could we keep Implementation Description as the name of the overall artifact and give the subsection a distinct title, such as Behavioral Semantics? That would also let this paragraph state the durable placement rule without the historical no existing home / new heading wording.
There was a problem hiding this comment.
Yes to both, and I used your name. The template section is now ### Behavioral Semantics, so Implementation Description only means the whole seven-field account.
The paragraph no longer describes what this PR adds:
Behavioral Semanticsis the one section that exists for this format; the other four fields fold into sections the template already had.Implementation Descriptionnames the whole seven-field account, never one section of it.
One knock-on I would like your read on. The #952 row under Worked examples used to name the heading, but #952 was written before the template had that section, so the name no longer matches. I changed it to grouped under a single heading of their own. Does that still carry what the row is there to show, or would naming the old heading and saying it is historical be clearer?
| the reviewer then has to keep consistent with the first. Use the repository | ||
| template as it stands. | ||
|
|
||
| When only part of a PR qualifies, describe that part and nothing else. Say |
There was a problem hiding this comment.
Since the guide later defines the whole PR body as the description, describe that part and nothing else can be read as telling authors of mixed PRs to omit documentation, build, or other non-qualifying changes from the ordinary template sections. Could we instead say to apply the seven fields only to the qualifying part, identify that scope in one line, and continue describing the rest of the diff through the repository template as usual?
There was a problem hiding this comment.
Yes, that reading is there and I did not mean it. Reworded to your suggestion:
When only part of a PR qualifies, apply the seven fields to that part alone, and say in one line which part of the diff they cover, so a reviewer does not read the omission as a gap. Describe the rest of the diff through the repository template as usual.
| of the diff. One to two screens. A reviewer should be able to decide how to | ||
| review without clicking anything. A `<details>` block relocates detail and | ||
| is not headroom; if the collapsed material grows past a screen of its own, | ||
| move it into a PR comment. |
There was a problem hiding this comment.
Earlier this section says that the description lives in exactly one place, in the PR body, while this sends part of it to a PR comment without defining how the two locations stay connected. Could we clarify that the seven required fields remain canonical in the body and that only supplementary evidence may move to a single comment linked from the body?
There was a problem hiding this comment.
Yes, the two sentences did contradict each other. Now:
A
<details>block relocates detail and is not headroom; if the collapsed material grows past a screen of its own, move that supporting evidence into a single PR comment and link to it from the body. The seven fields stay in the body either way; only the evidence behind them moves.
…Semantics The pull request template comment applied the Implementation Description format to every non-trivial code change, while the guide and AGENTS.md scoped it to changes whose implementation is largely AI-assisted. Narrow the comment to match, and tell authors to remove the heading when it does not apply so an out-of-scope pull request renders no empty section. The guide's own applicability section and the AGENTS.md pointer carried the same broad wording and are narrowed with it. Rename the template section to Behavioral Semantics. Implementation Description named both the whole seven-field account and the subsection carrying three of them, which invites a reader to mistake the subsection for the whole. The name now belongs to the account alone. State the placement rule in terms of what the template holds rather than what this change adds to it. Apply the seven fields to the qualifying part of a mixed pull request and describe the rest through the repository template, rather than describing the qualifying part alone. Keep the seven fields in the pull request body when collapsed material overflows into a comment, and link that comment from the body. Generated-by: Claude Code 2.1.258 (Claude Opus 5)
|
Thanks for the review. All four are fixed in The open point in the description is still open. Runtime flow ships as always present with its depth varying, rather than switching on above some size. I left it as it is this round. Let me know if you’d rather make it conditional. It should be a one-clause change. WDYT? |
Linked issue: #894
Purpose of change
A pull request whose implementation is largely AI-assisted gives its reviewer a diff and a summary, and the reviewer then rebuilds the runtime flow, the behavioral contracts and the failure paths from the diff. The tooling that produced the change had all of that and threw it away. Nothing in the repository says a contributor should write it down, what it should cover, or where it goes.
This adds
contribution-guides/ai-assisted-pr.md, which says when the format applies, the two-stage review it feeds, how its seven fields fold into the existing pull request template, which detail belongs in a collapsed block, and what not to write.AGENTS.mdand the pull request template point at it.Two notes on scope. The guide describes what each review stage is accountable for and deliberately ships no reviewer-side agent prompt. And it names no line count anywhere: runtime flow varies in depth rather than in presence, and the two linked examples carry the calibration instead of a number.
One open point worth a second opinion. The guide treats runtime flow as always present with its depth scaling, rather than as a field that switches on above some size. A threshold would need either a number, which the samples do not support, or a qualifier a contributor cannot evaluate. If the conditional shape is preferred, it is a one-clause change.
This is an initial version and the guide says so. It is meant to be refined as more examples accumulate.
Tests
No code changes, so no unit tests.
./tools/build.shand./tools/ut.shread none of the four files../tools/check-license.shwas run twice by design. Without thetools/.rat-excludesentry it exits 1, reporting one unknown license against the new file. With the entry it exits 0, classifying 967 files as Standards and 967 as Apache Licensed with zero unknown licenses. The first run is what shows the entry is necessary rather than copied from the neighbouring rule.Every repository-relative path and both linked pull request URLs were resolved by hand, case-exactly. No link checker runs in CI, so a wrong path would not otherwise be caught.
API
No public API change. Four files: one new guide, a pointer section in
AGENTS.md, one newBehavioral Semanticsheading in.github/PULL_REQUEST_TEMPLATE.md, and one line intools/.rat-excludes.The template heading is additive and optional. Its comment scopes it to a non-trivial code change whose implementation is largely AI-assisted and says to remove the heading otherwise, so a pull request outside that scope renders no empty section.
AGENTS.mdalso gains its first reference to.github/CONTRIBUTING.md, which it had never pointed at.Documentation
doc-neededdoc-not-neededdoc-includedWas this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Code 2.1.251 (Claude Opus 5)