Skip to content

Commit c96e6c4

Browse files
authored
docs(anti-patterns): don't file a follow-up for a deliberate, documented decision (#38)
## What Adds reviewer Process anti-pattern **#25** — the inverse of #20 ("file a follow-up for structural `(!)` items"): > Before raising a "gap" (missing automation, no schedule, a manual-only check) 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. If the rationale is there, it's `(/)` (or an `(i)` noting the tradeoff), not a follow-up. ## Why In a real QA batch I filed a follow-up flagging a vzdump drift-check as a "detectable-but-not-detected gap". It wasn't: the implementer had deliberately made it user-triggered because the Vault auth model allows no unattended credential — documented in the MR description and the CI comment. I'd read the code, not the documented rationale. The ticket owner pushed back and the follow-up was closed as obsolete. This generalizes the "treat findings as untrusted claims" discipline to the reviewer's *own* findings. ## Follow-up (not in this PR) A matching eval case could be added to `evals/qa-discipline.md`. Came from /retro: yes
2 parents c038157 + 6f389ab commit c96e6c4

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)