Skip to content

Commit c038157

Browse files
authored
docs: make Stage -1 claim explicit and add G4 enforcement-layer check (#37)
## Summary Two Round-1 QA discipline fixes found by a `/retro` pass: make the Stage -1 claim read as an imperative first action in SKILL.md, and add a Guardrails check for enforcement layers on a request path. ## Came from `/retro` session on 2026-07-23: `9beb8958-4776-4083-a5a7-b77ba29a5029` ### Finding 1 — A6/B15, claim step not firing - **Symptom:** An agent ran Round-1 QA on one ticket and implementation on a second while both stayed `Unassigned`, until the user challenged it: *"why did not assign the ticket to me when you are working on it? should be clearly stated in your rules!"* - **Cause:** `references/lifecycle.md` §Stage -1 is correct and has a proper assignee decision table, but SKILL.md compressed it into a single bullet carrying three separate rules. The agent read SKILL.md and never opened the reference. - **Required behavior:** Claiming is the first tool call of a QA pass, before Stage 0 discovery. - **Change:** SKILL.md only. The reference file was already right, so this is a prominence fix, not new policy. SKILL.md was already exactly at the 500-word cap, so the replacement bullet is net **-4 words**. ### Finding 2 — B18, enforcement layers not enumerated - **Symptom:** An agent verified a Hetzner cloud firewall was open to `::/0` for 80/443/ICMP, concluded the host was intended to be dual-stack, and published AAAA records for a service whose traefik `ipallowlist` allow-list carried IPv4 prefixes only. Office clients preferring IPv6 would have reached traefik over v6 from an address outside the allow-list and received a 403, with no fallback — Happy Eyeballs retries connection failures, not HTTP errors. Caught by the requester, then reverted. - **Cause:** G1 covers adjacent components and G2 shared-layer downstream. Neither covers *several enforcement layers stacked on one request path*, so verifying the packet filter felt like sufficient evidence. - **Required behavior:** When a change alters how a service is reached, enumerate every layer filtering by source address or identity and confirm each covers the new path. - **Change:** New `G4` row plus a matching entry under "Common false negatives this catches". ## Verification No `evals/` or `checkpoints.yaml` in this repo, so no automated gate was added. Both changes are prose guidance in the QA checklist. ## Test plan - [x] SKILL.md under the 500-word cap — `wc -w` incl. frontmatter, as the gate counts it: **496** (main: 500) - [x] `skill-repo/scripts/validate-skill.sh .` — 0 errors - [ ] Stage -1 wording in SKILL.md does not contradict `lifecycle.md` §Stage -1 or `edge-cases.md` §E - [ ] `G4` does not duplicate `G1`/`G2`/`G3` scope
2 parents 4f9653f + 0ad1b66 commit c038157

2 files changed

Lines changed: 4 additions & 3 deletions

File tree

skills/peer-qa-review/SKILL.md

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -28,9 +28,8 @@ discovery. Consult maintenance skills for overrides.
2828

2929
## Lifecycle
3030

31-
- **-1 Claim**: team queue, self-assign to *claim* only (clear on exit, see
32-
routing); someone else's, stop; never self-review, by authorship not
33-
assignee (`edge-cases.md` §E).
31+
- **-1 Claim — your FIRST tool call**, before Stage 0: assign to yourself;
32+
someone else's, stop; never self-review (`lifecycle.md` §-1).
3433
- **0 Discover**: `scripts/qa-gather.sh <KEY>` (`--json` to parse).
3534
- **1 Formal** (description, linkage, console-output, worklog); **2 Functional,
3635
Inventory, Guardrails** (re-run; update inventory; check adjacent components,

skills/peer-qa-review/references/checklist.md

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -62,6 +62,7 @@ The R pillar verifies the change fixed what the ticket said it would. The G pill
6262
| G1 | **Adjacent components** — name two adjacent components the change could have affected but is not trying to. Spot-check each is unchanged. If you cannot name two, the blast radius is not understood. | `(!)` SHOULD; `(?)` if blast radius unclear |
6363
| G2 | **Shared layer downstream** — if the change is in a shared layer (auth, logging, error handling, DB schema, build config), exercise one downstream consumer end-to-end, not just the changed code. | `(x)` MUST if shared layer; `n/a` otherwise |
6464
| G3 | **Unchanged default path** — if the change touches a config file, env var, or feature flag default, verify the unchanged default path still behaves as before. Don't just test the new branch. | `(x)` MUST |
65+
| G4 | **Every enforcement layer on the path** — if the change alters *how a service is reached* (DNS record, address family, port, protocol, route, hostname), enumerate every layer that filters by source address or identity — packet filter / cloud firewall, reverse-proxy allow-list, application-level IP check, WAF, rate limiter — and confirm each already covers the new path. An open firewall is not evidence a request survives at L7. | `(x)` MUST if reachability changes; `n/a` otherwise |
6566

6667
Severity uses the standard icon vocabulary `(/)` `(x)` `(!)` `(i)` `(?)`. A G-finding that demonstrates a real regression is `(x)` MUST and bounces the ticket. A G-finding that surfaces "I cannot tell if X is affected" is `(?)` and blocks on the answer.
6768

@@ -71,6 +72,7 @@ Severity uses the standard icon vocabulary `(/)` `(x)` `(!)` `(i)` `(?)`. A G-fi
7172
- New feature flag's "off" path silently flipped because the default changed in code, not in config.
7273
- Schema migration's rollback path was never tested (B-pillar checks the migration; G2 checks a real downstream query against rolled-back schema).
7374
- "Refactored for clarity" changed behavior in an edge case the original test suite didn't cover.
75+
- An AAAA record was published for a dual-stack host whose reverse-proxy allow-list carried IPv4 prefixes only: the name resolves, clients prefer IPv6, and every request is rejected at L7 with no fallback — Happy Eyeballs retries connection failures, not HTTP errors. The cloud firewall was open to `::/0`, which looked like proof the path was fine (G4).
7476

7577
For changes with no plausible adjacent surface (e.g. a typo fix in a comment, a one-line README update), G-pillar may be recorded as `n/a` rather than `(/)`.
7678

0 commit comments

Comments
 (0)