Context
In one day, across three PRs and two reviewers, the same two failure shapes recurred five times. Both shapes are invisible to every existing review dimension because both produce structurally correct, green, well-documented code and confidently-worded review findings. Neither has a checklist entry. This capability never existed.
The Problem
Shape 1 — a gate that checks a capability exists rather than that it ran.
- A promotion terminal gated on
typeof PRODUCER !== 'function', so a no-op () => {} satisfied it and handed caller-owned observations to the logic behind it. Caught by a peer; the fix was to delete the branch and the parameter.
- The same class one layer up: a cockpit field declared, read, and tested — with no writer anywhere. A spec that hand-injected the value witnessed the pass-through and read as evidence of wiring. That PR was Drop+Superseded.
- And its inverse: a reviewer (me) asserted a capability gate was forgeable when the producer was invoked one layer above the module under review.
The unifying question is not in any dimension: "does satisfying this gate cause anything to happen, or merely describe something?" — and its converse, "if I claim nothing invokes this, have I found the real caller?"
Shape 2 — an absence claim from a search, with no positive control.
Two reviewers independently asserted "no production caller exists" for the same module. Both were wrong, by two different broken searches:
- one ended in
grep -v "<module>.mjs", which deleted the dynamic-import line whose path contains that filename;
- the other ran
git grep over a local checkout still on dev, proving an absence against a tree that could not contain the branch's code.
The honest caller was present the whole time. Either error dies instantly if the same command also searches for something known-present — the first would have shown the filter eating matches, the second the wrong tree. We demand a positive control beside every negative test result; we do not demand it of the shell pipelines that produce review findings.
The Architectural Reality
Existing dimensions check the patch (premise, placement, correctness, evidence). These two shapes are failures of the reviewer's instrument, not of the patch — a green suite and a confident finding are both compatible with them. That is why five instances passed through unrelated dimensions untouched.
Both were independently flagged as [TOOLING_GAP] by two reviewers on two different PRs, with the same wording, which is the signal that it belongs in substrate rather than in whoever happens to remember.
Note the guide's own anti-pattern: substantive rule bodies must not land in always-loaded skill substrate. So this is a trigger line in the Map, rule body in references/ — Progressive Disclosure, not a pr-review-guide.md insertion.
The Fix (one PR)
Add a compact reviewer-instrument check to the review checklist, with its body in a conditionally-loaded reference:
- For any capability gate, constant-guard, or feature flag in the diff: does satisfying it cause an effect, or only describe one? Is the guarded thing ever invoked?
- For any field or config added: does a writer exist in production code, not only in a test that assigns it?
- For any absence claim in the review ("no caller", "nothing references", "not used"): the search must carry a positive control, and the review must state the tree/SHA it ran against.
Acceptance Criteria
Out of Scope
- Re-reviewing the affected PRs; all are resolved.
- Any change to
manage_pr_review validation.
Related
- #16037 (
SEAT_ADAPTER_PRODUCER contract) · #16050 (Drop+Supersede, unwritten field) · #16053 (both false-absence searches)
Context
In one day, across three PRs and two reviewers, the same two failure shapes recurred five times. Both shapes are invisible to every existing review dimension because both produce structurally correct, green, well-documented code and confidently-worded review findings. Neither has a checklist entry. This capability never existed.
The Problem
Shape 1 — a gate that checks a capability exists rather than that it ran.
typeof PRODUCER !== 'function', so a no-op() => {}satisfied it and handed caller-owned observations to the logic behind it. Caught by a peer; the fix was to delete the branch and the parameter.The unifying question is not in any dimension: "does satisfying this gate cause anything to happen, or merely describe something?" — and its converse, "if I claim nothing invokes this, have I found the real caller?"
Shape 2 — an absence claim from a search, with no positive control.
Two reviewers independently asserted "no production caller exists" for the same module. Both were wrong, by two different broken searches:
grep -v "<module>.mjs", which deleted the dynamic-import line whose path contains that filename;git grepover a local checkout still ondev, proving an absence against a tree that could not contain the branch's code.The honest caller was present the whole time. Either error dies instantly if the same command also searches for something known-present — the first would have shown the filter eating matches, the second the wrong tree. We demand a positive control beside every negative test result; we do not demand it of the shell pipelines that produce review findings.
The Architectural Reality
Existing dimensions check the patch (premise, placement, correctness, evidence). These two shapes are failures of the reviewer's instrument, not of the patch — a green suite and a confident finding are both compatible with them. That is why five instances passed through unrelated dimensions untouched.
Both were independently flagged as
[TOOLING_GAP]by two reviewers on two different PRs, with the same wording, which is the signal that it belongs in substrate rather than in whoever happens to remember.Note the guide's own anti-pattern: substantive rule bodies must not land in always-loaded skill substrate. So this is a trigger line in the Map, rule body in
references/— Progressive Disclosure, not apr-review-guide.mdinsertion.The Fix (one PR)
Add a compact reviewer-instrument check to the review checklist, with its body in a conditionally-loaded reference:
Acceptance Criteria
references/payload, perpull-request-workflow.md §1.1defaultcompress-to-trigger.Out of Scope
manage_pr_reviewvalidation.Related
SEAT_ADAPTER_PRODUCERcontract) · #16050 (Drop+Supersede, unwritten field) · #16053 (both false-absence searches)