Skip to content

Commit 6f389ab

Browse files
committed
docs(anti-patterns): don't file a follow-up for a deliberate, documented decision
Add reviewer Process anti-pattern #25 (inverse of #20): before raising a 'gap' as a (!) or a follow-up ticket, confirm it isn't a choice the implementer already made and documented. Read the MR/PR description and code/CI comments — the why — before the code; the untrusted-finding discipline applies to the reviewer's own findings too. Came from /retro. Signed-off-by: Sebastian Mendel <info@sebastianmendel.de>
1 parent c038157 commit 6f389ab

1 file changed

Lines changed: 1 addition & 0 deletions

File tree

skills/peer-qa-review/references/anti-patterns.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,3 +37,4 @@ Things to flag in the implementer's comments. Each item below is `(!)` unless no
3737
22. **Config presence treated as functional proof** — a set or grepped config flag, a present setting, or a green healthcheck proves the setting *exists*, not that the feature *works*. Trigger the feature end-to-end (a webhook fired with a real payload, a render plus screenshot, the role/flag actually taking effect) and attach that evidence to the ticket. "Reports healthy" over a shadowed or overridden config is a false pass — verify the *effective* running configuration, not the health endpoint. `(x)` if only presence/health backs a behavioural change (see checklist R7).
3838
23. **QA findings treated as noise to clear** — a verdict with open `(!)` / `(?)` items is not "passed". Findings are there to be worked, not clicked away; don't close or resolve while flagged items are unaddressed. When four of five checks pass, the fifth is the point — read it and act on it before the verdict. `(x)` if closed over an unresolved `(x)`.
3939
24. **Proposing a fix without checking the current state first** — recommending an addition (a certificate, a router, a config block) before verifying what already exists. The thing may already be in place — a wildcard certificate already on the proxy, a route already defined — so check the live/inventory state before proposing to add it. Skipping this wastes work and adds noise to the ticket.
40+
25. **Filing a follow-up for a deliberate, documented decision** — the inverse of #20. Before raising a "gap" (missing automation, no schedule, a check that only runs manually, an unclosed loose end) as a `(!)` or a follow-up ticket, confirm it isn't a choice the implementer already made *and* documented. Read the MR/PR **description** and the code/CI **comments** — the *why*, not just the *what* — before you read the code. A finding can be locally correct about the code and globally wrong because the design deliberately forecloses the "fix" (a security tradeoff, a cost tradeoff) and says so; the same discipline you apply to a SAST/subagent finding applies to your *own* reviewer finding. If the rationale is there, it's `(/)` — or at most an `(i)` noting the tradeoff — not a follow-up. (Caught the hard way: a reviewer filed a follow-up over a drift-check that "only runs on manual trigger", when the MR description documented that the Vault auth model deliberately allows no unattended credential — a considered decision, not an oversight; the ticket owner pushed back and the follow-up was closed as obsolete.)

0 commit comments

Comments
 (0)