internal/advpool at 201253a6e5, reviewed by gemini-3.6-flash, verified by claude-code.
authoredTestPath uses unanchored substring matching on devBase to insert the _corral marker, causing code file stems that match substrings in test filenames or extensions (such as 'st' in '_test.go' or 'go' in '.go') to mangle test file suffixes and extensions into uncollectable paths like 'login_test_corral.go' or 'foo_test.go_corral'.
cat << 'EOF' > internal/advpool/repro_test.go
package advpool
import (
"fmt"
"strings"
"testing"
)
func TestAuthoredTestPathManglesSuffix(t *testing.T) {
got1 := authoredTestPath("internal/auth/st.go", "internal/auth/login_test.go", nil)
if !strings.HasSuffix(got1, "_test.go") {
fmt.Printf("REPRODUCED BUG 1: authoredTestPath(\"internal/auth/st.go\", \"internal/auth/login_test.go\") = %q (loses _test.go suffix)\n", got1)
} else {
t.Fatalf("expected suffix loss for st.go, got %q", got1)
}
got2 := authoredTestPath("pkg/go.go", "pkg/foo_test.go", nil)
if !strings.HasSuffix(got2, ".go") {
fmt.Printf("REPRODUCED BUG 2: authoredTestPath(\"pkg/go.go\", \"pkg/foo_test.go\") = %q (loses .go extension)\n", got2)
} else {
t.Fatalf("expected extension loss for go.go, got %q", got2)
}
}
EOF
go test ./internal/advpool -v -run TestAuthoredTestPathManglesSuffix=== RUN TestAuthoredTestPathManglesSuffix
REPRODUCED BUG 1: authoredTestPath("internal/auth/st.go", "internal/auth/login_test.go") = "internal/auth/login_test_corral.go" (loses _test.go suffix)
REPRODUCED BUG 2: authoredTestPath("pkg/go.go", "pkg/foo_test.go") = "pkg/foo_test.go_corral" (loses .go extension)
--- PASS: TestAuthoredTestPathManglesSuffix (0.00s)
PASS
ok github.com/pdbethke/corralai/internal/advpool 0.008sauthoredTestPath (internal/advpool/gate.go:102) derives the authored path via an unanchored first-occurrence `strings.Replace(devBase, stem, stem+marker, 1)` at gate.go:143, entered whenever `strings.Contains(devBase, stem)` (gate.go:142). Neither the replace nor the only post-check (`taken`, gate.go:116-122 and 154, which tests collision with devTestPath/codePath/base only) constrains the result to keep the language's test suffix or the file extension, so the two inputs the finding names produce exactly the strings the reviewer's script printed. I tried to refute in three ways and all failed: (1) looking for a suffix/extension post-condition inside the function — there is none, the loop returns `pick` as soon as it is untaken; (2) looking for a downstream validator or rewrite at every call site of authoredTestPath (gate.go:306, 442, 530, 579, 756, 896 and roles.go:205) — gatedScoreOpts only substitutes codePath when the path is missing from the workspace (gate.go:307-313), and authoredWorkspace overlays the returned string as-is (gate.go:530); (3) checking whether the existing tests already exclude these shapes — authored_path_test.go and roles_authored_path_test.go only cover stem-prefixed names (cli.py/test_cli.py, login.go/login_test.go, pricing.rb/pricing_test.rb, foo.js/foo.test.js), none of which exercise a mid-name or extension-overlapping match. `login_test_corral.go` is not collected by `go test` (Go requires the `_test.go` suffix) and `foo_test.go_corral` is not a Go file at all, so 'uncollectable' is accurate as stated.
confirmed by pdbethke — real, reproduced: unanchored first-occurrence stem replace mangles the authored test's suffix for stems like st or go, so the authored test is never collected
ref a79b583bacea53ef9c7da0ccfb0efea21d17c897ada40efac344a4121e934da5#R1
Recorded so that "found nothing here" can be told apart from "never looked here".
Review of internal/advpool identified a high-severity defect in authored test file path generation (Finding 1), where naive substring replacement in `authoredTestPath` mangles test file suffixes and extensions when code file stems match common test words or file extensions (e.g., 'st.go' or 'go.go'). Other components audited in scope, including Wilson interval calculation, budget allocation, critic adjudication, and matrix test capping, were found sound.
R1: I could not refute it. The function at internal/advpool/gate.go:102-159 computes the authored test path by `strings.Replace(devBase, stem, stem+marker, 1)` (gate.go:143) whenever the code file's stem appears anywhere in the dev test's basename (gate.go:142) — an unanchored, first-occurrence substring match with no notion of where the stem sits in the name, and no post-condition that the result still carries the language's test suffix or even the original extension. The reviewer's two inputs land exactly in that hole: stem "st" first matches inside "login_te|st|.go", and stem "go" first matches inside "foo_test.|go". The only guard after `pick` is `taken()` (gate.go:116-122, 154), which checks collision with the dev test, the code file, and existing repo files — never the shape of the name. I looked for a downstream corrective at every consumer of the value (gate.go:306, 442, 530, 579, 756, 896; roles.go:205) and found none: gatedScoreOpts (gate.go:307-313) only degrades the compile check to codePath when the path is absent from the base workspace, and authoredWorkspace (gate.go:530) overlays the string verbatim. The existing regression suite (authored_path_test.go, roles_authored_path_test.go) pins only stem-prefixed pairings, so nothing in the package covers the mangling case. The reviewer's script passed and demonstrates the claim on the inputs it names; the outputs it prints match what I read in the code. The only narrowing I can honestly note — and it does not falsify anything the finding asserts — is that reaching this requires a code stem that is a substring of the dev/covering test's basename but not aligned with its stem, which is most plausible on the evidence-only covering-test path where no naming convention pairs the two. The reviewer's other five 'checked and sound' items are not claims I am asked to verify.