fix(review): stop verification-gap from demanding source-text tests (#2662)

The verification-gap prompt reliably pushed implementers to write tests
that grep source files for strings. Those tests pin wording, break on
rewording, and verify nothing.

The reviewer was right to treat a prompt as the executable artifact —
editing it does change behavior. The mistake was what followed: having
classified it as code, the reviewer reached the "a test counts only if
an assertion observes the changed output" rule and demanded an
assertion, and the only assertion reachable against prose is a match on
the file's own wording. The "Removed verification" finding category then
flagged deleting such a test as a regression, making it self-reinforcing.
Documentation was never affected; the failure was specific to prompts.

Three changes: list source-text assertions among the checks that do not
count, exempt their removal from "Removed verification", and have Step 1
screen each part of the change on its own so parts with no deterministic
outcome are skipped while the rest is reviewed normally. The screen is
per-part rather than whole-change, so a diff touching both a prompt and
a script still reports gaps for the script.

Applied identically to all three copies of the prompt.
This commit is contained in:
Alex Verkhovsky
2026-08-01 07:28:36 -07:00
committed by GitHub
parent e39cbbbaf5
commit 6cd3b646dd
3 changed files with 12 additions and 6 deletions
@@ -24,6 +24,8 @@ If the change is non-behavioral, stop here and output the clean result (see Outp
Common non-behavioral examples: formatting, comments, whitespace; pure renames; trivial getters/setters and pass-throughs; type-only or compiler-enforced changes with no runtime effect; etc.
Screen each part of the change separately. Only outcomes produced by deterministic code are worth automatically testing; tests are useless on static source text and brittle on LLM output. Skip those parts and review the rest normally.
### Step 2: Find the behavior that changed
Identify what behavior changed compared to the previous version: output, side effect, branch, error path, schema/event shape, config default, validation/authorization rule, external contract, etc. If the change affects more than one behavior, handle each separately.
@@ -48,7 +50,7 @@ Find and read the relevant test. Ask whether the Demonstration would make an ass
- For a regression-style Demonstration: if no test runs the path, the test is skipped/flaky/not run normally, or the test runs the code without checking the changed result, report a `Regression gap` or `Broken-verification gap`.
- For a qualifying Missing-adoption case: if none of the site tests you found assert it adopts the new behavior, report a `Missing-adoption gap`.
A test counts only if it runs normally and an assertion observes the changed output, branch, or contract. These do not count: no execution; success/no-throw/snapshot-only checks; mock/log-call checks; human-only checks; tests that mock away the integration; e2e tests that pass through without checking the changed output; stale assertions or fixtures.
A test counts only if it runs normally and an assertion observes the changed output, branch, or contract. These do not count: no execution; source-text assertions that match a file's wording instead of running it; success/no-throw/snapshot-only checks; mock/log-call checks; human-only checks; tests that mock away the integration; e2e tests that pass through without checking the changed output; stale assertions or fixtures.
Common patterns:
@@ -57,7 +59,7 @@ Common patterns:
- **Migration compatibility** — tests only create new-format rows or fresh schemas.
- **Phantom exception** — handled partial-failure path has no test.
- **Missing-adoption gap** — sibling site should use the new rule/helper and does not.
- **Removed verification** — deleted test or weakened assertion leaves behavior unpinned.
- **Removed verification** — deleted test or weakened assertion leaves behavior unpinned; removing a source-text assertion is not this, since it never counted.
### Step 5: Confirm each finding is real
@@ -24,6 +24,8 @@ If the change is non-behavioral, stop here and output the clean result (see Outp
Common non-behavioral examples: formatting, comments, whitespace; pure renames; trivial getters/setters and pass-throughs; type-only or compiler-enforced changes with no runtime effect; etc.
Screen each part of the change separately. Only outcomes produced by deterministic code are worth automatically testing; tests are useless on static source text and brittle on LLM output. Skip those parts and review the rest normally.
### Step 2: Find the behavior that changed
Identify what behavior changed compared to the previous version: output, side effect, branch, error path, schema/event shape, config default, validation/authorization rule, external contract, etc. If the change affects more than one behavior, handle each separately.
@@ -48,7 +50,7 @@ Find and read the relevant test. Ask whether the Demonstration would make an ass
- For a regression-style Demonstration: if no test runs the path, the test is skipped/flaky/not run normally, or the test runs the code without checking the changed result, report a `Regression gap` or `Broken-verification gap`.
- For a qualifying Missing-adoption case: if none of the site tests you found assert it adopts the new behavior, report a `Missing-adoption gap`.
A test counts only if it runs normally and an assertion observes the changed output, branch, or contract. These do not count: no execution; success/no-throw/snapshot-only checks; mock/log-call checks; human-only checks; tests that mock away the integration; e2e tests that pass through without checking the changed output; stale assertions or fixtures.
A test counts only if it runs normally and an assertion observes the changed output, branch, or contract. These do not count: no execution; source-text assertions that match a file's wording instead of running it; success/no-throw/snapshot-only checks; mock/log-call checks; human-only checks; tests that mock away the integration; e2e tests that pass through without checking the changed output; stale assertions or fixtures.
Common patterns:
@@ -57,7 +59,7 @@ Common patterns:
- **Migration compatibility** — tests only create new-format rows or fresh schemas.
- **Phantom exception** — handled partial-failure path has no test.
- **Missing-adoption gap** — sibling site should use the new rule/helper and does not.
- **Removed verification** — deleted test or weakened assertion leaves behavior unpinned.
- **Removed verification** — deleted test or weakened assertion leaves behavior unpinned; removing a source-text assertion is not this, since it never counted.
### Step 5: Confirm each finding is real
@@ -24,6 +24,8 @@ If the change is non-behavioral, stop here and output the clean result (see Outp
Common non-behavioral examples: formatting, comments, whitespace; pure renames; trivial getters/setters and pass-throughs; type-only or compiler-enforced changes with no runtime effect; etc.
Screen each part of the change separately. Only outcomes produced by deterministic code are worth automatically testing; tests are useless on static source text and brittle on LLM output. Skip those parts and review the rest normally.
### Step 2: Find the behavior that changed
Identify what behavior changed compared to the previous version: output, side effect, branch, error path, schema/event shape, config default, validation/authorization rule, external contract, etc. If the change affects more than one behavior, handle each separately.
@@ -48,7 +50,7 @@ Find and read the relevant test. Ask whether the Demonstration would make an ass
- For a regression-style Demonstration: if no test runs the path, the test is skipped/flaky/not run normally, or the test runs the code without checking the changed result, report a `Regression gap` or `Broken-verification gap`.
- For a qualifying Missing-adoption case: if none of the site tests you found assert it adopts the new behavior, report a `Missing-adoption gap`.
A test counts only if it runs normally and an assertion observes the changed output, branch, or contract. These do not count: no execution; success/no-throw/snapshot-only checks; mock/log-call checks; human-only checks; tests that mock away the integration; e2e tests that pass through without checking the changed output; stale assertions or fixtures.
A test counts only if it runs normally and an assertion observes the changed output, branch, or contract. These do not count: no execution; source-text assertions that match a file's wording instead of running it; success/no-throw/snapshot-only checks; mock/log-call checks; human-only checks; tests that mock away the integration; e2e tests that pass through without checking the changed output; stale assertions or fixtures.
Common patterns:
@@ -57,7 +59,7 @@ Common patterns:
- **Migration compatibility** — tests only create new-format rows or fresh schemas.
- **Phantom exception** — handled partial-failure path has no test.
- **Missing-adoption gap** — sibling site should use the new rule/helper and does not.
- **Removed verification** — deleted test or weakened assertion leaves behavior unpinned.
- **Removed verification** — deleted test or weakened assertion leaves behavior unpinned; removing a source-text assertion is not this, since it never counted.
### Step 5: Confirm each finding is real