Source PR: merge commit 796ddf2c204319dad84431498622b1433dfae3dd (20 substantive review rounds; this PR also failed and then fixed the Fro Bot check before merge).
The design the review endorsed: derive the verdict exclusively from the structured JSON report and treat the tool's exit code as an informational aside. Exit codes conflate "the tool broke" with "the code is bad," and refusing that conflation is what makes a closed verdict vocabulary possible. flattenReport returning undefined on any malformed entry, so the caller fails closed to instrumentation-failed rather than drifting to clean, is the right default; Timeout and NoCoverage get their own verdicts instead of being laundered through a score.
The blocking finding is a guard that suppresses its own alarm. The reviewer read Stryker's DirectiveBookkeeper and found it computes const reason = (optionalReason ?? DEFAULT_REASON).trim() with DEFAULT_REASON = 'Ignored using a comment'. A reasonless // Stryker disable next-line all therefore arrives in the JSON report carrying a non-empty statusReason, so the report-level hasIgnoredWithoutReason check can never fire for it. That makes the report-level check a near-vacuous backstop and leaves the textual scanDirectiveViolations as the sole load-bearing enforcement of the directive policy. The reviewer then executed the exported scanner against the real module and produced the failing truth table: a disable all whose reason text merely contained the substring next-line was ALLOWED, and the block-comment form /* Stryker disable all */ was ALLOWED, while the bare control case was correctly FLAGGED. The scope check was a substring match over the whole remainder, reason prose included.
The failing-then-passing Fro Bot check on this same PR is the second half of the same story rather than a separate incident. The fixing diff rewrites a plan-document Result block to record what the review rounds actually forced: the test count moving from 68 to 74, plus new coverage for multi-directive-per-line evaluation, the exit-code contract, empty-report handling, missing-mutate-file detection matching minimatch's grammar, report-path injection, and a report-key cross-check against the mutate list. The check caught documentation-of-record drifting behind the code it describes, and the fix was to reconcile the narrative with shipped behavior. Twenty review rounds converging on a single wrapper script is itself a signal: enforcement layers accumulate holes faster than their own tests notice.
Proposed learning: Verify where enforcement actually lives before trusting a guard's self-report, and never scope-match by substring over free-form text. Tools frequently normalize away the exact absence you are checking for — Stryker substitutes a default statusReason for reasonless directives, so a report-level "empty reason" check is structurally incapable of firing and the textual scanner silently becomes the only enforcement. When writing a policy scanner: match directive scope against parsed tokens rather than remainder.includes('next-line') over a string that also contains author-written reason prose; handle block-comment forms alongside line comments; support multiple directives per line; and cross-check report keys against the configured mutate list so a module that never got instrumented cannot report as clean. Derive verdicts from structured output, fail closed on malformed entries, and prove each rule discriminates by deleting it and watching a test go red. Finally, keep plan and Result documentation in the same commit as the behavior it narrates — a doc block lagging its code is a real check failure, not a formality.
Source PR: merge commit
796ddf2c204319dad84431498622b1433dfae3dd(20 substantive review rounds; this PR also failed and then fixed theFro Botcheck before merge).The design the review endorsed: derive the verdict exclusively from the structured JSON report and treat the tool's exit code as an informational aside. Exit codes conflate "the tool broke" with "the code is bad," and refusing that conflation is what makes a closed verdict vocabulary possible.
flattenReportreturningundefinedon any malformed entry, so the caller fails closed toinstrumentation-failedrather than drifting toclean, is the right default;TimeoutandNoCoverageget their own verdicts instead of being laundered through a score.The blocking finding is a guard that suppresses its own alarm. The reviewer read Stryker's
DirectiveBookkeeperand found it computesconst reason = (optionalReason ?? DEFAULT_REASON).trim()withDEFAULT_REASON = 'Ignored using a comment'. A reasonless// Stryker disable next-line alltherefore arrives in the JSON report carrying a non-emptystatusReason, so the report-levelhasIgnoredWithoutReasoncheck can never fire for it. That makes the report-level check a near-vacuous backstop and leaves the textualscanDirectiveViolationsas the sole load-bearing enforcement of the directive policy. The reviewer then executed the exported scanner against the real module and produced the failing truth table: adisable allwhose reason text merely contained the substringnext-linewas ALLOWED, and the block-comment form/* Stryker disable all */was ALLOWED, while the bare control case was correctly FLAGGED. The scope check was a substring match over the whole remainder, reason prose included.The failing-then-passing
Fro Botcheck on this same PR is the second half of the same story rather than a separate incident. The fixing diff rewrites a plan-documentResultblock to record what the review rounds actually forced: the test count moving from 68 to 74, plus new coverage for multi-directive-per-line evaluation, the exit-code contract, empty-report handling, missing-mutate-file detection matching minimatch's grammar, report-path injection, and a report-key cross-check against themutatelist. The check caught documentation-of-record drifting behind the code it describes, and the fix was to reconcile the narrative with shipped behavior. Twenty review rounds converging on a single wrapper script is itself a signal: enforcement layers accumulate holes faster than their own tests notice.Proposed learning: Verify where enforcement actually lives before trusting a guard's self-report, and never scope-match by substring over free-form text. Tools frequently normalize away the exact absence you are checking for — Stryker substitutes a default
statusReasonfor reasonless directives, so a report-level "empty reason" check is structurally incapable of firing and the textual scanner silently becomes the only enforcement. When writing a policy scanner: match directive scope against parsed tokens rather thanremainder.includes('next-line')over a string that also contains author-written reason prose; handle block-comment forms alongside line comments; support multiple directives per line; and cross-check report keys against the configuredmutatelist so a module that never got instrumented cannot report as clean. Derive verdicts from structured output, fail closed on malformed entries, and prove each rule discriminates by deleting it and watching a test go red. Finally, keep plan and Result documentation in the same commit as the behavior it narrates — a doc block lagging its code is a real check failure, not a formality.