Repository navigation
fix(test-runner): validate packages by discovery and -run before running - #24
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1f30038d3f
ℹ️ 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".
| if p.ImportPath == "command-line-arguments" { | ||
| return nil, errors.New(".go file lists are not supported; pass package patterns") |
There was a problem hiding this comment.
When the selected module itself declares module command-line-arguments, go list legitimately returns this import path with a non-nil Module, and go test command-line-arguments succeeds. This unconditional sentinel check therefore rejects a valid Go module as though it were a .go file list; restrict the rejection to discovery records that also lack module/package framing, and add this valid boundary case to the package-validation tests.
AGENTS.md reference: AGENTS.md:L42-L43
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in b24b189.
Reproduced with a module that declares module command-line-arguments: go list -json ./... reports ImportPath: command-line-arguments with a non-nil Module, and go test command-line-arguments returns ok. A .go file list (go list -json a_test.go) reports the same import path with Module: null.
So the sentinel check moved inside the branch that already handles missing module framing, and TestPackageValidation now asserts the module case is accepted (returning the import path) while the file-list record — same import path, no Module — is still rejected.
Both findings come from the Codex review on PR #23 (already merged), so they land as a follow-up. - Accept package patterns ending in ".go". A directory or import path may end in ".go" (go list resolves ./pkg.go to <module>/pkg.go), and the spelling check also rejected the tool's own generated rerun arguments. A real .go file list is rejected by its discovery result instead, because go list reports it as "command-line-arguments". - Reject a -run expression the test binary would reject at startup. Left to go test, "-run [" fails every package with zero failing tests, so an input error was reported as status "failed" with isError false. The element split mirrors testing.splitRegexp, including the escape branch (without it an escaped separator such as `a\/b` splits into `a\`, which does not compile) and the whitespace rewrite (without it `(?P<a b>x)` would be rejected although go test accepts it). Agreement with real "go test -run" verified on 22 expressions. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1f30038 to
633cea2
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 633cea2905
ℹ️ 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".
| normalized := strings.Map(func(r rune) rune { | ||
| if unicode.IsSpace(r) { | ||
| return '_' | ||
| } | ||
| return r |
There was a problem hiding this comment.
When run contains a non-printable, non-whitespace rune such as U+200B, this compiles the raw rune and accepts it, but Go's testing package rewrites it to \u200b, which its regexp parser rejects; the call therefore still becomes a package-level test failure instead of the promised input error. The inverse also occurs for \ followed by U+200B: this validator rejects it although go test accepts it after rewriting. Apply the same strconv.IsPrint/strconv.QuoteRune transformation as testing.rewrite before compiling, and cover both cases.
AGENTS.md reference: AGENTS.md:L42-L43
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed in both directions and fixed in b24b189.
Verified against a real go test -run:
- bare U+200B →
testing: invalid regexp for element 0 of -test.run (""): error parsing regexp: invalid escape sequence(the old validator accepted it) \+ U+200B →ok(the old validator rejected it)
validateRun now runs each element through rewriteRun, which applies the same isSpace / strconv.IsPrint + QuoteRune substitutions as testing.rewrite, and the error message shows the rewritten form when it differs.
One detail worth flagging: the space predicate is testing.isSpace copied verbatim rather than unicode.IsSpace. A sweep over every rune in U+0000–U+10FFFF found 0 divergences between them today, and isSpace is identical in the pinned 1.26.8 toolchain and 1.27.1 — but testing hand-writes that switch precisely because (its own comment) it is "not the same as Unicode Z class", so deriving it from the Unicode tables would start drifting the moment a release adds a White_Space rune. That is the same mismatch class this mirror exists to prevent.
TestRewriteRunMatchesGoTest pins the per-rune output. The expectations were not written by hand: feeding go test -run a [ plus the rune makes the element unconditionally invalid, and the resulting error echoes the rewritten element back, so U+0085/00A0/1680/2000/2005/200A/2028/202F/205F/3000 → _, U+200B → , U+0007 → \a are all observed values.
There was a problem hiding this comment.
Follow-up, because my previous reply overstated the fix: the mirror is now deliberately partial on the escape branch, as of 37fe01e.
Applying the transformation exactly, as suggested, is right for the space branch but wrong for the escape branch — testing.rewrite runs inside the caller's toolchain, while this validator runs inside the one that built the binary (pinned go1.26.8). strconv.IsPrint is generated per Unicode version, so those are not the same function.
Measured, sweeping every rune in U+0000–U+10FFFF: 10,615 runes are printable under go1.27.1 and not under go1.26.8. Reproduced end to end with U+088F:
| go1.26.8 | go1.27.1 | |
|---|---|---|
go test -run <U+088F> |
rejected ("") |
ok |
| exact mirror, built with 1.26.8 | rejected | rejected |
So the exact mirror reports a valid expression as bad input for anyone on Go 1.27 — including reruns this server builds itself via exactRun from a reported test name, which is the round-trip TestRerunRoundTripsThroughOptions is there to guarantee.
unicode.IsGraphic is not a usable discriminator: it tracks the same tables (0 of those runes under 1.26.8, all 10,615 under 1.27.1). What does separate them is the category. All 10,615 are Cn under 1.26.8, whereas the runes your finding requires us to keep rejecting are not — U+200B and U+FEFF are Cf, U+0007 is Cc, U+E000 is Co. The escape branch is therefore limited to Cc/Cf/Cs/Co, which no Unicode release can turn printable.
The asymmetry is deliberate: a rune this misses is passed through and accepted, which is just the pre-existing behaviour your finding describes, rather than a valid expression wrongly rejected. Both directions you identified still hold — bare U+200B is rejected, \+U+200B is accepted — and the suite is green under both 1.26.8 and 1.27.1 so the narrowed branch is exercised against the shipping toolchain's tables, not just the newer ones.
…nd-line-arguments Both findings from the Codex review on #24, each verified against a real "go test" run rather than from reading alone. - validateRun compiled the raw -run element, but go test compiles it after testing.rewrite. The two disagree in both directions: a lone U+200B becomes the escape "" that the regexp parser rejects, so the call still failed every package instead of reporting an input error, while a backslash before that rune became an escaped backslash go test accepts and this rejected. rewriteRun now applies the same substitutions. Its space predicate is testing.isSpace verbatim: unicode.IsSpace agrees on every rune today, but testing hand-writes the switch precisely because it is not the Unicode Z class, so deriving it would drift the moment a release adds a White_Space rune. The per-rune expectations were read off go test's own error text, which echoes the rewritten element back. - validatePackages rejected the import path "command-line-arguments" unconditionally, but a module may declare it: go list then reports it with module framing and "go test command-line-arguments" succeeds. A .go file list is reported with a null Module, so the rejection now sits in the branch that already handles missing framing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The 2026-09-19 entry concluded the non-printable escape could be skipped because it cannot change whether an element compiles. A real "go test -run" disproves that in both directions, so append the correction rather than edit it, plus the sentinel-vs-Module framing lesson from the same review. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…mirror Mirroring testing.rewrite exactly is wrong for the escape branch, because rewrite runs inside the caller's toolchain while this runs inside ours. strconv.IsPrint is generated per Unicode version: a sweep of every rune found 10615 that are printable under go1.27.1 and not under the pinned go1.26.8. Reproduced end to end -- "go test -run <U+088F>" is ok on 1.27.1 and rejected on 1.26.8 -- so the exact mirror rejects expressions the caller's go test accepts, including reruns exactRun builds from reported test names, which TestRerunRoundTripsThroughOptions exists to guarantee. unicode.IsGraphic is no help: it tracks the same tables, matching 0 of those runes under 1.26.8 and all of them under 1.27.1. But every one is Cn there, while the runes the review fix must keep rejecting are not -- U+200B and U+FEFF are Cf, U+0007 is Cc, U+E000 is Co. So the escape is limited to Cc/Cf/Cs/Co, which no Unicode release can turn printable. A rune this misses is passed through and accepted, which is the pre-existing behaviour, instead of being wrongly reported as bad input. Tests run green under both toolchains, so the new branch is exercised with 1.26.8's tables and not only 1.27.1's. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 37fe01e1dc
ℹ️ 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".
| if !ok || strings.ContainsRune(o.Run, 0) { | ||
| return o, fmt.Errorf("run must be a string without NUL bytes") | ||
| } | ||
| if err := validateRun(o.Run); err != nil { |
There was a problem hiding this comment.
When run contains a very large number of top-level / or | separators, this synchronous validation occurs before runTests creates its timeout context: splitRun allocates an element per separator and validateRun compiles every element, without observing request cancellation or timeout_seconds. Thus a multi-megabyte request can keep processing well beyond cancellation, and this time is omitted from elapsed_seconds; cap the expression/element count or perform validation inside a context-aware bounded stage.
AGENTS.md reference: AGENTS.md:L42-L43
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Bounded in f001b19.
Measured the cost first, and it is linear rather than explosive — splitRun/validateRun on "A|" repeated:
| elements | size | validation |
|---|---|---|
| 1,024 | 2 KB | 2.3 ms |
| 65,536 | 128 KB | 79 ms |
| 1,048,576 | 2 MB | 1.0 s |
Deeply nested parens stay linear too (depth 65,536 in 15 ms), so reaching the 300s ceiling would take a roughly 600 MB expression. The gap you identify is real regardless: the work happens in parseOptions, before runTests builds the timeout context, so it is outside timeout_seconds and outside the reported elapsed_seconds.
Capped at 64 KiB rather than made context-aware, since that matches the returned-log limit this server already documents and keeps the failure an input error instead of a timeout. At that size validation measures in tens of milliseconds even when every byte pair is its own element. TestRunSizeIsBounded asserts the limit is accepted, that the dense worst case stays fast, and that both oversize paths are rejected; README and the skill document the limit.
validateRun compiles one regexp per top-level element and runs inside parseOptions, before runTests builds the timeout context, so the work sits outside timeout_seconds and outside the reported elapsed_seconds. Measured, the cost is linear rather than explosive: 2 KB of "A|" elements validates in 2ms, 128 KB in 79ms and 2 MB in 1.0s, and deeply nested parens stay linear too (depth 65536 in 15ms). Reaching the 300s ceiling would take a roughly 600 MB expression. Still, nothing bounded it, so cap the expression at 64 KiB, matching the returned-log limit already documented. At that size validation measures in tens of milliseconds even when every byte pair is its own element, which the new test asserts along with both rejection paths. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 participant
Follow-up to #23 (already merged) addressing both Codex review findings. No human review comments were left on that PR.
P2: package paths ending in
.gowere rejectedparseOptionsrejected any pattern ending in.go. Measured withgo list -e -json:pkg.goresolves to import pathexample.com/exp/pkg.go— a legitimate package that was rejected;.gofile list resolves tocommand-line-argumentswith noModule;go test example.com/exp/pkg.goruns the package normally (file-list mode requires existing files).So the spelling check both rejected valid packages and broke the round-trip of the tool's own
rerunobject, whosepackagesholds a resolved import path. File lists are now rejected invalidatePackagesby their discovery result (checked before theModule == nilbranch so the message is specific).P2: invalid
-runexpressions were reported as test failuresMeasured:
go test -json -run '['emits a package-levelfailfor every package and exits 1, so the tool returnedstatus: "failed"withisError: falseand zero failing tests — a caller input error read as a project test failure.parseOptionsnow validates the expression and returnsstatus: "error".The validator mirrors
testing.splitRegexp(bracket/group/escape tracking) plus the whitespace→_rewrite thatverifyapplies. Both parts were checked against a variant with the branch removed:a\/b,a\|b) splits intoa\, which does not compile, so a valid expression is rejected.exactRunoutput is not affected: its elements are/-joined QuoteMeta'd pieces, so a merged element still compiles.(?P<a b>x)is rejected althoughgo testaccepts it.Agreement with real
go test -runwas verified on 22 expressions (12 valid, 10 invalid), andsplitRegexp/rewrite/isSpaceare byte-identical between the pinned toolchain and 1.27.1.Verification
go vet ./...andgo test -count=1 ./...pass under the pinned.go-version(go1.26.8)../build.sh test-runnerrebuilt the three committed binaries (required by the stale-detection gate).pkg.gonow runs (status: passed), a.gofile list returnsstatus: errorwith.go file lists are not supported.a.gomust-reject case moved fromparseOptionsdown tovalidatePackagesas acommand-line-argumentsrecord, plus valid/invalid-runtables (including the escaped separators) and a new rerun round-trip test for a package ending in.go.skills/test-runner-gemini/SKILL.md, and LESSONS.md.🤖 Generated with Claude Code