Add explicit review passes to the pre-review skill#1312
Conversation
Adds the full required-audit-passes list to the pre-review skill, including a new Builder pattern pass (rule 9) covering the two builder-pattern rules for dacite-loaded configs (field privacy; builder constructs sub-objects), the leaf-config / containers / tests / parent-sibling exceptions, the build-then-execute guidance for top-level entrypoints, and the pre-existing-violations carve-out so the backlog can be burned down over time.
|
|
||
| When leaving comments on a PR, always start your comment with "Pre-review agent: ". | ||
|
|
||
| ## Required audit passes |
There was a problem hiding this comment.
I've been using most of these for a while, they're originally drafted after an audit of reviews I got of PRs I had made largely with Claude. Since using this to pre-review PRs, I've gotten far fewer comments/change requests on PRs.
| - Stale log lines or comments left over from a refactor. | ||
| - Unnecessary removal of comments or docstrings unrelated to the change. | ||
|
|
||
| ### 9. Builder pattern pass |
There was a problem hiding this comment.
I've been thinking about this set of rules for a long while, but have only just added it to my pre-review skill. I would really like to go through our repo and update our existing code to follow these two rules - they're ones that I've frequently seen make the code harder to edit, understand, and use, and whenever I refactor the code to follow these rules it's much easier to test, re-use, refactor, and understand.
Especially if these can be enforced by an AI review pass, i.e. not even take additional context for PR writers and still leave the possibility for a user to override the findings, I think this is an easy win. I'd mainly like this merged to simplify writing future PRs that refactor our code to follow these guidelines.
I'll be making some example refactor PRs to get reviewed/merged as "stress tests" for the rules, before we consider merging this. I could split this section to another PR if for whatever reason we want the other rules/audit passes to get merged faster.
mcgibbon
changed the title
1 participant
Adds the standing "required audit passes" list to the
pre-reviewskill.Changes:
.claude/skills/pre-review/SKILL.md— add the required-audit-passes list; add the Builder pattern pass (§9) with the rules, exceptions, and pre-existing-violation carve-out.Tests added (n/a — docs/process only)
If dependencies changed, "deps only" image rebuilt (n/a)