|
| 1 | +--- |
| 2 | +name: pr-contributor-review |
| 3 | +description: > |
| 4 | + Use when reviewing and preparing a contributor's pull request (upstream or |
| 5 | + fork). Use when the user asks to review a PR, get a contributor PR ready, |
| 6 | + update a contributor's branch, or ensure a PR meets project standards before |
| 7 | + merge. |
| 8 | +argument-hint: "<PR number or URL>" |
| 9 | +user-invocable: true |
| 10 | +metadata: |
| 11 | + author: Ansible DevTools Team |
| 12 | + version: 1.0.0 |
| 13 | +--- |
| 14 | + |
| 15 | +# Review Contributor PR |
| 16 | + |
| 17 | +This skill defines how to review and assist with a **contributor's** pull |
| 18 | +request (someone else's PR, e.g. from a fork or another branch). Use it when |
| 19 | +you are helping make a contributor PR merge-ready, not when submitting your |
| 20 | +own PR (use `pr-new` for that). |
| 21 | + |
| 22 | +## Goals |
| 23 | + |
| 24 | +- PR is **up to date with upstream main** (no merge conflicts, clean rebase). |
| 25 | +- **Quality gates pass**: `tox -e lint` and `tox -e py` on the full tree. |
| 26 | +- **PR description** follows the project template (Summary, Changes, Test plan) |
| 27 | + so reviewers and history have clear context. |
| 28 | +- Avoid pushing to the contributor's branch with failing CI or an outdated base. |
| 29 | + |
| 30 | +## Workflow |
| 31 | + |
| 32 | +### 1. Fetch PR metadata and diff |
| 33 | + |
| 34 | +Use the GitHub API or `gh pr view` to get: |
| 35 | + |
| 36 | +- PR number, title, body, base/head refs, author. |
| 37 | +- List of changed files and patch/diff. |
| 38 | + |
| 39 | +Confirm the **base** branch (e.g. `ansible:main`) and that you know which |
| 40 | +remote/branch you will push to if you make changes. |
| 41 | + |
| 42 | +### 2. Check if the branch is up to date with upstream |
| 43 | + |
| 44 | +- Fetch `upstream main` (or the base branch). |
| 45 | +- Compare base ref of the PR to current `upstream/main`. If upstream has |
| 46 | + newer commits, the contributor's branch should be rebased (or merged) onto |
| 47 | + `upstream/main` before merge. |
| 48 | + |
| 49 | +If you are going to push changes to the contributor's branch (e.g. adding |
| 50 | +fixes or improving the PR): |
| 51 | + |
| 52 | +- Rebase the **local** branch that mirrors their PR onto `upstream/main` |
| 53 | + before pushing. That way the PR stays mergeable and CI runs against the |
| 54 | + latest main. |
| 55 | + |
| 56 | +### 3. Run quality gates before pushing |
| 57 | + |
| 58 | +Run tox quality gates on the **entire** tree, not only the changed files: |
| 59 | + |
| 60 | +```bash |
| 61 | +tox -e lint |
| 62 | +tox -e py |
| 63 | +``` |
| 64 | + |
| 65 | +Fix any failures (line length, untyped decorators, docstring sections, format, |
| 66 | +test regressions) before pushing to the contributor's branch. |
| 67 | + |
| 68 | +Do **not** run `ruff`, `mypy`, `pytest`, or `prek` directly — always use tox. |
| 69 | +See the `/tox` skill for the full environment reference. |
| 70 | + |
| 71 | +Do **not** push to the contributor's branch if tox fails; fix in a new commit |
| 72 | +and then push so CI stays green. |
| 73 | + |
| 74 | +### 4. PR description quality |
| 75 | + |
| 76 | +- If the PR body is minimal or missing structure, suggest or apply the |
| 77 | + **pr-new** template: Summary, Changes, Test plan. |
| 78 | + |
| 79 | +- You can update the PR body via GitHub (if you have permission) or draft |
| 80 | + text for the maintainer/contributor to paste: |
| 81 | + |
| 82 | + ```bash |
| 83 | + gh pr edit <N> --repo ansible/team-devtools --body-file path/to/body.md |
| 84 | + ``` |
| 85 | + |
| 86 | +- Keep the description accurate: list what changed and how to verify (tests, |
| 87 | + manual steps). |
| 88 | + |
| 89 | +### 5. Pushing to the contributor's branch |
| 90 | + |
| 91 | +- Only push to the contributor's fork/branch if you have permission and the |
| 92 | + user has asked you to. |
| 93 | + |
| 94 | +- Before pushing: |
| 95 | + |
| 96 | + 1. Rebase onto `upstream/main` so the PR is up to date. |
| 97 | + 2. Ensure `tox -e lint` and `tox -e py` pass (see step 3). |
| 98 | + 3. Use `--force-with-lease` when pushing a rebased branch: |
| 99 | + `git push <remote> <local-branch>:<their-branch> --force-with-lease`. |
| 100 | + |
| 101 | +- After pushing, the PR will update automatically. Optionally update the PR |
| 102 | + description to mention the new commits. |
| 103 | + |
| 104 | +### 5a. Comment on review threads |
| 105 | + |
| 106 | +When you push fixes that address a review comment, reply on that thread so |
| 107 | +the resolution is visible. Follow the **`pr-review`** skill for the full |
| 108 | +procedure (REST reply endpoint, finding comment IDs, GraphQL thread resolution). |
| 109 | + |
| 110 | +### 5b. Track all deferred work as issues |
| 111 | + |
| 112 | +When reviewing a contributor PR, any suggestion that work should happen in a |
| 113 | +follow-up PR — whether from you, the contributor, or another reviewer — **MUST** |
| 114 | +be captured as a GitHub issue immediately. Do not leave "TODO for later" or |
| 115 | +"out of scope, will address separately" without creating an issue. Untracked |
| 116 | +follow-ups are invisible debt. |
| 117 | + |
| 118 | +```bash |
| 119 | +gh issue create --repo ansible/team-devtools \ |
| 120 | + --title "<type>(scope): <description from review>" \ |
| 121 | + --body "$(cat <<'EOF' |
| 122 | +## Context |
| 123 | +
|
| 124 | +<What was deferred and why> |
| 125 | +
|
| 126 | +Flagged during review of PR #N: <link to comment> |
| 127 | +
|
| 128 | +## Proposal |
| 129 | +
|
| 130 | +<What should be done> |
| 131 | +
|
| 132 | +EOF |
| 133 | +)" |
| 134 | +``` |
| 135 | + |
| 136 | +Include the issue URL in the PR comment thread so reviewers can verify tracking. |
| 137 | + |
| 138 | +### 6. What not to include in the review |
| 139 | + |
| 140 | +- **Local-only or environment-specific issues** (e.g. commit signing, SSH |
| 141 | + config, IDE settings) should not be part of the contributor-PR review |
| 142 | + checklist unless they are project policy. Document those separately or in |
| 143 | + maintainer docs if needed. |
| 144 | + |
| 145 | +## Checklist (quick reference) |
| 146 | + |
| 147 | +When reviewing or preparing a contributor PR: |
| 148 | + |
| 149 | +- [ ] Fetched PR and know base/head and remotes. |
| 150 | +- [ ] Branch is up to date with upstream main (rebase if needed before push). |
| 151 | +- [ ] `tox -e lint` and `tox -e py` pass. |
| 152 | +- [ ] PR description has Summary, Changes, and Test plan (pr-new style). |
| 153 | +- [ ] If pushing to their branch: rebase onto upstream main, tox green, then |
| 154 | + `git push <remote> <local>:<their-branch> --force-with-lease`. |
| 155 | +- [ ] If you addressed a review comment: follow the `pr-review` skill to reply |
| 156 | + on the thread with explanation + commit SHA and resolve it. |
| 157 | + |
| 158 | +## References |
| 159 | + |
| 160 | +- **tox skill** (`/tox`): Full tox environment reference. |
| 161 | +- **pr-new** skill: PR body template and commit conventions. |
| 162 | +- **pr-review** skill: Responding to review comments and resolving threads. |
| 163 | +- **AGENTS.md**: Commit message standards and static check requirements. |
0 commit comments