Source PR: merge commit 91388ece495680645b76776a4971575e70b61f6e (2 substantive review rounds).
The review framed the right disposition toward mutation survivors: interrogate them instead of papering over them, and when a surviving mutant turns out to be load-bearing, write a test rather than a suppression directive. Applying that discipline surfaced a genuine security finding no coverage metric would have named — a single-element-array decoy where url: ["https://github.com/acme/widget"] coerces through Array.prototype.toString into a value that satisfies an exact-match URL comparison. That is an attribution-spoof vector sitting on the data → main promotion chokepoint. The review's judgment was that pinning this one case was worth more than the other 23 tests in the change combined.
Two mechanical details from the rounds are worth preserving. First, on suppression hygiene: Stryker disable next-line directives must be next-line-scoped, carry a non-empty reason, and name a single mutator. StrykerJS binds next-line to loc.start.line of the node the comment leads, not to the comment's own line — so a reason string that wraps across two lines does not misalign the suppression. That is non-obvious and easy to get wrong in the cautious direction. Second, on guard removal: dropping a grandfatherHash !== undefined && conjunct is behavior-neutral given that every caller supplies a hex digest, but it silently converts a runtime guard into an unstated caller invariant. If a caller later hands the detector a snapshot with a missing hash, the failure surfaces somewhere else entirely.
The review also modeled verification rather than accepting the PR body's claims: targeted vitest runs, pnpm check-types, and ESLint over the changed files were re-run locally, while the per-module mutation figures quoted in the plan document were explicitly marked unverified by me. Naming which claims you checked and which you did not is a reviewer practice worth codifying on its own.
Proposed learning: Type-coercion decoys at trust boundaries, and the hidden cost of deleting a defensive conjunct. Any comparison that treats a parsed value as a string should be tested against a single-element array, because Array.prototype.toString makes ["x"] compare equal to "x" — a live spoofing vector for URL and author attribution checks on promotion gates. Pair this with two rules: mutation survivors get tests, not directives, unless the survivor is provably semantically neutral; and removing a !== undefined guard is not free — it relocates a runtime check into an invariant that no signature enforces, so either keep the guard or make the invariant explicit in the type.
Source PR: merge commit
91388ece495680645b76776a4971575e70b61f6e(2 substantive review rounds).The review framed the right disposition toward mutation survivors: interrogate them instead of papering over them, and when a surviving mutant turns out to be load-bearing, write a test rather than a suppression directive. Applying that discipline surfaced a genuine security finding no coverage metric would have named — a single-element-array decoy where
url: ["https://github.com/acme/widget"]coerces throughArray.prototype.toStringinto a value that satisfies an exact-match URL comparison. That is an attribution-spoof vector sitting on thedata → mainpromotion chokepoint. The review's judgment was that pinning this one case was worth more than the other 23 tests in the change combined.Two mechanical details from the rounds are worth preserving. First, on suppression hygiene:
Stryker disable next-linedirectives must benext-line-scoped, carry a non-empty reason, and name a single mutator. StrykerJS bindsnext-linetoloc.start.lineof the node the comment leads, not to the comment's own line — so a reason string that wraps across two lines does not misalign the suppression. That is non-obvious and easy to get wrong in the cautious direction. Second, on guard removal: dropping agrandfatherHash !== undefined &&conjunct is behavior-neutral given that every caller supplies a hex digest, but it silently converts a runtime guard into an unstated caller invariant. If a caller later hands the detector a snapshot with a missinghash, the failure surfaces somewhere else entirely.The review also modeled verification rather than accepting the PR body's claims: targeted vitest runs,
pnpm check-types, and ESLint over the changed files were re-run locally, while the per-module mutation figures quoted in the plan document were explicitly marked unverified by me. Naming which claims you checked and which you did not is a reviewer practice worth codifying on its own.Proposed learning: Type-coercion decoys at trust boundaries, and the hidden cost of deleting a defensive conjunct. Any comparison that treats a parsed value as a string should be tested against a single-element array, because
Array.prototype.toStringmakes["x"]compare equal to"x"— a live spoofing vector for URL and author attribution checks on promotion gates. Pair this with two rules: mutation survivors get tests, not directives, unless the survivor is provably semantically neutral; and removing a!== undefinedguard is not free — it relocates a runtime check into an invariant that no signature enforces, so either keep the guard or make the invariant explicit in the type.