|
| 1 | +# PR Review Instructions |
| 2 | + |
| 3 | +Please read this in full and do not skip sections. |
| 4 | + |
| 5 | +## Working rule |
| 6 | + |
| 7 | +Skills execute workflow, maintainers provide judgment. |
| 8 | +Always pause between skills to evaluate technical direction, not just command success. |
| 9 | + |
| 10 | +These three skills must be used in order: |
| 11 | + |
| 12 | +1. `review-pr` |
| 13 | +2. `prepare-pr` |
| 14 | +3. `merge-pr` |
| 15 | + |
| 16 | +They are necessary, but not sufficient. Maintainers must steer between steps and understand the code before moving forward. |
| 17 | + |
| 18 | +Treat PRs as reports first, code second. |
| 19 | +If submitted code is low quality, ignore it and implement the best solution for the problem. |
| 20 | + |
| 21 | +Do not continue if you cannot verify the problem is real or test the fix. |
| 22 | + |
| 23 | +## PR quality bar |
| 24 | + |
| 25 | +- Do not trust PR code by default. |
| 26 | +- Do not merge changes you cannot validate with a reproducible problem and a tested fix. |
| 27 | +- Keep types strict. Do not use `any` in implementation code. |
| 28 | +- Keep external-input boundaries typed and validated, including CLI input, environment variables, network payloads, and tool output. |
| 29 | +- Keep implementations properly scoped. Fix root causes, not local symptoms. |
| 30 | +- Identify and reuse canonical sources of truth so behavior does not drift across the codebase. |
| 31 | +- Harden changes. Always evaluate security impact and abuse paths. |
| 32 | +- Understand the system before changing it. Never make the codebase messier just to clear a PR queue. |
| 33 | + |
| 34 | +## Unified workflow |
| 35 | + |
| 36 | +Entry criteria: |
| 37 | + |
| 38 | +- PR URL/number is known. |
| 39 | +- Problem statement is clear enough to attempt reproduction. |
| 40 | +- A realistic verification path exists (tests, integration checks, or explicit manual validation). |
| 41 | + |
| 42 | +### 1) `review-pr` |
| 43 | + |
| 44 | +Purpose: |
| 45 | + |
| 46 | +- Review only: correctness, value, security risk, tests, docs, and changelog impact. |
| 47 | +- Produce structured findings and a recommendation. |
| 48 | + |
| 49 | +Expected output: |
| 50 | + |
| 51 | +- Recommendation: ready, needs work, needs discussion, or close. |
| 52 | +- `.local/review.md` with actionable findings. |
| 53 | + |
| 54 | +Maintainer checkpoint before `prepare-pr`: |
| 55 | + |
| 56 | +``` |
| 57 | +What problem are they trying to solve? |
| 58 | +What is the most optimal implementation? |
| 59 | +Is the code properly scoped? |
| 60 | +Can we fix up everything? |
| 61 | +Do we have any questions? |
| 62 | +``` |
| 63 | + |
| 64 | +Stop and escalate instead of continuing if: |
| 65 | + |
| 66 | +- The problem cannot be reproduced or confirmed. |
| 67 | +- The proposed PR scope does not match the stated problem. |
| 68 | +- The design introduces unresolved security or trust-boundary concerns. |
| 69 | + |
| 70 | +### 2) `prepare-pr` |
| 71 | + |
| 72 | +Purpose: |
| 73 | + |
| 74 | +- Make the PR merge-ready on its head branch. |
| 75 | +- Rebase onto current `main`, fix blocker/important findings, and run gates. |
| 76 | + |
| 77 | +Expected output: |
| 78 | + |
| 79 | +- Updated code and tests on the PR head branch. |
| 80 | +- `.local/prep.md` with changes, verification, and current HEAD SHA. |
| 81 | +- Final status: `PR is ready for /mergepr`. |
| 82 | + |
| 83 | +Maintainer checkpoint before `merge-pr`: |
| 84 | + |
| 85 | +``` |
| 86 | +Is this the most optimal implementation? |
| 87 | +Is the code properly scoped? |
| 88 | +Is the code properly typed? |
| 89 | +Is the code hardened? |
| 90 | +Do we have enough tests? |
| 91 | +Are tests using fake timers where relevant? (e.g., debounce/throttle, retry backoff, timeout branches, delayed callbacks, polling loops) |
| 92 | +Do not add performative tests, ensure tests are real and there are no regressions. |
| 93 | +Take your time, fix it properly, refactor if necessary. |
| 94 | +Do you see any follow-up refactors we should do? |
| 95 | +Did any changes introduce any potential security vulnerabilities? |
| 96 | +``` |
| 97 | + |
| 98 | +Stop and escalate instead of continuing if: |
| 99 | + |
| 100 | +- You cannot verify behavior changes with meaningful tests or validation. |
| 101 | +- Fixing findings requires broad architecture changes outside safe PR scope. |
| 102 | +- Security hardening requirements remain unresolved. |
| 103 | + |
| 104 | +### 3) `merge-pr` |
| 105 | + |
| 106 | +Purpose: |
| 107 | + |
| 108 | +- Merge only after review and prep artifacts are present and checks are green. |
| 109 | +- Use squash merge flow and verify the PR ends in `MERGED` state. |
| 110 | + |
| 111 | +Go or no-go checklist before merge: |
| 112 | + |
| 113 | +- All BLOCKER and IMPORTANT findings are resolved. |
| 114 | +- Verification is meaningful and regression risk is acceptably low. |
| 115 | +- Docs and changelog are updated when required. |
| 116 | +- Required CI checks are green and the branch is not behind `main`. |
| 117 | + |
| 118 | +Expected output: |
| 119 | + |
| 120 | +- Successful merge commit and recorded merge SHA. |
| 121 | +- Worktree cleanup after successful merge. |
| 122 | + |
| 123 | +Maintainer checkpoint after merge: |
| 124 | + |
| 125 | +- Were any refactors intentionally deferred and now need follow-up issue(s)? |
| 126 | +- Did this reveal broader architecture or test gaps we should address? |
0 commit comments