feat(qa): W29 guards — per-PR self-QA block, config-presence≠functional, read-findings - #33
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the peer QA review guidelines, checklist, and anti-patterns. Specifically, it clarifies that the self-review guard applies per resolved PR/change rather than per ticket, introduces a new checklist item (R7) requiring functional proof instead of mere config presence, and adds new anti-patterns regarding treating QA findings as noise and proposing fixes without checking the current state first. There are no review comments, so I have no feedback to provide.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
…al, read-findings - Self-review guard is per resolved PR/change, not per ticket assignee: on a ticket bundling several PRs it holds for each; a PR the implementer resolved/approved/QA-closed themselves is a blocking (x) that invalidates that change's QA. A self-resolve on the internal path is the same violation as self-passing to QA2. (SKILL.md, edge-cases.md §E, anti-pattern #17) - checklist.md R7 + anti-pattern #21: config presence is not functional proof. A set/grepped flag or a green healthcheck shows the setting exists, not that the feature works — trigger it end-to-end and attach the evidence. A shadowed/overridden config that 'reports healthy' is a false pass; verify the effective running config, not the health endpoint. - anti-patterns #22-#23: QA findings are to be worked, not clicked away (a verdict with open (!)/(?) is not passed); check the current state before proposing a fix (the cert/router/config may already exist). Signed-off-by: Sebastian Mendel <info@sebastianmendel.de>
ede6648 to
42518d6
Compare
|
Three QA guards distilled from a week of Round-1 reviews where the same failure modes recurred. All lessons are generalised — no environment-, customer-, or ticket-specific detail.
Q1 — Self-QA / self-resolve is a per-PR hard blocker
The
implementer ≠ reviewerguard must hold for each resolved PR/change, not once at the ticket level. When a ticket bundles several PRs, a reviewer can legitimately QA the PRs they did not author while still owing a second pair of eyes on their own. A PR resolved, approved, or QA-closed by its own author is a blocking(x)that invalidates the QA for that change — "only a quick one" and "already deployed, just closing it out" are not exemptions. Self-resolving on the internal path is the same violation as self-passing to QA2.SKILL.md"When it applies" — terse per-PR pointer to §E (kept under the 500-word cap).references/edge-cases.md§E — new "per resolved PR/change, not per ticket" paragraph; terminal-verdict block now covers the internal Resolve path, not just QA2.references/anti-patterns.mdvalidate-skill fails on main: SKILL.md over 500-word cap (1117) and missing .gitignore #17 — per-PR clause + self-resolve = blocking(x).Coordination: PR #32 (
fix/self-review-team-queue-31) tackles the detection side of the same theme — inferring self-review from authorship rather than the assignee on an unassigned team queue. This PR is complementary (the per-PR granularity + self-resolve-as-blocker angle) and touches the same §E / #17 regions, so the two should be rebased together at merge time.Q2 — Config presence ≠ functionally proven
A set or grepped config flag, a present setting, or a green healthcheck proves the setting exists, not that the feature works. Exercise the feature end-to-end (fire the webhook with a real payload, render and screenshot the output, confirm the role/flag actually takes effect) and attach that evidence to the ticket. A shadowed or overridden config that still "reports healthy" is its own trap — verify the effective running configuration, not the health endpoint.
references/checklist.mdPillar R — new R7 (functional proof, not config presence); severity note extended so R7 is(x)for a behavioural change.references/anti-patterns.mddocs(qa): merged≠reviewed, dropped-AC bounce, claim/unassign on exit #21 — mirror.Q3 — Two smaller anti-patterns
(!)/(?)items is not "passed". Findings are to be worked, not clicked away; when four of five checks pass, the fifth is the point.Notes
CHANGELOG.mdUnreleased updated; no version bump (parity preserved).Draft — not for merge; opened for review and coordination with #32.