|
| 1 | +# Substantial PR |
| 2 | + |
| 3 | +> Use this template for PRs that change behavior, introduce new concepts, or would hurt if they broke. |
| 4 | +> |
| 5 | +> This template exists to keep reviewers from reverse-engineering context, risk, and intent from the diff. |
| 6 | +> |
| 7 | +> If filling this out feels heavy, that's usually a sign the PR is too large or still half-baked. |
| 8 | +> |
| 9 | +> Reviewers may block, close, or ask for this PR to be split if it's hard to reason about as a single unit. |
| 10 | +
|
| 11 | +--- |
| 12 | + |
| 13 | +## Reviewer Focus (Read This First) |
| 14 | + |
| 15 | +<details> |
| 16 | +<summary>What reviewers should focus on</summary> |
| 17 | + |
| 18 | +Where should reviewers spend most of their time? |
| 19 | + |
| 20 | +Call out: |
| 21 | +- The risky or non-obvious parts |
| 22 | +- Areas where you're least confident |
| 23 | +- The kind of feedback you actually want |
| 24 | + |
| 25 | +If your answer is "everything," explain why targeted review isn't possible. |
| 26 | + |
| 27 | +Limit this to 1-3 areas. |
| 28 | + |
| 29 | +</details> |
| 30 | + |
| 31 | +--- |
| 32 | + |
| 33 | +## Problem & Motivation |
| 34 | + |
| 35 | +<details> |
| 36 | +<summary>Why this exists</summary> |
| 37 | + |
| 38 | +Explain: |
| 39 | +- What problem this PR is solving |
| 40 | +- What's broken, fragile, or getting worse without it |
| 41 | +- Why this change is happening *now* |
| 42 | +- Links to issues, incidents, or prior discussion |
| 43 | + |
| 44 | +Keep this tight. Bullets are fine. |
| 45 | + |
| 46 | +</details> |
| 47 | + |
| 48 | +--- |
| 49 | + |
| 50 | +## What Changed (Concrete) |
| 51 | + |
| 52 | +<details> |
| 53 | +<summary>What actually changed</summary> |
| 54 | + |
| 55 | +List the concrete behavioral or structural changes in this PR. |
| 56 | + |
| 57 | +This should be a factual inventory, not a narrative. |
| 58 | +Prefer numbered bullets. |
| 59 | + |
| 60 | +If this list starts getting long, that's a signal the PR may need splitting. |
| 61 | + |
| 62 | +</details> |
| 63 | + |
| 64 | +--- |
| 65 | + |
| 66 | +## Design & Planning |
| 67 | + |
| 68 | +<details> |
| 69 | +<summary>How this approach was chosen</summary> |
| 70 | + |
| 71 | +- Link any design docs or notes that existed before or during implementation |
| 72 | +- If this didn't warrant upfront design, say why |
| 73 | +- Mention the most realistic alternatives you considered and why you didn't take them |
| 74 | +- If planning was lightweight, be explicit about that |
| 75 | + |
| 76 | +</details> |
| 77 | + |
| 78 | +- Planning artifacts: |
| 79 | +- Reviewed / approved by (only if there was a real review): |
| 80 | + |
| 81 | +--- |
| 82 | + |
| 83 | +## Self-Review |
| 84 | + |
| 85 | +<details> |
| 86 | +<summary>What you caught yourself</summary> |
| 87 | + |
| 88 | +Describe what changed *after* you reviewed your own diff end-to-end. |
| 89 | + |
| 90 | +This is not optional. |
| 91 | + |
| 92 | +</details> |
| 93 | + |
| 94 | +- Bugs caught: |
| 95 | +- Logic simplified: |
| 96 | +- Naming / terminology improved: |
| 97 | +- Dead or unnecessary code removed (or why none was): |
| 98 | + |
| 99 | +--- |
| 100 | + |
| 101 | +## Cross-Codebase Alignment |
| 102 | + |
| 103 | +<details> |
| 104 | +<summary>Related code you checked</summary> |
| 105 | + |
| 106 | +Show that you looked beyond just the files in this PR. |
| 107 | + |
| 108 | +- Terms you searched for to find related code or docs |
| 109 | +- Files or packages you reviewed but intentionally didn't change |
| 110 | +- Areas you left alone on purpose (and why) |
| 111 | + |
| 112 | +Focus on directly related domains. Don't boil the ocean. |
| 113 | + |
| 114 | +</details> |
| 115 | + |
| 116 | +- Search terms used: |
| 117 | +- Reviewed but unchanged: |
| 118 | +- Deferred alignment (with rationale): |
| 119 | + |
| 120 | +--- |
| 121 | + |
| 122 | +## Downstream & Consumer Impact |
| 123 | + |
| 124 | +<details> |
| 125 | +<summary>Who this affects and how</summary> |
| 126 | + |
| 127 | +Explain how this change impacts: |
| 128 | +- Callers |
| 129 | +- Readers |
| 130 | +- Operators |
| 131 | +- Future maintainers |
| 132 | + |
| 133 | +Call out: |
| 134 | +- Terminology or concepts that might confuse someone new |
| 135 | +- What you changed to reduce that confusion |
| 136 | + |
| 137 | +Point to actual diffs where possible. |
| 138 | + |
| 139 | +</details> |
| 140 | + |
| 141 | +- Public APIs affected: |
| 142 | +- Docs updated: |
| 143 | +- Naming decisions worth calling out: |
| 144 | + |
| 145 | +--- |
| 146 | + |
| 147 | +## Testing Evidence |
| 148 | + |
| 149 | +<details> |
| 150 | +<summary>How this was validated</summary> |
| 151 | + |
| 152 | +Explain: |
| 153 | +- How this was tested locally and/or in CI |
| 154 | +- What important behavior is *not* covered by tests |
| 155 | +- If this is wrong, what breaks first |
| 156 | + |
| 157 | +</details> |
| 158 | + |
| 159 | +- Testing performed: |
| 160 | +- Known gaps: |
| 161 | +- What reviewers have to reason about manually (and why): |
| 162 | + |
| 163 | +--- |
| 164 | + |
| 165 | +## Scope Reductions |
| 166 | + |
| 167 | +<details> |
| 168 | +<summary>What you intentionally didn't do</summary> |
| 169 | + |
| 170 | +List follow-ups you identified but explicitly deferred to keep this PR reviewable. |
| 171 | + |
| 172 | +Link issues where applicable and explain the tradeoffs. |
| 173 | + |
| 174 | +</details> |
| 175 | + |
| 176 | +- Follow-ups: |
| 177 | +- Why they were deferred: |
| 178 | + |
| 179 | +--- |
| 180 | + |
| 181 | +## Risk Analysis |
| 182 | + |
| 183 | +<details> |
| 184 | +<summary>How this could go wrong</summary> |
| 185 | + |
| 186 | +Call out: |
| 187 | +- Assumptions this PR relies on |
| 188 | +- Likely failure modes |
| 189 | +- Blast radius if it breaks |
| 190 | + |
| 191 | +</details> |
| 192 | + |
| 193 | +- Risk areas: |
| 194 | +- Mitigations or rollback options: |
| 195 | +- Named owner if this causes problems: |
| 196 | + |
| 197 | +--- |
| 198 | + |
| 199 | +## Pre-Review Checklist (Blocking) |
| 200 | + |
| 201 | +- [ ] I reviewed every line of this diff and understand it end-to-end |
| 202 | +- [ ] I'm prepared to defend this PR line-by-line in review |
| 203 | +- [ ] I'm comfortable being the on-call owner for this change |
| 204 | +- [ ] Relevant changesets are included (or explicitly not required) |
0 commit comments