internal/lang at de0d9860ee, reviewed by codex, verified by claude-code.
JavaScript/TypeScript collision repair performs an unscoped textual replacement that rewrites string literals and can change or invalidate the semantics of previously proven tests.
#!/bin/sh
set -eu
p=internal/lang/cold_review_test.go
trap 'rm -f "$p"' EXIT HUP INT TERM
cat >"$p" <<'EOF'
// SPDX-License-Identifier: Elastic-2.0
package lang
import (
"strings"
"testing"
)
func TestColdReviewConcatRewritesStringLiteral(t *testing.T) {
parts := []AuthoredPart{
{MutantID: "s0/m1", Source: `const helper = () => "helper";
test("one", () => assert.equal(helper(), "helper"));`},
{MutantID: "s0/m2", Source: `const helper = () => "helper";
test("two", () => assert.equal(helper(), "helper"));`},
}
got, err := (jsPlugin{}).ConcatTests(parts)
if err != nil {
t.Fatalf("unexpected refusal: %v", err)
}
t.Logf("concatenated output:\n%s", got)
if !strings.Contains(got, `"helper_s0m1"`) || !strings.Contains(got, `"helper_s0m2"`) {
t.Fatalf("defect not demonstrated: collision repair did not rewrite both string literals")
}
}
EOF
go test ./internal/lang -run '^TestColdReviewConcatRewritesStringLiteral$' -count=1 -v=== RUN TestColdReviewConcatRewritesStringLiteral
cold_review_test.go:20: concatenated output:
const helper_s0m1 = () => "helper_s0m1";
test("one", () => assert.equal(helper_s0m1(), "helper_s0m1"));
const helper_s0m2 = () => "helper_s0m2";
test("two", () => assert.equal(helper_s0m2(), "helper_s0m2"));
--- PASS: TestColdReviewConcatRewritesStringLiteral (0.00s)
PASS
ok github.com/pdbethke/corralai/internal/lang 0.003sI attempted to refute by looking for any scoping of the collision-repair replacement: a tokenizer or lexer for JS/TS, a step that masks string/template literals or comments before replacement, a JS-specific override of the shared rename loop, or a post-rename validation that would reject a body whose literals changed. There is none. concat.go:229-232 compiles a plain word-boundary regex from the colliding name and applies ReplaceAllString to the whole of bodies[i], which at that point (built at lines 172-207) is just the part's trimmed source lines joined by newline — literals, comments and test titles included. jsConcatSpec (concat.go:452-459) makes every top-level binding renameable in JS/TS, so this is the ordinary path, not an edge. I also considered narrowing the claim to 'harmless because both sides of the comparison are rewritten', which is true of the reviewer's exact example, but the finding claims the replacement is unscoped and rewrites string literals and CAN change or invalidate semantics — the rewrite is demonstrated by the script against the real exported ConcatTests, and any part that compares such a literal against a value produced outside the merged file breaks. No refutation available.
confirmed by pdbethke — real, reproduced: JS/TS collision repair is an unscoped textual replace that rewrites string literals in proven tests
ref 9e3f89b00949c53ac0b671fd3d950d5ced66af2876c66e6b7d8bda3541a6ee3e#R1
idSuffix deletes punctuation, so distinct mutant IDs such as "a/b" and "a-b" produce the same repaired identifier and the concatenator emits duplicate declarations.
#!/bin/sh
set -eu
p=internal/lang/cold_review_test.go
trap 'rm -f "$p"' EXIT HUP INT TERM
cat >"$p" <<'EOF'
// SPDX-License-Identifier: Elastic-2.0
package lang
import (
"strings"
"testing"
)
func TestColdReviewDistinctIDsCollapse(t *testing.T) {
parts := []AuthoredPart{
{MutantID: "a/b", Source: `function helper() { return 1; }
test("one", () => helper());`},
{MutantID: "a-b", Source: `function helper() { return 2; }
test("two", () => helper());`},
}
got, err := (jsPlugin{}).ConcatTests(parts)
if err != nil {
t.Fatalf("unexpected refusal: %v", err)
}
t.Logf("concatenated output:\n%s", got)
if n := strings.Count(got, "function helper_ab("); n != 2 {
t.Fatalf("defect not demonstrated: duplicate repaired declaration count = %d", n)
}
}
EOF
go test ./internal/lang -run '^TestColdReviewDistinctIDsCollapse$' -count=1 -v=== RUN TestColdReviewDistinctIDsCollapse
cold_review_test.go:20: concatenated output:
function helper_ab() { return 1; }
test("one", () => helper_ab());
function helper_ab() { return 2; }
test("two", () => helper_ab());
--- PASS: TestColdReviewDistinctIDsCollapse (0.00s)
PASS
ok github.com/pdbethke/corralai/internal/lang 0.003sI attempted to refute on two fronts. First, that idSuffix's collapse is prevented in the function itself: it is not — concat.go:108-120 keeps only alphanumerics and drops all other runes, so '/' and '-' both vanish and the two ids in the finding render the identical fragment 's0m1'/'ab'. The comment at lines 103-107 defends only against a shared SUBSTITUTE character and is silent on deletion, so the code does not do what its own doc claims. Second, that the collapse is caught downstream or made unreachable upstream: mergeParts never checks whether two owners of the same name received the same suffix (lines 230-232 rename each owner independently), ConcatAuthored (lines 79-101) accepts any merge that returns nil error and so never carries the duplicated part out as extra, and finishWriterFanout (internal/advpool/driver_writer_fanout.go:349) passes the generator's mutant ID through unchanged with no normalization or character constraint — with the sharded form 's<idx>/<generator id>' supplying the '/', a pair such as 's0/m1' and 's0/m-1' is reachable from real input. The reviewer's script drives the exported jsPlugin ConcatTests on the exact ids the finding names and shows two identical `function helper_ab(` declarations in the delivered file. Nothing I found makes the claim false or narrower.
confirmed by pdbethke — real, reproduced: idSuffix strips punctuation so a/b and a-b collide into duplicate declarations
ref 9e3f89b00949c53ac0b671fd3d950d5ced66af2876c66e6b7d8bda3541a6ee3e#R2
Recorded so that "found nothing here" can be told apart from "never looked here".
The authored-test concatenation logic is unsafe. Finding 1 shows that collision repair rewrites JavaScript string literals, changing the behavior of tests that were proven before concatenation. Finding 2 shows that distinct mutant IDs can collapse to the same suffix, leaving duplicate declarations in the delivered file.
Finding 1: I could not refute it. mergeParts (internal/lang/concat.go:229-232) builds `regexp.MustCompile("\\b" + QuoteMeta(name) + "\\b")` and calls ReplaceAllString over the ENTIRE joined body of the owning part. Nothing between line 172 and line 233 distinguishes code from string literals, template literals, comments, or `test('title')` strings — the body is a []string of trimmed lines, never tokenized, and the JS spec (jsConcatSpec, concat.go:452-459) sets renameOnCollision to unconditional `true`, so every colliding top-level binding in JS/TS goes through this path. I looked for a lexer, a literal-masking step, a post-rename validation, or a JS-specific override of the replacement, and there is none; the only guards in the file are the precheck (jsSpecifiersDiffer) and the refusal branch at line 227, neither of which touches replacement scoping. The reviewer's script exercises the real exported path — `jsPlugin{}.ConcatTests` — and its output shows both `"helper"` literals rewritten. The one honest narrowing I can offer is not a refutation: in the reviewer's specific example the rewrite happens to be semantics-preserving, because the returned literal and the asserted literal are both rewritten, so the assertion still passes. But the claim as stated is that the replacement is unscoped and rewrites string literals, and that this CAN change or invalidate a previously proven test — the first half is demonstrated verbatim by the script, and the second half follows directly for any part where the literal is compared against a value produced outside the merged file (or where a `test('helper …')` title, a regex source, or a JSON fixture key carries the name). I cannot show the claim false or materially narrower. Finding 2: I could not refute it either, and the code's own doc comment argues the reviewer's side. idSuffix (concat.go:108-120) keeps only [A-Za-z0-9] and DROPS everything else, and its comment at lines 103-107 asserts that ids differing only in punctuation "cannot collapse onto the same suffix by way of a shared substitute character" — which is true of the mechanism it names and false of the outcome: deleting the punctuation collapses them just as surely as substituting it, and the comment's own example pair ("s0/m1" and "s0-m1") both render "s0m1". I checked the two places that could rescue it. ConcatAuthored (concat.go:79-101) adds parts one at a time and keeps the last successful merge, but it never inspects the merged text for a duplicate declaration, so a post-rename collision is silently kept rather than carried out as `extra`. And upstream, finishWriterFanout (internal/advpool/driver_writer_fanout.go:331-356) forwards `a.mutant.ID` unmodified, with no normalization or punctuation constraint; the sharded prefix (`s<idx>/…`, pinned by TestShardedMutantIDsArePrefixed) supplies the '/' while the suffix after it is the generator's own mutant id, so a pair like "s0/m1" and "s0/m-1" is reachable in production, not only in the unit test. mergeParts also does nothing to detect that two owners of the same name received the same suffix — the loop at lines 230-232 just renames each owner in place. The reviewer's script proves exactly what the finding says on the input it names: two distinct MutantIDs, one repaired identifier, two `function helper_ab(` declarations in the delivered file — which in ESM is the SyntaxError the JS doc comment at lines 435-437 says the suffixing exists to prevent. Both findings stand.