diff --git a/cmd/docket/main_test.go b/cmd/docket/main_test.go index bba617d9b..1195d15d9 100644 --- a/cmd/docket/main_test.go +++ b/cmd/docket/main_test.go @@ -19,6 +19,7 @@ import ( var binPath string func TestMain(m *testing.M) { + // tempdir-exempt: TestMain builds the docket binary once for the whole package; there is no t to own a fixture dir. dir, err := os.MkdirTemp("", "docket-bin-*") if err != nil { panic(err) diff --git a/cmd/releasepkg/main_test.go b/cmd/releasepkg/main_test.go index 7b4cc23fd..4f8a1c36c 100644 --- a/cmd/releasepkg/main_test.go +++ b/cmd/releasepkg/main_test.go @@ -17,6 +17,7 @@ import ( var binPath string func TestMain(m *testing.M) { + // tempdir-exempt: TestMain builds the releasepkg binary once for the whole package; there is no t to own a fixture dir. dir, err := os.MkdirTemp("", "releasepkg-bin-*") if err != nil { panic(err) diff --git a/docs/results/2026-09-27-close-the-temp-dir-fixture-guard-s-remaining-gaps-internal-c-results.md b/docs/results/2026-09-27-close-the-temp-dir-fixture-guard-s-remaining-gaps-internal-c-results.md new file mode 100644 index 000000000..56d8f27fb --- /dev/null +++ b/docs/results/2026-09-27-close-the-temp-dir-fixture-guard-s-remaining-gaps-internal-c-results.md @@ -0,0 +1,43 @@ + +> ↩ **[Change 0462 — Close the temp-dir fixture guard's remaining gaps (internal/cli gateTempDir, scan-root removal)](https://github.com/danielhanold/docket/blob/docket/docs/changes/active/0462-close-the-temp-dir-fixture-guard-s-remaining-gaps-internal-c.md)** + +# Close the temp-dir fixture guard's remaining gaps (internal/cli gateTempDir, scan-root removal) — Results + +**Human action:** No human action is required. This change only touches tests and a repository guard, and the full suite passed. Reading the PR diff at merge time is enough. + +## Outcome + +The repository guard now makes sure that tests which start real processes create their temporary directories through the shared `testsupport.TempDir` fixture. That fixture waits for leftover writers before it deletes the directory. Change 0398 left two gaps in the guard, and this change closes both. + +- **The private helper is gone.** `internal/cli/gate_test.go` used to have its own temp-dir helper built on `os.MkdirTemp`. It is deleted, and all of its call sites now use the shared fixture. +- **Unmarked `MkdirTemp` calls now fail the guard.** An `.MkdirTemp(` call in a real-process test package is only allowed when it has a `// tempdir-exempt: ` comment with a non-empty reason. The comment can sit on the same line, or alone on the line directly above. Eight existing sites have a site-specific reason: + - `TestMain` binary builds. + - Directories created once per process. + - Failure evidence that has to survive the test. + - The macOS `/tmp` alias test. +- **The guard scans the whole repository.** It used to walk a hand-written list of folders (`internal`, `cmd`). It now uses the same repo-wide file walk the other guards use. Package floors (`internal/process`, `cmd/docket`) still catch a scan that loses coverage. + +This differs from the design in one way. The spec listed `internal/app/gate_drive_test.go` runroot as a site to mark. The builder converted it to the fixture instead, because it is an ordinary per-test directory. So 8 sites are marked, not 9. + +## Verification performed + +- **Full suite:** passed at head `6a8d356dc` (54/54 files). The review fix came after that run, so the suite was run again for the final certification. The PR body's build-evidence block records the certifying run. +- **Guard mutation checks:** each check below was run through the gate driver and then reverted, and each made the guard fail as intended: + - Restoring the old helper. + - Deleting a marker. + - Giving a marker an empty reason. + - Adding an unmarked helper in another package. + - Putting the marker two lines above the call. + - Excluding `cmd` from the walk, and separately excluding `internal`. +- **Masking check:** a `MkdirTemp` spelling inside a comment or a string does not trigger the guard. +- **Review:** one minor finding came back. A trailing marker on one line also exempted a `MkdirTemp` call on the next line. It was fixed in `96c2e96a9`, with a new test case that failed before the fix. + +## Known issues and follow-ups + +### Shared walker included `.docket/` when run from the main checkout — resolved in this change + +Running a guard from the primary checkout instead of a feature worktree made the whole-repo walk include the `.docket/` metadata worktree and the local harness installs (`.claude/`, `.codex/`, `.cursor/`, `.agents/`, `.superpowers/`). At the human's request after review, this change now resolves that inside its own scope. `repoguard.MaintainedFiles` prunes every hidden directory at any depth except `.github/`, which is the only tracked hidden directory and stays in the scan. That rule also covers the old `.git`/`.worktrees` exclusions. `TestMaintainedFilesIncludesAndExcludes` pins both directions with `.docket/…`, `.claude/…`, a nested `internal/app/.cache/…`, and the existing `.github/workflows/ci.yml` inclusion. It was mutation-tested: without the `.github` carve-out the test turns red, and before the rule was added the new exclusion cases were red. + +### Suite budget watch lines + +The green run printed `BUDGET WATCH` lines (streak 1/5) for long parallel integration suites such as `test_go_race` and `test_go_toolchain`. These are screening signals that depend on the machine. None of them is a serially confirmed budget breach, and this change did not touch those suites. diff --git a/docs/superpowers/plans/2026-09-27-close-the-temp-dir-fixture-guard-s-remaining-gaps-internal-c.md b/docs/superpowers/plans/2026-09-27-close-the-temp-dir-fixture-guard-s-remaining-gaps-internal-c.md new file mode 100644 index 000000000..8d0e232da --- /dev/null +++ b/docs/superpowers/plans/2026-09-27-close-the-temp-dir-fixture-guard-s-remaining-gaps-internal-c.md @@ -0,0 +1,488 @@ + +> ↩ **[Change 0462 — Close the temp-dir fixture guard's remaining gaps (internal/cli gateTempDir, scan-root removal)](https://github.com/danielhanold/docket/blob/docket/docs/changes/active/0462-close-the-temp-dir-fixture-guard-s-remaining-gaps-internal-c.md)** + +# Close the temp-dir fixture guard's remaining gaps — Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: execute with the build role this repo resolves +> (`docket-build`), task by task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Close change 0462's two residual gaps in `TestRealProcessPackagesUseFixtureTempDir`. The guard +does not see `.MkdirTemp(` calls, so `internal/cli`'s private `gateTempDir` gets past it, and it +walks a hand-listed `scanRoots`. + +**Architecture:** Three self-contained edits, each green on its own: +(1) delete `internal/cli`'s `gateTempDir` helper and use `testsupport.TempDir(t)` at every call site; +(2) add a second violation shape to the guard, `.MkdirTemp(` on the comment- and string-masked view. +A call is exempt only when a `//` comment on the same line or the line directly above says +`tempdir-exempt: ` with a non-empty reason. Then mark the legitimate sites the new guard reports; +(3) replace `scanRoots` with the shared `MaintainedFiles` whole-repo walker and keep `realProcFloors` as +the non-vacuity floor. + +**Tech Stack:** Go (`go/scanner`, `regexp`, `testing`); the repo's `internal/repoguard` and +`internal/testsupport` packages. + +**Spec:** `docs/superpowers/specs/2026-09-27-close-the-temp-dir-fixture-guard-s-remaining-gaps-internal-c-design.md` +(on the `docket` metadata branch). Read it alongside this plan. + +## Global Constraints + +- The fixture package `internal/testsupport` stays exempt by construction. Do not change `testsupport.TempDir`'s behavior and do not add fixture modes (spec, Out of scope). +- The MkdirTemp regex keys on the receiver-call **shape** (`\b([A-Za-z_][A-Za-z0-9_]*)\.MkdirTemp\(`), never on the `os` spelling. It runs over `maskProse(b, true)`. +- The marker is `// tempdir-exempt: `, read from the **raw** bytes, on the call's own line or the line immediately above. An empty or whitespace-only reason does **not** exempt. +- **Re-derive the exempt-site list by the guard's own red run plus a whole-repo grep during the build.** Never copy the spec's table blindly. Each marker's reason is a short sentence about that specific site, not a generic class label. +- Delete `scanRoots` outright. The population comes from `repoguard.MaintainedFiles(root)` filtered to `*_test.go`. Keep `realProcFloors = {"internal/process", "cmd/docket"}`. +- Out of scope: `os.CreateTemp`, calls through interface or function values, name-shadowing helpers, packages that spawn no real process, and the `cmd/` sites 0398 already converted. +- Cross-references in maintained source anchor on symbol names or quoted clauses, never on line numbers (ADR-0054). +- Mutation probes run with `go test -count=1` (a cached PASS against a mutated tree proves nothing). Commit before mutating, restore with `git checkout -- ` (HEAD then holds the work), and confirm `git status --porcelain` is empty after each restore. +- Full-suite gate: `build.test_command` resolved from `.docket.yml` (currently `go run ./cmd/docket development test`), run from this worktree. + +## Review Focus + +1. **A trailing same-line marker** (`d, err := os.MkdirTemp("", "x") // tempdir-exempt: …`) should exempt that call. Pinned by the `same-line marker` case in Task 2's `TestMkdirTempViolations`. +2. **Marker text that is not a `//` comment** should never exempt. That covers marker text inside a string literal on the line above and a `/* tempdir-exempt: x */` block. A careless raw-byte grep would accept both. Pinned by the `marker in string` and `block comment marker` cases in Task 2. +3. **Two consecutive `MkdirTemp` calls under one marker.** Only the first call is adjacent, so the second must be a violation. Pinned by the `consecutive calls` case in Task 2. +4. **Build-tagged test files** such as `internal/app/finalize_e2e_test.go` (`//go:build e2e`) must still be scanned. The walk is by file, not by build, so their exempt sites need markers too. Pinned by Task 2 Step 5: the red run on the real tree must list the `finalize_e2e_test.go` sites, and Step 6 marks them. +5. **The converted `internal/cli` gate tests must still tear down cleanly** during the supervisor's exit window. That race is the one `gateTempDir`'s 40×50ms retry existed for, and the fixture's drain-then-retry removal must cover it. Pinned by Task 1 Step 4, which runs the package with `-count=2` and expects no `directory not empty`. + +Note, not a task: when the suite runs from the **primary** checkout, `MaintainedFiles` also walks `.docket/` +(the metadata worktree), and its `docs/codex/fixtures/**/*_test.go` files are in-population. None of them +contains `exec.Command` today, so none joins the real-process set. The shared exclusion rules are out of +scope. Leave this alone, and mention it in the results file only if it ever reddens. + +--- + +### Task 1: Replace `internal/cli`'s `gateTempDir` with the fixture + +**Files:** +- Modify: `internal/cli/gate_test.go`. Delete the `gateTempDir` doc comment and function, which sit directly after `TestMain`. Rewrite every `gateTempDir(t)` occurrence (22 lines at groom time). + +**Interfaces:** +- Consumes: `testsupport.TempDir(t testing.TB) string` (already imported in this file). +- Produces: nothing new. After this task, `internal/cli/gate_test.go` contains no `MkdirTemp` call, which Task 2's guard relies on. + +- [ ] **Step 1: Confirm the starting shape** + +Run: `grep -n 'gateTempDir\|MkdirTemp' internal/cli/gate_test.go` +Expected: one `func gateTempDir` definition, one `os.MkdirTemp("", "docket-gate-*")`, and the call sites. + +- [ ] **Step 2: Delete the helper** + +Remove this whole block, from its doc comment through the closing brace: + +```go +// gateTempDir is a temp dir whose cleanup tolerates the external supervisor's +// brief exit window. Observe reports "passed" the instant the terminal record +// lands, which can precede the supervisor's final same-directory atomic write +// and lock release, so a single-shot RemoveAll (as t.TempDir does) races it and +// fails "directory not empty". The retry loop lets the supervisor finish. +func gateTempDir(t *testing.T) string { + t.Helper() + dir, err := os.MkdirTemp("", "docket-gate-*") + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { + for i := 0; i < 40; i++ { + if err := os.RemoveAll(dir); err == nil { + return + } + time.Sleep(50 * time.Millisecond) + } + _ = os.RemoveAll(dir) + }) + return dir +} +``` + +- [ ] **Step 3: Convert every call site and verify imports** + +Run: `sed -i '' 's/gateTempDir(t)/testsupport.TempDir(t)/g' internal/cli/gate_test.go` +Then: `grep -c 'gateTempDir' internal/cli/gate_test.go`. Expect `0`, which exits 1, so capture it rather than piping. +Keep the `os` import (`TestMain` uses `os.Exit`) and the `time` import (the observe poll loop still calls +`time.Sleep`). Run `go vet ./internal/cli/` and expect no unused-import error. If an import did become +unused, drop it. + +- [ ] **Step 4: Run the package and confirm clean teardown (Review Focus 5)** + +Run: `go test -count=2 ./internal/cli/` +Expected: PASS, and no `directory not empty` or `testsupport:` surviving-path failure in the output. + +- [ ] **Step 5: Commit** + +```bash +git add internal/cli/gate_test.go +git commit -m "test(cli): gate tests use testsupport.TempDir; delete private gateTempDir (change 0462)" +``` + +--- + +### Task 2: Ban unmarked `MkdirTemp` in real-process packages; mark the legitimate sites + +**Files:** +- Modify: `internal/repoguard/tempdir_fixture_test.go`: the header LIMITATION note, new regexes, the new helpers `exemptMarkerLines` and `mkdirTempViolations`, the new unit test `TestMkdirTempViolations`, and the wiring in `TestRealProcessPackagesUseFixtureTempDir`. +- Modify: each legitimate `MkdirTemp` site reported in Step 5 (a one-line `//` marker above the call). The groom-time candidates, which you must re-derive rather than trust, are `cmd/docket/main_test.go`, `cmd/releasepkg/main_test.go`, `internal/app/finalize_e2e_test.go` (two sites), `internal/app/status_git_test.go`, `internal/app/gate_drive_test.go`, `internal/gatedrive/driver_test.go`, `internal/release/package_integration_test.go`, and `internal/install/references_test.go`. + +**Interfaces:** +- Consumes: the existing `maskProse(src []byte, maskStrings bool) []byte` in the same file. +- Produces (used by Task 4's rewrite of the walk; keep the names exact): + - `var mkdirTempCallRe = regexp.MustCompile(`\b([A-Za-z_][A-Za-z0-9_]*)\.MkdirTemp\(`)` + - `var tempdirExemptRe = regexp.MustCompile(`^//[ \t]*tempdir-exempt:[ \t]*\S`)` + - `func exemptMarkerLines(src []byte) map[int]bool`: the 1-based lines of justified `//` marker comments. + - `func mkdirTempViolations(src []byte) (violations, exempt []int)`: the 1-based lines of unmarked and marked executable calls. + +- [ ] **Step 1: Write the failing unit test** + +Append to `internal/repoguard/tempdir_fixture_test.go`. Every fixture is a Go string literal. This package +is itself a real-process package, and its own guard masks string literals, so these fixtures never count +as violations or markers in this file. + +```go +func TestMkdirTempViolations(t *testing.T) { + cases := []struct { + name string + src string + wantViolation []int + wantExempt []int + }{ + {"bare call", "package p\nfunc f() {\n\td, _ := os.MkdirTemp(\"\", \"x\")\n\t_ = d\n}\n", []int{3}, nil}, + {"marker above", "package p\nfunc f() {\n\t// tempdir-exempt: built once in TestMain, no t\n\td, _ := os.MkdirTemp(\"\", \"x\")\n\t_ = d\n}\n", nil, []int{4}}, + {"same-line marker", "package p\nfunc f() {\n\td, _ := os.MkdirTemp(\"\", \"x\") // tempdir-exempt: process-lifetime dir\n\t_ = d\n}\n", nil, []int{3}}, + {"marker two lines above", "package p\nfunc f() {\n\t// tempdir-exempt: too far away\n\n\td, _ := os.MkdirTemp(\"\", \"x\")\n\t_ = d\n}\n", []int{5}, nil}, + {"empty reason", "package p\nfunc f() {\n\t// tempdir-exempt:\n\td, _ := os.MkdirTemp(\"\", \"x\")\n\t_ = d\n}\n", []int{4}, nil}, + {"whitespace reason", "package p\nfunc f() {\n\t// tempdir-exempt: \t\n\td, _ := os.MkdirTemp(\"\", \"x\")\n\t_ = d\n}\n", []int{4}, nil}, + {"spelling in comment and string", "package p\n// os.MkdirTemp(\"\", \"x\")\nvar s = \"os.MkdirTemp(\"\n", nil, nil}, + {"marker in string", "package p\nfunc f() {\n\t_ = \"// tempdir-exempt: not a comment\"\n\td, _ := os.MkdirTemp(\"\", \"x\")\n\t_ = d\n}\n", []int{4}, nil}, + {"block comment marker", "package p\nfunc f() {\n\t/* tempdir-exempt: block comments do not count */\n\td, _ := os.MkdirTemp(\"\", \"x\")\n\t_ = d\n}\n", []int{4}, nil}, + {"consecutive calls", "package p\nfunc f() {\n\t// tempdir-exempt: first only\n\ta, _ := os.MkdirTemp(\"\", \"a\")\n\tb, _ := os.MkdirTemp(\"\", \"b\")\n\t_, _ = a, b\n}\n", []int{5}, []int{4}}, + {"non-os receiver", "package p\nfunc f() {\n\td, _ := afs.MkdirTemp(\"\", \"x\")\n\t_ = d\n}\n", []int{3}, nil}, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + gotV, gotE := mkdirTempViolations([]byte(c.src)) + if !slices.Equal(gotV, c.wantViolation) || !slices.Equal(gotE, c.wantExempt) { + t.Fatalf("violations=%v exempt=%v, want violations=%v exempt=%v", gotV, gotE, c.wantViolation, c.wantExempt) + } + }) + } +} +``` + +- [ ] **Step 2: Run it and watch it fail** + +Run: `go test -count=1 -run TestMkdirTempViolations ./internal/repoguard/` +Expected: a build failure, `undefined: mkdirTempViolations`. + +- [ ] **Step 3: Implement the helpers** + +Add `"bytes"` to the import block. Add the following after `tempDirCallRe`/`testsupportAliasRe` and after +`maskProse`, respectively: + +```go +var mkdirTempCallRe = regexp.MustCompile(`\b([A-Za-z_][A-Za-z0-9_]*)\.MkdirTemp\(`) + +// tempdirExemptRe matches the text of a justified exemption marker: a line +// comment reading "tempdir-exempt:" followed by a non-blank reason. +var tempdirExemptRe = regexp.MustCompile(`^//[ \t]*tempdir-exempt:[ \t]*\S`) +``` + +```go +// exemptMarkerLines returns the 1-based lines that carry a justified +// tempdir-exempt marker. It lexes the RAW source with go/scanner (the masked +// view blanks comments), so only a real // comment token counts: marker text +// inside a string literal or a /* */ block never exempts. +func exemptMarkerLines(src []byte) map[int]bool { + lines := map[int]bool{} + fset := token.NewFileSet() + f := fset.AddFile("", fset.Base(), len(src)) + var s scanner.Scanner + s.Init(f, src, nil, scanner.ScanComments) + for { + pos, tok, lit := s.Scan() + if tok == token.EOF { + break + } + if tok == token.COMMENT && tempdirExemptRe.MatchString(lit) { + lines[f.Line(pos)] = true + } + } + return lines +} + +// mkdirTempViolations finds every executable .MkdirTemp( call in src +// (comments and string/char literals masked) and splits them by line into +// violations and exempt calls. A call is exempt only when its own line or the +// line immediately above carries a justified marker (exemptMarkerLines). +func mkdirTempViolations(src []byte) (violations, exempt []int) { + markers := exemptMarkerLines(src) + callView := maskProse(src, true) + for _, loc := range mkdirTempCallRe.FindAllIndex(callView, -1) { + // maskProse blanks in place and preserves newlines, so offsets in the + // masked view map to the same line numbers as the raw source. + line := 1 + bytes.Count(callView[:loc[0]], []byte("\n")) + if markers[line] || markers[line-1] { + exempt = append(exempt, line) + } else { + violations = append(violations, line) + } + } + return violations, exempt +} +``` + +- [ ] **Step 4: Run the unit test and watch it pass** + +Run: `go test -count=1 -run TestMkdirTempViolations ./internal/repoguard/` +Expected: PASS, all 11 subtests. + +- [ ] **Step 5: Wire the check into the guard and let the red run derive the site list** + +In `TestRealProcessPackagesUseFixtureTempDir`, inside the per-file loop, directly after the +`tempDirCallRe` loop, add: + +```go + // Change 0462: an executable MkdirTemp call is a hand-rolled temp dir + // that skips the fixture's drain-then-retry cleanup, unless the site + // needs a lifetime the per-test fixture cannot provide and says so. + bad, ok := mkdirTempViolations(b) + for _, line := range bad { + violations = append(violations, fmt.Sprintf("%s:%d: MkdirTemp call without a justified tempdir-exempt marker — use testsupport.TempDir(t); only a dir the per-test fixture cannot serve (no t in TestMain, process lifetime under sync.Once, failure evidence that must survive, a mandated parent) may carry an adjacent \"// tempdir-exempt: \"", f, line)) + } + for _, line := range ok { + t.Logf("exempt MkdirTemp: %s:%d", f, line) + } +``` + +Run: `go test -count=1 -run TestRealProcessPackagesUseFixtureTempDir ./internal/repoguard/` +Expected: FAIL, with one `MkdirTemp call without a justified tempdir-exempt marker` line for each current +site. **This output is the authoritative site list.** Cross-check it against a whole-repo grep (no pipe +into an early-exiting consumer): + +```bash +grep -rnE --include='*_test.go' --exclude-dir=.git --exclude-dir=.worktrees --exclude-dir=testdata -e '\b[A-Za-z_][A-Za-z0-9_]*\.MkdirTemp\(' . +``` + +Every grep hit in a real-process package, other than `internal/testsupport`, must show up in the guard's +list. A hit the guard misses is a guard defect: stop and investigate. It must not become a residual. Any +hit in `internal/cli/gate_test.go` means Task 1 is incomplete. Confirm that +`internal/app/finalize_e2e_test.go` (build tag `e2e`) is in the list (Review Focus 4). + +- [ ] **Step 6: Mark or convert each reported site** + +For each site in the Step 5 list, first decide whether it is a per-test dir deleted at test end. If so, +convert it to `testsupport.TempDir(t)` instead of marking it. Otherwise add a `//` marker line directly +above the call, indented to match. If a comment block already sits above the call, the marker becomes that +block's last line. Suggested reasons for the groom-time candidates follow. Adapt each one to what the code +actually does there: + +| Site (function) | Marker | +|---|---| +| `cmd/docket/main_test.go` `TestMain` | `// tempdir-exempt: TestMain builds the docket binary once for the whole package; there is no t to own a fixture dir.` | +| `cmd/releasepkg/main_test.go` `TestMain` | `// tempdir-exempt: TestMain builds the releasepkg binary once for the whole package; there is no t to own a fixture dir.` | +| `internal/app/finalize_e2e_test.go` `e2eNode` | `// tempdir-exempt: shared XDG config dir created once under e2eXDGOnce and reused by every e2e test in the process.` | +| `internal/app/finalize_e2e_test.go` `sharedBinaries` | `// tempdir-exempt: docket and gh binaries built once under sharedBinOnce and shared for the process lifetime.` | +| `internal/app/status_git_test.go` `backgroundOffGitEnv` | `// tempdir-exempt: background-off gitconfig written once under bgOffGitOnce and shared for the process lifetime.` | +| `internal/gatedrive/driver_test.go` `sampleWorktree` | `// tempdir-exempt: sample worktree built once under sampleWorktreeOnce and shared read-only across tests.` | +| `internal/release/package_integration_test.go` determinism mismatch | `// tempdir-exempt: failure evidence — the mismatched bundles must survive the test so the failure output can name them.` | +| `internal/install/references_test.go` `TestDeriveVersionReferencesCanonicalizesTmpAliases` | `// tempdir-exempt: must live under the literal /tmp to exercise the macOS /tmp -> /private/tmp alias.` | +| `internal/app/gate_drive_test.go` `runRootFixture` | `// tempdir-exempt: nested inside a testsupport.TempDir(t) parent, whose fixture cleanup removes it.` | + +No marker line may begin a comment with `tempdir-exempt:` unless it sits adjacent to the call it justifies. + +Run: `go test -count=1 -run 'TestRealProcessPackagesUseFixtureTempDir|TestMkdirTempViolations' -v ./internal/repoguard/` +Expected: PASS, with one `exempt MkdirTemp:` log line per marked site. + +- [ ] **Step 7: Update the header LIMITATION note** + +Replace the existing `// LIMITATION (asserted, per the byte-pattern-guard learning): …` paragraph in the +file header with: + +```go +// LIMITATION (asserted, per the byte-pattern-guard learning): the ban +// matches the receiver-call shapes `.TempDir(` and, since change +// 0462, `.MkdirTemp(`. It cannot see a call through an interface +// value, a function value, or a helper that shadows the name, and +// os.CreateTemp (temp files) is out of scope. The aliased-import check below +// closes the one cheap evasion (import testsupport under another name and +// the receiver test goes vacuous). +// +// MkdirTemp EXEMPTIONS: a few real-process sites need a lifetime the +// per-test fixture deliberately does not provide — a binary built in +// TestMain (no t), a process-lifetime dir created under sync.Once, failure +// evidence that must survive, a mandated /tmp parent. Such a call is exempt +// only when a line comment on its own line or the line immediately above +// carries the tempdir-exempt marker with a non-empty reason (see +// tempdirExemptRe). The marker is lexed from the RAW source, so marker text +// inside a string literal or a block comment never exempts. +``` + +Check that no line of this header begins its comment text with `tempdir-exempt:`, because such a line +would itself be a marker. + +- [ ] **Step 8: Run the package and commit** + +Run: `go test -count=1 ./internal/repoguard/` +Expected: PASS. + +```bash +git add internal/repoguard/tempdir_fixture_test.go +git commit -m "test(repoguard): ban unmarked MkdirTemp in real-process packages; mark justified sites (change 0462)" +``` + +Stage the files by explicit path. Never use `git add -A`. + +- [ ] **Step 9: Mutation-verify the ban (spec mutations 1–5 and 7)** + +Every probe runs `go test -count=1 -run TestRealProcessPackagesUseFixtureTempDir ./internal/repoguard/`. +Restore each with `git checkout -- ` (or `rm` for a new file) and confirm `git status --porcelain` +is empty before the next probe. + +1. Restore the `gateTempDir` helper from Step 2 of Task 1 into `internal/cli/gate_test.go`. Expected: **FAIL**, naming `internal/cli/gate_test.go` and `MkdirTemp call without a justified tempdir-exempt marker`. +2. Delete one site's marker line, for example in `cmd/docket/main_test.go`. Expected: **FAIL**, naming that file and line. +3. Change one marker to `// tempdir-exempt:` with no reason. Expected: **FAIL**, naming that site. +4. Create `internal/process/zz_mutation_test.go` with `package process`, `import ("os"; "testing")`, and `func zzHelper(t *testing.T) string { d, _ := os.MkdirTemp("", "zz-*"); return d }`. Use the package clause the directory's other test files use. Expected: **FAIL**, naming the new file. Then `rm` it. +5. Insert one unrelated line (for example `_ = 0`) between a marker and its call. Expected: **FAIL**, because adjacency is enforced. +7. In any real-process test file, add `// os.MkdirTemp("", "x")` and `var _ = "os.MkdirTemp("` at package level. Expected: **PASS**, because masking hides both. + +Record each probe's observed result for the results file. A probe that fails to redden is a finding about +the guard. Investigate it, and do not write it down as a residual. + +--- + +### Task 3: Derive the scan population from the whole repo + +**Files:** +- Modify: `internal/repoguard/tempdir_fixture_test.go`: delete `scanRoots`, rewrite the walk in `TestRealProcessPackagesUseFixtureTempDir`, and rewrite the header SCOPE note and the `realProcFloors` doc. + +**Interfaces:** +- Consumes: `MaintainedFiles(root string) ([]string, error)` (`internal/repoguard/repoguard.go`), which returns root-relative, slash-separated, sorted paths with the categorical exclusions already pruned. Also consumes Task 2's `mkdirTempViolations`. +- Produces: no new symbols. Violation messages now print module-relative slash paths. + +- [ ] **Step 1: Rewrite the walk** + +Delete the `scanRoots` doc comment and `var scanRoots = …`. Replace the start of +`TestRealProcessPackagesUseFixtureTempDir`, from the `pkgs := …` line through the floor loop, with: + +```go + files, err := MaintainedFiles(root) + if err != nil { + t.Fatal(err) + } + // Module-relative slash dir -> its module-relative slash _test.go files. + pkgs := map[string][]string{} + for _, f := range files { + if strings.HasSuffix(f, "_test.go") { + dir := path.Dir(f) + pkgs[dir] = append(pkgs[dir], f) + } + } + const fixtureDir = "internal/testsupport" + read := func(rel string) []byte { + b, err := os.ReadFile(filepath.Join(root, filepath.FromSlash(rel))) + if err != nil { + t.Fatal(err) + } + return b + } + var realProc []string + for dir, fs := range pkgs { + if dir == fixtureDir { + continue // the fixture itself is exempt by construction + } + for _, f := range fs { + if execCallRe.Match(read(f)) { + realProc = append(realProc, dir) + break + } + } + } + sort.Strings(realProc) + // Population floors (marker-scoped guards need one): the whole-repo + // derivation must find each floor package — internal/process, whose + // supervisor tests motivated the fixture, and cmd/docket, whose built-binary + // gate tests spawn the real supervisor. A missing floor means the + // exec.Command shape rotted or a shared MaintainedFiles exclusion swallowed + // a test root, and the guard would otherwise pass vacuously over it. + for _, floor := range realProcFloors { + if !slices.Contains(realProc, floor) { + t.Fatalf("derivation lost %s — real-process set: %v", floor, realProc) + } + } +``` + +In the violations loop, replace the `os.ReadFile(f)` block with `b := read(f)`. Keep the alias, TempDir, +and MkdirTemp checks unchanged. Update the imports: add `"path"`, and drop `"io/fs"` because nothing uses +it now. Note that the loop variable `fs` shadows nothing once `io/fs` is gone. If you prefer, name it +`pkgFiles`. Run `go vet ./internal/repoguard/`. + +- [ ] **Step 2: Rewrite the SCOPE note and the `realProcFloors` doc** + +Replace the header's `// SCOPE (explicit — see scanRoots and realProcFloors below): …` paragraph with: + +```go +// SCOPE (see realProcFloors below): since change 0462 the scan population is +// derived from the WHOLE repository through the shared MaintainedFiles +// walker, filtered to _test.go files — there is no guard-local list of roots +// left to shrink. Narrowing coverage now means editing the categorical +// exclusions in repoguard.go that every repoguard guard depends on. +// realProcFloors keeps a population floor in each known test root +// (internal/ from change 0373, cmd/ from change 0398), so a rotted +// exec.Command shape or an exclusion that swallows a root fails loudly +// instead of passing vacuously. +``` + +Replace the `realProcFloors` doc comment with: + +```go +// realProcFloors are module-relative (slash-separated) packages the +// whole-repo derivation must always find. An empty or rotted derivation, or +// a shared exclusion that swallows internal/ or cmd/, then fails loudly +// instead of passing vacuously (marker-scoped guards need a population +// floor). +``` + +- [ ] **Step 3: Run the package** + +Run: `go test -count=1 -v -run 'TestRealProcessPackagesUseFixtureTempDir|TestMkdirTempViolations' ./internal/repoguard/` +Expected: PASS. The `exempt MkdirTemp:` log lines now print module-relative paths, and there are exactly as +many as in Task 2 Step 6. A changed count means the population changed: investigate it. +Then: `go test -count=1 ./internal/repoguard/`. Expected: PASS. + +- [ ] **Step 4: Confirm there is no guard-local root list left** + +Run: `out=$(grep -rn 'scanRoots' --include='*.go' . || true); echo "[$out]"` +Expected: `[]`. + +- [ ] **Step 5: Commit** + +```bash +git add internal/repoguard/tempdir_fixture_test.go +git commit -m "test(repoguard): derive the temp-dir guard's population from the whole repo (change 0462)" +``` + +- [ ] **Step 6: Mutation-verify coverage (spec mutation 6)** + +In `internal/repoguard/repoguard.go` `isExcludedDir`, temporarily add `"cmd"` to the exact-location +`case` list (`case "docs", "tests/fixtures", "internal/install/legacydata", "cmd":`). +Run: `go test -count=1 -run TestRealProcessPackagesUseFixtureTempDir ./internal/repoguard/` +Expected: **FAIL** with `derivation lost cmd/docket`. Other repoguard tests may redden too, which is +expected. Restore it with `git checkout -- internal/repoguard/repoguard.go` and confirm +`git status --porcelain` is empty. Repeat the probe with `"internal"` and expect +`derivation lost internal/process`. Restore again. + +--- + +### Task 4: Full-suite gate + +No code changes. `docket-build` runs this gate once at the end. + +- [ ] **Step 1:** Resolve `build.test_command` from `.docket.yml` and run it from this worktree (currently `go run ./cmd/docket development test`). +Expected: the `SUITE …` summary is green. Read the budget report even when the run is green. Act on any `SERIAL CONFIRMED OVER BUDGET:` line, and record any `BUDGET WATCH:` or `PARALLEL-SENSITIVE:` lines. +- [ ] **Step 2:** Confirm `git status --porcelain` is empty and that `git log` shows the three task commits on `chore/close-the-temp-dir-fixture-guard-s-remaining-gaps-internal-c`. + +--- + +## Self-review (plan author) + +- Spec coverage: §1 is covered by Task 1. §2 is covered by Task 2: the shape regex, the masked view, the raw-lexed marker, the non-empty reason, the fixture exemption carried over from the existing `fixtureDir` skip, the message, the LIMITATION note, and the re-derived site list. §3 is covered by Task 3: `scanRoots` deleted, `MaintainedFiles`, floors kept, SCOPE and floor docs rewritten. Verification mutations 1–5 and 7 are in Task 2 Step 9, mutation 6 is in Task 3 Step 6, and the full suite is Task 4. +- Names are consistent across tasks: `mkdirTempCallRe`, `tempdirExemptRe`, `exemptMarkerLines`, `mkdirTempViolations`, `realProcFloors`, `maskProse`, `MaintainedFiles`. +- Every task leaves the tree green. Task 1 removes the only unmarked call the Task 2 guard would otherwise flag in `internal/cli`, and Task 2 adds markers in the same commit as the ban. diff --git a/internal/app/finalize_e2e_test.go b/internal/app/finalize_e2e_test.go index 5d3dc2e52..e80a2f4f1 100644 --- a/internal/app/finalize_e2e_test.go +++ b/internal/app/finalize_e2e_test.go @@ -89,6 +89,7 @@ var ( func e2eNode(t *testing.T, dir string) realNode { t.Helper() e2eXDGOnce.Do(func() { + // tempdir-exempt: shared XDG config dir created once under e2eXDGOnce and reused by every e2e test in the process. d, err := os.MkdirTemp("", "docket-e2e-xdg-*") if err != nil { t.Fatalf("isolate global config: %v", err) @@ -210,6 +211,7 @@ var ( func sharedBinaries(t *testing.T) (docketBin, ghBin string) { t.Helper() sharedBinOnce.Do(func() { + // tempdir-exempt: docket and gh binaries built once under sharedBinOnce and shared for the process lifetime. dir, err := os.MkdirTemp("", "docket-e2e-bin-*") if err != nil { sharedBinErr = err diff --git a/internal/app/gate_drive_test.go b/internal/app/gate_drive_test.go index c844f09cb..a2a26fcc5 100644 --- a/internal/app/gate_drive_test.go +++ b/internal/app/gate_drive_test.go @@ -339,11 +339,7 @@ func TestOwnerConstructorUnresolvedCommandNamesRemedy(t *testing.T) { // run root, so a test can assert whether mapDriveOutcome removed it. func runRootFixture(t *testing.T) string { t.Helper() - d, err := os.MkdirTemp(testsupport.TempDir(t), "runroot-*") - if err != nil { - t.Fatalf("mktemp: %v", err) - } - return d + return testsupport.TempDir(t) } func dirExists(t *testing.T, p string) bool { diff --git a/internal/app/status_git_test.go b/internal/app/status_git_test.go index 49d4d03e2..62d5b6d1a 100644 --- a/internal/app/status_git_test.go +++ b/internal/app/status_git_test.go @@ -74,6 +74,7 @@ var ( // drain-then-retry removal. func backgroundOffGitEnv() string { bgOffGitOnce.Do(func() { + // tempdir-exempt: background-off gitconfig written once under bgOffGitOnce and shared for the process lifetime. dir, err := os.MkdirTemp("", "docket-bgoff-git-*") if err != nil { bgOffGitErr = err diff --git a/internal/cli/gate_test.go b/internal/cli/gate_test.go index 47dc9360f..be293c218 100644 --- a/internal/cli/gate_test.go +++ b/internal/cli/gate_test.go @@ -29,29 +29,6 @@ func TestMain(m *testing.M) { os.Exit(m.Run()) } -// gateTempDir is a temp dir whose cleanup tolerates the external supervisor's -// brief exit window. Observe reports "passed" the instant the terminal record -// lands, which can precede the supervisor's final same-directory atomic write -// and lock release, so a single-shot RemoveAll (as t.TempDir does) races it and -// fails "directory not empty". The retry loop lets the supervisor finish. -func gateTempDir(t *testing.T) string { - t.Helper() - dir, err := os.MkdirTemp("", "docket-gate-*") - if err != nil { - t.Fatal(err) - } - t.Cleanup(func() { - for i := 0; i < 40; i++ { - if err := os.RemoveAll(dir); err == nil { - return - } - time.Sleep(50 * time.Millisecond) - } - _ = os.RemoveAll(dir) - }) - return dir -} - // decodeOneJSON proves stdout is exactly one newline-terminated JSON document // and returns it decoded, mirroring the cmd/docket harness. func decodeOneJSON(t *testing.T, stdout string) map[string]any { @@ -96,7 +73,7 @@ func TestGateGroupMissingCommand(t *testing.T) { // TestGateLaunchRequiresDashBoundary: the `--` argv boundary is mandatory, and // no positional words may precede it. func TestGateLaunchRequiresDashBoundary(t *testing.T) { - root := gateTempDir(t) + root := testsupport.TempDir(t) // No `--` at all: invalid input naming the requirement. out, errS, code := runCLI(t, "gate", "launch", "--root", root, "--cwd", root) if code != 2 || out != "" { @@ -147,8 +124,8 @@ func pollObserveJSON(t *testing.T, runDir string) map[string]any { // TestGateLaunchJSONOneDocument drives a real supervised /bin/echo through the // CLI and proves the launch + observe protocol documents. func TestGateLaunchJSONOneDocument(t *testing.T) { - root := gateTempDir(t) - cwd := gateTempDir(t) + root := testsupport.TempDir(t) + cwd := testsupport.TempDir(t) out, errS, code := runCLI(t, "--json", "gate", "launch", "--root", root, "--cwd", cwd, "--", "/bin/echo", "hi") if code != 0 || errS != "" { t.Fatalf("launch: out=%q err=%q code=%d", out, errS, code) @@ -176,8 +153,8 @@ func TestGateLaunchJSONOneDocument(t *testing.T) { // TestGateStopAndRecoverWiring proves stop and recover reach the app layer and // carry their protocol documents. func TestGateStopAndRecoverWiring(t *testing.T) { - root := gateTempDir(t) - cwd := gateTempDir(t) + root := testsupport.TempDir(t) + cwd := testsupport.TempDir(t) out, errS, code := runCLI(t, "--json", "gate", "launch", "--root", root, "--cwd", cwd, "--", "/bin/echo", "hi") if code != 0 || errS != "" { t.Fatalf("launch: out=%q err=%q code=%d", out, errS, code) @@ -222,7 +199,7 @@ func TestGateStopAndRecoverWiring(t *testing.T) { // recover --root : the nil-collection convention marshals an empty // scan as "recovery":[], never an absent field. - out, errS, code = runCLI(t, "--json", "gate", "recover", "--root", gateTempDir(t)) + out, errS, code = runCLI(t, "--json", "gate", "recover", "--root", testsupport.TempDir(t)) if code != 0 || errS != "" { t.Fatalf("recover empty: out=%q err=%q code=%d", out, errS, code) } @@ -251,7 +228,7 @@ func gitCmd(t *testing.T, dir string, args ...string) { // non-bare repository with a resolvable HEAD. func gateDriveRepo(t *testing.T) string { t.Helper() - dir := gateTempDir(t) + dir := testsupport.TempDir(t) gitCmd(t, dir, "init", "-q", "-b", "main") gitCmd(t, dir, "config", "user.email", "t@t") gitCmd(t, dir, "config", "user.name", "t") @@ -290,7 +267,7 @@ func gateDriveConfiguredRepo(t *testing.T, configBody string) string { } t.Setenv("XDG_CONFIG_HOME", testsupport.TempDir(t)) - root := gateTempDir(t) + root := testsupport.TempDir(t) origin := filepath.Join(root, "origin.git") writer := filepath.Join(root, "writer") invocation := filepath.Join(root, "invocation") @@ -329,7 +306,7 @@ func gateDriveConfiguredRepo(t *testing.T, configBody string) string { // exit 0. func TestGateDriveStartRunsToPassed(t *testing.T) { wt := gateDriveConfiguredRepo(t, "metadata_branch: main\nbuild:\n gate: local\n test_command: /bin/echo hi\n") - root := gateTempDir(t) + root := testsupport.TempDir(t) out, errS, code := runCLI(t, "--json", "gate", "drive", "start", "--repo-dir", wt, "--run-root", root, "--owner", "build") if code != 0 || errS != "" { t.Fatalf("start: out=%q err=%q code=%d", out, errS, code) @@ -361,7 +338,7 @@ func TestGateDriveStartOwnerRoutesToOwnCommand(t *testing.T) { "build:\n gate: local\n test_command: touch " + buildMarker + "\n" + "finalize:\n test_command: touch " + finalizeMarker + "\n" wt := gateDriveConfiguredRepo(t, cfg) - root := gateTempDir(t) + root := testsupport.TempDir(t) out, errS, code := runCLI(t, "--json", "gate", "drive", "start", "--repo-dir", wt, "--run-root", root, "--owner", "build") if errS != "" { @@ -385,7 +362,7 @@ func TestGateDriveStartOwnerRoutesToOwnCommand(t *testing.T) { // success (exit 0). The process exit MUST derive from the typed outcome instead. func TestGateDriveStartFailedIsNonZeroExit(t *testing.T) { wt := gateDriveConfiguredRepo(t, "metadata_branch: main\nbuild:\n gate: local\n test_command: /usr/bin/false\n") - root := gateTempDir(t) + root := testsupport.TempDir(t) out, errS, code := runCLI(t, "--json", "gate", "drive", "start", "--repo-dir", wt, "--run-root", root, "--owner", "build") if errS != "" { t.Fatalf("start: err=%q", errS) @@ -411,7 +388,7 @@ func TestGateDriveStartFailedIsNonZeroExit(t *testing.T) { // handoff, and claim are commandless — they never resolve config. func TestGateDriveAdvanceHandoffClaim(t *testing.T) { wt := gateDriveConfiguredRepo(t, "metadata_branch: main\nbuild:\n gate: local\n test_command: /bin/echo hi\n") - root := gateTempDir(t) + root := testsupport.TempDir(t) out, _, code := runCLI(t, "--json", "gate", "drive", "start", "--repo-dir", wt, "--run-root", root, "--owner", "build") if code != 0 { t.Fatalf("start failed: %q", out) @@ -466,7 +443,7 @@ func TestGateDriveAdvanceHandoffClaim(t *testing.T) { // gone: no drive start ever accepts an operator command. func TestGateDriveStartRequiresOwner(t *testing.T) { wt := gateDriveRepo(t) - root := gateTempDir(t) + root := testsupport.TempDir(t) // Omitting --owner: cobra's required-flag check fails before RunE, exit 2. _, _, code := runCLI(t, "gate", "drive", "start", "--repo-dir", wt, "--run-root", root) if code != 2 { @@ -493,7 +470,7 @@ func TestGateDriveStartRequiresOwner(t *testing.T) { func TestGateDriveStartNoCommandLeak(t *testing.T) { const secret = "SENTINEL_no_leak_TOKEN_98217" wt := gateDriveConfiguredRepo(t, "metadata_branch: main\nbuild:\n gate: local\n test_command: /bin/echo "+secret+"\n") - root := gateTempDir(t) + root := testsupport.TempDir(t) out, _, code := runCLI(t, "--json", "gate", "drive", "start", "--repo-dir", wt, "--run-root", root, "--owner", "build") if code != 0 { t.Fatalf("start failed: %q", out) @@ -567,7 +544,7 @@ func TestGateDrivePrepareScopeGrantAndRedaction(t *testing.T) { // fail-closed check. func TestGateDriveScopeBoundStartRoundTrips(t *testing.T) { wt := gateDriveConfiguredRepo(t, "metadata_branch: main\n") - root := gateTempDir(t) + root := testsupport.TempDir(t) // Prepare a task recovery scope for this worktree. out, errS, code := runCLI(t, "--json", "gate", "drive", "prepare-scope", @@ -834,7 +811,7 @@ func TestGateDriveAcknowledgeRequiresFlags(t *testing.T) { // malformed successor start never consumes the predecessor. func TestGateDriveStartPredecessorPairBothOrNeither(t *testing.T) { wt := gateDriveRepo(t) - root := gateTempDir(t) + root := testsupport.TempDir(t) _, errS, code := runCLI(t, "gate", "drive", "start", "--repo-dir", wt, "--run-root", root, "--owner", "task", "--predecessor-drive-id", "prev", "--", "/bin/echo", "hi") if code != 2 { @@ -864,7 +841,7 @@ func TestGateDriveStartPredecessorPairBothOrNeither(t *testing.T) { // running under load (the Task-12 defect). func TestGateDriveStartOwnerTaskRunsArgv(t *testing.T) { wt := gateDriveConfiguredRepo(t, "metadata_branch: main\n") - root := gateTempDir(t) + root := testsupport.TempDir(t) out, errS, code := runCLI(t, "--json", "gate", "drive", "start", "--repo-dir", wt, "--run-root", root, "--owner", "task", "--", "/bin/echo", "hi") if code != 0 || errS != "" { @@ -884,7 +861,7 @@ func TestGateDriveStartOwnerTaskRunsArgv(t *testing.T) { // contract, and never launches a drive. func TestGateDriveStartOwnerTaskRequiresArgv(t *testing.T) { wt := gateDriveRepo(t) - root := gateTempDir(t) + root := testsupport.TempDir(t) _, errS, code := runCLI(t, "gate", "drive", "start", "--repo-dir", wt, "--run-root", root, "--owner", "task") if code != 2 { t.Fatalf("task without argv: code=%d, want 2", code) @@ -899,7 +876,7 @@ func TestGateDriveStartOwnerTaskRequiresArgv(t *testing.T) { // operator argv. Exit 2, no drive launched. func TestGateDriveStartBuildRejectsArgv(t *testing.T) { wt := gateDriveRepo(t) - root := gateTempDir(t) + root := testsupport.TempDir(t) _, _, code := runCLI(t, "gate", "drive", "start", "--repo-dir", wt, "--run-root", root, "--owner", "build", "--", "/bin/echo") if code != 2 { t.Fatalf("build with argv: code=%d, want 2", code) @@ -915,7 +892,7 @@ func TestGateDriveStartBuildRejectsArgv(t *testing.T) { // a bare `--`. func TestGateDriveStartRejectsPositionalBeforeDash(t *testing.T) { wt := gateDriveRepo(t) - root := gateTempDir(t) + root := testsupport.TempDir(t) _, _, code := runCLI(t, "gate", "drive", "start", "--repo-dir", wt, "--run-root", root, "--owner", "task", "/bin/echo") if code != 2 { t.Fatalf("positional without dash: code=%d, want 2", code) @@ -1038,7 +1015,7 @@ func TestCLIDoesNotImportProcess(t *testing.T) { func TestGateLaunchInsideWorktreeSecondRefused(t *testing.T) { wt := gateDriveConfiguredRepo(t, "metadata_branch: main\n") - out1, err1, code1 := runCLI(t, "--json", "gate", "launch", "--root", gateTempDir(t), "--cwd", wt, "--", "/bin/sleep", "60") + out1, err1, code1 := runCLI(t, "--json", "gate", "launch", "--root", testsupport.TempDir(t), "--cwd", wt, "--", "/bin/sleep", "60") if code1 != 0 || err1 != "" { t.Fatalf("first launch: out=%q err=%q code=%d", out1, err1, code1) } @@ -1051,7 +1028,7 @@ func TestGateLaunchInsideWorktreeSecondRefused(t *testing.T) { // would silently leave the live first run (and its supervisor) running. t.Cleanup(func() { runCLI(t, "gate", "stop", runDir, "--reason", "test cleanup") }) - out2, err2, code2 := runCLI(t, "--json", "gate", "launch", "--root", gateTempDir(t), "--cwd", wt, "--", "/bin/echo", "hi") + out2, err2, code2 := runCLI(t, "--json", "gate", "launch", "--root", testsupport.TempDir(t), "--cwd", wt, "--", "/bin/echo", "hi") if err2 != "" { t.Fatalf("second launch stderr=%q", err2) } diff --git a/internal/gatedrive/driver_test.go b/internal/gatedrive/driver_test.go index 61e1e8258..f5e6a950a 100644 --- a/internal/gatedrive/driver_test.go +++ b/internal/gatedrive/driver_test.go @@ -145,6 +145,7 @@ var ( func sampleWorktree() string { sampleWorktreeOnce.Do(func() { + // tempdir-exempt: sample worktree built once under sampleWorktreeOnce and shared across tests for the process lifetime. dir, err := os.MkdirTemp("", "gatedrive-sample-worktree-") if err != nil { panic("gatedrive test: mkdir sample worktree: " + err.Error()) diff --git a/internal/install/references_test.go b/internal/install/references_test.go index 380229563..48619931f 100644 --- a/internal/install/references_test.go +++ b/internal/install/references_test.go @@ -159,6 +159,7 @@ func TestDeriveVersionReferencesCanonicalizesTmpAliases(t *testing.T) { if runtime.GOOS != "darwin" { t.Skip("/tmp and /private/tmp are aliases on macOS") } + // tempdir-exempt: must live under the literal /tmp to exercise the macOS /tmp -> /private/tmp alias. base, err := os.MkdirTemp("/tmp", "docket-reference-") if err != nil { t.Fatalf("MkdirTemp: %v", err) diff --git a/internal/release/package_integration_test.go b/internal/release/package_integration_test.go index 265ce2eec..87ec14156 100644 --- a/internal/release/package_integration_test.go +++ b/internal/release/package_integration_test.go @@ -229,6 +229,7 @@ func TestIntegrationReleasePackageDeterministic(t *testing.T) { // bundles to a directory the test framework does not remove, and name // it in the failure output. Failure-only work — the passing path does // nothing extra. + // tempdir-exempt: failure evidence — the mismatched bundles must survive the test so the failure output can name them. keep, kerr := os.MkdirTemp("", "docket-0406-determinism-mismatch-*") if kerr == nil { for _, cp := range []struct{ src, dst string }{ diff --git a/internal/repoguard/repoguard.go b/internal/repoguard/repoguard.go index 36c4c4db6..fc4528d61 100644 --- a/internal/repoguard/repoguard.go +++ b/internal/repoguard/repoguard.go @@ -27,7 +27,11 @@ // Accepted ADRs) — the convention forbids rewriting it, so a guard cannot // demand a repair there (frozen-fixture-corpus-trips-repo-wide-scans). // - tests/fixtures: crafted fixtures for the shell suite, not maintained source. -// - .git, .worktrees: version-control internals and sibling checkouts. +// - any hidden directory except .github: .git and .worktrees (version-control +// internals and sibling checkouts), .docket (the metadata checkout, which +// carries its own copy of the tree when a guard runs from the main +// checkout), and the local harness installs (.claude, .codex, .cursor, ...). +// .github is tracked, maintained source and stays in-population. // // # Fail-closed // @@ -84,8 +88,11 @@ func isExcludedDir(rel, name string) bool { if name == "testdata" { return true } - // Version-control internals / sibling checkouts, at any depth. - if name == ".git" || name == ".worktrees" { + // Hidden directories, at any depth: version-control internals (.git), sibling + // checkouts (.worktrees), the metadata checkout (.docket), and local harness + // installs (.claude, .codex, .cursor, ...). .github is the one tracked, + // maintained hidden directory, so it stays in. + if strings.HasPrefix(name, ".") && name != ".github" { return true } // Exact-location corpora. diff --git a/internal/repoguard/repoguard_test.go b/internal/repoguard/repoguard_test.go index a02fda145..86790d1f5 100644 --- a/internal/repoguard/repoguard_test.go +++ b/internal/repoguard/repoguard_test.go @@ -49,6 +49,11 @@ func fixtureTree(t *testing.T) string { // .git internals must never be walked. writeFile(t, root, ".git/config", "[core]\n") writeFile(t, root, ".worktrees/sib/go.mod", "module y\n") + // Hidden directories (other than .github) are local harness installs and + // metadata checkouts, at any depth. + writeFile(t, root, ".docket/internal/x/x_test.go", "package x\n") + writeFile(t, root, ".claude/agents/docket-status.md", "agent\n") + writeFile(t, root, "internal/app/.cache/stale.go", "package app\n") return root } @@ -79,6 +84,9 @@ func TestMaintainedFilesIncludesAndExcludes(t *testing.T) { "tests/fixtures/hygiene/bad.sh", ".git/config", ".worktrees/sib/go.mod", + ".docket/internal/x/x_test.go", + ".claude/agents/docket-status.md", + "internal/app/.cache/stale.go", } for _, bad := range mustExclude { if slices.Contains(files, bad) { diff --git a/internal/repoguard/tempdir_fixture_test.go b/internal/repoguard/tempdir_fixture_test.go index 60056eb75..ff111bf98 100644 --- a/internal/repoguard/tempdir_fixture_test.go +++ b/internal/repoguard/tempdir_fixture_test.go @@ -25,27 +25,39 @@ package repoguard // never hide a violation). // // LIMITATION (asserted, per the byte-pattern-guard learning): the ban -// matches the receiver-call shape `.TempDir(`. It cannot see a call -// through an interface value or a helper that shadows the name; the -// aliased-import check below closes the one cheap evasion (import -// testsupport under another name and the receiver test goes vacuous). +// matches the receiver-call shapes `.TempDir(` and, since change +// 0462, `.MkdirTemp(`. It cannot see a call through an interface +// value, a function value, or a helper that shadows the name, and +// os.CreateTemp (temp files) is out of scope. The aliased-import check below +// closes the one cheap evasion (import testsupport under another name and +// the receiver test goes vacuous). // -// SCOPE (explicit — see scanRoots and realProcFloors below): this guard -// enforces the fixture rule for the real-process test packages under the -// module's two Go test roots, `internal/` (change 0373) and `cmd/` (change -// 0398). The walk roots are a visible property of the test via the named -// scanRoots list, not an accident of a buried literal, so any later change -// of coverage is a deliberate edit to that list. Each root carries a -// population floor in realProcFloors, kept as a SEPARATE list so that -// dropping a root from the walk reddens its floor instead of silently -// taking the floor with it. +// MkdirTemp EXEMPTIONS: a few real-process sites need a lifetime the +// per-test fixture deliberately does not provide — a binary built in +// TestMain (no t), a process-lifetime dir created under sync.Once, failure +// evidence that must survive, a mandated /tmp parent. Such a call is exempt +// only when a line comment on its own line or the line immediately above +// carries the tempdir-exempt marker with a non-empty reason (see +// tempdirExemptRe). The marker is lexed from the RAW source, so marker text +// inside a string literal or a block comment never exempts. +// +// SCOPE (see realProcFloors below): since change 0462 the scan population is +// derived from the WHOLE repository through the shared MaintainedFiles +// walker, filtered to _test.go files — there is no guard-local list of roots +// left to shrink. Narrowing coverage now means editing the categorical +// exclusions in repoguard.go that every repoguard guard depends on. +// realProcFloors keeps a population floor in each known test root +// (internal/ from change 0373, cmd/ from change 0398), so a rotted +// exec.Command shape or an exclusion that swallows a root fails loudly +// instead of passing vacuously. import ( + "bytes" "fmt" "go/scanner" "go/token" - "io/fs" "os" + "path" "path/filepath" "regexp" "slices" @@ -54,22 +66,21 @@ import ( "testing" ) -// scanRoots names the guard's deliberate coverage boundary: the module -// subtrees walked for real-process test packages (see the SCOPE note in the -// file header). Widening or narrowing coverage is an explicit edit to this -// list, never an accident of a buried walk-root literal. -var scanRoots = []string{"internal", "cmd"} - // realProcFloors are module-relative (slash-separated) packages the -// derivation must always find — one per scan root. An empty or rotted -// derivation, or a root silently dropped from scanRoots, then fails loudly +// whole-repo derivation must always find. An empty or rotted derivation, or +// a shared exclusion that swallows internal/ or cmd/, then fails loudly // instead of passing vacuously (marker-scoped guards need a population -// floor). Deliberately separate from scanRoots: see the SCOPE note. +// floor). var realProcFloors = []string{"internal/process", "cmd/docket"} var execCallRe = regexp.MustCompile(`\bexec\.Command`) var tempDirCallRe = regexp.MustCompile(`\b([A-Za-z_][A-Za-z0-9_]*)\.TempDir\(`) var testsupportAliasRe = regexp.MustCompile(`(?m)^\s*(?:([A-Za-z_][A-Za-z0-9_]*)\s+)?"[^"]*internal/testsupport"`) +var mkdirTempCallRe = regexp.MustCompile(`\b([A-Za-z_][A-Za-z0-9_]*)\.MkdirTemp\(`) + +// tempdirExemptRe matches the text of a justified exemption marker: a line +// comment reading "tempdir-exempt:" followed by a non-blank reason. +var tempdirExemptRe = regexp.MustCompile(`^//[ \t]*tempdir-exempt:[ \t]*\S`) // maskProse returns src with every comment token blanked to spaces (newlines // preserved so line-anchored regexes keep their line structure), and, when @@ -102,61 +113,107 @@ func maskProse(src []byte, maskStrings bool) []byte { return out } +// exemptMarkerLines maps each 1-based line that carries a justified +// tempdir-exempt marker to whether that marker is standalone — the first +// non-whitespace token on its line — rather than trailing code. It lexes the +// RAW source with go/scanner (the masked view blanks comments), so only a real +// // comment token counts: marker text inside a string literal or a /* */ +// block never exempts. +func exemptMarkerLines(src []byte) map[int]bool { + lines := map[int]bool{} + fset := token.NewFileSet() + f := fset.AddFile("", fset.Base(), len(src)) + var s scanner.Scanner + s.Init(f, src, nil, scanner.ScanComments) + for { + pos, tok, lit := s.Scan() + if tok == token.EOF { + break + } + if tok == token.COMMENT && tempdirExemptRe.MatchString(lit) { + off := f.Offset(pos) + lineStart := f.Offset(f.LineStart(f.Line(pos))) + lines[f.Line(pos)] = len(bytes.TrimSpace(src[lineStart:off])) == 0 + } + } + return lines +} + +// mkdirTempViolations finds every executable .MkdirTemp( call in src +// (comments and string/char literals masked) and splits them by line into +// violations and exempt calls. A call is exempt only when its own line carries +// a justified marker, or the line immediately above carries a standalone one +// (exemptMarkerLines): a marker trailing one call never also covers the next. +func mkdirTempViolations(src []byte) (violations, exempt []int) { + markers := exemptMarkerLines(src) + callView := maskProse(src, true) + for _, loc := range mkdirTempCallRe.FindAllIndex(callView, -1) { + // maskProse blanks in place and preserves newlines, so offsets in the + // masked view map to the same line numbers as the raw source. + line := 1 + bytes.Count(callView[:loc[0]], []byte("\n")) + _, ownLine := markers[line] + if ownLine || markers[line-1] { + exempt = append(exempt, line) + } else { + violations = append(violations, line) + } + } + return violations, exempt +} + func TestRealProcessPackagesUseFixtureTempDir(t *testing.T) { root, err := Root() if err != nil { t.Fatal(err) } + files, err := MaintainedFiles(root) + if err != nil { + t.Fatal(err) + } + // Module-relative slash dir -> its module-relative slash _test.go files. pkgs := map[string][]string{} - for _, sr := range scanRoots { - err = filepath.WalkDir(filepath.Join(root, sr), func(p string, d fs.DirEntry, err error) error { - if err != nil || d.IsDir() || !strings.HasSuffix(p, "_test.go") { - return err - } - dir := filepath.Dir(p) - pkgs[dir] = append(pkgs[dir], p) - return nil - }) + for _, f := range files { + if strings.HasSuffix(f, "_test.go") { + dir := path.Dir(f) + pkgs[dir] = append(pkgs[dir], f) + } + } + const fixtureDir = "internal/testsupport" + read := func(rel string) []byte { + b, err := os.ReadFile(filepath.Join(root, filepath.FromSlash(rel))) if err != nil { t.Fatal(err) } + return b } - fixtureDir := filepath.Join(root, "internal", "testsupport") var realProc []string - for dir, files := range pkgs { + for dir, pkgFiles := range pkgs { if dir == fixtureDir { continue // the fixture itself is exempt by construction } - for _, f := range files { - b, err := os.ReadFile(f) - if err != nil { - t.Fatal(err) - } - if execCallRe.Match(b) { + for _, f := range pkgFiles { + if execCallRe.Match(read(f)) { realProc = append(realProc, dir) break } } } sort.Strings(realProc) - // Population floors (marker-scoped guards need one): the derivation must - // find each floor package — internal/process, whose supervisor tests - // motivated the fixture, and cmd/docket, whose built-binary gate tests - // spawn the real supervisor. A missing floor means the grep shape rotted - // or a root was dropped from scanRoots, and the guard would otherwise pass - // vacuously over that root. + // Population floors (marker-scoped guards need one): the whole-repo + // derivation must find each floor package — internal/process, whose + // supervisor tests motivated the fixture, and cmd/docket, whose built-binary + // gate tests spawn the real supervisor. A missing floor means the + // exec.Command shape rotted or a shared MaintainedFiles exclusion swallowed + // a test root, and the guard would otherwise pass vacuously over it. for _, floor := range realProcFloors { - if !slices.Contains(realProc, filepath.Join(root, filepath.FromSlash(floor))) { + if !slices.Contains(realProc, floor) { t.Fatalf("derivation lost %s — real-process set: %v", floor, realProc) } } var violations []string for _, dir := range realProc { for _, f := range pkgs[dir] { - b, err := os.ReadFile(f) - if err != nil { - t.Fatal(err) - } + b := read(f) // Comments masked, string literals kept: import paths survive so an // aliased testsupport import is still visible. aliasView := maskProse(b, false) @@ -173,9 +230,49 @@ func TestRealProcessPackagesUseFixtureTempDir(t *testing.T) { violations = append(violations, fmt.Sprintf("%s: bare %s.TempDir() — use testsupport.TempDir(t)", f, m[1])) } } + // Change 0462: an executable MkdirTemp call is a hand-rolled temp dir + // that skips the fixture's drain-then-retry cleanup, unless the site + // needs a lifetime the per-test fixture cannot provide and says so. + bad, ok := mkdirTempViolations(b) + for _, line := range bad { + violations = append(violations, fmt.Sprintf("%s:%d: MkdirTemp call without a justified tempdir-exempt marker — use testsupport.TempDir(t); only a dir the per-test fixture cannot serve (no t in TestMain, process lifetime under sync.Once, failure evidence that must survive, a mandated parent) may carry an adjacent \"// tempdir-exempt: \"", f, line)) + } + for _, line := range ok { + t.Logf("exempt MkdirTemp: %s:%d", f, line) + } } } if len(violations) > 0 { t.Fatalf("real-process packages must use the testsupport fixture:\n%s", strings.Join(violations, "\n")) } } + +func TestMkdirTempViolations(t *testing.T) { + cases := []struct { + name string + src string + wantViolation []int + wantExempt []int + }{ + {"bare call", "package p\nfunc f() {\n\td, _ := os.MkdirTemp(\"\", \"x\")\n\t_ = d\n}\n", []int{3}, nil}, + {"marker above", "package p\nfunc f() {\n\t// tempdir-exempt: built once in TestMain, no t\n\td, _ := os.MkdirTemp(\"\", \"x\")\n\t_ = d\n}\n", nil, []int{4}}, + {"same-line marker", "package p\nfunc f() {\n\td, _ := os.MkdirTemp(\"\", \"x\") // tempdir-exempt: process-lifetime dir\n\t_ = d\n}\n", nil, []int{3}}, + {"marker two lines above", "package p\nfunc f() {\n\t// tempdir-exempt: too far away\n\n\td, _ := os.MkdirTemp(\"\", \"x\")\n\t_ = d\n}\n", []int{5}, nil}, + {"empty reason", "package p\nfunc f() {\n\t// tempdir-exempt:\n\td, _ := os.MkdirTemp(\"\", \"x\")\n\t_ = d\n}\n", []int{4}, nil}, + {"whitespace reason", "package p\nfunc f() {\n\t// tempdir-exempt: \t\n\td, _ := os.MkdirTemp(\"\", \"x\")\n\t_ = d\n}\n", []int{4}, nil}, + {"spelling in comment and string", "package p\n// os.MkdirTemp(\"\", \"x\")\nvar s = \"os.MkdirTemp(\"\n", nil, nil}, + {"marker in string", "package p\nfunc f() {\n\t_ = \"// tempdir-exempt: not a comment\"\n\td, _ := os.MkdirTemp(\"\", \"x\")\n\t_ = d\n}\n", []int{4}, nil}, + {"block comment marker", "package p\nfunc f() {\n\t/* tempdir-exempt: block comments do not count */\n\td, _ := os.MkdirTemp(\"\", \"x\")\n\t_ = d\n}\n", []int{4}, nil}, + {"consecutive calls", "package p\nfunc f() {\n\t// tempdir-exempt: first only\n\ta, _ := os.MkdirTemp(\"\", \"a\")\n\tb, _ := os.MkdirTemp(\"\", \"b\")\n\t_, _ = a, b\n}\n", []int{5}, []int{4}}, + {"trailing marker does not cover next line", "package p\nfunc f() {\n\ta, _ := os.MkdirTemp(\"\", \"a\") // tempdir-exempt: first only\n\tb, _ := os.MkdirTemp(\"\", \"b\")\n\t_, _ = a, b\n}\n", []int{4}, []int{3}}, + {"non-os receiver", "package p\nfunc f() {\n\td, _ := afs.MkdirTemp(\"\", \"x\")\n\t_ = d\n}\n", []int{3}, nil}, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + gotV, gotE := mkdirTempViolations([]byte(c.src)) + if !slices.Equal(gotV, c.wantViolation) || !slices.Equal(gotE, c.wantExempt) { + t.Fatalf("violations=%v exempt=%v, want violations=%v exempt=%v", gotV, gotE, c.wantViolation, c.wantExempt) + } + }) + } +}