Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
path uncovered, so an implementer could self-*resolve* their own work with no
second reviewer. §E, #17, and "-1 Claim" now forbid self-transitioning to
`Resolve` *and* `QA passed` until a second set of eyes acknowledges.
- **The self-review guard applies per resolved PR/change, not per ticket**
(`edge-cases.md` §E, anti-pattern #17). When a ticket bundles several PRs the
guard holds for each one; a PR the implementer resolved/approved/QA-closed
themselves is a blocking `(x)` that invalidates that change's QA. If a bundle
mixes your own PRs with a colleague's, split the verdict.
- **`checklist.md` F1.5**: a description that *contradicts the delivered
outcome* at resolve time (still frames the work as not-yet-done: in-progress
status/phase table, unticked boxes for completed work, superseded version) is
Expand All @@ -30,6 +35,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

- **Eval E5** (`evals/qa-discipline.md`): self-review on an unassigned queue,
detect by authorship, do not self-resolve on the IT-internal path.
- **`checklist.md` R7 — functional proof, not config presence.** A set/grepped
config flag or a green healthcheck is not proof the feature works: trigger it
end-to-end and attach the evidence. A shadowed/overridden config that still
"reports healthy" is its own trap — verify the *effective* running config,
not the health endpoint. Mirrored as anti-pattern #21.
- **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 certificate/router/config may already exist).

## [0.5.0] - 2026-06-18

Expand Down
2 changes: 1 addition & 1 deletion skills/peer-qa-review/SKILL.md
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ detail in `references/`.

Triggers: see description. **Skip** if: still In Progress (transition
first); already QA2 (different scope); already closed (post-mortem only); or you
are the implementer (no self-review).
are the implementer (no self-review; per-PR, §E).

A ticket-system skill is required (Jira: `jira-communication`) for Stage 0
discovery. Consult maintenance skills for overrides.
Expand Down
5 changes: 4 additions & 1 deletion skills/peer-qa-review/references/anti-patterns.md
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,10 @@ Things to flag in the implementer's comments. Each item below is `(!)` unless no

## 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 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)`.
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.
3 changes: 2 additions & 1 deletion skills/peer-qa-review/references/checklist.md
Original file line number Diff line number Diff line change
Expand Up @@ -47,8 +47,9 @@ The reviewer **re-runs** these. Copy-pasting the implementer's output does not s
| R4 | **No collateral damage**: failed services, error-level logs since the change, container health, dependent services | `systemctl --failed`, `journalctl -p err --since '1h ago'`, etc. |
| R5 | **Tag/release exists** where claimed (annotated vs lightweight is `(i)`; missing is `(x)`) | `git for-each-ref refs/tags/<v>` |
| R6 | **CI pipeline** for the merged commit/tag was green | `glab api projects/<id>/pipelines?ref=<tag>` or equivalent |
| R7 | **Functional proof, not config presence** — a set/grepped config flag, a present setting, or a green healthcheck is *not* proof the feature works. Exercise it end-to-end (fire the webhook with a real payload, render and screenshot the output, confirm the role/flag actually takes effect) and attach the evidence to the ticket. A shadowed/overridden config that still "reports healthy" is its own trap — verify the *effective* running configuration, not the health endpoint. | Trigger the feature; capture the evidence |

Severity: each R-check failing on its own is `(x)` for R1–R4 (the fix doesn't actually work or breaks something else); `(!)` for R5–R6 (artefacts present but quality flag).
Severity: each R-check failing on its own is `(x)` for R1–R4 and R7 (the fix doesn't actually work, isn't functionally proven, or breaks something else); `(!)` for R5–R6 (artefacts present but quality flag).

## Pillar G — Guardrails (what the change *wasn't* trying to touch)

Expand Down
2 changes: 2 additions & 0 deletions skills/peer-qa-review/references/edge-cases.md
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,8 @@ You cannot review your own work. The first-person bias is too strong; even with

If your account appears in any of these, §E applies regardless of who the ticket is currently assigned to, and regardless of it being Unassigned.

**Per resolved PR/change, not per ticket.** When a ticket bundles several changes or PRs, the guard holds for *each one separately*: apply implementer ≠ reviewer to every resolved PR/change, not once to the ticket as a whole. You may QA the PRs you did not author, but a PR you authored still needs a second reviewer. A PR resolved, approved, or QA-closed by its own author is a blocking `(x)` and invalidates the QA for that change — "only a quick one" and "already deployed, just closing it out" are not exemptions. If a bundle mixes your own PRs with a colleague's, split the verdict: pass the ones you can review, and leave your own in QA for a second pair of eyes.

**Decision**: Hand off to another teammate. If genuinely no other reviewer is available right now:

1. **Document the constraint** in the QA comment explicitly: *"Self-review by implementer due to no available reviewer at <time>. Requesting asynchronous sanity check from <colleague> when available."*
Expand Down
Loading