| name | code-review-and-quality |
| description | A reusable five-axis code-review workflow for PowerShell/DSC changes: review for design, correctness, complexity, tests, and clarity/naming; size changes to stay reviewable; label findings by severity (Blocker / Major / Minor / Nit); separate must-fix from opinion; and keep review turnaround fast. The bar is "would a senior engineer approve this?" USE FOR: code review, review this change, PR review, reviewer checklist, review standards, severity labels, nit vs blocker, change sizing, reviewable diff, what to look for in a review, approve or request changes, review feedback tone, self-review before pushing. DO NOT USE FOR: the multi-phase PowerShell security review with SARIF/CVSS (use the code-review prompt or the security-reviewer agent), agentic/LLM security review (use agent-security-review), adversarial critique of a design or document (use devils-advocate-review), multi-perspective document peer review (use the peer-review prompt). |
Code Review & Quality
A structured workflow for reviewing a code change — as the reviewer, or as the author self-reviewing before asking for one. Scope is general engineering quality, PowerShell/DSC-flavoured. For a dedicated security audit use the code-review prompt (SARIF + CVSS via the security-reviewer agent) or agent-security-review for LLM/agent wiring; for adversarial design critique use devils-advocate-review.
When to Use
- Reviewing a pull request or a diff before it merges.
- Self-reviewing your own change before you push or open a PR.
- Setting or applying a consistent review standard across a team.
The five axes
Review every change against these, in order — design first, because a well-named typo is cheap and a well-tested wrong design is not:
- Design. Does the change belong here? Does it fit the module's structure and surrounding patterns, or bolt on a parallel way of doing the same thing? Is the public surface (parameters, output objects) the one you want to live with?
- Correctness. Does it do what it claims across the real input range — empty,
$null, pipeline vs parameter, large input, non-ASCII, wrong types? Are error paths handled (-ErrorAction Stop, try/catch) rather than swallowed?
- Complexity. Is it as simple as it can be for what it does? Flag dead code, needless indirection, and cleverness a maintainer must decode. Do not require more generality than the task needs.
- Tests. Is the new behaviour covered by a test that would fail without the change? Do the tests assert behaviour, not internals? See
test-driven-development and pester-patterns.
- Clarity & naming. Approved verbs (
Get-Verb) and intention-revealing names; comments explain the reason, not the mechanics; consistent terminology. See powershell.instructions.md.
Severity labels
Label every comment so the author knows what blocks the merge:
| Label | Meaning | Blocks merge? |
|---|
| Blocker | Bug, security hole, data loss, broken build or test. | Yes |
| Major | Wrong design or missing tests that will cost later. | Yes |
| Minor | Real but small; safe to fix in a follow-up. | No |
| Nit | Style or preference. Prefix the comment with "Nit:". | No |
Separate must-fix from opinion. If a comment is your preference, say so; do not hold a change hostage to taste. Praise what is genuinely good — review is not only fault-finding.
Size the change
Small changes get real reviews; large ones get rubber-stamped. Keep a reviewable diff focused on one logical change; split unrelated refactors, renames, and reformatting into their own commits or PRs so the substance is not buried in noise. If a change is too big to review well, the right feedback is "split this", not a shallow approval.
Speed and tone
- Review promptly — a change in review blocks the author; a fast, focused pass beats a slow, exhaustive one.
- Be specific: cite the file and line, and propose the concrete alternative rather than a vague objection.
- Critique the code, not the author. Ask questions where intent is unclear instead of asserting fault.
Author self-review first
Before requesting review, clear the shared Post-flight Definition of Done gate: Invoke-ScriptAnalyzer clean, Pester green (separate process), approved verbs, Memory Bank and CHANGELOG updated. Read your own diff top to bottom first — most Nits should never reach the reviewer.
Anti-rationalization table
| Rationalization | Reality |
|---|
| "It works, so it's fine to approve." | Working is the floor. Design, tests, and clarity are what the review is for. |
| "The diff is huge but it's mostly mechanical." | Mechanical noise hides real changes. Ask for a split so the substance is reviewable. |
| "I'll just fix it myself and approve." | Silent rewrites erase the author's intent and the review trail. Comment; let them fix. |
| "No tests, but I trust the author." | Trust is not coverage. Untested behaviour is unverified behaviour. |
Red flags
- Approving a change you did not read end to end.
- A diff too large to review, approved anyway.
- Blocking a merge over pure preference, unlabelled as a Nit.
- No tests accompany a behaviour change, and it passes review.
- Review comments attack the person rather than the code.
Verification
A review is done when:
- All five axes were considered and every comment is severity-labelled.
- No Blocker or Major is left unresolved before approval.
- The change (author side) clears the shared Post-flight Definition of Done gate.
- The decision is explicit: approve, or request changes with the specific must-fix list.
Related
