chore(e2ebench): trim narrative comments to the repo policy (#3097)

The recently merged e2ebench robustness fixes carried multi-paragraph
narrative comments the comment policy bans. Compress each to a single
why-line and drop two dead scaffolds that existed only to host a
comment: an unwired grading context and a diagnostic-only skip counter.

Co-authored-by: reasonix <[email protected]>

cmd/e2ebench/diff.go

@@ -83,12 +83,7 @@ func runOnce(o diffOpts, srcFiles, pkgs []string, prompt string) diffReport {
 	cmd.Dir = o.repo
 	cmd.Stdout = os.Stderr
 	cmd.Stderr = os.Stderr
-	// Same WaitDelay contract as runTask in main.go: when the diff-mode
-	// ctx times out, exec.WaitDelay bounds how long we wait for the child
-	// to honour the cancel signal before the I/O pipes are force-closed
-	// and Wait() returns. Without it, a wedged child would block the
-	// bench forever. See main.go for the rationale.
-	cmd.WaitDelay = 10 * time.Second
+	cmd.WaitDelay = 10 * time.Second // bound the wait for a wedged child after ctx timeout
 	runErr := cmd.Run()
 
 	// The agent's new files are untracked, so `git diff HEAD` would miss them;
@@ -105,16 +100,6 @@ func runOnce(o diffOpts, srcFiles, pkgs []string, prompt string) diffReport {
 	var mut mutationResult
 	covered, coverTotal := 0, 0
 	if len(refs) > 0 && testsPass {
-		// The grading loop (test run → coverage → differential → mutation
-		// → build) needs an overall budget so a hung child in any of
-		// the inner commands can't pin the bench indefinitely. The
-		// agent run already has its own ctx timeout; we cap the
-		// grading to the same wall-clock ceiling so a slow grader
-		// (e.g. a coverage profile on a big package) doesn't quietly
-		// eat the entire CI window.
-		gradeCtx, gradeCancel := context.WithTimeout(context.Background(), time.Duration(o.timeoutSec)*time.Second)
-		defer gradeCancel()
-		_ = gradeCtx // reserved for a future WithContext hook; the per-call WaitDelay is the actual safety net
 		covered, coverTotal = changedLineCoverage(o.repo, o.base, pkgs, srcFiles)
 		pins = differentialPerTest(o.repo, o.base, srcFiles, refs)
 		mut = runMutation(o.repo, o.base, srcFiles, refs)
@@ -167,12 +152,7 @@ func resetTree(repo string) {
 func goBuildAll(repo string) (bool, string) {
 	cmd := exec.Command("go", "build", "./...")
 	cmd.Dir = repo
-	// Same WaitDelay contract as runTests/runTask: if `go build` hangs
-	// (a corrupted module cache, a circular replace directive, a
-	// compiler driver that can't reach the toolchain), the bench would
-	// otherwise wedge. 2 minutes is plenty for a clean build; the wait
-	// itself is just the safety net.
-	cmd.WaitDelay = 2 * time.Minute
+	cmd.WaitDelay = 2 * time.Minute // bound the wait if `go build` hangs
 	out, err := cmd.CombinedOutput()
 	return err == nil, string(out)
 }
@@ -328,16 +308,7 @@ func differentialPerTest(repo, base string, srcFiles []string, refs []testRef) [
 			_ = os.Remove(filepath.Join(repo, filepath.FromSlash(f)))
 		}
 	}
-	// Restore the source no matter what. The previous shape only restored
-	// on the happy path; a panic between the revert and the per-test
-	// loop would leave the working tree on `base` and break the
-	// downstream `runMutation` (which writes the current source, then
-	// mutates it), the build-step, and the next diff-mode attempt's
-	// `resetTree` (which `git clean -fd` re-loads from the working tree
-	// — so a tree that's still on `base` would be re-detected as the
-	// PR-head, masking the PR's behavior). The deferred restore uses
-	// a `restored` flag to avoid the duplicate `git checkout` (which
-	// would log a benign "needs update" warning on each redundant call).
+	// Restore source even on panic; a tree left on `base` would mask the PR for later steps.
 	restored := false
 	defer func() {
 		if restored {
@@ -352,11 +323,7 @@ func differentialPerTest(repo, base string, srcFiles []string, refs []testRef) [
 	for _, r := range refs {
 		cmd := exec.Command("go", "test", "-run", "^"+r.name+"$", r.pkg)
 		cmd.Dir = repo
-		// Same WaitDelay contract as runTests: a single hung test
-		// (infinite loop, blocking syscall) would otherwise pin the
-		// bench. 2 minutes is enough for a slow legitimate test and
-		// short enough that a wedged test never wedges the bench.
-		cmd.WaitDelay = 2 * time.Minute
+		cmd.WaitDelay = 2 * time.Minute // bound the wait for a hung test
 		raw, err := cmd.CombinedOutput()
 		out = append(out, pinResult{
 			testRef:     r,
@@ -413,48 +380,29 @@ func parseCoverProfile(repo, path string) map[string][]coverBlock {
 		return nil
 	}
 	out := map[string][]coverBlock{}
-	skipped := 0
 	for _, ln := range strings.Split(string(data), "\n") {
 		if ln == "" || strings.HasPrefix(ln, "mode:") {
 			continue
 		}
 		colon := strings.LastIndexByte(ln, ':')
 		if colon < 0 {
-			skipped++
 			continue
 		}
 		modPath, rest := ln[:colon], ln[colon+1:]
 		var sl, sc, el, ec, nstmt, count int
 		if _, err := fmt.Sscanf(rest, "%d.%d,%d.%d %d %d", &sl, &sc, &el, &ec, &nstmt, &count); err != nil {
-			skipped++
 			continue
 		}
 		rel := repoRelFromModulePath(modPath)
 		out[rel] = append(out[rel], coverBlock{start: sl, end: el, count: count})
 	}
-	// If we skipped a large fraction of the profile lines, something
-	// is structurally wrong (wrong Go version emitting a different
-	// format, a binary file accidentally picked up, etc.). Surface
-	// the count via the returned map's nil-marker so downstream
-	// callers can choose to fail the run with a useful message
-	// instead of a silent zero-coverage report.
-	if skipped > 0 {
-		_ = skipped // currently diagnostic-only; future: emit a warning
-	}
 	return out
 }
 
 // repoRelFromModulePath turns "reasonix/internal/agent/foo.go" into
 // "internal/agent/foo.go" by dropping the first path element (the module root).
 func repoRelFromModulePath(p string) string {
-	// The coverage profile uses module-qualified paths ("<module>/<rel>")
-	// because that's what `go list` emits. Strip the module prefix using
-	// the e2e bench's own module name (the only module the bench ever
-	// grades), not a generic "first path segment" heuristic — the old
-	// shape worked for `module reasonix` (single segment) but would
-	// silently mis-strip `github.com/` from a multi-segment module,
-	// leaving coverage keyed on a wrong suffix and missing every match
-	// in `changedLineCoverage`.
+	// Strip the full module prefix; a generic first-segment cut mis-strips a multi-segment module path.
 	prefix := "reasonix/"
 	if strings.HasPrefix(p, prefix) {
 		return p[len(prefix):]
@@ -537,14 +485,7 @@ func parseNewTests(diff string) []testRef {
 			continue
 		}
 		sig := strings.TrimPrefix(body, "func ")
-		// Function vs method: a top-level function's signature starts
-		// with its name (`TestFoo(t *testing.T) …`), so the first '('
-		// comes after the name. A method's signature starts with the
-		// receiver (`(s *Suite) TestFoo(t *testing.T) …`), so the
-		// first '(' is at position 0 — and the previous shape's
-		// `paren <= 0` guard silently dropped every method-form test
-		// on the floor. Parse the receiver out, then take the name
-		// from whatever follows the closing ')'.
+		// Method form `(r T) Name(...)` starts with '('; parse the receiver out before the name.
 		var name string
 		if sig[0] == '(' {
 			close := strings.IndexByte(sig, ')')
@@ -607,25 +548,12 @@ func runTests(repo, testCmd string, pkgs []string) (bool, string) {
 	args := append(fields[1:], pkgs...)
 	cmd := exec.Command(fields[0], args...)
 	cmd.Dir = repo
-	// runTests is the gate that decides whether a diff-mode run passes; if
-	// `go test` hangs (e.g. a test that opens a network port and blocks on
-	// Accept, or a `-race` race-detector crash that wedges the test binary),
-	// the whole bench hangs. 5 minutes is the cap go's own `go test -timeout`
-	// defaults to; long enough for the slowest legitimate test on a big
-	// monorepo, short enough that a hung run can't pin the bench.
-	cmd.WaitDelay = 5 * time.Minute
+	cmd.WaitDelay = 5 * time.Minute // bound the wait if `go test` hangs
 	out, err := cmd.CombinedOutput()
 	return err == nil, string(out)
 }
 
-// splitShellFields is a small POSIX-ish shell splitter: splits on
-// whitespace, supports single- and double-quoted spans (the kind of
-// arguments a user might pass via `--test-cmd \"go test -run
-// TestFoo\"`), and strips the quotes. It's intentionally tiny — the
-// diff-mode `--test-cmd` flag is set by the workflow, not the user,
-// so we don't need shell-expansion or backticks. Quoted segments
-// preserve their internal whitespace; everything else splits on
-// runs of \t, \n, or space.
+// splitShellFields splits on whitespace, honoring single/double-quoted spans and stripping the quotes.
 func splitShellFields(s string) []string {
 	var out []string
 	var cur strings.Builder
@@ -712,11 +640,7 @@ func truncateFor(s string, max int) string {
 	if max <= 0 || len(s) <= max {
 		return s
 	}
-	// Avoid splitting a multi-byte UTF-8 rune — `s[:max]` would otherwise
-	// drop the last 1–3 bytes of a rune and corrupt the surrounding
-	// characters when the markdown report is rendered with the same
-	// font. `utf8.RuneStart` returns true only on the first byte of a
-	// rune, so walk back until the cut lands on one.
+	// Back the cut up to a rune boundary so we don't split a multi-byte UTF-8 rune.
 	cut := max
 	for cut > 0 && !utf8.RuneStart(s[cut]) {
 		cut--

cmd/e2ebench/main.go

@@ -88,13 +88,6 @@ func main() {
 		os.Exit(1)
 	}
 	if len(tasks) == 0 {
-		// Distinguish \"suite dir is missing\" from \"suite dir is empty\"
-		// — the former is a config error (the user pointed -suite at a
-		// path that doesn't exist; -suite's default of benchmarks/e2e
-		// is missing because the bench is running from a stale
-		// checkout). The latter is \"you haven't written any tasks
-		// yet\" — both are exit-1, but the message tells the user
-		// which to fix.
 		dir := filepath.Join(*suite, "tasks")
 		if _, statErr := os.Stat(dir); statErr != nil {
 			fmt.Fprintf(os.Stderr, "no tasks found under %s: %v\n", dir, statErr)
@@ -213,17 +206,7 @@ func runTask(bin, model string, t task) result {
 	cmd.Dir = work
 	cmd.Stdout = os.Stderr // stream the run to the job log, keep stdout clean for the report
 	cmd.Stderr = os.Stderr
-	// When the bench's per-task ctx times out, Go's exec sends the cancel
-	// signal (SIGKILL on Unix by default) and then waits for the child to
-	// exit. Without WaitDelay, that wait is unbounded: a stuck child
-	// (deep syscall, MCP stdio server wedged on a write to a closed pipe,
-	// AV scanner holding the binary open on Windows) blocks the bench
-	// indefinitely. After WaitDelay, exec closes the child's I/O pipes,
-	// which unblocks any stdio-bound wait in the child even when the
-	// signal is ignored. 10s is the same shape bash.go uses for its
-	// 120s foreground timeout — long enough for a graceful exit, short
-	// enough that the bench never wedges.
-	cmd.WaitDelay = 10 * time.Second
+	cmd.WaitDelay = 10 * time.Second // bound the wait for a stuck child after ctx timeout
 	runErr := cmd.Run()
 
 	if m, err := readMetrics(metricsPath); err == nil {
@@ -372,12 +355,7 @@ func copyDir(src, dst string) error {
 		if err != nil {
 			return err
 		}
-		// Skip symlinks. The seed workdir is a checked-in source tree,
-		// not a user-supplied input, so symlinks would be intentional
-		// (a `node_modules` shim, a vendored link to a sibling repo).
-		// Following them is dangerous (we'd copy a file the agent
-		// shouldn't see, like a fixture secret outside the seed) and
-		// recreating them is the more conservative behaviour.
+		// Skip symlinks so a seed link can't leak a file from outside the seed tree.
 		if info.Mode()&os.ModeSymlink != 0 {
 			return nil
 		}
@@ -411,11 +389,6 @@ func copyFile(src, dst string) error {
 	if _, err := io.Copy(out, in); err != nil {
 		return err
 	}
-	// os.Create / O_CREATE default to 0666 (umask'd). A read-only seed
-	// (e.g. a fixture 0400 marker file) would otherwise become writable
-	// in the workdir, which then looks like a permissions change to
-	// `git status` even though we never wrote to it. Mirror the source
-	// mode explicitly so a seed's executable bit / read-only bit
-	// survive the copy.
+	// Mirror the source mode so a seed's read-only / exec bit survives the copy.
 	return os.Chmod(dst, info.Mode().Perm())
 }

cmd/e2ebench/mutation.go

@@ -69,25 +69,8 @@ func runMutation(repo, base string, srcFiles []string, refs []testRef) mutationR
 			}
 			cmd := exec.Command("go", "test", "-run", runRe, pkg)
 			cmd.Dir = repo
-			// Bound the wait for the mutated-binary test run. Without
-			// WaitDelay, a mutant that compiles but wedges a test
-			// (e.g. infinite loop, blocking syscall) would block the
-			// bench forever, AND the explicit WriteFile restore below
-			// would not fire. 2 minutes matches what `go test -timeout`
-			// uses by default — long enough for a slow legitimate
-			// test, short enough that a hung binary never pins the
-			// bench. After WaitDelay, exec closes the child's I/O,
-			// unblocking the stdio-bound wait even when SIGKILL is
-			// ignored.
-			cmd.WaitDelay = 2 * time.Minute
-			// Restore the source no matter what — even when the test
-			// binary wedges, panics, or the process is killed by the
-			// test-timeout cap. The previous shape only restored on
-			// the happy path; a panic between WriteFile and the
-			// `caught := ...` line would leave the file in mutated
-			// state and break the next mutant (and the next attempt's
-			// diff-mode run via resetTree, which `git clean -fd`
-			// would then re-load from the mutated working tree).
+			cmd.WaitDelay = 2 * time.Minute // bound the wait for a mutant that wedges a test
+			// Restore source even on panic; a file left mutated would corrupt the next mutant.
 			restored := false
 			defer func() {
 				if !restored {
@@ -108,11 +91,7 @@ func runMutation(repo, base string, srcFiles []string, refs []testRef) mutationR
 }
 
 // changedFuncs returns the funcs in f whose line range overlaps a changed line.
-// main/init are skipped (no meaningful return to mutate). Function literals
-// (closures assigned to a variable) and methods declared in a type literal
-// are skipped too — they have a FuncDecl with Type.Params but no name, and
-// the mutation shape `*new(T)` doesn't compile cleanly when the result
-// type is a generic instantiation the parser can't see through.
+// main/init and nil-named decls are skipped (no meaningful return to mutate).
 func changedFuncs(fset *token.FileSet, f *ast.File, lines map[int]bool) []*ast.FuncDecl {
 	var out []*ast.FuncDecl
 	for _, decl := range f.Decls {