Record equivalent mutants by what they sit inside, and catch drift on every commit - #2037
Conversation
`scripts/mutation/anchor.ts` names the thing a mutant sits inside — the function, method, or value it belongs to — plus an ordinal when more than one mutant of the same kind shares that name. This is the stable replacement for `path:line:column` in the equivalent-mutant registry, which any edit *above* a recorded expression silently invalidated. Checked against the live registry: all 463 currently-resolving entries produce 463 distinct anchors, no collisions. 133 need an ordinal. Not done yet: the registry format still records line:column, nothing reads anchors, and this module has no tests. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01P4fF7zjiMi9bkHG22oFzh1
…lumn
The registry recorded each known-equivalent mutant as `path:line:column`, so
any edit *above* a recorded expression silently invalidated its entry — the
mutant stopped being suppressed and the audit failed, without the recorded
expression having changed at all. It happened twice on one branch, both times
found by a reviewer rather than by a check.
Entries now read `path::anchor from → to`, where the anchor names the thing
the mutant sits inside:
src/fp.ts::collectionCache.generation 0 → 1
src/features/public/ticket-payment.ts::checkAvailability ?? → ||
Adding a comment, an import, or a whole unrelated function above one moves no
anchor. Renaming the function it sits in, or adding another mutant of the same
kind inside that function, does — and both are real changes to the thing being
recorded. Where several mutants of one kind share a name they get `@1`, `@2`
in source order; `@` rather than `#`, because a registry line ends with a `#`
comment and a `#` in the anchor would be read as the start of one.
All 509 entries are migrated. 463 took their anchor from current source. The
other 46 had *already* gone stale — the drift the TODO describes, which had
grown from the 18 recorded there — and were repointed by reading each entry's
source as it was at the commit that added the line, taking the anchor from
there, and carrying it forward. Six in `groups.ts` and one in `images.ts`
needed a hand: each was resolved against the reason written beside it, and the
six turned out to be a clean ten-line shift, same column.
The anchor lives on `Mutant` itself, so `mutantKey` stays a plain function of
the mutant and every caller is unchanged. `generateMutants` anchors on the way
out, reusing the parse it already did.
Also: loading a registry file now fails on a line that is neither blank nor a
comment but does not parse. It used to skip such a line, which is how an
earlier attempt at this change quietly dropped 145 entries — the first ordinal
separator was `#`, so every ordinalised entry was truncated at what looked
like a comment. Nothing reported it; the entries simply stopped existing.
Not done yet: the audit is not in precommit, and none of this is tested.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01P4fF7zjiMi9bkHG22oFzh1
Twelve cases for `anchor.ts`, the two load-bearing ones being that adding unrelated lines above a recorded expression — and growing the function it sits in above it — both leave its anchor alone. That is the drift the old format could not survive. The existing mutation tests move to the new format. One changes meaning rather than shape: a registry line that neither parses nor reads as a comment used to be skipped, and is now an error, so the test that asserted it was ignored now asserts it fails. That silent skip is how an earlier attempt at this change dropped 145 entries without a word. README.txt now describes anchors, the dotted nesting, and why the ordinal is `@n` rather than `#n`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01P4fF7zjiMi9bkHG22oFzh1
`deno task check:equivalents` resolves every known-equivalent entry against freshly generated mutants and nothing else — no lint, no type-check, no tests. It runs in 1.4 seconds, so it joins `precommit` rather than staying a thing somebody remembers to run. That was the second half of the TODO, and it is what turns the drift from a review finding into a build failure. Re-proving that each entry is still genuinely equivalent is the expensive half: it applies every mutant through lint and type-check, and stays `mutation:audit-equivalents`. Checked both directions by hand. Renaming the function an entry is anchored on fails the check, naming the six entries that moved with it. Adding lines above that same function does not — which is the case that used to break every entry below it in the file, and the reason this whole change exists. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01P4fF7zjiMi9bkHG22oFzh1
`precommit` typechecks test files and `test:files` does not, so these two literals built a Mutant without the new required field and only the full gate noticed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01P4fF7zjiMi9bkHG22oFzh1
`check:equivalents` had its own copy of the loader's parse-or-throw loop, so the two could have drifted on what counts as a malformed line — the one thing this whole change is about not letting happen quietly. Both now call `parseRegistryText`. Also folds the two identical nullish-mutant filters in the anchor tests into one helper. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01P4fF7zjiMi9bkHG22oFzh1
Coverage found the two branches nothing reached: a class member whose name is
written as a string literal ("read-it"), and one written as an empty string,
which says nothing about where the mutant is and so falls back to the class.
Both are real shapes, so they get cases rather than being deleted.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01P4fF7zjiMi9bkHG22oFzh1
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (11)
📝 WalkthroughWalkthroughThe mutation system now uses stable AST-based anchors and fingerprints instead of line and column locations. Registry validation, catalogs, survivor reporting, tests, and precommit automation were updated. Turso upload handling now suppresses a specific late body-stream rejection. ChangesEquivalent-mutant infrastructure
Turso upload handling
Project follow-ups
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant MutationRunner
participant generateMutants
participant anchorMutants
participant EquivalentRegistry
participant SurvivorSummary
MutationRunner->>generateMutants: generate raw mutants
generateMutants->>anchorMutants: anchor generated mutants
anchorMutants-->>MutationRunner: return anchored mutants
MutationRunner->>EquivalentRegistry: match anchored registry keys
EquivalentRegistry-->>SurvivorSummary: provide registry entry
SurvivorSummary-->>MutationRunner: render survivor details and recording hint
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
Merging main brought its own registry changes, and the new check immediately found 13 entries in `attendee-page-data.ts` pointing at nothing — plus one main had just added at a column six characters off the real mutant. My branch changes no `src/` file and that file is byte-identical to main's, so this is drift that was already sitting on main with nothing to catch it. Which is the case for the check. Twelve were placed by the expression each entry's own reason names. The thirteenth described `amountPaid`, a field no longer in the file, so it is removed rather than repointed. Three more needed their ordinal corrected: they were migrated before the merge and main has since added mutants above them inside the same function, which is exactly when an ordinal is supposed to move. `check:equivalents` is clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01P4fF7zjiMi9bkHG22oFzh1
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2b01588c6b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const nth = (seen.get(key) ?? 0) + 1; | ||
| seen.set(key, nth); | ||
| const { name, ...rest } = mutant; | ||
| return { | ||
| ...(rest as unknown as M), | ||
| anchor: totals.get(key)! > 1 ? `${name}@${nth}` : name, |
There was a problem hiding this comment.
Preserve mutation identity when ordinal neighbors move
When an enclosing name has several identical from → to mutants, the anchor identifies them only by their current ordinal. If an earlier same-kind expression is inserted, deleted, or reordered while the recorded ordinal still exists, the registry entry silently points to a different mutant; checkEquivalentMutants then passes and isIgnored can suppress a non-equivalent survivor that was never audited. Include stable local expression context in the anchor rather than relying only on @n.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct, and this was the important one. Fixed in 37f70db.
An anchor now carries a fingerprint of the expression it mutates, taken from the smallest node that strictly contains the mutated span. For a swapped operator that is the expression itself (a ?? 0), not the array it shares with its neighbours; for a replaced literal it is the node one step out, which is what gives that literal its meaning. So resolving an anchor and verifying it became the same act — pointing at a neighbour is no longer expressible.
Ordinals survive only for mutants sharing a name, a from → to, and character-identical text, which nothing can tell apart anyway.
Your exact scenario is now a test — survives a same-kind mutant inserted above it — inserting c ?? 9 ahead of a ?? 0, b ?? 1 and asserting both original anchors are unchanged. Under the old scheme they shifted by one.
Worth saying plainly: this was a shortcut, not an oversight. The TODO this PR implements asked for "the expression text plus its enclosing function name", and I substituted an ordinal to keep the registry line easy to parse. My own repointing commit on this branch then hit the failure in the wild — three entries needed their ordinals corrected because main had added mutants above them inside the same function — and I wrote that up as the system working correctly rather than as the bug you found.
Generated by Claude Code
| // Neither a path nor an anchor contains a space, so the first run of | ||
| // whitespace after the anchor ends the location and starts the `from → to`. | ||
| const match = body.match(/^(\S+)::(\S+)\s+(.*?)\s*→\s*(.+?)$/); |
There was a problem hiding this comment.
Encode legal member names before serializing anchors
For a legal quoted member such as class Reader { "read it"() { return x ?? 0; } }, nameOf includes the space in the anchor, but this parser requires the entire anchor to match \S+; a # in a quoted name is even removed as a comment first. Any equivalent survivor inside such a member therefore cannot be represented in the registry and makes the mutation gate permanently fail. Escape or encode anchor names instead of assuming they contain neither spaces nor comment markers.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Right, and broader than the name. Fixed in 37f70db.
Anchor names are percent-encoded to a fixed set (A-Za-z0-9_$-, plus . joining segments), so "read it" becomes R.read%20it and "a#b" becomes R.a%23b. Nothing an anchor can hold ends a line early or starts its reason.
Writing the round-trip test for this then found the half your comment did not reach: the operator side has the same problem. A mutated string literal carries its own text into the displayed from → to, so a literal holding a # was truncated into a comment exactly like the name would be. Two more of the same shape came out with it — a literal holding a newline, and one with edge whitespace, where "; " lost its trailing space to the spacing around the arrow and collided with ";". All three are escaped now, interior spaces left alone so a removed statement still reads as itself.
The test is anchor-round-trip.test.ts: every mutant in four real source files plus six deliberately awkward shapes, written as a registry line, read back, asserted identical and unique. That is what turned this from "fix the case in the report" into "the class is checked mechanically" — and it is the same class as the # ordinal that silently dropped 145 entries earlier on this branch.
Generated by Claude Code
| ); | ||
| }; | ||
|
|
||
| export const checkEquivalentMutants = async (): Promise<string[]> => { |
There was a problem hiding this comment.
Add direct tests for the precommit equivalent check
No test in this commit imports checkEquivalentMutants; its only caller is the new precommit subprocess, so its success, duplicate, and stale-entry branches have no direct in-process coverage. Add unit tests with controllable registry/source inputs rather than relying on incidental CLI execution, as the repository explicitly requires deterministic direct coverage for subprocess code.
AGENTS.md reference: AGENTS.md:L74-L74
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fair. Fixed in 37f70db and 7bbd57f.
Adding the tests immediately proved the second half of your point: importing the module in-process pulled its import.meta.main block into the coverage gate, where no test can reach it. So the rules moved to scripts/mutation/check-equivalents.ts and the command line stayed a thin shell nothing imports — the split AGENTS.md prescribes for exactly this.
Nine cases in test/scripts/mutation/check-equivalents.test.ts, each with its own registry and source under a temp root: a clean pass, comments and blanks skipped, a stale entry, a duplicate, three rejected path spellings, and a malformed line raising.
Making it testable also removed two branches worth losing — the registry directory and project root are named by the caller now instead of defaulted, and the "which spelling is this path" branch went away because registryFilePath already answered it.
Generated by Claude Code
| const keysFor = async (sourcePath: string): Promise<Set<string>> => { | ||
| const file = resolve(projectRoot, sourcePath); | ||
| const content = await Deno.readTextFile(file); |
There was a problem hiding this comment.
Reject non-canonical registry source paths
When an entry uses an absolute path or an in-project alias such as ../tickets/src/foo.ts, resolve reads the intended file and generated keys retain that exact spelling, so this precommit check reports success. The mutation runner, however, keys the same file through its canonical project-relative path (src/foo.ts), meaning the accepted entry never suppresses its mutant and is skipped by per-file registry validation. Reject absolute paths and require sourcePath to equal the canonical relative path, matching the audit's path checks.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed. Fixed in 37f70db.
whyPathUnusable now rejects three spellings: an absolute path, one escaping the project, and one that resolves to a file inside the project but is not written as the canonical relative path. The last is the case you named — the error says path must be written as "src/foo.ts" rather than just refusing.
Three of the nine checker tests cover them, including ../<root>/src/read.ts reaching out and back.
Generated by Claude Code
| * The equivalent-mutant registry used to record `path:line:column`, so any edit | ||
| * *above* a recorded expression silently invalidated its entry — the mutant | ||
| * stopped being suppressed and the audit failed, without the recorded | ||
| * expression having changed at all. That happened repeatedly, and was always | ||
| * found by a reviewer rather than by a check. |
There was a problem hiding this comment.
Remove historical implementation narratives from comments
This module header describes the former line-and-column implementation, past review failures, and what the new code replaced rather than documenting only the current anchor contract; similar historical narratives were also added to the checker and ignore-list comments. Move that history to the commit message and keep only the present constraint, as the repository explicitly forbids old-implementation comparisons in source comments and requires comments to stay short.
AGENTS.md reference: AGENTS.md:L71-L72
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
You're right, and I wrote those deliberately, which makes it worse. Fixed in 37f70db.
anchor.ts, ignore.ts and the checker now describe the contract only: what an anchor is made of, which characters it may hold, and when it moves. The history — the line-and-column format, the reviews that caught it, the entries it lost — is in the commit messages, where git keeps it for whoever needs it.
I kept one sentence that reads like history but is not: the note on descendTo explaining why the fingerprint comes from the strictly-containing node rather than the deepest one. That is a constraint on the current code — pick the wrong node and neighbouring expressions collide — not a comparison with what came before.
Generated by Claude Code
|
|
||
| /** A mutant before it has been told what it sits inside. The generators build | ||
| * these; `generateMutants` anchors them on the way out. */ | ||
| export interface RawMutant { |
There was a problem hiding this comment.
Keep the raw mutant type private
RawMutant is referenced only inside generate.ts, so exporting it creates an unused internal API without giving any production caller a capability it needs. Remove the export modifier rather than preserving a test-only or hypothetical surface, as the repository explicitly requires unused exports to be deleted.
AGENTS.md reference: AGENTS.md:L81-L81
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct. Fixed in 37f70db — RawMutant is private again.
It was only ever exported as a slip while splitting the builders' return type from the anchored one; nothing outside generate.ts referenced it.
Generated by Claude Code
…cucumber-bq6mji # Conflicts: # scripts/mutation/equivalent-mutants/features.txt # scripts/mutation/equivalent-mutants/shared-db.txt
Six review findings, and one the fix for them turned up. **An ordinal could point somewhere else.** An anchor said `name@2`, so inserting another mutant of that kind earlier in the same function shifted every number below it — each recorded entry silently naming its neighbour, with the check none the wiser. An anchor now carries a fingerprint of the expression it mutates, taken from the smallest node strictly containing the mutated span: `a ?? 0`, not the array it shares with its siblings. Resolving an anchor and verifying it became the same act. Ordinals remain only for mutants whose text is character-identical, which nothing can tell apart. **A name could not always be written down.** A member called `"read it"` made an anchor with a space in it, which the parser cannot read; one called `"a#b"` made a `#`, which starts a line's reason. Names are percent-encoded. **So could an operator.** Writing the round-trip test found what the review did not: a mutated string literal carries its own text into the displayed `from → to`, so a literal holding a `#` was truncated into a comment, and one holding edge whitespace — `"; "` — lost it to the spacing around the arrow and collided with `";"`. Both are escaped now. **The checker had no tests** beyond the precommit subprocess, and accepted any path spelling that resolved. It takes its registry and root as options, has nine cases, and rejects a path that is absolute, escapes the project, or is not written as the canonical project-relative one the runner keys by. `RawMutant` is no longer exported, and the module comments describe what the code does rather than what it replaced. All 522 entries are migrated, joined on line and column so each kept the mutant it already named. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01P4fF7zjiMi9bkHG22oFzh1
Adding direct tests pulled the whole file into the coverage gate, including the `import.meta.main` block no in-process test can reach — the trap AGENTS.md describes for exactly this case. The rules move to `scripts/mutation/check-equivalents.ts`, tested; the command line stays a thin shell that nothing imports. That split also removed two branches worth having gone: the registry directory and project root are now named by the caller rather than defaulted, and the "which spelling is this path" branch is gone, since `registryFilePath` already answered it. The span fallback in `descendTo` is gone too. The program is itself a span holding every offset, so there was never a case with none. Six cases for what a mutated literal drags into a key: a comment mark, a newline, a percent, and whitespace at either edge — each of which would otherwise end the line, start its reason, or vanish into the spacing around the arrow and let `"; "` and `";"` share one key. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01P4fF7zjiMi9bkHG22oFzh1
A file that is one statement with nothing after it: removing the statement covers the file, so no node strictly contains the mutant and the fingerprint falls back to the statement itself. Reachable, so it gets a case rather than being deleted as impossible. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01P4fF7zjiMi9bkHG22oFzh1
…cucumber-bq6mji # Conflicts: # scripts/mutation/equivalent-mutants/shared-a-l.txt
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a2ac3fd65a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const containing = spans.filter((span) => span.end - span.start > width); | ||
| return { context: containing.at(-1) ?? spans.at(-1)!, names }; |
There was a problem hiding this comment.
Fingerprint whole-span mutations from the mutated node
Fresh evidence beyond the earlier ordinal issue is the new > width selection: when a removal mutant exactly spans its statement, this excludes the statement itself. Inside a function it fingerprints the enclosing block, so an unrelated edit makes valid registry entries stale; when the statement spans the whole file, the fallback can fingerprint its deepest child, so changing foo(1); to foo(2); can retain the old anchor and let the checker suppress a mutant that was never audited. Choose a context that represents the exact mutated statement rather than relying on width alone.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed, and worse than you described. Fixed in ba55141.
I reproduced both halves before touching anything. At file scope, foo(1); and foo(2); produced the identical anchor %3cfile%3e~1b5leqf — the fallback took the deepest node under the mutant's first character, which is the foo identifier, unchanged between the two. So an entry recorded against one would have suppressed the other outright. Inside a block, foo(1);→(removed) and bar();→(removed) both fingerprinted the block, and adding a third statement moved both.
The fingerprint now stops at the mutant's own statement. It still climbs to the expression that gives a swapped operator or a replaced literal its meaning — a ?? 0 for a ??, foo(1) for the 1 — but never past the statement, and the candidate set is filtered to nodes holding the whole mutated span, so a node narrower than the mutant is no longer reachable at all.
Three tests lock it: survives a statement being added beside a removed one, tells apart two statements removed from one block, and anchors a mutant that spans the whole file on that statement (which is your foo(1); vs foo(2); case).
Five registry entries were anchored under the old rule and are re-recorded, including two cart.ts entries that had been sharing one block fingerprint.
Generated by Claude Code
| # Format — one entry per line, plus a reason: | ||
| # <path>::<anchor> <from> → <to> # why it is equivalent | ||
| # | ||
| # The anchor names what the mutant sits inside — a function, a method, a value, | ||
| # nested names joined by dots — then `~` and a fingerprint of the expression it |
There was a problem hiding this comment.
Print canonical anchors in survivor reports
When a mutation run finds a new equivalent survivor, this format requires a generated fingerprinted anchor, but survivorLocation in scripts/mutation/summary.ts:98-99 still prints only path:line:column, and no command or report exposes the anchor. An operator therefore cannot turn the reported survivor into a valid registry entry without writing custom code to call the generator; include the canonical key or anchor in the terminal and Markdown survivor output.
AGENTS.md reference: AGENTS.md:L77-L77
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fair, and a real gap — I changed what a registry entry looks like without changing the one place that tells an operator what to write. Fixed in ba55141.
Survivor reports now print the whole registry line, not a location. Terminal:
src/fp.ts:12:9
src/fp.ts::collectionCache.generation~0dbfxl4 0→1
Proven unkillable by any test? Paste its line above into a file under
scripts/mutation/equivalent-mutants/, followed by # and the reason.
The Markdown table's second column is the same string, and its header changed from mutation to registry entry. The location stays alongside because it is what you click to reach the code.
The line is genuinely pasteable, not just informative: it is the exact output of mutantKeyForPath, which the round-trip test proves reads back as itself.
One thing worth surfacing: making the summary reach the key exposed a cycle — rel lived in summary.ts, which the registry imports. rel is a path helper, not a summary concept, so it moved to project-root.ts (as a thin call to the existing toDisplayPath) and both sides now reach it without importing each other.
Generated by Claude Code
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f132c513ec
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const { base, ...rest } = mutant; | ||
| return { | ||
| ...(rest as unknown as M), | ||
| anchor: totals.get(key)! > 1 ? `${base}@${nth}` : base, |
There was a problem hiding this comment.
Replace source-order fallback for identical expressions
When character-identical expressions occur under the same collected name but in different lexical contexts, they are not necessarily equivalent: for example, two object callbacks can both contain x ?? "" while one accepts string | null and the other accepts number | null, making ?? → || equivalent only for the first. Because object property names are not collected, these receive @1/@2; reordering them or inserting another identical callback reassigns an existing anchor, so checkEquivalentMutants still passes and the runner can suppress the non-equivalent mutant. Fresh evidence beyond the resolved ordinal thread is that this remaining @n fallback still identifies character-identical expressions solely by source order.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Half fixed, half irreducible — worth separating the two. Fixed in ba55141.
Your concrete case was right and is gone: Property is now a naming type, so { first: (x) => x ?? "", second: (x) => x ?? "" } yields o.first~… and o.second~… instead of @1/@2. That matters far more here than the example suggests, because so much of this codebase is config objects and dispatch maps — the anchors got visibly more precise, e.g. defineTable.updateStatement~1p65ca8 became defineTable.updateStatement.args~1p65ca8.
It re-anchored 124 of the 519 entries. Every one was carried across mechanically by matching its old anchor to the mutant it named and writing the new one — 0 unmatched, 0 ambiguous — and check:equivalents confirms all 519 still resolve.
The residual @n I do not think can be removed, and I would rather say so than claim otherwise. It only fires when two mutants share an enclosing name, a from → to, and character-identical text. At that point there is no content left to distinguish them: widening the fingerprint outward gives both the same wider text, so any further signal is positional by construction. Ordering is the only thing left, and it is at least honest about what it is.
What I have done instead is shrink the surface and stop hiding it: 4 of 519 entries carry an ordinal, and both the module doc and the registry README now say plainly that the ordinal is the one part of an anchor a reordering can move, and that an entry carrying one is worth re-checking when its neighbours change.
Generated by Claude Code
| const escapeForLine = (text: string): string => | ||
| text | ||
| .replaceAll("%", "%25") | ||
| .replaceAll("#", "%23") | ||
| .replaceAll("\n", "%0a") |
There was a problem hiding this comment.
Escape the registry arrow inside mutation labels
When a runtime string literal contains a spaced arrow, such as "left → right", its value becomes the mutant's displayed operator, but this encoder leaves → untouched. parseIgnoreLine treats the first arrow as the field delimiter and absorbs its surrounding spaces, changing the generated key from left → right→"" to left→right→""; the entry can therefore never match the generated mutant and the new precommit check reports it as stale. Encode the delimiter character in mutation labels as well.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct. Fixed in ba55141 — escapeForLine now encodes → as %e2%86%92, alongside the %, #, newline and edge-whitespace cases it already handled.
The more useful part of your comment is that it names a hole in the round-trip test I added for exactly this class. That test writes every mutant of four real files plus a set of deliberately awkward shapes as a registry line, reads it back, and asserts it is identical and unique — and it did have an arrow case, but on the name side (class R { "a→b"() ... }), which goes through encodeName rather than escapeForLine. The operator side had no arrow case at all, so the check passed while the hole stayed open.
"left → right" is in the corpus now, along with "a#b" as a literal and a repeated-statement shape.
Generated by Claude Code
| } from "./ignore.ts"; | ||
|
|
||
| /** Where the checked-in registry lives. */ | ||
| export const DEFAULT_REGISTRY_DIR: URL = EQUIVALENT_MUTANTS_DIR; |
There was a problem hiding this comment.
Remove the registry-directory alias export
DEFAULT_REGISTRY_DIR adds a second exported name for EQUIVALENT_MUTANTS_DIR without adding a default, transformation, or guard; its only caller could import the shared registry directory directly. Keeping both names creates the internal alias layer that this repository explicitly forbids, so expose and use the underlying constant itself.
AGENTS.md reference: AGENTS.md:L79-L79
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Right. Fixed in ba55141 — DEFAULT_REGISTRY_DIR is gone and the command line imports EQUIVALENT_MUTANTS_DIR directly.
It was a leftover from splitting the checker's rules out of its CLI: the caller needed the constant, and instead of importing it I re-exported it under a new name in the file it had just moved out of. No default, no transformation, no guard — the alias layer the rule is about.
Generated by Claude Code
…nder A statement-removal mutant exactly fills its own node, so the old "smallest node strictly wider than the mutant" rule skipped past it: inside a block it fingerprinted the whole block, and at the top of a file it fell back to the deepest node under the mutant's first character. Both are wrong in the dangerous direction — every entry in a block went stale when any one line beside it was edited, and `foo(1);` and `foo(2);` shared an anchor at file scope, so a registry entry for one silently suppressed the other. The fingerprint now stops at the mutant's own statement: it still climbs to the expression that gives a swapped operator or a replaced literal its meaning, but never past the statement, and never to something narrower than the mutant. Object properties now contribute their name, so two callbacks in one config object no longer share an anchor and fall back to source order. This re-anchors 124 existing entries; every one was carried across by matching its old anchor, and `check:equivalents` confirms all 519 still resolve. Survivor reports print the whole registry line rather than just a location, so a proven-equivalent survivor can be pasted straight in — and an arrow inside a mutated string literal is now escaped, so such a line reads back as what was written. `rel` moved to `project-root.ts`, where it belongs and where the summary and the registry can both reach it without a cycle. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01P4fF7zjiMi9bkHG22oFzh1
…cucumber-bq6mji # Conflicts: # scripts/mutation/equivalent-mutants/features.txt # scripts/mutation/equivalent-mutants/shared-a-l.txt # scripts/mutation/equivalent-mutants/shared-m-z.txt
`still rejects when the server replies before the upload finishes` has a server that answers 400 without reading the 8MB body, so the upload promise settles while the client is still streaming. `uploadTursoDatabaseFile` stands its unhandled-rejection watch down the moment it settles — by design, so an unrelated failure after the upload still fails loudly — which leaves a window where the node:http polyfill's duplicate body-stream rejection lands with nobody watching. That reads as a suite-level failure with no test location, which is the shape this file has failed with twice in CI (`unknown location - Turso migration file`). The test server now keeps the requests it hands out and destroys them before shutting down, so a socket's failure stays inside the test that made it. Stated plainly: this closes a real leak whose shape matches the failure, but the failure does not reproduce here — 18 runs of the file are clean both before and after. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01P4fF7zjiMi9bkHG22oFzh1
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a82c6a1230
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const withinStatement = holding.slice(holding.findLastIndex(isBoundary)); | ||
| const wider = withinStatement.findLast( | ||
| (found) => found.end - found.start > width, | ||
| ); | ||
| return { context: wider ?? withinStatement.at(-1)!, names }; |
There was a problem hiding this comment.
Fingerprint the type context that makes equivalence valid
When equivalence depends on the operand's type, this selection stops at the logical expression and ignores its parameter or type declaration. Changing const read = (x: string | null) => x ?? "" to accept number | null leaves the anchor unchanged, so precommit passes and ?? → || remains ignored even though input 0 distinguishes it. Fresh evidence beyond the resolved whole-span finding is this unchanged-expression/type-change case; include the semantic declaration context in the anchor, or otherwise invalidate the entry, so the survivor must be re-audited.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
You are right about the hole, and I am not fixing it here — I tried, measured the cost, and reverted. Recorded in TODO.md under "An equivalence proof rests on types the anchor cannot see".
First, a correction to the severity: isIgnored is applied only to mutants that already survived (run-file.ts:96). Every mutant is still fully evaluated. So after your type change, if any test exercises read(0) the mutant dies and reports as killed — the entry cannot hide it. It hides something only when no test distinguishes the two, which is a real test gap, but a narrower one than "precommit passes and the mutant is silently ignored".
I did implement your suggestion: fingerprint the enclosing function's head (everything before its body, so parameters and their types count and the body does not). It works — and it costs more than it buys:
- It invalidates far too much. Adding or renaming any parameter changes the head, so every entry in that function's body goes stale, including ones whose proof has nothing to do with the new parameter. My own test
survives a same-kind mutant inserted above itfailed immediately, because it adds a parameter. - It still misses most of the class. I counted: 166 of the 535 recorded reasons rest on a type that comes from a call's return, a database row, or an imported shape —
getWebhookSignatureHeader → null|truthy,intent.allocations is an array or undefined,amount is bigint|undefined. A function head cannot see any of those.
So it would close a minority of cases while making the gate noisy enough that people learn to re-record stale entries without re-reading the proof — which makes the registry less trustworthy, not more.
The honest statement is that an anchor is a syntactic identity for which mutant this is, and it cannot be a validity check on a type-based proof, because the types come from all over the codebase. Re-proving needs a different mechanism, and the TODO says what I think it looks like: give mutation:audit-equivalents a way to attempt a distinguishing input, or a re-audit stamp so an entry has to be re-confirmed rather than resting on a proof nobody has re-read since it was written.
Generated by Claude Code
There was a problem hiding this comment.
Two more unused exports from this PR are now private as well — EquivalentCheckOptions and AnchoredMutant. Same slip as RawMutant and ANCHOR_PATTERN: I exported the types I happened to name while splitting things up, without checking anything imported them.
Replying here rather than on those threads only because they are the same point; each is fixed.
Generated by Claude Code
| const literalKey = node.key?.value; | ||
| return typeof literalKey === "string" && literalKey !== "" | ||
| ? literalKey | ||
| : null; |
There was a problem hiding this comment.
Name numeric properties before falling back to ordinals
When object members use legal numeric keys, Oxc exposes key.value as a number, so this string-only branch contributes no property name. Two character-identical callbacks under keys 1 and 2 therefore share the enclosing name and fingerprint and receive source-order @n; reordering those properties can make an entry audited for one callback suppress the other callback, whose contextual type and equivalence may differ. Fresh evidence beyond the resolved named-property thread is that numeric literal keys remain unnamed; encode legal non-string literal keys before anchoring.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct — nameOf only accepted a string key.value, so { 1: … } contributed nothing. Fixed: a numeric key now names its member, so const o = { 7: (x) => x ?? 0 } anchors on o.7.
Test: names an object member written as a number, plus a { 1: (x) => x ?? "", 2: (x) => x ?? "" } case in the round-trip corpus, which is exactly your two-identical-callbacks shape — both keys come back distinct with no ordinal.
No registry entry moved, so nothing here rested on a numeric key today. That is the point of catching it now rather than the first time one appears.
Generated by Claude Code
| * Every past failure of this registry broke one leg of that trip. A key that | ||
| * could not be written kept its `#` and was truncated into a comment. A key | ||
| * that could not be read back was skipped in silence. A key that read back but | ||
| * named a different mutant let an unaudited survivor pass as equivalent. These | ||
| * cases walk the trip end to end over real source, so the next break is a |
There was a problem hiding this comment.
Remove the historical failure narrative from the test header
This added header recounts past registry failures and what the previous behavior did, while lines 2–4 already state the current invariant completely. Remove the historical narrative and keep only the present contract, as repository guidance explicitly forbids comments that compare current code with old implementations and asks for short comments.
AGENTS.md reference: AGENTS.md:L71-L72
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fair, and it is the same slip you caught in anchor.ts last round — I removed the narrative there and then wrote a fresh one in the test file. The header is now four lines: what the invariant is, and that the cases walk it end to end.
The history it recounted belongs in the commit that added the test, which is where it now lives.
Generated by Claude Code
| .replaceAll("%", "%25") | ||
| .replaceAll("#", "%23") | ||
| .replaceAll("\n", "%0a") | ||
| .replaceAll("→", "%e2%86%92") |
There was a problem hiding this comment.
Escape carriage returns inside mutation labels
When a string literal contains an interior carriage return, such as "a\rb", Oxc supplies that decoded value as the displayed operator, but this encoder escapes only \n. The generated registry line therefore contains a raw \r, which the parser's dot-based mutation pattern cannot match, so the survivor cannot be recorded and check:equivalents rejects the entry. Fresh evidence beyond the resolved arrow-delimiter issue is this other legal line terminator; encode \r and the remaining JavaScript line separators as well.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Right, and it went wider than \r. Fixed.
escapeForLine now encodes \r, and alongside \n, and the edge-whitespace rule encodes each character byte by byte rather than emitting %20 per character. The old form quietly collapsed a leading tab and a leading space onto the same escape — the exact defect the trailing-space case was added to prevent, in the other direction. CodeRabbit spotted that half independently; both are closed by the same change.
Three tests: escapes a carriage return, which ends the line just as a newline does, tells a leading tab from a leading space, and an "a\rb" case in the round-trip corpus.
The by-byte encoder is now shared with encodeName in anchor.ts as percentEncode — jscpd flagged the two as one thing the moment I wrote the second, and it was right.
Generated by Claude Code
| /** Every character an anchor can hold, for callers validating one. */ | ||
| export const ANCHOR_PATTERN = /^[A-Za-z0-9_$\-.%~@]+$/; |
There was a problem hiding this comment.
Remove the test-only anchor-pattern export
ANCHOR_PATTERN has no production caller; its sole import is in anchor-round-trip.test.ts, only to inspect this internal representation. Remove the export and its test-only use rather than creating a public surface solely for a test, as repository guidance explicitly treats an export used only by tests as dead code.
AGENTS.md reference: AGENTS.md:L81-L81
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct. ANCHOR_PATTERN and the test that used it are both gone.
The assertion it backed — "every anchor holds only anchor characters" — was already redundant: the round-trip test writes each key to a registry line, reads it back, and asserts it is identical, which fails on any character that would not survive the line. That is the property that matters, tested through the behaviour rather than by inspecting the representation.
I exported it to make a test possible, which is the thing the rule is about.
Generated by Claude Code
| const survivorRow = survivorFormatter( | ||
| (location, operator, newOperator) => | ||
| `| \`${location}\` | \`${operator}\` → \`${newOperator}\` |`, | ||
| (location, entry) => `| \`${location}\` | \`${entry}\` |`, | ||
| ); |
There was a problem hiding this comment.
Escape registry entries before inserting them into Markdown tables
When a survivor uses || or another mutation label containing |, this renderer places the raw registry entry inside a Markdown table cell. Raw pipes delimit table columns even when surrounded by an inline-code span, so common entries such as ??→|| split the row into extra cells; labels containing backticks can also terminate the code span. Render the entry with table-safe escaping or HTML code markup while keeping the underlying registry text unchanged.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct, and it was worse than the entry — the location column had the same problem, and so did the old `??` → `||` rendering this replaced. GFM treats | as a column delimiter before it treats a backtick as a code span, so every ?? → || survivor has been splitting its own row into extra cells.
Fixed: both cells now render as <code> with &, < and | escaped as entities. That keeps mutantKeyForPath canonical — the escaping is presentation only, and the text inside the element is still exactly the line to paste.
Regression test: keeps a survivor holding pipes inside one table cell renders a real ?? → || survivor and asserts the row splits into exactly the two columns the header declares, and that the pipes survive as ||.
A backtick can't reach the cell any more, because <code> has no delimiter to close — that was the other half of the problem with a fixed one-backtick span.
Generated by Claude Code
| /** Canonical key for a mutant at a project-relative path. */ | ||
| export const mutantKeyForPath = (relPath: string, mutant: Mutant): string => | ||
| `${relPath}:${mutant.line}:${mutant.column} ${mutant.operator}→${mutant.newOperator}`; | ||
| `${relPath}::${mutant.anchor} ${escapeForLine(mutant.operator)}→${escapeForLine(mutant.newOperator)}`; |
There was a problem hiding this comment.
Encode source paths before writing registry keys
When a mutable source file's canonical path contains whitespace, this function writes the path verbatim, but parseIgnoreLine requires the entire path before :: to match \S+. A survivor in a legal path such as src/read it.ts therefore produces a suggested registry entry that cannot be parsed, while a # in the path is truncated as a comment. Fresh evidence beyond the resolved anchor-name encoding issue is that the source-path field remains unencoded; encode it too or parse the explicit delimiter without forbidding legal path characters.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid, and I took your second option — parse the delimiter properly rather than encode the path.
parseIgnoreLine matched the path with \S+, which forbade a legal space. It now matches (.+)::, so the last :: ends the path. An anchor can only hold A-Za-z0-9_$-.%~@, so it never contains a :: and the last one is always the separator. A path with a space now round-trips with no encoding, and nothing needs a decoder.
I deliberately did not encode the path. Encoding would mean the registry no longer shows the real path, so check-equivalents would need to decode before opening the file and its "path must be written as src/foo.ts" error would have to quote an escaped form — a decoder with no real caller, to serve a filename this repo will not create.
A # in a path is the one case still unhandled, and it now fails loudly: since this PR, a non-blank, non-comment line that does not parse raises rather than being skipped. That is what made the original class of bug dangerous — 145 entries dropped in silence — and it is no longer silent. Under "don't defend against the impossible" that is where I would rather leave it than build encode/decode for it.
Generated by Claude Code
There was a problem hiding this comment.
Separately — your P2 on the deleted-source read is right, and it is the one of this batch that would have actually bitten someone. Fixed.
keysFor now treats a missing source as producing no keys, so the entry naming it reports as stale, which is exactly the line the author has to delete. Before, deleting or renaming a source failed the commit with a raw NotFound and no indication of which registry line caused it — a bad first experience for a check that now runs on every commit.
Test: reports an entry whose source file is gone.
The read became readTextFileOrNull in scripts/not-found.ts, since loadIgnoreList was already doing the same read-or-treat-as-absent dance — jscpd flagged the pair, and nullIfNotFound was already sitting there waiting to be used.
Generated by Claude Code
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/mutation/anchor.ts`:
- Around line 59-65: Update nameOf to accept numeric literalKey values and
convert them to their canonical string representation before returning the
anchor name, while preserving existing string-key behavior and null handling.
Add a regression test that inserts an earlier numeric property before two
identical callbacks and verifies their registry identities remain stable.
In `@scripts/mutation/check-equivalents.ts`:
- Around line 105-114: Update the required lookup in the loop over usable
entries to replace the non-null assertion on byPath.get(entry.sourcePath) with
requiredMapValue, supplying an appropriate context message so missing source
paths fail explicitly while preserving the existing key membership check.
In `@scripts/mutation/ignore.ts`:
- Around line 63-74: Make escapeForLine lossless by encoding every whitespace
character distinctly, including leading/trailing tabs and carriage returns,
while preserving existing escaping behavior for other characters. In
scripts/mutation/ignore.ts, update escapeForLine and ensure mutantKeyForPath
output remains parseable by parseIgnoreLine. In
test/scripts/mutation/ignore.test.ts lines 47-81, add mutantKeyForPath →
parseIgnoreLine regression coverage proving tab-versus-space keys remain
distinct and carriage-return-containing keys round-trip successfully.
In `@scripts/mutation/summary.ts`:
- Around line 238-239: Update the survivorRow formatter to escape pipe and
backtick characters in registry entries before rendering them, while keeping
mutantKeyForPath canonical and unchanged. Ensure the output remains valid GFM
table syntax and a fixed one-backtick code span, and add direct tests covering
both characters.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 156ad9f1-de9d-4199-9a19-786e5230bf6f
📒 Files selected for processing (33)
deno.jsonscripts/check-equivalent-mutants.tsscripts/mutation/anchor.tsscripts/mutation/check-equivalents.tsscripts/mutation/equivalent-audit.tsscripts/mutation/equivalent-mutants/README.txtscripts/mutation/equivalent-mutants/features.txtscripts/mutation/equivalent-mutants/scripts.txtscripts/mutation/equivalent-mutants/shared-a-l.txtscripts/mutation/equivalent-mutants/shared-db.txtscripts/mutation/equivalent-mutants/shared-m-z.txtscripts/mutation/equivalent-mutants/src-root.txtscripts/mutation/equivalent-mutants/test-utils.txtscripts/mutation/equivalent-mutants/ui-client.txtscripts/mutation/equivalent-mutants/ui-templates.txtscripts/mutation/evaluate.tsscripts/mutation/generate.tsscripts/mutation/ignore.tsscripts/mutation/runner.tsscripts/mutation/summary.tsscripts/precommit/steps.tsscripts/project-root.tstest/scripts/mutation/anchor-round-trip.test.tstest/scripts/mutation/anchor.test.tstest/scripts/mutation/check-equivalents.test.tstest/scripts/mutation/equivalent-audit.test.tstest/scripts/mutation/evaluate.test.tstest/scripts/mutation/ignore.test.tstest/scripts/mutation/summary/fixtures.tstest/scripts/mutation/summary/markdown.test.tstest/scripts/mutation/summary/score.test.tstest/scripts/mutation/summary/terminal.test.tstest/scripts/project-root.test.ts
💤 Files with no reviewable changes (1)
- test/scripts/mutation/summary/score.test.ts
Seven review findings, six fixed and one recorded rather than fixed.
A numeric object key now names its member, so `{ 1: cb, 2: cb }` no longer
leaves two identical callbacks sharing a name and telling themselves apart by
source order alone.
Escaping is lossless. A carriage return and the two Unicode line separators end
a registry line exactly as a newline does, and edge whitespace is encoded byte
by byte, so a leading tab and a leading space no longer collapse onto the same
escape — the same defect the trailing-space rule exists to prevent, in the
other direction. The path is now read back as everything before the LAST `::`,
which an anchor can never contain, so a path holding a space parses without
needing an encoder or a decoder for it.
Markdown survivor rows render both cells as `<code>` with entities. A raw `|`
delimits a table column even inside a code span, so every `?? → ||` survivor
was splitting its own row into extra cells — true of the report this PR
replaced as well.
`ANCHOR_PATTERN` existed only for a test that inspected the representation;
the round-trip test already proves the property through behaviour. Both are
gone. The registry checker's map lookup uses `requiredMapValue` rather than a
non-null assertion, and the by-byte encoder the escaping and the anchor names
both wanted is now one shared `percentEncode`.
Not fixed: an equivalence proof rests on types an anchor cannot see. Widening
a parameter from `string | null` to `number | null` leaves the mutated
expression identical, so the entry keeps resolving. Fingerprinting the
enclosing function's head was tried and reverted — it invalidates every entry
in a function whenever any parameter changes, and still misses the 166 of 535
recorded reasons whose types come from a call's return, a row, or an imported
shape. TODO.md carries the measurements and what re-proving would need.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01P4fF7zjiMi9bkHG22oFzh1
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 368b20377e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return { context: wider ?? withinStatement.at(-1)!, names }; | ||
| }; | ||
|
|
||
| export interface AnchoredMutant { |
There was a problem hiding this comment.
Keep the anchor result type private
A repo-wide rg AnchoredMutant only finds this declaration and anchorMutants' own return type, so exporting it creates an internal API with no production caller. Make the interface private, or inline the { anchor: string } return shape, instead of leaving a dead public surface.
AGENTS.md reference: AGENTS.md:L81-L81
Useful? React with 👍 / 👎.
| registryFilePath, | ||
| } from "./ignore.ts"; | ||
|
|
||
| export interface EquivalentCheckOptions { |
There was a problem hiding this comment.
Keep the checker options type private
A repo-wide rg EquivalentCheckOptions only finds this declaration and the parameter annotation in this same module; callers do not import the type. Dropping the export keeps the new checker from adding an unused internal API surface.
AGENTS.md reference: AGENTS.md:L81-L81
Useful? React with 👍 / 👎.
| root: string, | ||
| ): Promise<Set<string>> => { | ||
| const file = resolve(root, sourcePath); | ||
| const content = await Deno.readTextFile(file); |
There was a problem hiding this comment.
Report deleted registry sources as stale
When a branch deletes or renames a source file that still has a registry entry, this read throws Deno.errors.NotFound before the checker can add a stale problem. Because check:equivalents now runs in precommit, that leaves users with an uncaught file-read failure instead of the registry line they need to remove; catch NotFound here and report those entries as stale.
Useful? React with 👍 / 👎.
CI has failed three times with `unknown location - Turso migration file` and no diagnostic. The full log shows what the summary hid: it is the *suite* that fails, and every test after `reports an upload API failure` never runs. That is an uncaught error aborting the test module, and the test it aborts on is the next one — the 8MB case where the server answers before the body is sent. `sendDatabaseFile` stood its unhandled-rejection watch down one timer turn after the upload settled, on the guess that the polyfill's internal task surfaces its duplicate rejection by then. Nothing guarantees that. Lose the race and the rejection is uncaught, which kills the module. The watch now installs once and stays up. It earns that by being narrow about what it ignores rather than when: the message is built by node:http's polyfill internals and no code of ours can produce it, so a real failure never reaches that branch. The test that asserted the watch stands down now asserts the opposite — a late rejection is precisely what it is for. This does not reproduce locally. The write genuinely breaks here, but the duplicate rejection never fires at all, even with the stand-down removed entirely, so the race cannot be lost on this machine. Also from review: a registry entry whose source file was deleted now reports as stale instead of failing the commit with a raw NotFound, which matters now that check:equivalents gates every commit; that read and loadIgnoreList's share `readTextFileOrNull`; and `EquivalentCheckOptions` and `AnchoredMutant` are private, having had no caller outside their own modules. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01P4fF7zjiMi9bkHG22oFzh1
…cucumber-bq6mji # Conflicts: # TODO.md # scripts/mutation/equivalent-mutants/shared-db.txt # scripts/mutation/equivalent-mutants/shared-m-z.txt # scripts/mutation/equivalent-mutants/test-utils.txt
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 17c85b5e4b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| /** Canonical key for a mutant at a project-relative path. */ | ||
| export const mutantKeyForPath = (relPath: string, mutant: Mutant): string => | ||
| `${relPath}:${mutant.line}:${mutant.column} ${mutant.operator}→${mutant.newOperator}`; | ||
| `${relPath}::${mutant.anchor} ${escapeForLine(mutant.operator)}→${escapeForLine(mutant.newOperator)}`; |
There was a problem hiding this comment.
Encode source paths in registry keys
When a survivor is in a legal path containing #, this writes relPath raw into the suggested registry line. Fresh evidence beyond the earlier source-path thread is that parseIgnoreLine still strips #.* before parsing, so copying a line for src/a#b.ts is read as src/a and becomes malformed instead of suppressing or checking that mutant. Encode the path too, or isolate the fields before treating # as the reason marker.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
You're right, and I was wrong to leave it last round. My reasoning then ("it fails loudly, so it's fine") ignored that this PR added the feature that suggests a line to paste — emitting an unusable one is a defect in the new thing, not a pre-existing edge.
Fixed, but by your second option rather than encoding. A # only starts the reason when whitespace comes before it:
const body = line.replace(/(?:^|\s)#.*$/, "").trim();
The reason is always spaced away from the entry, so that finds it; a # inside a path has a path character before it, so it does not. src/a#b.ts::anchor 0→1 # a reason now parses back to exactly the key that was written.
This is better than encoding the path, which was what I was resisting: an encoded path stops being the real path, so check-equivalents would need to decode before opening the file and its "path must be written as src/foo.ts" message would have to quote an escaped form. No encoder, no decoder, and the registry still shows the path you would type.
Test: keeps a comment mark inside a path, which no space precedes.
Generated by Claude Code
| export const mutantKey = (file: string, mutant: Mutant): string => | ||
| mutantKeyForPath(rel(file), mutant); | ||
|
|
||
| export interface ParsedIgnoreLine { |
There was a problem hiding this comment.
Keep the parsed line shape private
ParsedIgnoreLine is only referenced inside this module (a repo-wide rg ParsedIgnoreLine finds no imports), so exporting it adds a public internal surface that no production caller needs. Keep the return shape private and leave only parseIgnoreLine exported, rather than preserving a test-only or hypothetical API.
AGENTS.md reference: AGENTS.md:L81-L81
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct — nothing imports it. ParsedIgnoreLine is private now; parseIgnoreLine and parseRegistryText still annotate their returns with it, which TypeScript is happy with for a type callers never need to name.
That is the fourth of these on this PR (RawMutant, ANCHOR_PATTERN, AnchoredMutant, EquivalentCheckOptions, and now this). Same cause each time: splitting a module into pieces, naming the shapes at each seam, and exporting them by reflex rather than because something across the seam needed them.
Generated by Claude Code
| * silently stop suppressing its mutant — which is how an ordinal separator | ||
| * clashing with the comment marker once dropped 145 entries unnoticed. */ |
There was a problem hiding this comment.
Remove the historical failure note
This comment explains a past incident (once dropped 145 entries) instead of only the current invariant that malformed entries must fail. The repo asks comments not to compare with old implementations and to stay short, so leave the old failure to git history and keep only the present rule this test protects.
AGENTS.md reference: AGENTS.md:L71-L72
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fair — third time on this PR I've written history into a comment, and this one I added while replying to the second. The note is now just the rule it protects:
/** A line that neither parses nor reads as a comment is a record that would
* otherwise stop suppressing its mutant without saying so. */
The incident belongs in the commit and the PR description, both of which have it.
Generated by Claude Code
| "MethodDefinition", | ||
| "Property", | ||
| "PropertyDefinition", | ||
| "TSEnumDeclaration", |
There was a problem hiding this comment.
Name enum members before using ordinal anchors
When a TypeScript enum has repeated mutable initializers, this collects only the enum name, not the member name, so A = x ?? 0 and B = x ?? 0 fall back to @1/@2. If a later edit reorders or inserts members, a registry entry audited for one member can suppress another member's mutant even though the program may expose those members differently. Add TSEnumMember to the named nodes so enum initializers anchor on Enum.member instead of source order.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Right, same shape as the function-expression one. TSEnumMember is now a naming type, so enum E { A = 1 ?? 0 } anchors on E.A rather than just E.
Test: names an enum member, not just its enum.
Generated by Claude Code
| * one object share a name and can only be told apart by their order. */ | ||
| const NAMING_TYPES = new Set([ | ||
| "ClassDeclaration", | ||
| "FunctionDeclaration", |
There was a problem hiding this comment.
Name function expressions before falling back to order
When named function or class expressions are used somewhere that has no property or variable name of its own, such as array entries or call arguments, this list does not collect their id. Two entries like function first(x) { return x ?? "" } and function second(x) { return x ?? "" } therefore share the enclosing name and fingerprint and only differ by @1/@2, so reordering them can make an audited registry entry suppress the other function's mutant. Include named FunctionExpression/ClassExpression nodes in the anchor path before using the ordinal fallback.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct. FunctionExpression and ClassExpression now contribute their name, so your exact example anchors on xs.first and xs.second with no ordinal.
Both are in the test: names a function expression by its own name, and tells apart identical function expressions by their names — which is your two-in-an-array case, asserting the names differ and that neither falls back to @n.
Worth naming the pattern the last three of these share: a node carries a name in a position where nothing around it does. Property was that (config objects), then numeric keys, now function expressions and enum members. Each time the ordinal was doing work a name could do.
Generated by Claude Code
One class can hold a static member and an instance member under the same key. Their bodies can read alike while each takes a different type, so what is equivalent on one side need not be on the other — and with only the key to go on, the two were told apart by order, which reordering the class elements moves. A static member now contributes `<static>` ahead of its name, written the way `<file>` is. No static member exists in the tree today, so no recorded entry moves. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01P4fF7zjiMi9bkHG22oFzh1
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9ea6e2a3fb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const label = node.id ?? node.key ?? node.test; | ||
| if (typeof label?.name === "string" && label.name !== "") return label.name; | ||
| const written = label?.value; | ||
| if (typeof written === "number") return String(written); | ||
| return typeof written === "string" ? written : null; |
There was a problem hiding this comment.
Name non-literal switch labels before using ordinals
Fresh evidence beyond the fixed string-literal case labels is that case Kind.Text:/case Kind.Count: still reach this helper as MemberExpression test nodes, but this code only reads name or primitive value, so those labels contribute no path segment. In a discriminated-union switch with identical bodies such as return item.value ?? "", the anchors fall back to @1/@2; reordering or inserting enum-member cases can then make an entry audited for the string-valued arm suppress the number-valued arm where 0 distinguishes ?? from ||.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in c37fae9. A case label that is written as an expression is now named by how it is written, so case Kind.Text: gives read.Kind%2eText and case Kind.Count: gives read.Kind%2eCount — no ordinal on either. Taking the label's own source text covers every shape a label can take, not just member expressions, so case FOO(): or case a + b: are named too.
Only a switch case earns the text fallback. The same fallback on a binding pattern would name a destructuring declarator %7ba%2c%20b%7d, which moves the anchor whenever an unrelated field is destructured — churn with no identity gained. default: still writes no label and contributes nothing, as before.
Regression tests: your two-arm case with character-identical bodies asserting the distinct names and no ordinal, plus a single-case one. Every case label in the tree today is a string literal, so check:equivalents stays green with nothing re-anchored.
Generated by Claude Code
| * Regression: node throws an "error" event nothing is listening for. A | ||
| * request raises one when the write breaks, and the file stopping because of | ||
| * that destroys the request with an error of its own — so taking the | ||
| * listeners off after the first left the second to crash the whole file from | ||
| * a place no caller could catch it. |
There was a problem hiding this comment.
Remove the historical listener narrative
This added block opens as a regression note and explains the old one-shot listener behavior (taking the listeners off after the first) instead of stating only the current rule the test protects. The repo asks comments to describe current code and stay short, so keep the test name/current invariant and leave the old failure path to git history.
AGENTS.md reference: AGENTS.md:L71-L72
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in c37fae9. You're right — it opened as a regression note and spent most of its length on the listeners the code no longer has. It now states only the rule the test holds:
One upload can raise two errors — the write breaking, then the file stopping because of it — and node throws an "error" nothing is listening for, from a place no caller can catch.
Three lines, all about what is true now. The old one-shot behaviour stays in git history where it belongs.
Generated by Claude Code
| // Keys hold each path as it is written on a line, so match the written form. | ||
| const relFiles = mutatedFiles.map((file) => escapeForLine(rel(file))); | ||
| const targetsMutatedFile = (key: string): boolean => | ||
| relFiles.some((file) => key.startsWith(`${file}:`)); |
There was a problem hiding this comment.
Match scoped entries on the full path delimiter
When a registry contains a legal source path that starts with the mutated file plus a colon, such as src/read.ts:backup, a mutation run for src/read.ts treats that other file's entry as targeting the current run because this only checks the single-colon prefix. The possible keys are generated from src/read.ts, so the src/read.ts:backup::... entry is falsely reported stale; match ${file}:: or parse the key path before scoping.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in c37fae9 — and it needed to go further than ${file}::, because a path can hold a :: of its own: src/a::b.ts::anchor still starts with src/a::, so the prefix would have been wrong again one level down.
The path is now taken off the key by the same shape the line parser uses (greedy up to the last :: before an anchor-shaped token) and compared whole against the mutated files. Both modules share that one pattern, so a key and a line can never disagree about where the path ends. A string that does not read as a key throws rather than being skipped — every key here comes from mutantKeyForPath or from a line this module parsed, so one that does not parse is a caller's mistake, not a path it failed to find.
Regression test uses your example: an entry for src/example.ts:backup while mutating src/example.ts is left alone rather than reported stale.
Generated by Claude Code
A `case Kind.Text:` label is not a plain word, so it lent no name and its arm fell back to source order. It is now named by how it is written, since it picks out its arm just as a plain label does. Only a case earns this: naming a binding pattern by its text would move an anchor whenever an unrelated field was destructured. Scoping a registry entry to the files being mutated compared the front of the key, so a path that merely began with a mutated file's path — `src/read.ts` and `src/read.ts:backup` — was pulled into the run and reported stale against mutants never generated for it. The path is now taken off the key by the same shape a line's is, and compared whole. The turso listener test's comment kept the old one-shot behaviour alive in prose; it now states only the rule the test holds. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01P4fF7zjiMi9bkHG22oFzh1
Scoping the ignore list to a run's files re-derived the path from each key, which needed a guard for a key that is not one — a state the loader already makes impossible, so nothing could reach it and nothing could cover it. Each entry now carries the path its line named, so the comparison is a plain whole-path match with no parsing and no unreachable branch. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01P4fF7zjiMi9bkHG22oFzh1
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 50ab94b494
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const written = label?.value; | ||
| if (typeof written === "number") return String(written); | ||
| return typeof written === "string" ? written : null; |
There was a problem hiding this comment.
Name computed member keys before using ordinals
When an object or class member uses a computed key such as [Kind.Text], the key reaches this branch as an expression node with neither name nor primitive value, so the member contributes no path segment. Two dispatch callbacks under [Kind.Text] and [Kind.Count] can then contain identical item.value ?? "" expressions with different contextual types and fall back to @1/@2; reordering or inserting members can make a registry entry audited for one key suppress the other. Include the computed key's source text for member nodes before falling back to source order.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in de83423. A computed key reaches the helper as an expression node with neither name nor a primitive value, exactly as you describe, so it now takes its name from how it is written: { [Kind.Text]: … } gives o.Kind%2eText, [Kind.Count] gives o.Kind%2eCount, no ordinal on either.
It shares one rule with the case-label fix rather than adding a second: whichever of a key or a case label wrote no plain name is named by its own source text. That keeps a computed literal key ({ ["lit"]: … }) reading as lit, the same as { lit: … }, since the literal value is found first.
The line I am holding is the one from the earlier thread: only a key or a label earns this. A binding pattern is not a name, and naming a destructuring declarator by its text would move the anchor whenever an unrelated field was added to the pattern.
This one was real in the tree too — admin-feature-triggers.ts had an entry inside a [source.table] member, re-anchored from featureWriteTrigger.uses to featureWriteTrigger.uses.source%2etable with its ~0toeppz fingerprint intact.
Generated by Claude Code
| * A `default:` case writes no name of its own, so it contributes none. */ | ||
| const nameOf = (node: NamedNode): string | null => { | ||
| const label = node.id ?? node.key ?? node.test; | ||
| if (typeof label?.name === "string" && label.name !== "") return label.name; |
There was a problem hiding this comment.
Preserve private member markers in anchors
When a class contains both #read and read, Oxc exposes both keys with the same name value, so this records both paths as R.read. If their bodies contain identical x ?? "" text but the private helper is only used with strings while the public method accepts numbers, the anchors differ only by @1/@2; reordering the class elements can make an entry audited for the private method suppress the public method where 0 distinguishes ?? from ||. Include the private marker in the member name before falling back to source order.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in de83423, and I checked oxc rather than taking it on trust — you're right: #read comes through as a PrivateIdentifier whose name is "read", character-for-character the same as the public method's. The # is now part of the name, so a class holding both gives R.%23read and R.read with no ordinal.
This one was not theoretical. Re-anchoring found two real entries in the tree sitting inside private members, both of which had been anchored on a name a public sibling could take:
scripts/compact-test-reporter.ts—CompactTapReporter.recordFailure.location→CompactTapReporter.%23recordFailure.locationsrc/shared/uptime-kuma/socket.ts—KumaSocket.changeListeners→KumaSocket.%23changeListeners
Both kept their fingerprints exactly (~13av20m, ~0x9mfb4), which is the check that they moved onto the same expression they always named rather than onto something that merely looked right.
Regression test is your case: #read and read in one class with character-identical bodies, asserting the distinct names and no ordinal.
Generated by Claude Code
A class may hold `#read` and `read` at once, and their bare names are the
same, so both anchored on `R.read` and were told apart by order — which
reordering the class elements moves. The `#` is now part of the name.
A key written as an expression — `{ [Kind.Text]: … }` — is not a plain word
either, so it lent no name at all. It is now named by how it is written, the
same way a case label already was, and the two share one rule.
Three recorded entries move as a result, each onto the same expression it
always named: two private members and one computed key. Their fingerprints are
unchanged, which is what says the expression itself did not.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01P4fF7zjiMi9bkHG22oFzh1
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: de83423606
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const namesOf = (node: NamedNode, source: string): string[] => { | ||
| const own = nameOf(node) ?? writtenAs(node, source); | ||
| if (own === null) return []; | ||
| return node.static === true ? ["<static>", own] : [own]; |
There was a problem hiding this comment.
Include accessor kind in member anchors
When a class or object has both get value and set value, both members contribute only the shared key name here, so identical expressions such as an outer-string x ?? "" in the getter and a number-typed setter parameter x ?? "" collapse to value~...@1/@2. Reordering the accessors can then make an entry audited for the getter suppress the setter mutant where 0 distinguishes ?? from ||; include the member kind (get/set) in the anchor before falling back to ordinals.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
You're right and I was wrong — thank you for pushing back with the concrete case. I declined this two rounds ago on the grounds that "a getter and a setter under one key cannot have character-identical bodies, since the setter takes a parameter and assigns while the getter returns". That reasoning was about bodies, and the fingerprint does not cover the body — it covers the mutated expression. x ?? "" in return x ?? "" and x ?? "" in this.y = x ?? "" are the same seven characters.
I ran it rather than reasoning again:
R.value~1otmp15@1 ← get value
R.value~1otmp15@2 ← set value
Exactly the collapse you described. Fixed in 28749a4: a member now says whether it reads or writes, alongside the marker saying which side of its class it sits on, so those become R.%3cget%3e.value and R.%3cset%3e.value with no ordinal. The two markers share one mechanism rather than being bolted on separately.
Regression test is your case, and it carries the reason in its comment so the next person does not repeat my mistake: the fingerprint is the expression, not the body.
No registry entry sits inside an accessor today, so this re-anchored nothing.
Generated by Claude Code
| "TSInterfaceDeclaration", | ||
| "TSModuleDeclaration", | ||
| "TSTypeAliasDeclaration", | ||
| "VariableDeclarator", |
There was a problem hiding this comment.
Name defaulted parameters before using ordinal anchors
When a function has two default parameter initializers with identical mutable text, such as a string-typed first = value ?? "" beside a number-typed second = value ?? "", this naming set never records the parameter binding, so both mutants share only the function name and expression fingerprint before falling back to @1/@2. Reordering or inserting parameters can then make a registry entry audited for one default suppress the other; carry the parameter name into the anchor before the ordinal fallback.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 28749a4. A defaulted parameter now lends its name — a default is code of its own, and two parameters of one function can default to the same words — so your case gives f.first and f.second instead of @1/@2.
This was live in the tree, and more so than any earlier naming finding: thirteen entries were sitting on the enclosing function alone, told apart only by order. Each moved onto the parameter it always sat in — bookedRangeLabel → bookedRangeLabel.fallbackDurationDays, dateToRange → dateToRange.durationDays, CartController.add → CartController.add.quantity, and so on. Two pairs were the exact hazard you name: dateToRange~18guaiu and buildCapacityCondition~18guaiu in one file, same fingerprint, same 1 → 0, distinguished by nothing but which came first.
Every one kept its fingerprint, and every new name extends the old one rather than replacing it — that pair of facts is what makes the re-anchoring checkable rather than a guess.
The binding-pattern line still holds: a parameter that destructures (({ a, b } = {}) => …) writes no name, so it lends none.
Generated by Claude Code
| // hunting for a delimiter: an anchor holds only these characters and ends at | ||
| // the first whitespace after it, so whatever precedes is the path, whichever | ||
| // characters it happens to use — a `:`, or a `::` of its own, included. | ||
| const located = text.match(/^(.+)::([A-Za-z0-9_$\-.%~@]+)\s+(.*)$/); |
There was a problem hiding this comment.
Stop parsing
:: in reasons as anchor delimiters
When a valid registry line's reason contains text like a::b case (or a mutation label contains a::b c), this greedy match selects that later ::b as the path/anchor split instead of the real <path>::<anchor> delimiter. The parsed source path becomes the original entry plus part of the reason, so the entry no longer suppresses the survivor and the checker reports a misleading stale path; isolate the reason/mutation text before applying this path-anchor match, or escape the delimiter sequence in free text.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 28749a4, both halves of it — the reason and the mutation label — and by removing the ambiguity rather than parsing around it.
The reason is cut off before anything is matched. Every field before it escapes its own #, so the first # left on the line starts the reason. Nothing after that point is searched for a delimiter, so a reason may hold a::b, an arrow, or both.
A path now escapes its colons. That is the half your parenthetical points at, and it is the one that actually mattered: a mutated literal really can hold :: — an IPv6 address, a C++ qualified name — and the old greedy match would take that as the split. With no colon left in a path, the first :: on a line is always the one ending it, and the match is [^:]+ rather than a greedy .+. The from and to may then hold as many colons as they like.
Regression tests: four reasons, including a::b → c::d and see Foo::bar and x::y z; and a mutation of "[::1]" → "[::ffff:1.2.3.4]" round-tripping with its path read back intact. Paths holding a:b.ts and a::b.ts joined the existing awkward-path list. No registry path has a colon today, so nothing was re-anchored by this half.
Generated by Claude Code
…nything A fingerprint covers the mutated expression, not the whole body, so `x ?? ""` in a getter and `x ?? ""` in its setter are the same text under the same key — they were told apart by order alone. A member now says whether it reads or writes, the way it already says which side of its class it sits on. A parameter's default is code of its own, and two parameters of one function can default to the same words, so the parameter now lends its name. Thirteen recorded entries move onto the parameter they always sat in; each keeps its fingerprint, and each new name extends the old one rather than replacing it. A reason is prose and may hold anything, including the marks the fields before it use to say where they end. Every field escapes its own `#`, so the first one left starts the reason and the rest of the line is not searched for delimiters. A path now escapes its colons too, so the first `::` on a line is always the one ending the path — a mutated literal holding `::`, an IPv6 address say, can no longer be read as the delimiter. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01P4fF7zjiMi9bkHG22oFzh1
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/scripts/turso-migration-file.test.ts (1)
278-281: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winGive the listener test a fresh isolate.
// test-groups: run-aloneisolates the file, but its tests share module and global state. Earlier uploads initialize the cachedoncelistener, so the test can pass without the current upload path installing it. Move this test to its ownrun-alonefile or reset the listener state before it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/scripts/turso-migration-file.test.ts` around lines 278 - 281, Give the test “ignores the duplicate body-stream rejection and nothing else” a fresh module/global-state isolate by moving it into a dedicated run-alone test file or resetting the cached once-listener state before execution. Ensure the test exercises the current upload path installing the listener rather than relying on earlier uploads.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/mutation/ignore.ts`:
- Around line 120-139: In the parsing flow around the located regex match,
replace the double assertion on the capture array with direct indexed
assignments using non-null assertions for captures 1–3. Then remove the
redundant non-null assertions from writtenPath, anchor, and mutation usages
while preserving the existing match validation and return behavior.
---
Outside diff comments:
In `@test/scripts/turso-migration-file.test.ts`:
- Around line 278-281: Give the test “ignores the duplicate body-stream
rejection and nothing else” a fresh module/global-state isolate by moving it
into a dedicated run-alone test file or resetting the cached once-listener state
before execution. Ensure the test exercises the current upload path installing
the listener rather than relying on earlier uploads.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: cf93ec29-e854-4736-8cc6-cb7257181203
📒 Files selected for processing (14)
TODO.mdscripts/mutation/anchor.tsscripts/mutation/equivalent-mutants/README.txtscripts/mutation/equivalent-mutants/features.txtscripts/mutation/equivalent-mutants/scripts.txtscripts/mutation/equivalent-mutants/shared-a-l.txtscripts/mutation/equivalent-mutants/shared-db.txtscripts/mutation/equivalent-mutants/shared-m-z.txtscripts/mutation/equivalent-mutants/ui-client.txtscripts/mutation/ignore.tstest/scripts/mutation/anchor.test.tstest/scripts/mutation/check-equivalents.test.tstest/scripts/mutation/ignore.test.tstest/scripts/turso-migration-file.test.ts
A double assertion over the whole match told the checker to stop looking at it. Each group is now taken on its own, where the regex having matched is what says it is there. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01P4fF7zjiMi9bkHG22oFzh1
|
@coderabbitai — replying here to the outside-diff finding on Declining it: I checked, and the test does not pass on state left by earlier tests. The premise is that an earlier upload initialises the cached It fails. There is exactly one place the watch is ever installed, so removing it means no upload installs it and the test catches that — which is the property the isolation concern is really about. The test is also self-sufficient by construction: it starts its own upload and awaits The other half of it — that a fresh isolate would be stricter — is true in principle but would mean splitting the file, and I am not restructuring this particular file right now: it carries a live intermittent failure that is under active investigation and awaiting a decision from the repo owner, and reshuffling which tests share an isolate would muddy that diagnosis. Worth revisiting once that is settled. The inline finding on Generated by Claude Code |
|
Tip For best results, initiate chat on the files or code changes.
Your experiment disproves the finding’s premise. The test fails when I withdraw the isolation finding. Keeping the current test-file structure is appropriate while the intermittent failure remains under investigation. 🐇 ✏️ Learnings added
You are interacting with an AI system. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c0d11af38b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const NAMING_TYPES = new Set([ | ||
| "AssignmentPattern", |
There was a problem hiding this comment.
Name assignment targets before using ordinals
When callbacks are installed through property assignment, such as handlers.text = (item) => item.value ?? "" next to handlers.count = ..., none of the naming nodes here lends the text/count target: there is no VariableDeclarator/Property, and the anonymous arrow has no id. Those identical ?? mutants therefore share the enclosing anchor and fall back to @1/@2; reordering or inserting an assignment can make an entry audited for the string handler suppress the number handler, where 0 distinguishes ?? from ||. Include the assignment's left-hand member text/name before using the ordinal fallback.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 227c086. An assignment's target now lends its name, so your example gives install.handlers%2etext and install.handlers%2ecount with no ordinal. A plain total = x ?? 0 picks up total the same way — the target is an identifier there rather than a member expression, and both go through one rule.
Together with the JSX finding this moved 59 recorded entries, by far the largest re-anchoring on this branch. Every one kept its fingerprint. Fifty-eight gained the new segment on the end, exactly as expected; one gained it mid-name — sumupApi.testSumupConnection.mode → sumupApi.testSumupConnection.result%2eapiKey.mode, where the assignment wraps the object the property sits in. I checked that one by hand rather than letting the rule guess: the fingerprint ~109o082 picks out one expression in the file, and it is the same settings.sumup.keyMode ?? "unknown" the entry was recorded against.
While making the two changes fit, the "where is this node's name written" logic became a chain of conditions, so it is now a table keyed by node kind. A new kind says where its name lives and nothing else has to change — and AssignmentPattern is deliberately absent from it, since a destructured parameter's left is a binding pattern, not a name.
Generated by Claude Code
| "FunctionDeclaration", | ||
| "FunctionExpression", | ||
| "MethodDefinition", | ||
| "Property", |
There was a problem hiding this comment.
Name JSX attributes before using ordinals
When JSX props supply anonymous callbacks, such as <Widget text={(x) => x ?? ""} count={(x) => x ?? ""} />, the prop name is the stable context that tells the callbacks apart, and TypeScript can contextually type x differently for each prop. Because this naming set does not include JSXAttribute or read its name, those identical mutants share the enclosing component and fingerprint and fall back to @1/@2; reordering or inserting props can make an entry audited for the string callback suppress the number callback, where 0 distinguishes ?? from ||. Carry the JSX attribute name into the anchor before using the ordinal fallback.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 227c086. JSXAttribute now lends its prop name, so your example gives Page.text and Page.count with no ordinal. A namespaced prop (xlink:href) has no plain name, so it falls through to the same "named by how it is written" rule the computed keys use.
This one mattered most in the tree by volume: of the 59 entries the two findings moved, 37 were in ui-client.txt — the browser code is dense with callbacks handed straight to props and to properties, exactly the shape where nothing but order was telling neighbours apart. Every one kept its fingerprint.
Regression tests: your two-prop case asserting the names and that no anchor carries an ordinal, plus a single-prop one. The test helper now takes a .tsx name where it needs the source read as markup rather than as a comparison.
Generated by Claude Code
A callback installed by assignment or handed to a JSX prop is never declared, so it had no name of its own and neighbours doing the same job could only be told apart by order. The assignment's target and the prop's name now lend theirs, the same way a property key already did. Fifty-nine recorded entries move onto the target or prop they always sat in. Each keeps its fingerprint, which is what says the expression itself did not change; all but one gains its new segment on the end, and that one gains it mid-name where an assignment wraps the property it sits in. Where a node's name may be written as an expression rather than a plain word is now a table rather than a chain of conditions, so a new kind says where its name lives and nothing else has to change. A binding pattern is deliberately absent from it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01P4fF7zjiMi9bkHG22oFzh1
Looking the kind up a second time meant coping with its being absent, which it never is — a node is only named once its kind has been read. It is passed down instead, so there is no absence to cope with and no branch that cannot run. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01P4fF7zjiMi9bkHG22oFzh1
A webhook test about dropped answers failed once in CI on a branch that changes no source file, and passed the whole suite locally on the same commit. The note records what is known, what is already ruled out, and — most usefully — that the failing assertion itself is still unknown, because the job log keeps only its last 5,000 lines and the diagnostic falls outside them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01P4fF7zjiMi9bkHG22oFzh1
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a9f9fbdc50
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "MethodDefinition", | ||
| "Property", | ||
| "PropertyDefinition", | ||
| "SwitchCase", |
There was a problem hiding this comment.
Name ternary arms before using ordinals
Fresh evidence beyond the fixed switch-case path is the ternary form of the same narrowing: item.kind === "text" ? item.value ?? "" : item.value ?? "" produces two ??→|| mutants with the same enclosing name and expression fingerprint, even though the true arm can be string | null and the false arm number | null. The string-arm entry then falls back to @1; inserting or swapping a branch can make it suppress the numeric arm where 0 changes behavior, so carry the condition/arm marker into the anchor before using the ordinal fallback.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed the collapse — cond ? x ?? "" : x ?? "" really does give read~1u8xis1@1 and @2 — but declining this one, and the two alongside it, on evidence rather than on effort.
Which arm of a ternary an expression sits in is a position, not a name. Every rule accepted so far names a thing by something the code itself writes for it: a key, a label, a prop, a parameter, an assignment target. A <then>/<else> marker is not that — it is an ordinal wearing a better hat. Invert the condition and swap the arms and the code means exactly what it did before, while both markers move. That is the same failure the ordinal has, so it would trade a rare drift for an equally real one.
Folding the condition's text in instead would be stable, but then editing the condition moves both arms' anchors — drift on an edit that changed neither expression.
The measurement that decided it. After the seven naming rules already added, 3 of 540 entries carry an ordinal — 0.6%. None is in a ternary, a call argument, or a discriminated schema entry. They are two continue; statements in one loop and two string literals in one method: character-identical expressions with genuinely nothing to name them by. No naming rule reaches those, and none of the three shapes raised in this round accounts for any current exposure.
Set against that, each new rule re-anchors real entries — the last one moved 59 — and every re-anchor is a chance to land an entry on the wrong expression. The registry is a safety mechanism; past this point more rules cost more than they buy.
The boundary is now written down at NAMING_TYPES so the next person can see where the list stops and why, rather than inferring that these cases were simply missed.
Generated by Claude Code
| "FunctionExpression", | ||
| "JSXAttribute", | ||
| "MethodDefinition", | ||
| "Property", |
There was a problem hiding this comment.
Name schema entries before repeated field anchors
When a typed list has repeated object entries such as { kind: "text", read: (item) => item.value ?? "" } beside { kind: "count", read: ... }, this only names the repeated read property, not the entry's own kind, so the two callbacks collapse to entries.read~...@1/@2. Reordering or inserting entries can then make an equivalent-mutant record audited for the text entry suppress the count entry where 0 distinguishes ?? from ||; include an entry label/discriminant before falling back to source order.
AGENTS.md reference: AGENTS.md:L90-L90
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed the collapse (entries.read~1u8xis1@1/@2), and declining — this one on a sharper objection than the other two in this round.
Which sibling of an object is its "kind" is a guess about meaning, not something the code writes for that member. Every rule accepted so far reads a name the source states outright: this property's key, this case's label, this prop's name, this parameter's binding. Picking kind out of { kind, read } means deciding that kind is the discriminant and read is not — and nothing in the syntax says so. The next schema calls it type, or id, or name, or has two candidates.
And it fails in the direction that matters. An anchor built on a guessed sibling moves when that sibling changes — rename kind: "text" to kind: "free_text" and the entry drifts, though the expression it was recorded against is untouched. That is drift caused by an unrelated edit, which is the exact failure this whole PR exists to remove. The ordinal at least only moves when the members themselves are reordered.
The measurement behind the decision is in the ternary thread: 3 of 540 entries carry an ordinal, none of them in a schema entry. The boundary is now written down at NAMING_TYPES, naming both excluded shapes and why.
If a real schema in this tree ever does collapse this way, the fix I would reach for is not inference — it is giving that entry a read whose text differs, or recording the mutant against the enclosing declaration instead.
Generated by Claude Code
| * a JSX prop are here because both hand a callback somewhere without ever | ||
| * declaring it — `handlers.text = (x) => …`, `<Widget text={(x) => …} />` — | ||
| * and the target or the prop is the only name that callback has. */ | ||
| const NAMING_TYPES = new Set([ |
There was a problem hiding this comment.
Name call targets before using ordinal anchors
When anonymous callbacks are handed directly to different calls, such as useText((item) => item.value ?? "") beside useCount((item) => item.value ?? ""), the callee is the stable context and TypeScript can contextually type item.value differently for each callback. Because call expressions do not lend their callee or argument context here, both mutants share the enclosing name and fingerprint before falling back to @1/@2; reordering or inserting calls can make an entry audited for the text callback suppress the count callback where 0 distinguishes ?? from ||.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed the collapse (install~1u8xis1@1/@2), and declining — this is the closest of the three to being worth it, so it deserves the specific reason rather than the general one.
A callee genuinely is a name the code writes, unlike a ternary arm or a guessed discriminant. What sinks it is that it does not do the job. I ran the case that matters:
xs.map((i) => i.value ?? ""); xs.map((i) => i.value ?? "");
→ f~1u8xis1@1
f~1u8xis1@2
Naming by callee turns those into f.map@1 and f.map@2 — the ordinal survives, one segment longer. In this codebase that is the common shape by a distance: map, filter, pipe and friends repeat within a single function constantly, while useText/useCount next to each other is rare. So the rule would lengthen almost every anchor in the tree, re-anchor a large number of entries, and remove the ordinal only in the uncommon case.
Every other naming rule accepted here removes the ordinal outright, because a key, a prop, a case label and a parameter are each unique among their siblings by construction. A callee is not, and that difference is the whole argument.
The measurement is in the ternary thread: 3 of 540 entries carry an ordinal, none in a call argument. The boundary and its reasoning are now at NAMING_TYPES.
Generated by Claude Code
Seven rounds of review added a kind each time, and three more shapes have been raised that this deliberately does not add. Without the reason written down the omissions read as oversights, and the next person adds them. The rule that lets a kind in is the rule that keeps one out: it must write a name of its own. A ternary arm is a position, not a name — swapping the arms of an inverted condition would move it while the code means the same thing. Which sibling of an object is its "kind" is a guess about meaning, and a wrong guess moves an anchor when an unrelated field changes. Both would trade a rare ordinal for a commoner kind of drift. Three of 540 entries carry an ordinal, and none sits in either shape. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01P4fF7zjiMi9bkHG22oFzh1
Both halves of the
TODO.mdentry "Record equivalent mutants by something that survives an edit".The problem
We keep a list of mutants that no test could ever catch, so the mutation tester can flag genuinely new gaps instead of re-reporting the same known ones. Each was recorded by its line and column number.
That meant any edit above a recorded line silently broke its entry. Add a comment, add an import, and the entry now points at the wrong place — so the mutant stops being suppressed and the next mutation run fails, without the recorded code having changed at all. It happened twice on one branch, both times caught by a reviewer rather than by any check.
Entries now name what the mutant sits inside
Two halves, both taken from the code itself: the name of the thing it sits inside, and a short fingerprint of the expression it changes. So resolving an entry and checking it are the same act — an entry that resolves has found the code it was recorded against, not merely something in roughly the right place.
Adding a comment, an import, or a whole unrelated function above one moves nothing. Editing the expression itself, or renaming what it sits in, does move it — and both are real changes to the thing being recorded.
It can no longer break in silence
The first attempt at this silently dropped 145 entries. The separator between two otherwise-identical entries was a
#, which is also what starts the comment on a registry line, so each of those entries was cut in half when read back. The loader skipped any line it could not parse, so nothing was reported — they simply stopped existing.Two things came out of that:
#, a line break, an arrow, or a tab where a space was expected.All entries are migrated
The list now holds 536 entries. Of the original 509, 46 had already gone stale — the drift the TODO describes, grown from the 18 it recorded. So 46 mutants had quietly stopped being suppressed and nothing said so. Each was repointed by finding the commit that added it, reading the code as it was then, and carrying the anchor forward.
Drift now fails on the branch that caused it
deno task check:equivalentschecks every entry still points at a real mutant and does nothing else — no lint, no type-check, no tests. It runs in about a second, so it is aprecommitstep rather than something to remember.Splitting it out is what made that affordable. Re-proving that an entry is genuinely unkillable takes minutes and stays an on-demand tool; simply asking "does this still point at something" is cheap enough to run on every commit.
It has already earned that place, twice. Merging main into this branch, it found an entry that had gone stale on main — a
?? 0inlogin-attempts.tswas removed by another PR and its registry line left behind, which main cannot see because the check does not exist there yet. It then caught a mistake of my own: a change to how registry lines are read made the README's own quoted examples parse as records.That merge is also the clearest evidence the new format works: main had shifted the line numbers of 15 entries, and 11 needed no attention at all. Under the old scheme every one would have needed repointing by hand.
What review changed
Twenty-six rounds of review comments, all answered. Most of them were one question asked over and over in different clothes: what else can two pieces of code share, leaving only their order to tell them apart? Every answer to that closed a way for an entry to drift onto its neighbour.
foo(1);andbar();in one block shared an anchor — and at the top of a file,foo(1);andfoo(2);came out identical, which means an entry for one would have suppressed the other.switchcase labels, defaulted parameters, the target a callback is assigned to, and the JSX prop it is passed as. A label written as an expression (case Kind.Text:,[Kind.Text]) is named by how it is written.static readandread,#readandread,get valueandset value: each pair shared one name, and each half can take a different type.These were not hypothetical. They moved 75 entries that were sitting on a name a sibling could take — 3 in private members and computed keys, 13 in defaulted parameters, 59 in assignment targets and JSX props. Two of them were the hazard exactly:
dateToRangeandbuildCapacityConditionin one file, same fingerprint, same1 → 0, distinguished by nothing but which came first. Every entry kept its fingerprint through the move, which is what says it landed on the expression it always named.Also changed:
#, a line break of any kind, an arrow, a tab, a space at either edge, a::. That includes the file's own path, escaped like every other field: a line starting with#is therefore always a comment, and a reason may say anything at all without being read as a delimiter.??→||contains the character that splits a table into columns.One thing deliberately not fixed
Nearly every reason in the list is a claim about a type — "this is
string | undefined, so the two operators agree". Widening that type tonumber | nullmakes the claim false while leaving the code character-identical, so the entry keeps suppressing something it should not.I tried the obvious fix — fingerprint the function's parameters too — and reverted it. It invalidates every entry in a function whenever any parameter is added or renamed, and still misses most cases: 166 of the 535 reasons rest on a type that comes from a called function, a database row, or an imported shape, none of which a signature can see. A gate that noisy teaches people to re-record entries without re-reading them, which is worse than the problem.
It is written up in
TODO.mdwith the measurements and what a real fix needs: re-proving entries, not re-locating them.Two tests that fail at random in CI
Neither is caused by this branch, and neither is fixed. Both are written up in
TODO.md.Turso migration filedies with no location and no diagnostic, its remaining cases never run, and the rest of the suite carries on:It dies entering the case after that one — an 8 MB early-reply test that races the upload against the reply, and whose own assertion accepts either outcome. This branch does include a real fix to the upload code, kept on its own merits: both of the request's error handlers were
once, so once the write broke the request had none left, and the file stopping because of that break destroyed the request with an error nothing was listening for. That was not the cause — the failure recurred with the new listener in place. Whether to make the test deterministic through the scripted transport (its contract is already covered twice over) or keep hunting is an open question for the repo owner, so the test is deliberately untouched.A webhook test about dropped answers failed once, alone, out of 21,686 passing cases — on a branch that changes no
src/file, and the same commit passed the whole suite locally. Which of its assertions failed is still unknown: GitHub's job-log API keeps only the last 5,000 lines and the diagnostic falls outside them. The note records what is ruled out (the console-log race and the error spy, both eliminated by reading the code) so the next person starts from evidence rather than from the shape of the test.Checks
precommitpasses in full — lint, typecheck, no duplicated code, the copy check,check:equivalents, the edge build, and the whole suite at 100% line and branch coverage.🤖 Generated with Claude Code
https://claude.ai/code/session_01P4fF7zjiMi9bkHG22oFzh1
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Quality Improvements