Skip to content

review-rigor: a finding about detection is not closed by a change to prevention #271

Description

@ClintonSarkar

What happened

A self-review loop ran three passes over a change that established a hard invariant: a shared
library had to export exactly one symbol (its plugin entry point), because anything more let a
dependency bind the wrong copy of a runtime symbol and crash the process during startup.

Pass 1 named the right risk in so many words — "nothing gates this against regrowth; a future
generic template instantiation would export a colliding name, with no gate to catch it."
The author
resolved it by adding a declarative linker constraint that pins the export list. Passes 2 and 3
then cleared the change as clean.

The human reviewer's first response was CHANGES_REQUESTED, with exactly one blocker: there was no
committed test or automated assertion for the invariant.
Delete the constraint and the dangerous
symbols silently return.

The gap

The reviewer and the self-review had identified the same risk. They diverged on what counts as
resolving it. A change to the enforcement mechanism — making the bad state structurally hard to
reach — was accepted as satisfying a finding about the absence of verification.

Those are different things, and the substitution is seductive precisely because the new mechanism is
genuinely better than what it replaced: it moves the invariant from "currently true" to "declared
true". But declared is not checked. Nothing failed when the declaration was removed.

This is not the ordinary "forgot to write a test" miss, and the existing rules don't catch it:

  • V1 (no speculative clears) keys on hedge-words and unverified claims. Here nothing was hedged —
    the resolution was stated confidently and was real, working code.
  • Two further passes then anchored on that resolution instead of re-asking whether the original
    finding was closed. Per Re-review continuity, iterations 2+ inherit the earlier pass's
    dispositions rather than re-litigating them, so the substitution propagated.
  • A self-review loop is especially exposed, because the accepted resolution is often the reviewer's
    own earlier suggestion — there is no independent party to notice the goalposts moved.

The rule

When a finding is "X is unverified / nothing catches X if it regresses", it closes only when
something FAILS on regression. A change that makes X less likely — hardening, a stricter default, a
declarative constraint, a type that makes the state unrepresentable — does not close it.

The settling question: if someone deletes this hardening, what turns red? If the answer is
"nothing", the finding stands.

Proposed delta

Add V3 to core/review-rigorVerdict integrity, alongside V1/V2, with the detection cue
(finding says no test / nothing catches / unenforced / could silently regress; resolution changed a
mechanism instead of adding an observer), the self-review exposure, and a ✅/❌ pair. Extend the
Re-review continuity note so a later pass re-asks closed-vs-mitigated, and add one cross-reference
from Tests so the "is there a regression test?" check names hardening as a non-substitute.

One further sub-rule worth encoding with it, learned the hard way in the same incident: a
red-then-green demonstration is only evidence if the artifact under test actually moved.
Two
"negative tests" passed vacuously before a valid one was built — one widened a declarative allowlist
that a different flag already suppressed (the built output came out byte-identical), and one used a
sed that silently failed to match because of line-ending differences, so the flag was never removed
and the build stayed green. Both read exactly like a working check.


🤖 Generated with Claude Code

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions