|
| 1 | +# /mxcli-dev:review — PR Review |
| 2 | + |
| 3 | +Run a structured review of the current branch's changes against the CLAUDE.md |
| 4 | +checklist, then check the recurring findings table below for patterns that have |
| 5 | +burned us before. |
| 6 | + |
| 7 | +## Steps |
| 8 | + |
| 9 | +1. Run `gh pr view` and `gh pr diff` (or `git diff main...HEAD`) to read the change. |
| 10 | +2. Work through the CLAUDE.md "PR / Commit Review Checklist" in full. |
| 11 | +3. Then check every row in the Recurring Findings table below — flag any match. |
| 12 | +4. Report: blockers first, then moderate issues, then minor. Include a concrete fix |
| 13 | + option for every blocker (not just "this is wrong"). |
| 14 | +5. After the review: **add a row** to the Recurring Findings table for any new |
| 15 | + pattern not already covered. |
| 16 | + |
| 17 | +--- |
| 18 | + |
| 19 | +## Recurring Findings |
| 20 | + |
| 21 | +Patterns caught in real reviews. Each row is a class of mistake worth checking |
| 22 | +proactively. Add a row after every review that surfaces something new. |
| 23 | + |
| 24 | +| # | Finding | Category | Canonical fix | |
| 25 | +|---|---------|----------|---------------| |
| 26 | +| 1 | Formatter emits a keyword not present in `MDLParser.g4` → DESCRIBE output won't re-parse (e.g. `RANGE(...)`) | DESCRIBE roundtrip | Grep grammar before assuming a keyword is valid; if construct can't be expressed yet, emit `-- TypeName(field=value) — not yet expressible in MDL` | |
| 27 | +| 2 | Output uses `$currentObject/Attr` prefix — non-idiomatic; Studio Pro uses bare attribute names | Idiomatic output | Verify against a real Studio Pro BSON sample before choosing a prefix convention | |
| 28 | +| 3 | Malformed BSON field (missing key, wrong type) produces silent garbage output (e.g. `RANGE($x, , )`) | Error handling | Default missing numeric fields to `"0"`; or emit `-- malformed <TypeName>` rather than broken MDL | |
| 29 | +| 4 | No DESCRIBE roundtrip test — grammar gap went undetected until human review | Test coverage | Add roundtrip test: format struct → MDL string → parse → confirm no error | |
| 30 | +| 5 | Hardcoded personal path in committed file (e.g. `/c/Users/Ylber.Sadiku/...`) | Docs quality | Use bare commands (`go test ./...`) without absolute paths in any committed doc or skill | |
| 31 | +| 6 | Docs-only PR cites an unmerged PR as a "model example" — cited PR had blockers | Docs quality | Only cite merged, verified PRs; or annotate with known gaps if citing in-flight work | |
| 32 | +| 7 | Skill/doc table references a function that doesn't exist (e.g. `formatActionStatement()` vs `formatAction()`) | Docs quality | Grep function names before writing: `grep -r "func formatA" mdl/executor/` | |
| 33 | +| 8 | "Always X" rule is too absolute for trivial edge cases (e.g. "always write failing test first" for one-char typos) | Docs quality | Soften to "prefer X" or add an exception clause; include the reasoning so readers can judge edge cases | |
| 34 | + |
| 35 | +--- |
| 36 | + |
| 37 | +## After Every Review |
| 38 | + |
| 39 | +- [ ] All blockers have a concrete fix option stated. |
| 40 | +- [ ] Recurring Findings table updated with any new pattern. |
| 41 | +- [ ] If docs-only PR: every function name, path, and PR reference verified against |
| 42 | + live code before approving. |
0 commit comments