|
| 1 | +# Pull Request Review Template |
| 2 | + |
| 3 | +Use this shape as a copyable reference for substantive PR reviews; GitHub does |
| 4 | +not auto-apply this file to review comments. Omit sections that do not add |
| 5 | +useful signal. Lead with confirmed findings; keep speculative notes out of the |
| 6 | +public review unless they are framed as a concrete open question. |
| 7 | + |
| 8 | +## Small PR Path |
| 9 | + |
| 10 | +For narrow docs, typo, test-only, or obvious local fixes, a short review is |
| 11 | +enough: |
| 12 | + |
| 13 | +```md |
| 14 | +LGTM after checking: |
| 15 | +- scope: |
| 16 | +- validation: |
| 17 | +- residual risk: |
| 18 | +``` |
| 19 | + |
| 20 | +Use the fuller structure below for larger, risky, multi-finding, or |
| 21 | +security-sensitive reviews. |
| 22 | + |
| 23 | +## Findings |
| 24 | + |
| 25 | +**<sub><sub></sub></sub> issue (test): Short issue title** |
| 26 | + |
| 27 | +- **Problem:** Concrete broken flow, contract, input, or risk. |
| 28 | + |
| 29 | +- **Impact:** Why this matters to users, CI, maintainers, data, security, or scale. |
| 30 | + |
| 31 | +- **Ask:** Smallest practical correction or decision the author should make. |
| 32 | + |
| 33 | +- **Location:** `path:line` |
| 34 | + |
| 35 | +## Open Questions |
| 36 | + |
| 37 | +- **question (scope, non-blocking): Short author question** Ask the concrete |
| 38 | + intent, scope, or tradeoff question. |
| 39 | + |
| 40 | +## Validation |
| 41 | + |
| 42 | +- Ran: |
| 43 | +- Not run: |
| 44 | +- Residual risk: |
| 45 | + |
| 46 | +## PR Hygiene |
| 47 | + |
| 48 | +- Target/template/checks: |
| 49 | +- Related, duplicate, or superseding context: |
| 50 | + |
| 51 | +## No Findings Variant |
| 52 | + |
| 53 | +```md |
| 54 | +## Findings |
| 55 | + |
| 56 | +none confirmed |
| 57 | + |
| 58 | +## Validation |
| 59 | + |
| 60 | +- Ran: |
| 61 | +- Not run: |
| 62 | +- Residual risk: |
| 63 | +``` |
| 64 | + |
| 65 | +## Legend |
| 66 | + |
| 67 | +- **Findings:** Verified, author-actionable issues that should be fixed or |
| 68 | + consciously accepted before merge. |
| 69 | +- **Priority badges:** The shields.io badges below are optional formatting for |
| 70 | + priority labels. Plain `P0`, `P1`, `P2`, or `P3` text is also acceptable when |
| 71 | + an external image dependency is undesirable or may not render. |
| 72 | + - **P0:** `` - |
| 73 | + release-blocking or actively dangerous. |
| 74 | + - **P1:** `` - |
| 75 | + serious bug, security risk, data-loss risk, or broken primary flow. |
| 76 | + - **P2:** `` - |
| 77 | + meaningful correctness, test, maintainability, or edge-case issue. |
| 78 | + - **P3:** `` - |
| 79 | + minor polish or low-risk cleanup. |
| 80 | +- **Intent labels:** |
| 81 | + - **`issue`:** A confirmed defect, regression, broken contract, or concrete |
| 82 | + risk. |
| 83 | + - **`suggestion`:** A non-blocking improvement that would make the PR clearer, |
| 84 | + safer, or easier to maintain. |
| 85 | + - **`nit`:** A tiny, non-blocking cleanup or style note. Use it only when the |
| 86 | + author can safely ignore it without changing the review outcome. |
| 87 | + - **`question`:** A real author-facing clarification about intent, scope, or |
| 88 | + tradeoffs. Do not use questions to hide an issue that should be stated |
| 89 | + directly. |
| 90 | + - **`LGTM`:** "Looks good to me." Use only when the review found no blocking |
| 91 | + issues, or when any remaining notes are clearly optional. |
| 92 | +- **Decorations:** Optional labels in parentheses that clarify the finding type, |
| 93 | + scope, or merge impact. |
| 94 | + - **`security`:** Auth, authorization, ownership, secrets, SSRF, injection, |
| 95 | + unsafe external input, or other trust-boundary concerns. |
| 96 | + - **`test`:** Missing, failing, misleading, brittle, or insufficient tests. |
| 97 | + - **`scope`:** PR scope, feature boundaries, unrelated churn, or work that |
| 98 | + should be split into a separate issue or PR. |
| 99 | + - **`ci`:** CI configuration, workflow failures, flaky checks, or validation |
| 100 | + signal quality. |
| 101 | + - **`api`:** Route, request/response, public function, schema, persistence, or |
| 102 | + integration contract changes. |
| 103 | + - **`docs`:** User-facing docs, contributor docs, examples, or comments that |
| 104 | + need to change with the code. |
| 105 | + - **`non-blocking`:** Useful feedback that should not prevent merge by |
| 106 | + itself. |
| 107 | +- **Finding fields:** |
| 108 | + - **Problem:** What is wrong, what contract is ambiguous, or what risk the PR |
| 109 | + introduces. |
| 110 | + - **Impact:** Why the problem matters in practical terms. |
| 111 | + - **Ask:** The smallest concrete fix, test, or decision requested from the PR |
| 112 | + author. |
| 113 | + - **Location:** The most useful repo-relative file and line reference for the |
| 114 | + finding, using `path:line`. |
| 115 | +- **Optional sections:** |
| 116 | + - **Open Questions:** Genuine scope or intent questions; omit when there are |
| 117 | + no real questions. |
| 118 | + - **Validation:** What the reviewer ran, what was intentionally not run, and |
| 119 | + what risk remains after review. |
| 120 | + - **PR Hygiene:** Target-branch, template, CI/check, duplicate, related-work, |
| 121 | + or superseding-PR notes. |
| 122 | +- **`none confirmed`:** Use only when no review-worthy findings were confirmed; |
| 123 | + still list validation gaps or residual risk when relevant. |
0 commit comments