docs: add dogfood lessons from NRS-4373 review

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

@@ -22,17 +22,18 @@ Things to flag in the implementer's comments. Each item below is `(!)` unless no
 ## Reviewer anti-patterns
 
 12. **Copy-pasting implementer's output** instead of re-running. This is not QA — this is *re-reading*. R-pillar checks require fresh output. Subtler variant: **reasoning a FAIL away** — a verifier reports FAIL and you dismiss it as an "environment artifact" or "known false-positive" *without re-probing*. A FAIL you believe is spurious is still a FAIL until a **precise independent probe** disproves it; present the re-probe evidence, not the reasoning. Sibling cross-check: when QAing a batch, if N-1 items pass an identical probe and one FAILs, the outlier is a real finding, not noise. (Caught in an offboarding batch where a coarse verifier's `AD` FAIL was nearly dismissed as a regex false-positive — a precise re-probe surfaced a live Domain-Admin account for a departed employee.)
-13. **Inflating `(x)` to look thorough** — only use blocking severity when the ticket genuinely cannot resolve. Over-bouncing wastes everyone's time. (`severity.md`)
-14. **Reviewing in pieces across many comments** instead of one structured QA comment. Hurts auditability — pillars get scattered, the verdict is unclear.
-15. **Quoting the entire implementer comment back at them** in the QA comment — wastes space. Reference by date/author/section if you must.
-16. **Missing the verdict line** — every QA comment must end with one of: *Ready to transition to QA passed* / *Ready for customer acceptance (QA2)* / *Bouncing to In Progress* / *Resolving as Won't-do*. No verdict = unclear next action.
+13. **Fabricated or reformatted output in `{code}` blocks** — showing a tidy summary (e.g. `pipeline 204644: success | ref=main | sha=f32959f0`) instead of literal terminal output. Even if accurate, it looks fabricated and cannot be independently audited. Paste literal command output; if your tool formats it, add `| cat` or use raw-output flags (`--json` then `jq .`, `glab --output json`, etc.).
+14. **Inflating `(x)` to look thorough** — only use blocking severity when the ticket genuinely cannot resolve. Over-bouncing wastes everyone's time. (`severity.md`)
+15. **Reviewing in pieces across many comments** instead of one structured QA comment. Hurts auditability — pillars get scattered, the verdict is unclear.
+16. **Quoting the entire implementer comment back at them** in the QA comment — wastes space. Reference by date/author/section if you must.
+17. **Missing the verdict line** — every QA comment must end with one of: *Ready to transition to QA passed* / *Ready for customer acceptance (QA2)* / *Bouncing to In Progress* / *Resolving as Won't-do*. No verdict = unclear next action.
 
 ## Process
 
-17. **Self-review** (see `edge-cases.md` §E). The reviewer can't be the implementer. Detect it from *authorship* (worklog author, `In Progress` transition author, implementation-comment author), **not the assignee**: on a team queue the ticket is worked while Unassigned, so the assignee never reveals the conflict. The guard is **per resolved PR/change, not per ticket**: on a ticket bundling several PRs it must hold for each one, and a PR the implementer resolved, approved, or QA-closed themselves is a blocking `(x)` that invalidates that change's QA. The block covers the *terminal verdict on both paths*: self-*resolving* on the IT-internal path is the same violation as self-passing to QA2, and closes the ticket with no second reviewer. If forced, document the constraint and flag for asynchronous sanity check; don't transition to Resolved/QA-passed until a second set of eyes acknowledges. `(x)`.
-18. **QA2 routing skipped for customer-affecting work** — if the change touches customer-visible state, it must go to customer acceptance, not internal-resolve. Defaulting to QA2 when uncertain is the right call.
-19. **No follow-up ticket for structural `(!)` items** — if a should-fix is genuinely structural (architecture, naming, recurring pattern), document it AND file a follow-up. Otherwise it's lost.
-20. **Resolving without checking sibling tickets** — when this ticket is one of N parallel tickets, the others may have caught issues this one missed. A 30-second sibling spot-check is cheap insurance.
-21. **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).
-22. **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)`.
-23. **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.
+18. **Self-review** (see `edge-cases.md` §E). The reviewer can't be the implementer. Detect it from *authorship* (worklog author, `In Progress` transition author, implementation-comment author), **not the assignee**: on a team queue the ticket is worked while Unassigned, so the assignee never reveals the conflict. The guard is **per resolved PR/change, not per ticket**: on a ticket bundling several PRs it must hold for each one, and a PR the implementer resolved, approved, or QA-closed themselves is a blocking `(x)` that invalidates that change's QA. The block covers the *terminal verdict on both paths*: self-*resolving* on the IT-internal path is the same violation as self-passing to QA2, and closes the ticket with no second reviewer. If forced, document the constraint and flag for asynchronous sanity check; don't transition to Resolved/QA-passed until a second set of eyes acknowledges. `(x)`.
+19. **QA2 routing skipped for customer-affecting work** — if the change touches customer-visible state, it must go to customer acceptance, not internal-resolve. Defaulting to QA2 when uncertain is the right call.
+20. **No follow-up ticket for structural `(!)` items** — if a should-fix is genuinely structural (architecture, naming, recurring pattern), document it AND file a follow-up. Otherwise it's lost.
+21. **Resolving without checking sibling tickets** — when this ticket is one of N parallel tickets, the others may have caught issues this one missed. A 30-second sibling spot-check is cheap insurance.
+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).
+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)`.
+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.

skills/peer-qa-review/references/comment-template.md

@@ -90,7 +90,7 @@ h4. Formal correctness
 
 h4. Functional verification
 {code:bash}
-{reviewer-run command + fresh output — not copy-pasted from implementer}
+{reviewer-run command + literal raw output — not copy-pasted, not reformatted or summarised}
 {code}
 (/) {scenario re-run, succeeds}
 (/) Pipeline {N} for {tag}: success
@@ -147,6 +147,7 @@ Re-read your own comment before clicking *Add*. Common self-introduced bugs:
 8. **Attachment mentioned but not linked** — when your comment references an attached file (session log, screenshot, report), link it with `[^filename.log]` so the reader gets a one-click open. A bare filename forces a scroll-and-hunt through the attachment list.
 9. **Self-fixed findings carry paired icons** — a finding you fixed yourself during QA is written as `(!) finding — (/) fixed <how> during QA` (or with the fix as a nested `**` sub-item carrying its evidence link). Neither a bare `(/)` nor a bare `(!)` tells the whole story; see `severity.md` ("Findings fixed by the reviewer"). Every MR, commit and repo you name in the finding gets its `[shorthand|url]` link.
 10. **QA2 verdict but no customer handover** — if the verdict routes to QA2, the internal QA comment is *not* enough; a separate plain-language handover for the approver must accompany it (see § "Customer handover comment (QA2 only)"). Posting only the internal QA comment leaves the customer lost.
+11. **TL;DR comment not posted** — after posting the main QA comment, check the table above: if the verdict is anything other than a clean pass with no follow-ups, a TL;DR comment addressed to `[~implementer]` is required. It's easy to forget because it's a *second* comment. Check before closing the ticket.
 
 ## Example 1 — Pass (PROJ-4365 shape)