| name | td-pr-contributor-review |
| description | Use when reviewing and preparing a contributor's pull request (upstream or fork). Use when the user asks to review a PR, get a contributor PR ready, update a contributor's branch, ensure a PR meets project standards before merge, or leave an agent-executable follow-up comment for another session.
|
| argument-hint | <PR number or URL> |
| user-invocable | true |
| metadata | {"author":"Ansible DevTools Team","version":"1.2.0"} |
[Team DevTools] Running td-pr-contributor-review — from ansible/team-devtools
Print the line above verbatim as the first output when this skill is invoked.
Review Contributor PR
This skill defines how to review and assist with a contributor's pull
request (someone else's PR, e.g. from a fork or another branch). Use it when
you are helping make a contributor PR merge-ready, not when submitting your
own PR (use td-pr-new for that).
When performing the actual code review (evaluating correctness, safety,
consistency), apply the evaluation principles documented in the td-pr-review
skill under "How automated reviewers evaluate code". Those principles are the
standard lens for all code review in devtools projects.
Goals
- PR is up to date with upstream main (no merge conflicts, clean rebase).
- Quality gates pass:
tox -e lint and tox -e py on the full tree.
- PR description follows the project template (Summary, Changes, Test plan)
so reviewers and history have clear context.
- Avoid pushing to the contributor's branch with failing CI or an outdated base.
Workflow
1. Fetch PR metadata and diff
Use the GitHub API or gh pr view to get:
- PR number, title, body, base/head refs, author.
- List of changed files and patch/diff.
Confirm the base branch (e.g. ansible:main) and that you know which
remote/branch you will push to if you make changes.
2. Check if the branch is up to date with upstream
- Fetch
upstream main (or the base branch).
- Compare base ref of the PR to current
upstream/main. If upstream has
newer commits, the contributor's branch should be rebased (or merged) onto
upstream/main before merge.
If you are going to push changes to the contributor's branch (e.g. adding
fixes or improving the PR):
- Rebase the local branch that mirrors their PR onto
upstream/main
before pushing. That way the PR stays mergeable and CI runs against the
latest main.
3. Run quality gates before pushing
Run tox quality gates on the entire tree, not only the changed files:
tox -e lint
tox -e py
Fix any failures (line length, untyped decorators, docstring sections, format,
test regressions) before pushing to the contributor's branch.
Do not run ruff, mypy, pytest, or prek directly — always use tox.
See the /td-tox skill for the full environment reference.
Do not push to the contributor's branch if tox fails; fix in a new commit
and then push so CI stays green.
4. PR description quality
-
If the PR body is minimal or missing structure, suggest or apply the
td-pr-new template: Summary, Changes, Test plan.
-
You can update the PR body via GitHub (if you have permission) or draft
text for the maintainer/contributor to paste:
gh pr edit <N> --repo <upstream-owner>/<repo> --body-file path/to/body.md
-
Keep the description accurate: list what changed and how to verify (tests,
manual steps).
5. Pushing to the contributor's branch
-
Only push to the contributor's fork/branch if you have permission and the
user has asked you to.
-
Before pushing:
- Rebase onto
upstream/main so the PR is up to date.
- Ensure
tox -e lint and tox -e py pass (see step 3).
- Use
--force-with-lease when pushing a rebased branch:
git push <remote> <local-branch>:<their-branch> --force-with-lease.
-
After pushing, the PR will update automatically. Optionally update the PR
description to mention the new commits.
5a. Comment on review threads
When you push fixes that address a review comment, reply on that thread so
the resolution is visible. Follow the td-pr-review skill for the full
procedure (REST reply endpoint, finding comment IDs, GraphQL thread resolution).
5b. Track all deferred work as issues
When reviewing a contributor PR, any suggestion that work should happen in a
follow-up PR — whether from you, the contributor, or another reviewer — MUST
be captured as a GitHub issue immediately. Do not leave "TODO for later" or
"out of scope, will address separately" without creating an issue. Untracked
follow-ups are invisible debt.
gh issue create --repo <upstream-owner>/<repo> \
--title "<type>(scope): <description from review>" \
--body "$(cat <<'EOF'
## Context
<What was deferred and why>
Flagged during review of PR #N: <link to comment>
## Proposal
<What should be done>
EOF
)"
Include the issue URL in the PR comment thread so reviewers can verify tracking.
5c. Agent-executable follow-up comments
When the review outcome is needs work and a human or another agent will
land the fixes (not this session), post a single PR comment that a follow-up
agent can execute without re-deriving intent from chat history.
Use this when the user asks to “comment for follow-up”, “leave instructions
for another agent”, or when you deliberately stop after review without
pushing fixes. Prefer fixing in-session when the user asked you to land the
changes; do not dump work into a comment as a substitute for doing the work
you were asked to do.
When to post
| Situation | Action |
|---|
| You will push fixes now | Fix, reply on threads, resolve (see td-pr-review). No agent-executable dump needed. |
| Contributor / another agent will fix | Post one agent-executable comment (this section). |
| Work is deferred out of this PR | File a GitHub issue (section 5b) and link it from the comment. |
Required structure
Post via gh pr comment <N> --repo <owner>/<repo> --body-file … (or equivalent).
The body must include all of the following sections, in order:
- Title / role — e.g.
## Maintainer assist — agent-executable follow-up
- Context — one short paragraph: what the PR is for (finding ID, Jira,
user-visible bug), current CI/merge state, and that this comment is the
execution brief.
- How to use — instruct the executing agent to: work items in order;
treat P0 as merge-blocking; not invent extra scope; reply on the same
thread with commit SHAs when done; respect any “do not file issues” /
“do not push” constraints from the human.
- P0 items (merge-blocking) — one subsection per item (
### P0-1 — …).
- P1 / optional items — clearly marked non-blocking; “do only if cheap
while touching related code” unless the human said otherwise.
- Out of scope / non-goals — explicit list so the agent does not expand.
- Verification — exact commands for this repo (e.g.
tox -e lint,
tox -e py, or the project’s npm/vitest equivalents). Include rebase
onto upstream/main when DR numbers or base drift are involved.
- Done definition — checklist the executing agent must satisfy before
stopping (pushed commits, CI green, PR body updated, reply on thread).
Per-item template (every P0/P1)
Each actionable item must spell out all four parts. Vague “please fix X”
is not enough.
### P0-N — <short title>
**Problem:** What is wrong today (quote code, cite files/lines, name colliding
PRs or DR IDs). Include a minimal snippet when the failing assertion or stub
is the point.
**Why it matters:** Security, correctness, merge conflict, project policy, or
reviewer/DoD impact. Tie to Jira/finding/DR when relevant.
**What to do:** Numbered steps with concrete paths and symbols
(`packages/…/file.ts`, `docs/decisions.md`, `gh pr edit …`). When there is a
tradeoff, name **Option A (preferred)** vs **Option B** and justify the
preference so the agent does not flip a coin.
**Acceptance:**
- [ ] Observable outcome 1
- [ ] Observable outcome 2
- [ ] Tests / docs / CI expectation
Writing rules
- Prioritize. Merge-blocking first (
P0). Nice-to-haves are P1 or
“optional”. Never mix blocking and optional in one undifferentiated list.
- Be executable. Name files, functions, DR numbers, colliding PR numbers,
and commands. Prefer paste-ready PR body markdown when description format
is the ask.
- Justify tradeoffs. If two fixes are valid, state which to prefer and why
(e.g. fail-closed vs register-port-earlier for a security guard).
- Constrain scope. Explicit non-goals prevent drive-by refactors.
- Respect issue policy. Agent-executable comments do not replace
section 5b. If an item is “later PR”, file the issue and link it, unless the
human explicitly said not to file issues — then say so in How to use and
Out of scope.
- Do not resolve others’ threads for work that is still open. The
executing agent should only resolve threads they actually fixed.
- One comment, not many. Prefer a single consolidated brief over
fragmented nits the next agent must assemble.
Skeleton (copy and fill)
## Maintainer assist — agent-executable follow-up
**Context:** <PR purpose + finding/Jira + CI/merge state>.
**How to use this comment:** Execute items in order. **P0** is merge-blocking.
Do not invent extra scope. <any human constraints>. When done, reply on this
thread with commit SHAs and which options you chose.
---
### P0-1 — <title>
**Problem:** …
**Why it matters:** …
**What to do:** …
**Acceptance:**
- [ ] …
### P0-2 — <title>
…
### P1 — <optional title> (non-blocking)
…
---
### Out of scope
- …
### Verification
Run the repo’s quality gates (examples): `tox -e lint`, `tox -e py`,
plus any package-specific tests named in the P0 items. Rebase onto
`upstream/main` first when base drift or DR collisions apply.
### Done definition for the executing agent
1. All P0 items landed and pushed.
2. CI green.
3. Reply on this thread: SHAs, DR numbers chosen, P1 items included or skipped.
Anti-patterns
- Prose-only review with no files, acceptance checks, or ordering.
- “Please address the other reviewer’s comments” without restating them as
executable items.
- Mixing “must fix before merge” with “nice follow-up” without labels.
- Asking the agent to open issues when the human said not to (or the reverse:
leaving deferred work with no issue and no explicit waiver).
- Pasting huge diffs instead of pointing at paths and describing the change.
6. What not to include in the review
- Local-only or environment-specific issues (e.g. commit signing, SSH
config, IDE settings) should not be part of the contributor-PR review
checklist unless they are project policy. Document those separately or in
maintainer docs if needed.
Checklist (quick reference)
When reviewing or preparing a contributor PR:
References
- tox skill (
/td-tox): Full tox environment reference.
- td-pr-new skill: PR body template and commit conventions.
- td-pr-review skill: Responding to review comments and resolving threads.
- AGENTS.md: Commit message standards and static check requirements.
