Skip to content

Commit 572843b

Browse files
authored
docs(agents): clarify user-visible PR review feedback (#2910)
Agents will often give a ton of feedback, but it can be hard to quantify the impact of the issue the feedback is about. This change updates the PR review feedback template to be more human-readable, rooting any concerns in user-visible behavior when appropriate, and comparing the new behavior to old behavior so that PR authors can make a determination of whether they want to accept or reject the feedback. Signed-off-by: Kris Hicks <khicks@nvidia.com>
1 parent c399342 commit 572843b

1 file changed

Lines changed: 25 additions & 2 deletions

File tree

.agents/skills/review-github-pr/SKILL.md

Lines changed: 25 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -102,15 +102,38 @@ Read through the full diff (and the PR description if available). Produce a summ
102102
<short fenced code snippets that illustrate the most important changes -- max 3 snippets>
103103
104104
### Potential Concerns <- omit if none
105-
- <risk or issue worth discussing>
105+
- **<concise user-visible behavior>** — Before this PR, <affected persona>
106+
experienced <previous behavior>. With this PR, <new concerning behavior>, so
107+
<user-visible impact>. Details: `<file>:<line>`.
106108
```
107109

108110
**Guidelines for the summary:**
109111

110112
- **Overview**: State what changed and why. Pull context from the PR description if available.
111113
- **Key Design Decisions**: Focus on _why_ something was done a particular way, not _what_ changed. Include `file_path:line_number` references. Examples: choice of algorithm, new abstraction introduced, API contract change, migration strategy.
112114
- **Notable Code**: Include only the most instructive or surprising snippets. Keep each snippet under 15 lines. Always include the file path above the code block.
113-
- **Potential Concerns**: Only include if there are genuine risks — missing error handling, breaking changes, performance implications, security issues. Do not fabricate concerns.
115+
- **Potential Concerns**: Only include genuine risks that warrant a change or a
116+
deliberate accept/reject decision. Describe each concern in terms of observable
117+
behavior for the affected persona, such as a sandbox creator, sandbox user,
118+
operator, administrator, SDK consumer, or developer maintaining the system.
119+
Always compare the previous behavior with the new concerning behavior and state
120+
the resulting user-visible impact. Prefer the compact form: "Before this PR,
121+
`<persona>` experienced `<old behavior>`. With this PR, `<new behavior>`, so
122+
`<impact>`." Add only the minimum file and line references needed to substantiate
123+
the finding.
124+
- Use the PR base as the normal previous-behavior baseline. Review older history
125+
only when the change is fixing or extending an earlier feature and that history
126+
is necessary to explain the behavioral contract. In that case, describe the
127+
relevant transitions explicitly: "Before `<commit>`, ... After `<commit>`, ...
128+
With this PR, ...".
129+
- Translate internal failure modes and race conditions into what the affected
130+
person would observe. Internal implementation details belong in the trailing
131+
file and line references, not in place of the behavior description.
132+
- Do not assign P0/P1/P2 or similar priority labels. The behavioral comparison
133+
and impact should give maintainers enough context to accept or reject the
134+
suggested change.
135+
- Do not fabricate concerns or claim a behavioral regression without evidence
136+
for both the prior and proposed behavior.
114137
- **Agent infrastructure**: When the PR changes behavior, commands, or development workflows, use the `sync-agent-infra` maintenance map to check that related skills were updated. When it adds, removes, or renames skills or crates; changes workflow relationships or skill coverage; modifies issue or PR templates; or changes agent cross-references, apply the full consistency checklist. Report missing companion updates or drift under **Potential Concerns**.
115138

116139
## Step 5: Output

0 commit comments

Comments
 (0)