internal/reposcan at de0d9860ee, reviewed by claude-code, verified by codex.
demoteAmbiguousPairings excludes explicit (--tests map) pairings from the collision grouping entirely rather than treating them as non-demotable claimants, so a convention-derived pairing that lands on the SAME test file as an operator-mapped one is left alone — two source files are graded by one test, which is exactly the wrong-pairing/wrong-verdict shape ReasonAmbiguousTest exists to prevent.
#!/bin/sh
set -e
cat > internal/reposcan/zz_review_ambiguity_test.go <<'GOEOF'
package reposcan
import (
"encoding/json"
"os"
"path/filepath"
"testing"
)
// One test file must grade exactly one source file (demoteAmbiguousPairings'
// own stated global property). An explicit --tests mapping is dropped out of
// the grouping, so a convention claimant on the same test survives alone.
func TestReviewExplicitPairingCollidesWithConvention(t *testing.T) {
root := t.TempDir()
write := func(rel, body string) {
p := filepath.Join(root, filepath.FromSlash(rel))
if err := os.MkdirAll(filepath.Dir(p), 0o755); err != nil {
t.Fatal(err)
}
if err := os.WriteFile(p, []byte(body), 0o644); err != nil {
t.Fatal(err)
}
}
write("pkg/a.py", "def a():\n return 1\n")
write("pkg/shared.py", "def shared():\n return 2\n")
write("tests/test_shared.py", "def test_shared():\n assert True\n")
mapPath := filepath.Join(t.TempDir(), "tests.json")
b, err := json.Marshal(map[string]string{"pkg/a.py": "tests/test_shared.py"})
if err != nil {
t.Fatal(err)
}
if err := os.WriteFile(mapPath, b, 0o644); err != nil {
t.Fatal(err)
}
tm, err := NewFileTestMap(mapPath)
if err != nil {
t.Fatal(err)
}
cands, excl, err := EnumerateWithTests(root, tm)
if err != nil {
t.Fatal(err)
}
byTest := map[string][]string{}
for _, c := range cands {
t.Logf("candidate %-16s -> %s", c.Path, c.TestPath)
byTest[c.TestPath] = append(byTest[c.TestPath], c.Path)
}
for _, e := range excl {
t.Logf("exclusion %-16s (%s)", e.Path, e.Reason)
}
srcs := byTest["tests/test_shared.py"]
if len(srcs) < 2 {
t.Fatalf("fixture did not reproduce the shape: tests/test_shared.py grades %v", srcs)
}
t.Errorf("ONE TEST GRADES %d SOURCE FILES: tests/test_shared.py -> %v (neither was demoted as %s)", len(srcs), srcs, ReasonAmbiguousTest)
}
GOEOF
GOPROXY=off go test -count=1 -v -run TestReviewExplicitPairingCollidesWithConvention ./internal/reposcan/ > /tmp/rev1.txt 2>&1 || true
cat /tmp/rev1.txt
grep -q "ONE TEST GRADES" /tmp/rev1.txt || exit 1
exit 0=== RUN TestReviewExplicitPairingCollidesWithConvention
zz_review_ambiguity_test.go:47: candidate pkg/a.py -> tests/test_shared.py
zz_review_ambiguity_test.go:47: candidate pkg/shared.py -> tests/test_shared.py
zz_review_ambiguity_test.go:51: exclusion tests/test_shared.py (is-test)
zz_review_ambiguity_test.go:57: ONE TEST GRADES 2 SOURCE FILES: tests/test_shared.py -> [pkg/a.py pkg/shared.py] (neither was demoted as ambiguous-test)
--- FAIL: TestReviewExplicitPairingCollidesWithConvention (0.00s)
FAIL
FAIL github.com/pdbethke/corralai/internal/reposcan 0.011s
FAILI traced explicitPairs into demoteAmbiguousPairings. candidate.go:676 skips explicit candidates before constructing the TestPath groups, so the explicit claimant is invisible when deciding whether a convention claimant is ambiguous. The exemption's rationale supports deliberately mapping multiple sources to one suite, but this input maps only pkg/a.py; it does not make pkg/shared.py's convention-derived claim deliberate. Thus the mixed explicit/convention collision demonstrated by the reviewer survives, contrary to the function's global-property documentation. I found no later collision check.
confirmed by pdbethke — real, reproduced: explicit --tests pairings are left out of the collision grouping, so a convention pairing on the same test file is not demoted
ref 28c4ae555cbc0e686bc6a9ff86a267453c9619e306f250a9c5d36bc6db42f8ce#R1
CanonicalKV joins name=value pairs with `,` and `=` without escaping or length-prefixing, so a single operator-supplied model name containing those bytes renders byte-identically to a different, larger role→model map — and KeyInputs.CacheKey therefore returns the SAME content address for two genuinely different herds (e.g. a run with no challenger writer vs. a run with one).
#!/bin/sh
set -e
cat > internal/reposcan/zz_review_kv_test.go <<'GOEOF'
package reposcan
import "testing"
// modelSetKey (cmd/corral/certify_local.go) builds exactly this map shape and
// hands it to CanonicalKV. The sorted key order is
// critic < mutant-generator < shadow < test-writer < test-writer-shadow, and
// test-writer-shadow is OMITTED when no challenger writer is named — so a
// test-writer name carrying CanonicalKV's own delimiters forges it.
func TestReviewCanonicalKVDelimiterCollision(t *testing.T) {
forged := map[string]string{
"critic": "gemini-3.6-flash",
"mutant-generator": "gemini-3.6-pro",
"shadow": "off",
"test-writer": "claude-sonnet-5,test-writer-shadow=claude-opus-5",
}
real := map[string]string{
"critic": "gemini-3.6-flash",
"mutant-generator": "gemini-3.6-pro",
"shadow": "off",
"test-writer": "claude-sonnet-5",
"test-writer-shadow": "claude-opus-5",
}
a, b := CanonicalKV(forged), CanonicalKV(real)
t.Logf("no-challenger run ModelSet = %q", a)
t.Logf("challenger run ModelSet = %q", b)
if a != b {
t.Fatalf("no collision: %q vs %q", a, b)
}
mk := func(ms string) string {
return KeyInputs{
SourceDigest: "src", PackageDigest: "pkg", GoalDigest: "goal",
TestSurfaceDigest: "surf", EngineVersion: VerdictGeneration,
ModelSet: ms, AuditConfig: "", Substrate: SubstrateJail,
}.CacheKey()
}
ka, kb := mk(a), mk(b)
if ka != kb {
t.Fatalf("ModelSet strings collided but CacheKeys did not: %s vs %s", ka, kb)
}
t.Errorf("CACHE KEY COLLISION ACROSS DIFFERENT HERDS: both render ModelSet=%q and CacheKey=%s; one run named NO challenger writer, the other named claude-opus-5", a, ka)
}
GOEOF
GOPROXY=off go test -count=1 -v -run TestReviewCanonicalKVDelimiterCollision ./internal/reposcan/ > /tmp/rev2.txt 2>&1 || true
cat /tmp/rev2.txt
grep -q "CACHE KEY COLLISION" /tmp/rev2.txt || exit 1
exit 0=== RUN TestReviewCanonicalKVDelimiterCollision
zz_review_kv_test.go:25: no-challenger run ModelSet = "critic=gemini-3.6-flash,mutant-generator=gemini-3.6-pro,shadow=off,test-writer=claude-sonnet-5,test-writer-shadow=claude-opus-5"
zz_review_kv_test.go:26: challenger run ModelSet = "critic=gemini-3.6-flash,mutant-generator=gemini-3.6-pro,shadow=off,test-writer=claude-sonnet-5,test-writer-shadow=claude-opus-5"
zz_review_kv_test.go:41: CACHE KEY COLLISION ACROSS DIFFERENT HERDS: both render ModelSet="critic=gemini-3.6-flash,mutant-generator=gemini-3.6-pro,shadow=off,test-writer=claude-sonnet-5,test-writer-shadow=claude-opus-5" and CacheKey=efbc620c81a3fd5d9046f47479f06fbd83c8747279e50df78750d0807eb55db6; one run named NO challenger writer, the other named claude-opus-5
--- FAIL: TestReviewCanonicalKVDelimiterCollision (0.00s)
FAIL
FAIL github.com/pdbethke/corralai/internal/reposcan 0.009s
FAILI traced role resolution and modelSetKey. resolveRoleModels trims model strings but does not reject or encode ',' or '='; modelSetKey passes those values directly to CanonicalKV, whose serialization at canonical.go:31-36 concatenates raw keys and values. The reviewer's two maps therefore serialize identically, and CacheKey's per-field length framing cannot distinguish already-identical ModelSet strings. I looked for model-name validation that would make the named input unreachable and found none.
confirmed by pdbethke — real, reproduced: CanonicalKV is not injective — a model name carrying , or = collides the cache key with a different role map
ref 28c4ae555cbc0e686bc6a9ff86a267453c9619e306f250a9c5d36bc6db42f8ce#R2
WidenCandidacyByEvidence reads lang.FileCoverage.HasStatements' ZERO VALUE (false) as a positive "coverage's static parse found no executable code" finding, so any selector whose Index does not explicitly set the field relabels every genuinely-uncovered file as the benign ReasonNoExecutableCode instead of ReasonUncovered — absence of evidence read as evidence of absence, in the direction that silences the loudest finding the audit produces.
#!/bin/sh
set -e
cat > internal/reposcan/zz_review_statements_test.go <<'GOEOF'
package reposcan
import (
"testing"
"github.com/pdbethke/corralai/internal/lang"
)
// reviewSelector is a lang.TestSelector whose Index returns FileCoverage
// values built the way a plugin that has no opinion about HasStatements
// would build them: the field is simply never set. lang.FileCoverage's own
// doc says it "defaults to true (conservative) when a plugin could not
// determine the count" — a default the bool zero value is the OPPOSITE of.
type reviewSelector struct {
lang.Plugin
index map[string]lang.FileCoverage
}
func (reviewSelector) Name() string { return "reviewfake" }
func (reviewSelector) Instrument(cmd []string) ([]string, bool) { return nil, false }
func (reviewSelector) WithAuthoredTest(sel lang.Selection, testCmd []string, authored string) []string {
return nil
}
func (reviewSelector) ForSpan(sel lang.Selection, span lang.LineRange) ([]string, []string, string) {
return nil, nil, ""
}
func (reviewSelector) Select(evidence []byte, repoRoot, codePath, testPath string, testCmd []string) (lang.Selection, error) {
return lang.Selection{}, nil
}
func (s reviewSelector) Index(evidence []byte) (map[string]lang.FileCoverage, error) {
return s.index, nil
}
func TestReviewUnsetHasStatementsSilencesUncovered(t *testing.T) {
// pkg/dead.go: a Go file (no lang.LibraryCodeClassifier, so isLibraryCode
// is true), measured, zero covering tests, no import-time coverage.
// The honest verdict is ReasonUncovered.
sel := reviewSelector{index: map[string]lang.FileCoverage{
"pkg/dead.go": {Tests: map[string]int{}}, // HasStatements left unset
}}
idx, ok := ParseEvidenceIndex(SelectionEvidence{Ran: true, Raw: []byte("x")}, sel)
if !ok {
t.Fatal("ParseEvidenceIndex: ok=false")
}
excl := []Exclusion{{Path: "pkg/dead.go", Reason: ReasonNoPairedTest}}
_, got, _ := WidenCandidacyByEvidence(nil, excl, idx, true)
if len(got) != 1 {
t.Fatalf("exclusions = %v, want exactly one", got)
}
t.Logf("reason for a measured, zero-covering-test file = %q", got[0].Reason)
if got[0].Reason == ReasonNoExecutableCode {
t.Errorf("FAIL-OPEN DEFAULT: an UNSET HasStatements produced %q (benign, never counted alongside uncovered) instead of %q — the loudest finding is silenced by a field the plugin never filled in", got[0].Reason, ReasonUncovered)
}
}
GOEOF
GOPROXY=off go test -count=1 -v -run TestReviewUnsetHasStatementsSilencesUncovered ./internal/reposcan/ > /tmp/rev3.txt 2>&1 || true
cat /tmp/rev3.txt
grep -q "FAIL-OPEN DEFAULT" /tmp/rev3.txt || exit 1
exit 0=== RUN TestReviewUnsetHasStatementsSilencesUncovered
zz_review_statements_test.go:50: reason for a measured, zero-covering-test file = "no executable code"
zz_review_statements_test.go:52: FAIL-OPEN DEFAULT: an UNSET HasStatements produced "no executable code" (benign, never counted alongside uncovered) instead of "uncovered — no test executes this file" — the loudest finding is silenced by a field the plugin never filled in
--- FAIL: TestReviewUnsetHasStatementsSilencesUncovered (0.00s)
FAIL
FAIL github.com/pdbethke/corralai/internal/reposcan 0.009s
FAILThe finding overstates a hypothetical contract violation as a production audit failure. lang.FileCoverage documents that an Index implementation must set HasStatements=true when it cannot determine the count. The only production TestSelector.Index implementation is pyPlugin.Index (internal/lang/python.go:1442), and at python.go:1473 it does exactly that with `f.Statements == nil || *f.Statements > 0`. No other production language implements Index. The review fixture injects a fake selector that knowingly violates this contract and bypasses production language detection; it demonstrates how a future nonconforming plugin could fail, not that any current audit silently relabels uncovered files.
ref 28c4ae555cbc0e686bc6a9ff86a267453c9619e306f250a9c5d36bc6db42f8ce#R3
ParseEvidenceIndex picks mostCovering by taking the single test ID with the most executed lines and then mapping it to its containing file, never summing per FILE, so evidenceFileEntry.mostCovering / Candidate.CoveringTestPath — documented as "the covering test FILE with the most executed lines of this file" — can name a file whose tests collectively cover far less than a rival's, giving an evidence-only candidate the wrong authored-test landing hint.
I inspected ParseEvidenceIndex at evidence_index.go:108-125. It compares each test ID's line count independently, converts only the winning ID to a file path, and never aggregates counts after multiple IDs collapse to the same file. That conflicts with evidenceFileEntry.mostCovering's documented file-level meaning. Existing tests explicitly pin the single-test-ID interpretation rather than exercising multiple test IDs from one file, so they do not refute the claimed counterexample.
confirmed by pdbethke — real, read: the covering test FILE is picked from the single most-covering test id, never summed per file
ref 28c4ae555cbc0e686bc6a9ff86a267453c9619e306f250a9c5d36bc6db42f8ce#R4
fileChurn parses `git log --format= --name-only` output as raw paths, but git quotes and C-escapes any path with non-ASCII or special bytes under the default core.quotePath, so such a candidate never matches a churn entry and is silently ranked with churn=1 — a degraded ordering that RankInfo reports as the undegraded "churn-x-size" signal.
I inspected fileChurn at rank.go:92-107. It invokes `git log --format= --name-only` without `-z` or a quotePath override, then treats each textual line—after TrimSpace—as the literal repository path. Git's default quoted/C-escaped representation therefore cannot match Candidate.Path for affected filenames. The missing map entry is silently converted to churn 1 by Rank while RankInfo remains `churn-x-size`; I found no decoding or per-file degradation disclosure.
confirmed by pdbethke — real, read: git log --name-only output is C-quoted for special bytes and never matches; such a file ranks with churn 1
ref 28c4ae555cbc0e686bc6a9ff86a267453c9619e306f250a9c5d36bc6db42f8ce#R5
Recorded so that "found nothing here" can be told apart from "never looked here".
This package is unusually well-documented and the comments are mostly honest about their own limits, which makes the gaps that remain interesting: they are all places where a stated global property is enforced at one door and skipped at another. Finding 1 is the sharpest — demoteAmbiguousPairings claims to enforce, as a GLOBAL property, that one test file grades exactly one source file, and then removes explicit (--tests map) pairings from the grouping entirely rather than keeping them as non-demotable claimants, so a tenant who maps ONE source onto a test that some OTHER source also resolves by convention gets exactly the wrong-pairing shape ReasonAmbiguousTest exists to refuse: mutants planted in one file, graded by a test the operator designated for another. That is reproduced on a real tree. Finding 2 is the mirror of a defence the codebase already built elsewhere: auditConfigKey deliberately folds the operator's argv to a sha256 because it "routinely contains `=` and `,` — the delimiters CanonicalKV itself uses, so operator text could otherwise forge or corrupt neighbouring components" — and then modelSetKey hands operator-named model strings to the same CanonicalKV raw. TestCacheKeyIsUnambiguous only pins the cross-FIELD boundary; nothing pins the inside of ModelSet. Finding 3 is the fail-open direction of a bool whose lang.go contract says it "defaults to true (conservative)" — a default Go cannot express, and reposcan reads the zero value as a positive finding that silences the loudest exclusion the tool produces. Findings 4 and 5 are smaller reading-level claims. I could not break the digest layer, the accounting arithmetic in Aggregate, or Select's aliasing discipline.
R1, R2, R4, and R5 stand after tracing their exact inputs through the implementation. R3 is refuted as a current production defect: its fixture supplies a deliberately contract-violating selector that cannot be selected by the production language registry; the sole registered TestSelector implements the documented conservative default.