Td Pr Contributor Review

[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:

  1. Rebase onto upstream/main so the PR is up to date.
  2. Ensure tox -e lint and tox -e py pass (see step 3).
  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

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:

1. Title / role — e.g. ## Maintainer assist — agent-executable follow-up
2. 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.
3. 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.
4. P0 items (merge-blocking) — one subsection per item (### P0-1 — …).
5. P1 / optional items — clearly marked non-blocking; “do only if cheap while touching related code” unless the human said otherwise.
6. Out of scope / non-goals — explicit list so the agent does not expand.
7. 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.
8. 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:

• Fetched PR and know base/head and remotes.
• Branch is up to date with upstream main (rebase if needed before push).
• tox -e lint and tox -e py pass.
• PR description has Summary, Changes, and Test plan (td-pr-new style).
• If pushing to their branch: rebase onto upstream main, tox green, then git push <remote> <local>:<their-branch> --force-with-lease.
• If you addressed a review comment: follow the td-pr-review skill to reply on the thread with explanation + commit SHA and resolve it.
• If leaving work for a contributor/another agent: post one agent-executable comment (section 5c) with P0/P1, problem/why/what/acceptance, non-goals, verification, and a done definition.
• Deferred out-of-PR work has a GitHub issue (section 5b) unless the human explicitly waived filing.

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.