Skip to content

Commit 1f328f1

Browse files
committed
docs: update pr-review policy — auto-fix safe P3 issues, defer complex ones
1 parent 06488b1 commit 1f328f1

File tree

1 file changed

+11
-6
lines changed

1 file changed

+11
-6
lines changed

.github/skills/pr-review/SKILL.md

Lines changed: 11 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -56,13 +56,17 @@ Before acting on any review comment or suggestion, classify it by importance:
5656
|-------|----------|--------|
5757
| **P1 — Must Fix** | Bug, security issue, broken behavior, API contract violation, CI failure | Fix immediately |
5858
| **P2 — Should Fix** | Correctness risk, meaningful maintainability improvement, clear code smell with real impact | Fix with brief justification |
59-
| **P3 — Optional** | Style preference, minor naming nitpick, debatable design choice, "could be cleaner" | **Do NOT auto-fix** — surface to user for decision |
59+
| **P3 — Conditional** | Style preference, minor naming, "could be cleaner" | Fix if it aligns with best practices **and** the change is safe + trivial; defer if complex or has any potential functional impact |
6060
| **P4 — Reject** | Contradicts project conventions, introduces unnecessary complexity, or is factually wrong | Reject with explanation |
6161

6262
**Rules:**
63-
- **Do not implement P3 suggestions automatically.** Note them in the final summary and let the user decide.
63+
- For **P3**, apply this two-question test before touching the code:
64+
1. **Best practice?** — Does the change follow language/framework conventions (e.g. prefer imports over FQNs, use existing imports, standard patterns)?
65+
2. **Safe & trivial?** — Is the diff mechanical with zero risk of behavioral change and low effort?
66+
- Both **yes** → fix it silently, mark thread resolved.
67+
- Either **no** → do NOT modify code; record in the summary table and let the user decide.
6468
- **Do not add comments to code** unless the comment explains non-obvious logic that is truly necessary. Never add comments just to acknowledge a review suggestion was applied.
65-
- When in doubt about importance, prefer the lower severity (P3/P4) and defer to the user rather than making the change.
69+
- When the two-question test is ambiguous, prefer deferring rather than guessing.
6670

6771
## Procedure
6872

@@ -80,7 +84,7 @@ Before acting on any review comment or suggestion, classify it by importance:
8084
3. **Address review comments** — apply the Review Restraint Policy to each comment:
8185
- P1/P2: implement the fix, then **immediately resolve the thread** using the GraphQL mutation above
8286
- Outdated threads: resolve them regardless of priority (no action needed, just mark resolved)
83-
- P3: record in summary, do **not** modify code, ask user
87+
- P3: apply the two-question test — fix + resolve if both answers are yes; otherwise record in summary and defer to user
8488
- P4: record rejection reason in summary
8589
4. **Commit & push** — single commit covering all non-workflow fixes (workflow fixes are pushed incrementally during step 2)
8690
5. **Final verification** — once all fixes are applied, confirm every check is green: `GH_PAGER= gh pr checks <PR>`
@@ -94,8 +98,9 @@ Before acting on any review comment or suggestion, classify it by importance:
9498
```markdown
9599
| # | Source (comment / CI) | Issue description | Priority | Action taken | Reason if not fixed |
96100
|---|-----------------------|-------------------|----------|--------------|---------------------|
97-
| 1 | Reviewer @xxx | e.g. rename foo() | P3 | Not fixed | Style preference, no functional impact — deferred to user |
98-
| 2 | CI: lint | e.g. null-check | P1 | Fixed ||
101+
| 1 | Reviewer @xxx | use import instead of FQN | P3 | Fixed | Best practice + trivial mechanical change |
102+
| 2 | Reviewer @xxx | rename internal var foo→bar | P3 | Not fixed | Non-standard opinion, no best-practice backing — deferred to user |
103+
| 3 | CI: lint | null-check missing | P1 | Fixed ||
99104
```
100105

101106
All P3/P4 items that were **not** fixed must appear in this table with a clear reason, so the user can make an informed decision.

0 commit comments

Comments
 (0)