diff --git a/README.md b/README.md index 31d78a6..47126b9 100644 --- a/README.md +++ b/README.md @@ -191,8 +191,63 @@ Rules worth knowing before you write one: a builder, and a verdict nobody stated must not decide that either way. - **Gates verify claims, they do not judge quality.** The registry is `artifacts_exist`, `files_non_empty`, `diff_matches_claims`, - `verdict_consistent`, and `tests_pass(command)`. Repo-specific - verification is a code phase, not a new gate. + `verdict_consistent`, `tests_pass(command)`, and `tests_intact`. + Repo-specific verification is a code phase, not a new gate. +- **A builder cannot cheaply buy green by weakening tests.** `tests_intact` + is a tripwire against careless or opportunistic test tampering, not an + adversarially complete analysis. Every test file changed since + `JIG_BASE_SHA` — committed, uncommitted, or untracked, gitignored + included — is compared whole against its base version: both sides are + normalized (comments stripped by a lexer that knows strings, regex + literals, and Ruby heredocs and %-literals; literals blanked, + whitespace collapsed so a token split across lines rejoins), then tests, + assertions, and markers are counted. It fails when a test file is + deleted, ends with fewer tests (Go `Example` and `Fuzz` functions in a + `_test.go` file included), assertions (a Go example's `// Output:` or + `// Unordered output:` line included, so deleting expected output is a + loss), or `m.Run(` calls, or with + more of any marker that stops tests running: a skip or focus (`t.Skip`, + `test.only.each`, a vitest options object with `skip`, `only`, `fails`, + or `todo` set to anything but `false`, `0`, `null`, `undefined`, or + `""` — in any argument position, after any first argument, through + `.each(…)(…)`, with the key bare or quoted, as in `test("…", {skip: + true}, fn)` or `test(name(), fn, {"only": 1})`, `mark.skip` in any + form, `pytest.importorskip(`, `xit`, RSpec `skip: true` or `:focus`), a + `//go:build` constraint, an added + `TestMain` or `os.Exit(`, `pytestmark`, `__test__ = False`, an aliased + `mark`, or a conftest `collect_ignore` or collection hook. Commenting a + test out, or wrapping it in `/* */` or a string, is a loss. Counts are + per file and per marker kind, so renaming a test inside its file, + converting `test(…)` to `test.each(…)`, swapping an assertion for a + stronger one, or rewording a skip's message passes, and new tests always + do; renaming a test FILE reads as a deletion. What it cannot inspect + fails closed: a binary or over-4 MiB test file, a source test file with + a NUL byte or a UTF-16/UTF-32 byte-order mark, a comment, string, or + heredoc still open at end of file, a tracked test file + flagged assume-unchanged or skip-worktree, an embedded git repository + holding test files. Untracked files under `node_modules`, Go `vendor`, + and Python virtualenvs are skipped, as the runners skip them. Each + finding names `path:line` where a changed line shows it, and says what + to put back. It covers Go, JavaScript and TypeScript, Python, and + Ruby/RSpec in v1, recognizing test files the way `factory.yaml`'s risk + classifier does, plus `conftest.py`. When a change is meant to remove + tests, say so in the definition: `{name: tests_intact, allow: + ["legacy/*_test.go"]}` exempts matching paths. `allow` uses `path.Match` + globs against the repo-relative path — `*` stops at `/` and `**` is + refused — and is rejected on any other gate. `factory.yaml` and + `factory-parallel.yaml` run it on `build`. Known limits: it does not + catch an assertion replaced by a trivially true one, a test runner + neutered through configuration outside `conftest.py` (jest or pytest + config, `package.json`, a `Makefile`), a skip hidden in a non-test + helper, a skip hidden inside a JS template-literal expression or behind + an exotic string escape, an existing marker whose arguments are widened + (`skipif(False)` → `skipif(True)`, edited `collect_ignore` contents, + `if testing.Short()` → `if true`), arithmetic that keeps `m.Run(` but + discards its result, `pytest.exit(..., 0)`, vitest options passed + through a variable (`const opts = {skip: true}`), spread (`{...opts}`) + or nested in another object, or written as `{ skip }` shorthand, a + gutted `f.Fuzz(...)` body, or changes inside + submodules — the reviewers and the risk classifier are the check there. - **Repair loops must be declared and bounded.** `on_fail` is the only loop construct; a cycle or a missing budget is rejected at save time, not discovered at 2 a.m. @@ -261,7 +316,8 @@ Rules worth knowing before you write one: heads, changed paths, and outcome in the result. The publish-only retry judges CI but never repairs — a continuation lives only in the process that ran the chain. Budget is 1–3. A fix that weakens a check instead of the - code is the risk to design for: `factory.yaml` scores edits to CI or lint + code is the risk to design for: `factory.yaml` refuses lost tests and new + skips at the build with `tests_intact`, and scores edits to CI or lint configuration, and test files that lose more lines than they gain, as not low, so such a "fix" is held for a person. - **A flaky Actions job can be re-run before a round is spent.** diff --git a/examples/definitions/factory-parallel.yaml b/examples/definitions/factory-parallel.yaml index d49058e..c21b9b1 100644 --- a/examples/definitions/factory-parallel.yaml +++ b/examples/definitions/factory-parallel.yaml @@ -269,12 +269,17 @@ phases: - name: plan kind: agent owner: planner + # tests_intact refuses a build that deletes a test file, loses tests or + # assertions, or adds a skip/focus marker since the pinned base — the + # cheapest way to turn red tests or red CI green. It runs on every build, + # repair rounds included; name exempt paths with `allow: []`. - name: build kind: agent owner: builder gates: - {name: artifacts_exist} - {name: diff_matches_claims} + - {name: tests_intact} # The commit phases stage with `git add -A` deliberately: by the time a # code phase runs, the write boundary has already verified every path the # agent changed against its allowlist and rolled back anything else (R10), diff --git a/examples/definitions/factory.yaml b/examples/definitions/factory.yaml index 011dd9f..ce1352c 100644 --- a/examples/definitions/factory.yaml +++ b/examples/definitions/factory.yaml @@ -257,12 +257,17 @@ phases: - name: plan kind: agent owner: planner + # tests_intact refuses a build that deletes a test file, loses tests or + # assertions, or adds a skip/focus marker since the pinned base — the + # cheapest way to turn red tests or red CI green. It runs on every build, + # repair rounds included; name exempt paths with `allow: []`. - name: build kind: agent owner: builder gates: - {name: artifacts_exist} - {name: diff_matches_claims} + - {name: tests_intact} # The commit phases stage with `git add -A` deliberately: by the time a # code phase runs, the write boundary has already verified every path the # agent changed against its allowlist and rolled back anything else (R10), diff --git a/internal/engine/boundary.go b/internal/engine/boundary.go index 0d0f27a..bebf0c1 100644 --- a/internal/engine/boundary.go +++ b/internal/engine/boundary.go @@ -507,12 +507,16 @@ func restoreSnapshot(ctx context.Context, dir string, before treeSnapshot) error // `.git/hooks/pre-commit`, a `core.fsmonitor` command, or an `ext::` remote // would otherwise execute jig-side, with jig's privileges, during the very // commands that are supposed to be POLICING the agent. `core.quotepath=off` -// keeps non-ASCII paths literal to match the `-z` enumeration. +// keeps non-ASCII paths literal to match the `-z` enumeration. Auto gc and +// auto maintenance are off so no jig command detaches a background git that +// rewrites the agent's .git after jig has returned. var gitHardening = []string{ "-c", "core.hooksPath=/dev/null", "-c", "core.fsmonitor=", "-c", "protocol.ext.allow=never", "-c", "core.quotepath=off", + "-c", "gc.auto=0", + "-c", "maintenance.auto=false", } // gitEnv is the EXPLICIT environment every jig-side git command runs with — diff --git a/internal/engine/gates.go b/internal/engine/gates.go index 4c33a1d..a561a5f 100644 --- a/internal/engine/gates.go +++ b/internal/engine/gates.go @@ -24,6 +24,12 @@ type gateContext struct { worktree string envelope parsedEnvelope command string // tests_pass only + // baseSHA is the commit the attempt was pinned to — the JIG_BASE_SHA a + // code phase sees — so a gate can judge the whole change since then, + // committed or not, without guessing a base from history the agents + // wrote. allow is tests_intact's exemption list, from the frozen spec. + baseSHA string + allow []string // env is the complete subprocess environment a gate command runs with: // the owning role's allowlist plus the attempt's ephemeral HOME (KTD10, // KTD11). A gate command runs in the worktree the agent just wrote — a @@ -39,6 +45,7 @@ type gateContext struct { // integrity failure, reported as a failed check rather than a panic. func runGate(gc gateContext, spec protocol.GateSpec) protocol.GateReport { gc.command = spec.Command + gc.allow = spec.Allow switch spec.Name { case protocol.GateArtifactsExist: return gateArtifactsExist(gc) @@ -50,6 +57,8 @@ func runGate(gc gateContext, spec protocol.GateSpec) protocol.GateReport { return gateVerdictConsistent(gc) case protocol.GateTestsPass: return gateTestsPass(gc) + case protocol.GateTestsIntact: + return gateTestsIntact(gc) } var report protocol.GateReport report.Check(spec.Name, false, "gate is not in the built-in registry") diff --git a/internal/engine/phase.go b/internal/engine/phase.go index 2a3e3f2..610ade7 100644 --- a/internal/engine/phase.go +++ b/internal/engine/phase.go @@ -928,6 +928,7 @@ func (e *execution) runGates( ctx: ctx, worktree: e.attempt.WorktreePath, envelope: envelope, + baseSHA: e.attempt.BaseSHA, env: env, timeout: e.timeouts, }, gate) diff --git a/internal/engine/phase_test.go b/internal/engine/phase_test.go index 03da343..5cc3685 100644 --- a/internal/engine/phase_test.go +++ b/internal/engine/phase_test.go @@ -1558,6 +1558,65 @@ acceptance: [all_phases_passed] } } +// The gate diffs against the attempt's pinned base, not HEAD: a weakening the +// agent committed itself is still a weakening. The correction names the +// marker, and an emission that removes it passes. +func TestTestsIntactJudgesTheChangeSinceThePinnedBase(t *testing.T) { + repo := initRepo(t) + const original = "package src\n\nimport \"testing\"\n\nfunc TestA(t *testing.T) {\n\tt.Fatal(\"a\")\n}\n" + if err := os.MkdirAll(filepath.Join(repo, "src"), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(repo, "src", "a_test.go"), []byte(original), 0o644); err != nil { + t.Fatal(err) + } + for _, args := range [][]string{{"add", "src"}, {"commit", "--quiet", "-m", "tests"}} { + command := exec.Command("git", args...) + command.Dir = repo + if output, err := command.CombinedOutput(); err != nil { + t.Fatalf("git %v: %v: %s", args, err, output) + } + } + command := exec.Command("git", "rev-parse", "HEAD") + command.Dir = repo + base, err := command.Output() + if err != nil { + t.Fatal(err) + } + done := envelope(map[string]any{"status": "success", "summary": "done"}) + fake := enginetest.New( + enginetest.Step{Files: map[string]string{"src/a_test.go": strings.Replace(original, + "\tt.Fatal", "\tt.Skip(\"later\")\n\tt.Fatal", 1)}, Text: done}, + enginetest.Step{Files: map[string]string{"src/a_test.go": original}, Text: done}, + ) + sink := &recordingSink{} + runner := newTestRunner(t, fake, sink, nil) + + snapshot := "name: t\n" + oneWriterRoster + ` +phases: + - name: build + kind: agent + owner: writer + gates: + - {name: tests_intact} +acceptance: [all_phases_passed, tests_intact] +` + attempt := testAttempt(snapshot, nil, repo) + attempt.BaseSHA = strings.TrimSpace(string(base)) + outcome := runner.Execute(context.Background(), attempt) + if outcome.State != protocol.AttemptAcceptedUnpublished { + t.Fatalf("state = %q (%s), want accepted_unpublished", outcome.State, outcome.Error) + } + if fail, pass := sink.count(protocol.EventGateFail, "tests_intact"), + sink.count(protocol.EventGatePass, "tests_intact"); fail != 1 || pass != 1 { + t.Fatalf("tests_intact failed %d and passed %d time(s), want 1 and 1", fail, pass) + } + if calls := fake.Calls(); !strings.Contains(calls[1].Prompt, "src/a_test.go:6") || + !strings.Contains(calls[1].Prompt, "skip/focus marker") { + t.Fatalf("correction prompt does not point at the skip: %.400q", calls[1].Prompt) + } +} + // ---- scenario: a missing verdict is not a rejection (R9) ------------------- // `approved` is read as a bool, so an omitted field and a stated false carry diff --git a/internal/engine/tests_intact.go b/internal/engine/tests_intact.go new file mode 100644 index 0000000..b05fe10 --- /dev/null +++ b/internal/engine/tests_intact.go @@ -0,0 +1,1179 @@ +// tests_intact.go — the tests_intact gate (R9). A repair loop that feeds red +// tests or red CI back to a builder invites the cheapest "fix": delete the +// failing test, drop its assertion, or mark it skipped (the CI-repair plan's +// "weakening the check" risk). The risk classifier only holds such work for a +// person after the fact; this gate refutes it at the phase that wrote it, so +// the builder is told in the same session to put the check back. +// +// It is a tripwire against careless or opportunistic tampering, not an +// adversarially complete analysis. Each changed test file is compared +// whole, base against worktree: both sides are normalized (comments +// stripped by a small string-aware lexer, string contents blanked, +// whitespace collapsed so tokens split across lines rejoin), then tests, +// assertions, and every skip/focus marker kind are counted on each side. +// Fewer tests or assertions, or more of any marker, fails. The hunk diff +// only supplies a best-effort line number for the finding. Whatever the +// gate cannot inspect — a binary or oversized file, a hidden index flag, an +// embedded repository — fails closed rather than passing unread. +package engine + +import ( + "fmt" + "io/fs" + "os" + "path" + "path/filepath" + "regexp" + "sort" + "strconv" + "strings" + "unicode" + + "github.com/StructuPath/jig/internal/protocol" +) + +// testPathPattern is factory.yaml's classify-risk `tests` pattern plus +// pytest's test_*.py and conftest.py (where collect_ignore and collection +// hooks hide whole files), so the gate and the risk classifier agree on +// what a test file is. +var testPathPattern = regexp.MustCompile( + `(_test\.go|_test\.py|\.(test|spec)\.[cm]?[jt]sx?|_spec\.rb)$|(^|/)(test_[^/]*|conftest)\.py$|(^|/)(tests?|__tests__|spec)/`) + +// maxTestFileBytes is the largest test file the gate reads. A larger one is +// a finding, never a silent truncation: the part left unread is exactly +// where a skip would go. +const maxTestFileBytes = 4 << 20 + +// lexStyle is what a language's comment-and-string lexer recognizes. +type lexStyle struct { + slashComments bool // `//` and `/* */` + hashComments bool // `#` + ruby bool // `=begin`/`=end`, heredocs, `%r{…}` and other % literals + tripleQuotes bool // Python `"""` and `'''` + backticks bool // a backtick string: Go raw, JS template + backtickEscapes bool // JS templates honor `\`; Go raw strings do not + goDirectives bool // keep `//go:build` and `// +build` lines as tokens + goOutput bool // keep an example's `// Output:` and `// Unordered output:` lines as tokens + // regexLiterals: a `/` in expression position opens a regex, so a `/*` + // or `#` inside one is not a comment (JS, Ruby). + regexLiterals bool + // optionStrings keeps what an options object needs from a literal: an + // identifier's text (a quoted key), and whether it is empty. Everything + // else becomes `"x"`. + optionStrings bool +} + +// markerPattern finds one thing that stops tests from running: a skip, a +// focus that silences the rest, a constraint or collection rule that drops a +// file, an exit that ends the run early. A named `kind` group, when present, +// identifies the marker; otherwise the whole match does. Patterns match the +// normalized form: no comments, strings as "", whitespace only between two +// word characters. +type markerPattern struct{ re *regexp.Regexp } + +// lossCount is a count that must not fall: tests, assertions, m.Run calls. +type lossCount struct { + re *regexp.Regexp + what string // "test function(s)" + fix string +} + +// testLanguage is one v1 language. A test file in a language with none (a +// fixture under tests/, say) is still checked for deletion and size. +type testLanguage struct { + lex lexStyle + losses []lossCount + markers []markerPattern + // optionCalls finds a test call's name; every `{…}` argument in its + // argument lists is an options object, whose marker keys set to + // anything but a falsy literal count as that kind, so `test.skip(…)` → + // `test(…, {skip: true}, …)` is no change. + optionCalls *regexp.Regexp +} + +const ( + fixTests = "restore the removed tests, or rewrite each as an equivalent test; deleting a failing test does not fix the code" + fixAssertions = "restore the removed assertions or replace each with one at least as strict" +) + +var ( + langGo = &testLanguage{ + lex: lexStyle{slashComments: true, backticks: true, goDirectives: true}, + losses: []lossCount{ + {regexp.MustCompile(`\bfunc Test\w*\(`), "test function(s)", fixTests}, + {regexp.MustCompile(`\bt\.(Error|Fatal|Fail)|\b(assert|require)\.\w+\(`), "assertion(s)", fixAssertions}, + // os.Exit(m.Run()) → os.Exit(0) keeps TestMain and os.Exit + // and runs nothing. + {regexp.MustCompile(`\bm\.Run\(`), "m.Run call(s)", + "TestMain must still call m.Run() and exit with its result, or the suite never runs"}, + }, + markers: []markerPattern{ + {regexp.MustCompile(`\b[tb]\.Skip\w*`)}, + // A TestMain owns the run: it can return without m.Run(). + {regexp.MustCompile(`\bfunc TestMain\(`)}, + {regexp.MustCompile(`\bos\.Exit\(`)}, + // The whole constraint is the kind, so changing one counts as + // adding one: `//go:build linux` → `//go:build ignore`. + {regexp.MustCompile("\x00build:[^\x00]*\x00")}, + }, + } + // langGoTest is a _test.go file, where `go test` also runs examples and + // fuzz targets. A named func with no receiver is always top-level. An + // example's `// Output:` line is its assertion: without one it is only + // compiled, never run. + langGoTest = &testLanguage{ + lex: lexStyle{slashComments: true, backticks: true, goDirectives: true, goOutput: true}, + losses: []lossCount{ + {regexp.MustCompile(`\bfunc (Test\w*\(|Example\w*\(\)|Fuzz\w*\(\w+\*testing\.F\))`), + "test function(s)", fixTests}, + {regexp.MustCompile(`\bt\.(Error|Fatal|Fail)|\b(assert|require)\.\w+\(|\x00output\x00`), + "assertion(s)", fixAssertions}, + langGo.losses[2], + }, + markers: langGo.markers, + } + langJS = &testLanguage{ + lex: lexStyle{slashComments: true, backticks: true, backtickEscapes: true, regexLiterals: true}, + losses: []lossCount{ + // Any chain counts as the test it declares, so test(…) → + // test.each(…)(…) is no loss; a .only/.skip is a marker below. + {regexp.MustCompile(`(^|[^.\w$])(it|test)(\.\w+)*(\(|"")`), "test function(s)", fixTests}, + {regexp.MustCompile(`\bexpect\(|\bassert(\.\w+)?\(`), "assertion(s)", fixAssertions}, + }, + markers: []markerPattern{ + // vitest's skipIf/runIf/fails included: each turns a test off + // or inverts it. + {regexp.MustCompile(`\b(describe|context|suite|it|test|specify|bench)(\.\w+)*?\.(?Ponly|skip|todo|skipIf|runIf|fails)\b`)}, + {regexp.MustCompile(`(^|[^.\w$])(?Pxit|xdescribe|xtest|xcontext|fit|fdescribe)[.(]`)}, + }, + // vitest's options object, in any argument position: + // test("…", {skip: true}, fn), test("…", fn, {skip: true}). + optionCalls: regexp.MustCompile(`(^|[^.\w$])(describe|context|suite|it|test|specify|bench)(\.\w+)*`), + } + langPython = &testLanguage{ + lex: lexStyle{hashComments: true, tripleQuotes: true}, + losses: []lossCount{ + {regexp.MustCompile(`\bdef test\w*\(`), "test function(s)", fixTests}, + {regexp.MustCompile(`(^|[^.\w])assert\b|\bself\.assert\w*\(`), "assertion(s)", fixAssertions}, + }, + markers: []markerPattern{ + // `mark.skip`, not `@pytest.mark.skip`, so parenthesized, + // aliased, and pytest.param(marks=…) forms all count. + {regexp.MustCompile(`\bmark\.(skip|skipif|xfail)\b`)}, + {regexp.MustCompile(`\bunittest\.(skip\w*|expectedFailure)\b`)}, + {regexp.MustCompile(`\bpytest\.(skip|xfail|importorskip)\(|\bself\.skipTest\(`)}, + // unittest's decorators imported bare: `from unittest import skip`. + {regexp.MustCompile(`@(?PskipIf|skipUnless|skip|expectedFailure)\b`)}, + {regexp.MustCompile(`\braise (unittest\.)?(?PSkipTest)\b`)}, + {regexp.MustCompile(`(^|[^.\w])(?Ppytestmark)(:[^=]+)?\+?=`)}, + {regexp.MustCompile(`(^|[^.\w])(?P__test__=False)\b`)}, + // An alias puts mark.skip out of a pattern's reach: flag the + // alias itself. + {regexp.MustCompile(`\b(?P(mark|skip|skipIf|skipUnless|expectedFailure|SkipTest) as)\b`)}, + {regexp.MustCompile(`=(?Ppytest\.mark)([^.\w]|$)`)}, + {regexp.MustCompile(`\bdef (?Ppytest_(ignore_collect|collection_modifyitems))\(`)}, + {regexp.MustCompile(`(^|[^.\w])(?Pcollect_ignore(_glob)?)(:[^=]+)?(\+?=|\.(append|extend|insert)\()`)}, + }, + } + langRuby = &testLanguage{ + lex: lexStyle{hashComments: true, ruby: true, regexLiterals: true}, + losses: []lossCount{ + // `it` followed by a description, a paren, a block — not + // Ruby 3.4's implicit block parameter. + {regexp.MustCompile(`(^|[^.\w])(it|specify|example)(\(|""|\{| ?do\b)`), "test function(s)", fixTests}, + {regexp.MustCompile(`\bexpect[({]|\bshould(_not)?\b`), "assertion(s)", fixAssertions}, + }, + markers: []markerPattern{ + // As a call (`skip`, `xit "…"`) or as metadata (`skip: true`, + // `:focus`) — the normalized form reads both the same way. + {regexp.MustCompile(`(^|[^.\w])(?Pskip|pending|xit|xspecify|xexample|xdescribe|xcontext|fit|fdescribe|fcontext|focus)\b`)}, + }, + } + jsExtension = regexp.MustCompile(`\.[cm]?[jt]sx?$`) + goDirective = regexp.MustCompile(`^//\s*(go:build|\+build)\b`) + // goOutput is how go/doc recognizes an example's expected output. + goOutput = regexp.MustCompile(`(?i)^//\s*(unordered )?output:`) + jsIdentifier = regexp.MustCompile(`^[A-Za-z_$][\w$]*$`) +) + +// optionKinds are the options-object keys that stop or invert a test, named +// as the chain markers name them. +var optionKinds = map[string]bool{"skip": true, "only": true, "fails": true, "todo": true} + +// falsyOptions are the literal values that leave an option off, in the +// optionStrings normalized form. +var falsyOptions = map[string]bool{"false": true, "0": true, "null": true, "undefined": true, `""`: true} + +func testLanguageOf(file string) *testLanguage { + switch { + case strings.HasSuffix(file, "_test.go"): + return langGoTest + case strings.HasSuffix(file, ".go"): + return langGo + case strings.HasSuffix(file, ".py"): + return langPython + case strings.HasSuffix(file, ".rb"): + return langRuby + case jsExtension.MatchString(file): + return langJS + } + return nil +} + +// stripCode drops comments and blanks string, regex, and heredoc contents to +// `""`, so neither a commented-out test nor a marker spelled inside a +// literal counts. A comment becomes a space, so `a/*x*/b` stays two tokens. +// It is a lexer, not a parser. The second result is a problem when a +// comment, string, or literal is still open at end of file: everything after +// an unclosed opener went unread, and the gate does not vouch for it. +func stripCode(src string, style lexStyle) (string, string) { + src = strings.TrimPrefix(src, string(rune(0xFEFF))) + out := make([]byte, 0, len(src)) + n := len(src) + var heredocs []string // Ruby heredoc terminators opened on this line + for i := 0; i < n; { + c := src[i] + lineStart := i == 0 || src[i-1] == '\n' + switch { + case style.slashComments && strings.HasPrefix(src[i:], "//"): + end := lineEnd(src, i) + switch { + case style.goDirectives && goDirective.MatchString(src[i:end]): + out = append(out, " \x00build:"+strings.Join(strings.Fields(src[i+2:end]), " ")+"\x00 "...) + case style.goOutput && goOutput.MatchString(src[i:end]): + out = append(out, " \x00output\x00 "...) + } + out = append(out, ' ') + i = end + case style.slashComments && strings.HasPrefix(src[i:], "/*"): + end := strings.Index(src[i+2:], "*/") + if end < 0 { + return "", "a /* block comment" + } + out = append(out, ' ') + i += 2 + end + 2 + case style.hashComments && c == '#': + out = append(out, ' ') + i = lineEnd(src, i) + case style.ruby && lineStart && strings.HasPrefix(src[i:], "=begin"): + end := strings.Index(src[i:], "\n=end") + if end < 0 { + return "", "an =begin block" + } + out = append(out, ' ') + i = lineEnd(src, i+end+1) + case style.tripleQuotes && (strings.HasPrefix(src[i:], `"""`) || strings.HasPrefix(src[i:], `'''`)): + end := strings.Index(src[i+3:], src[i:i+3]) + if end < 0 { + return "", "a triple-quoted string" + } + out = append(out, `""`...) + i += 3 + end + 3 + case style.regexLiterals && c == '/' && regexPosition(out, src, i, style.ruby): + if end, ok := scanRegex(src, i); ok { + out = append(out, blankLiteral("/", style)...) + i = end + continue + } + out = append(out, c) // no closing `/` on the line: division after all + i++ + case style.ruby && c == '%' && i+2 < n && strings.IndexByte("qQwWiIrsx", src[i+1]) >= 0 && + isLiteralDelimiter(src[i+2]): + end, ok := scanPercentLiteral(src, i+2) + if !ok { + return "", "a %" + string(src[i+1]) + " literal" + } + out = append(out, `""`...) + i = end + case style.ruby && strings.HasPrefix(src[i:], "<<") && rubyHeredocStart(src[i:]) != nil: + match := rubyHeredocStart(src[i:]) + heredocs = append(heredocs, match[3]) + out = append(out, `""`...) + i += len(match[0]) + case c == '\n' && len(heredocs) > 0: + // The rest of the opening line has been read; the bodies follow, + // in order, and none of them is code. + out = append(out, '\n') + i++ + for _, terminator := range heredocs { + var ok bool + if i, ok = skipHeredoc(src, i, terminator); !ok { + return "", "a heredoc" + } + } + heredocs = nil + case c == '"' || c == '\'' || (c == '`' && style.backticks): + escapes := c != '`' || style.backtickEscapes + j, closed := i+1, false + for j < n { + if escapes && src[j] == '\\' { + j += 2 + continue + } + if src[j] == c { + j++ + closed = true + break + } + if src[j] == '\n' && c != '`' { + break // a one-line string ends at the line, closed or not + } + j++ + } + if !closed && j >= n { + return "", "a string or template literal" + } + content := src[i+1 : j] + if closed { + content = src[i+1 : j-1] + } + out = append(out, blankLiteral(content, style)...) + i = min(j, n) + default: + out = append(out, c) + i++ + } + } + if len(heredocs) > 0 { + return "", "a heredoc" + } + return string(out), "" +} + +// blankLiteral is what a string or regex literal with the given contents +// normalizes to: `""`, or under optionStrings `""` when empty, the quoted +// text of an identifier, and `"x"` otherwise. +func blankLiteral(content string, style lexStyle) string { + switch { + case !style.optionStrings || content == "": + return `""` + case jsIdentifier.MatchString(content): + return `"` + content + `"` + } + return `"x"` +} + +// regexKeywords end an expression's left side: a `/` after one opens a regex. +var regexKeywords = map[string]bool{ + "return": true, "typeof": true, "instanceof": true, "in": true, "of": true, "new": true, + "delete": true, "void": true, "throw": true, "case": true, "do": true, "else": true, + "yield": true, "await": true, "when": true, "if": true, "unless": true, "and": true, + "or": true, "not": true, "while": true, "until": true, "elsif": true, "then": true, +} + +// regexPosition reports whether a `/` at src[i] is in expression position: +// at a line start, after an operator or opener, or after a keyword — where +// it cannot be division. In Ruby, `match /re/` (a name, a space, then no +// space) is a regex argument, which is how Ruby itself reads it. +func regexPosition(out []byte, src string, i int, ruby bool) bool { + k := len(out) - 1 + spaced := false + for k >= 0 && (out[k] == ' ' || out[k] == '\t' || out[k] == '\r') { + k-- + spaced = true + } + if k < 0 || out[k] == '\n' { + return true + } + prev := out[k] + // `=>` returns an expression; a bare `>` is a comparison, whose right + // side is never directly a `/`, so it stays out. + if strings.IndexByte("(,=:[!&|?{};~", prev) >= 0 || prev == '>' && k > 0 && out[k-1] == '=' { + return true + } + if !isWordByte(prev) { + return false + } + start := k + for start > 0 && isWordByte(out[start-1]) { + start-- + } + if regexKeywords[string(out[start:k+1])] { + return true + } + return ruby && spaced && i+1 < len(src) && src[i+1] != ' ' && src[i+1] != '=' +} + +// scanRegex finds the end of a regex literal opening at src[i]: the first +// unescaped `/` outside a `[...]` class. A newline first means it was never +// a regex. +func scanRegex(src string, i int) (int, bool) { + inClass := false + for j := i + 1; j < len(src); j++ { + switch src[j] { + case '\\': + j++ + case '[': + inClass = true + case ']': + inClass = false + case '/': + if !inClass { + return j + 1, true + } + case '\n': + return 0, false + } + } + return 0, false +} + +func isLiteralDelimiter(c byte) bool { + return c > ' ' && c < 0x7f && !isWordByte(c) +} + +// scanPercentLiteral finds the end of a Ruby %-literal whose delimiter is at +// src[open]; bracket delimiters nest. +func scanPercentLiteral(src string, open int) (int, bool) { + closer := src[open] + switch closer { + case '(': + closer = ')' + case '[': + closer = ']' + case '{': + closer = '}' + case '<': + closer = '>' + } + depth := 1 + for j := open + 1; j < len(src); j++ { + switch { + case src[j] == '\\': + j++ + case closer != src[open] && src[j] == src[open]: + depth++ + case src[j] == closer: + depth-- + if depth == 0 { + return j + 1, true + } + } + } + return 0, false +} + +var rubyHeredocPattern = regexp.MustCompile("^<<([~-]?)([\"'`]?)([A-Za-z_]\\w*)([\"'`]?)") + +// rubyHeredocStart matches a heredoc opener. A bare `<= 0 { + return from + end + } + return len(src) +} + +func isWordByte(c byte) bool { + return c == '_' || c == '$' || c >= 0x80 || unicode.IsLetter(rune(c)) || unicode.IsDigit(rune(c)) +} + +// collapseSpace removes all whitespace, newlines included, except one space +// between two word characters: `t.\n\tSkip` → `t.Skip`, `test\n .only` → +// `test.only`, while `func TestMain` keeps the space that separates them. +func collapseSpace(s string) string { + out := make([]byte, 0, len(s)) + pending := false + for i := 0; i < len(s); i++ { + c := s[i] + if c == ' ' || c == '\t' || c == '\n' || c == '\r' || c == '\f' || c == '\v' { + pending = true + continue + } + if pending && len(out) > 0 && isWordByte(out[len(out)-1]) && isWordByte(c) { + out = append(out, ' ') + } + pending = false + out = append(out, c) + } + return string(out) +} + +// normalize is a whole file's normalized form, or the construct left open at +// end of file. +func (l *testLanguage) normalize(src string) (string, string) { + stripped, problem := stripCode(src, l.lex) + return collapseSpace(stripped), problem +} + +// normalizeLine normalizes one line out of context, for line numbers only; +// a construct it leaves open is expected there and costs nothing. +func (l *testLanguage) normalizeLine(src string) string { + normalized, _ := l.normalize(src) + return normalized +} + +// lineKinds counts marker kinds on one line out of context, for line +// numbers only. +func (l *testLanguage) lineKinds(src string) map[string]int { + return l.markerKinds(l.normalizeLine(src), src) +} + +// uninspectableText names why content cannot be read as source text: a NUL +// byte, or a UTF-16/UTF-32 byte-order mark (those encodings put NULs +// between the ASCII a pattern looks for, so nothing would match). +func uninspectableText(content string) string { + switch { + case strings.HasPrefix(content, "\xFF\xFE"), strings.HasPrefix(content, "\xFE\xFF"), + strings.HasPrefix(content, "\x00\x00\xFE\xFF"): + return "is UTF-16 or UTF-32 encoded" + case strings.IndexByte(content, 0) >= 0: + return "contains a NUL byte" + } + return "" +} + +// markerKinds counts each marker kind in src, given its normalized form. +func (l *testLanguage) markerKinds(normalized, src string) map[string]int { + kinds := map[string]int{} + for _, marker := range l.markers { + kindIndex := marker.re.SubexpIndex("kind") + for _, match := range marker.re.FindAllStringSubmatch(normalized, -1) { + kind := match[0] + if kindIndex >= 0 { + kind = match[kindIndex] + } + kinds[kind]++ + } + } + if l.optionCalls != nil { + l.countOptions(src, kinds) + } + return kinds +} + +// countOptions adds the marker keys of every options object passed to a +// test call. It reads its own normalized form, one that keeps quoted keys +// and empty strings, and finds each argument by balanced-bracket scanning, +// so an object counts in any position and after any first argument. A +// construct left open only costs a line its number: compare has already +// failed the file on one. +func (l *testLanguage) countOptions(src string, kinds map[string]int) { + style := l.lex + style.optionStrings = true + stripped, _ := stripCode(src, style) + text := collapseSpace(stripped) + for _, call := range l.optionCalls.FindAllStringIndex(text, -1) { + for _, args := range callArguments(text, call[1]) { + for _, arg := range splitTopLevel(args) { + if !strings.HasPrefix(arg, "{") || closingBracket(arg, 0) != len(arg)-1 { + continue + } + for _, entry := range splitTopLevel(arg[1 : len(arg)-1]) { + key, value, ok := strings.Cut(entry, ":") + if !ok { + continue // shorthand, spread, or a method: not read + } + if unquoted, err := strconv.Unquote(key); err == nil { + key = unquoted + } + if optionKinds[key] && !falsyOptions[value] { + kinds[key]++ + } + } + } + } + } +} + +// callArguments lists the argument lists of the call chain at text[at]: one +// for test(…), two for test.each(…)(…). A list never closed runs to the end +// of text, which is how a single line out of context reads. +func callArguments(text string, at int) []string { + var lists []string + for at < len(text) { + switch text[at] { + case '(': + end := closingBracket(text, at) + lists = append(lists, text[at+1:min(end, len(text))]) + at = end + 1 + case '.': + at++ + for at < len(text) && isWordByte(text[at]) { + at++ + } + default: + return lists + } + } + return lists +} + +// closingBracket is the index of the bracket closing the one at text[open], +// or len(text) when none does. Normalized text holds no brackets inside +// literals, so depth alone is exact. +func closingBracket(text string, open int) int { + depth := 0 + for i := open; i < len(text); i++ { + switch text[i] { + case '(', '[', '{': + depth++ + case ')', ']', '}': + depth-- + if depth == 0 { + return i + } + } + } + return len(text) +} + +// splitTopLevel splits s at the commas outside any bracket. +func splitTopLevel(s string) []string { + var parts []string + depth, start := 0, 0 + for i := 0; i < len(s); i++ { + switch s[i] { + case '(', '[', '{': + depth++ + case ')', ']', '}': + depth-- + case ',': + if depth == 0 { + parts = append(parts, s[start:i]) + start = i + 1 + } + } + } + return append(parts, s[start:]) +} + +// diffLine is one added or removed line. For an added line, line is its +// number in the worktree; for a removed one it is where the line used to sit +// in the worktree's numbering, so the finding points at the gap. +type diffLine struct { + line int + text string +} + +type fileDiff struct { + path string + deleted bool + created bool // not in the base: its base content is empty + // binary: git reported the file as binary, so it has no hunks to read. + binary bool + added, removed []diffLine +} + +var hunkHeader = regexp.MustCompile(`^@@ -\d+(?:,\d+)? \+(\d+)(?:,(\d+))? @@`) + +// parseUnifiedDiff reads `git diff` output. Header lines are only read +// before a file's first hunk, so a removed line that itself starts with +// "--- " is never mistaken for one; "diff --git" can only be a header, +// because every hunk line starts with a marker. +func parseUnifiedDiff(output string) []*fileDiff { + var files []*fileDiff + var current *fileDiff + inHunk := false + newLine := 0 + for _, line := range strings.Split(output, "\n") { + if strings.HasPrefix(line, "diff --git ") { + current = &fileDiff{path: diffGitPath(strings.TrimPrefix(line, "diff --git "))} + files = append(files, current) + inHunk = false + continue + } + if current == nil { + continue + } + if match := hunkHeader.FindStringSubmatch(line); match != nil { + inHunk = true + newLine, _ = strconv.Atoi(match[1]) + // A hunk with no new lines starts at the line BEFORE the gap; + // its removals sit at the line after. + if match[2] == "0" { + newLine++ + } + continue + } + if !inHunk { + switch { + case strings.HasPrefix(line, "deleted file mode"): + current.deleted = true + case strings.HasPrefix(line, "new file mode"): + current.created = true + case strings.HasPrefix(line, "Binary files "), strings.HasPrefix(line, "GIT binary patch"): + current.binary = true + case strings.HasPrefix(line, "--- a/"), strings.HasPrefix(line, `--- "a/`): + current.path = diffHeaderPath(strings.TrimPrefix(line, "--- "), "a/") + case strings.HasPrefix(line, "+++ b/"), strings.HasPrefix(line, `+++ "b/`): + current.path = diffHeaderPath(strings.TrimPrefix(line, "+++ "), "b/") + } + continue + } + switch { + case strings.HasPrefix(line, "+"): + current.added = append(current.added, diffLine{newLine, line[1:]}) + newLine++ + case strings.HasPrefix(line, "-"): + current.removed = append(current.removed, diffLine{newLine, line[1:]}) + case strings.HasPrefix(line, " "): + // Context, should a repository's config ever add some back. + newLine++ + } + } + return files +} + +// diffGitPath recovers the path from a `diff --git a/P b/P` header. With +// --no-renames both sides name the same path, so an unquoted header splits +// exactly in half even when the path contains spaces. +func diffGitPath(rest string) string { + if strings.HasPrefix(rest, `"`) { + quoted, err := strconv.QuotedPrefix(rest) + if err != nil { + return rest + } + return diffHeaderPath(quoted, "a/") + } + if len(rest) < 5 { + return rest + } + return rest[2 : 2+(len(rest)-5)/2] +} + +// diffHeaderPath strips a side prefix from a header path, undoing git's +// C-style quoting of unusual names first. Git ends a ---/+++ path that +// contains a space with a tab; a name holding a real tab is always quoted, +// so a literal trailing tab is never part of the name. +func diffHeaderPath(raw, prefix string) string { + raw = strings.TrimSuffix(raw, "\t") + if strings.HasPrefix(raw, `"`) { + if unquoted, err := strconv.Unquote(raw); err == nil { + raw = unquoted + } + } + return strings.TrimPrefix(raw, prefix) +} + +func shortSHA(sha string) string { + if len(sha) > 12 { + return sha[:12] + } + return sha +} + +// gateTestsIntact: no test file changed since the pinned base may lose a +// test, an assertion, or an m.Run call, or gain a skip/focus marker, and +// none may be deleted — unless the definition's allow list names it. Counts +// are whole-file, so a rename inside the file, an assertion replaced with a +// stronger one, or a reworded skip passes; a renamed FILE is a deletion, +// since --no-renames keeps the old path visible as gone. +func gateTestsIntact(gc gateContext) protocol.GateReport { + var report protocol.GateReport + if gc.baseSHA == "" { + report.Check("base commit", false, "the attempt has no pinned base commit to diff against") + return report + } + j := &intactJudge{gc: gc, report: &report, base: shortSHA(gc.baseSHA)} + if !j.judgeTracked() || !j.judgeUntracked() || !j.judgeIndexFlags() { + return report + } + if j.checked == 0 { + report.Check("tests", true, "no test file changed since base "+j.base) + } + return report +} + +type intactJudge struct { + gc gateContext + report *protocol.GateReport + base string + checked int +} + +// git runs one hardened git command; a failure is a failed check, and the +// gate stops, since what it could not list it cannot vouch for. +func (j *intactJudge) git(item string, args ...string) (string, bool) { + output, err := runGit(j.gc.ctx, j.gc.worktree, args...) + if err != nil { + j.report.Check(item, false, err.Error()) + return "", false + } + return output, true +} + +// exempt counts a test path and reports whether allow covers it. +func (j *intactJudge) exempt(file string) bool { + j.checked++ + if glob := allowedBy(file, j.gc.allow); glob != "" { + j.report.Check(file, true, fmt.Sprintf("exempt: allowed by %q", glob)) + return true + } + return false +} + +// judgeTracked compares every tracked test file the diff shows as changed. +func (j *intactJudge) judgeTracked() bool { + // The diff only finds changed files and line numbers; the verdict comes + // from whole-file contents. Prefixes are pinned against diff.noprefix + // and diff.mnemonicPrefix, external drivers and textconv are off, + // --text overrides a `-diff` attribute, and no inter-hunk context keeps + // line numbers exact: the worktree's config must not shape what this + // reads. + output, ok := j.git("git diff", "diff", "--no-renames", "--no-ext-diff", "--no-textconv", + "--text", "--no-color", "--unified=0", "--inter-hunk-context=0", + "--src-prefix=a/", "--dst-prefix=b/", j.gc.baseSHA, "--") + if !ok { + return false + } + for _, file := range parseUnifiedDiff(output) { + j.judgeDiffFile(file) + } + return true +} + +// judgeDiffFile judges one file the diff shows as changed. +func (j *intactJudge) judgeDiffFile(file *fileDiff) { + if !testPathPattern.MatchString(file.path) || j.exempt(file.path) { + return + } + switch { + case file.deleted: + j.report.Check(file.path, false, fmt.Sprintf( + "test file deleted since base %s; restore it (git checkout %s -- %s). "+ + "Removing a test is only allowed when the definition's tests_intact allow list names it", + j.base, j.base, file.path)) + return + case file.binary: + // Unreachable with --text, unless git changes; a file the gate + // cannot read is never one it vouches for. + j.report.Check(file.path, false, + "git reports this test file as binary, so its changes cannot be inspected; "+ + "restore it as text or name it in the tests_intact allow list") + return + } + before := "" + if !file.created { + var problem string + before, problem = j.baseContent(file.path) + if problem != "" { + j.report.Check(file.path, false, problem) + return + } + } + after, problem := readTestFile(j.gc.worktree, file.path) + if problem != "" { + j.report.Check(file.path, false, problem) + return + } + j.compare(file.path, before, after, file) +} + +// baseContent reads a file at the pinned base, refusing one too large to +// read whole. +func (j *intactJudge) baseContent(file string) (string, string) { + object := j.gc.baseSHA + ":" + file + size, err := runGit(j.gc.ctx, j.gc.worktree, "cat-file", "-s", object) + if err != nil { + return "", "base content could not be read: " + err.Error() + } + if n, err := strconv.Atoi(strings.TrimSpace(size)); err != nil || n > maxTestFileBytes { + return "", tooLarge("base version", strings.TrimSpace(size)) + } + content, err := runGit(j.gc.ctx, j.gc.worktree, "show", "--no-textconv", object) + if err != nil { + return "", "base content could not be read: " + err.Error() + } + return content, "" +} + +func tooLarge(which, size string) string { + return fmt.Sprintf("too large to inspect: the %s is %s bytes, over the %d-byte limit; "+ + "split the file or name it in the tests_intact allow list", which, size, maxTestFileBytes) +} + +// readTestFile reads a worktree file whole. Anything but a regular file +// fails: a symlink could point outside the worktree, and the gate neither +// follows it nor vouches for what it cannot read. +func readTestFile(worktree, name string) (string, string) { + full, ok := artifactPath(worktree, filepath.FromSlash(name)) + if !ok { + return "", "path is not inside the worktree" + } + info, err := os.Lstat(full) + if err != nil { + return "", "could not be read: " + err.Error() + } + if !info.Mode().IsRegular() { + return "", "is not a regular file, so it cannot be inspected; restore it as one" + } + if info.Size() > maxTestFileBytes { + return "", tooLarge("worktree version", strconv.FormatInt(info.Size(), 10)) + } + content, err := os.ReadFile(full) + if err != nil { + return "", "could not be read: " + err.Error() + } + if len(content) > maxTestFileBytes { + return "", tooLarge("worktree version", strconv.Itoa(len(content))) + } + return string(content), "" +} + +// runnerSkips is a dependency tree the test runners themselves never +// collect from: jest ignores node_modules, `go test ./...` skips vendor, +// pytest skips virtualenvs. Ignored files there are installed packages, not +// the agent's tests, and their own skips are not this change's. +func runnerSkips(file string) bool { + segments := strings.Split(file, "/") + for _, segment := range segments[:len(segments)-1] { + switch { + case segment == "node_modules": + return true + case segment == "vendor" && strings.HasSuffix(file, ".go"): + return true + case (segment == "venv" || segment == ".venv" || segment == "site-packages") && + strings.HasSuffix(file, ".py"): + return true + } + } + return false +} + +// judgeUntracked reads every untracked test file, gitignored ones included — +// a .gitignore entry hides a file from git, not from the runner — as all +// added. Embedded repositories, which git lists as one opaque directory, +// fail when they hold test files. +func (j *intactJudge) judgeUntracked() bool { + listed, ok := j.git("git ls-files", "ls-files", "-z", "--others") + if !ok { + return false + } + for _, name := range strings.Split(listed, "\x00") { + switch { + case name == "": + case strings.HasSuffix(name, "/"): + j.judgeEmbedded(strings.TrimSuffix(name, "/")) + case !testPathPattern.MatchString(name) || runnerSkips(name) || j.exempt(name): + default: + content, problem := readTestFile(j.gc.worktree, name) + if problem != "" { + j.report.Check(name, false, "untracked test file "+problem) + continue + } + file := &fileDiff{path: name, created: true} + for i, text := range strings.Split(content, "\n") { + file.added = append(file.added, diffLine{i + 1, text}) + } + j.compare(name, "", content, file) + } + } + return true +} + +// judgeEmbedded fails an embedded git repository holding test files: git +// shows it as one entry and never its contents, so changes in it are +// invisible to the diff. +func (j *intactJudge) judgeEmbedded(dir string) { + root, ok := artifactPath(j.gc.worktree, filepath.FromSlash(dir)) + if !ok { + return + } + var found []string + _ = filepath.WalkDir(root, func(full string, entry fs.DirEntry, err error) error { + if err != nil { + return nil + } + rel, relErr := filepath.Rel(j.gc.worktree, full) + if relErr != nil { + return nil + } + rel = filepath.ToSlash(rel) + if entry.IsDir() { + if entry.Name() == ".git" { + return filepath.SkipDir + } + return nil + } + if testPathPattern.MatchString(rel) && !runnerSkips(rel) && allowedBy(rel, j.gc.allow) == "" { + found = append(found, rel) + } + return nil + }) + if len(found) == 0 { + return + } + j.checked++ + j.report.Check(dir+"/", false, fmt.Sprintf( + "embedded git repository holds %d test file(s) (%s) that git cannot show this gate; "+ + "remove its .git directory or move the tests into the repository", len(found), found[0])) +} + +// judgeIndexFlags fails a tracked test file marked assume-unchanged or +// skip-worktree: git diff then reports it unchanged whatever its contents. +func (j *intactJudge) judgeIndexFlags() bool { + listed, ok := j.git("git ls-files -v", "ls-files", "-v", "-z") + if !ok { + return false + } + for _, entry := range strings.Split(listed, "\x00") { + if len(entry) < 3 { + continue + } + tag, name := entry[0], entry[2:] + if !(tag == 'S' || unicode.IsLower(rune(tag))) || !testPathPattern.MatchString(name) || j.exempt(name) { + continue + } + j.report.Check(name, false, "index flag assume-unchanged or skip-worktree hides this test file's changes "+ + "from git; clear it (git update-index --no-assume-unchanged --no-skip-worktree "+name+")") + } + return true +} + +// compare judges one test file, base content against current. file supplies +// the raw changed lines, used only to put a line number on a finding. +func (j *intactJudge) compare(name, before, after string, file *fileDiff) { + lang := testLanguageOf(name) + if lang == nil { + j.report.Check(name, true, "changed, not deleted") + return + } + failed := len(j.report.Violations()) + // Only source files are read as text: a binary fixture under tests/ is + // judged on deletion alone, above. + sides := []struct{ which, content string }{{"base", before}, {"worktree", after}} + for _, side := range sides { + if problem := uninspectableText(side.content); problem != "" { + j.report.Check(name, false, fmt.Sprintf( + "uninspectable: the %s version %s, so it cannot be read as source; "+ + "save it as UTF-8 text or name it in the tests_intact allow list", side.which, problem)) + return + } + } + old, oldOpen := lang.normalize(before) + now, nowOpen := lang.normalize(after) + for _, open := range []struct{ which, construct string }{{"base", oldOpen}, {"worktree", nowOpen}} { + if open.construct != "" { + j.report.Check(name, false, fmt.Sprintf( + "unterminated: in the %s version %s is still open at end of file, so the rest of the "+ + "file cannot be read; close it or name the file in the tests_intact allow list", + open.which, open.construct)) + return + } + } + for _, loss := range lang.losses { + was, is := len(loss.re.FindAllStringIndex(old, -1)), len(loss.re.FindAllStringIndex(now, -1)) + if is >= was { + continue + } + j.report.Check(findingItem(name, lineOf(lang, file.removed, loss.re)), false, fmt.Sprintf( + "net loss of %d %s (%d at base %s, %d now); %s", was-is, loss.what, was, j.base, is, loss.fix)) + } + oldKinds, nowKinds := lang.markerKinds(old, before), lang.markerKinds(now, after) + // One finding per line: `pytestmark = pytest.mark.skip` gains two kinds + // but is one thing to remove. + reported := map[int]bool{} + for _, kind := range sortedKeys(nowKinds) { + gained := nowKinds[kind] - oldKinds[kind] + if gained <= 0 { + continue + } + lines := markerLines(lang, file, kind) + for k := 0; k < gained; k++ { + shown, line := kind, diffLine{} + if k < len(lines) { + line = lines[k] + shown = strings.TrimSpace(line.text) + } + if line.line > 0 && reported[line.line] { + continue + } + reported[line.line] = true + j.report.Check(findingItem(name, line.line), false, fmt.Sprintf( + "adds a skip/focus marker (%s); remove it and make the test pass instead — "+ + "a skipped or excluded test checks nothing, and a focused one silences the rest of the suite", + shown)) + } + } + if len(j.report.Violations()) == failed { + j.report.Check(name, true, "no test or assertion lost, no skip added") + } +} + +func findingItem(name string, line int) string { + if line > 0 { + return fmt.Sprintf("%s:%d", name, line) + } + return name +} + +// lineOf is the first line whose own normalized text matches re, or 0. One +// line out of context normalizes imperfectly; that only costs the number. +func lineOf(lang *testLanguage, lines []diffLine, re *regexp.Regexp) int { + for _, line := range lines { + if re.MatchString(lang.normalizeLine(line.text)) { + return line.line + } + } + return 0 +} + +// markerLines lists added lines carrying a marker of kind, preferring those +// whose text the file did not also remove (a moved line is not the gain). +func markerLines(lang *testLanguage, file *fileDiff, kind string) []diffLine { + removed := map[string]int{} + for _, line := range file.removed { + if lang.lineKinds(line.text)[kind] > 0 { + removed[strings.TrimSpace(line.text)]++ + } + } + var fresh, moved []diffLine + for _, line := range file.added { + if lang.lineKinds(line.text)[kind] == 0 { + continue + } + text := strings.TrimSpace(line.text) + if removed[text] > 0 { + removed[text]-- + moved = append(moved, line) + continue + } + fresh = append(fresh, line) + } + return append(fresh, moved...) +} + +func sortedKeys(counts map[string]int) []string { + keys := make([]string, 0, len(counts)) + for key := range counts { + keys = append(keys, key) + } + sort.Strings(keys) + return keys +} + +// allowedBy returns the first allow glob matching file, or "". Globs were +// validated at save time, so a match error cannot occur here. +func allowedBy(file string, allow []string) string { + for _, glob := range allow { + if ok, _ := path.Match(glob, file); ok { + return glob + } + } + return "" +} diff --git a/internal/engine/tests_intact_test.go b/internal/engine/tests_intact_test.go new file mode 100644 index 0000000..03f83ff --- /dev/null +++ b/internal/engine/tests_intact_test.go @@ -0,0 +1,960 @@ +package engine + +import ( + "context" + "fmt" + "os" + "os/exec" + "path/filepath" + "sort" + "strings" + "testing" + + "github.com/StructuPath/jig/internal/protocol" +) + +const goTests = `package a + +import "testing" + +func TestOne(t *testing.T) { + if 1 != 1 { + t.Fatal("one") + } +} + +func TestTwo(t *testing.T) { + if 2 != 2 { + t.Errorf("two") + } +} +` + +const jsTests = `describe("a", () => { + it("one", () => { + expect(1).toBe(1); + }); + test("two", () => { + assert.equal(2, 2); + }); +}); +test.each([1, 2])("each %i", (n) => { + expect(n).toBe(n); +}); +` + +const goSkipped = `package a + +import "testing" + +func TestSkipped(t *testing.T) { + t.Skip("old reason") +} +` + +// jsRegexTests holds regex literals whose `/*` would open a comment to a +// lexer without a regex state, blinding the rest of the file on both sides. +const jsRegexTests = `const re = /^\/*/; +const cls = /[/*]/; +const arrow = () => /[/*]/; +it("matches", () => { + expect(re.test("x")).toBe(true); + expect(cls.test("*")).toBe(true); + expect(arrow().test("*")).toBe(true); +}); +` + +// goExamples holds the other functions `go test` runs, and a method that +// only looks like one. +const goExamples = `package a + +import "testing" + +type helper struct{} + +func ExampleOne() { + // Output: +} + +func FuzzOne(f *testing.F) { + f.Fuzz(func(t *testing.T, s string) {}) +} + +func (helper) ExampleMethod() {} +` + +// goOutputExamples holds examples whose expected output is their assertion. +const goOutputExamples = `package a + +import "fmt" + +func ExampleHello() { + fmt.Println("hello") + // Output: hello +} + +func ExampleOrder() { + fmt.Println("a") + fmt.Println("b") + // Unordered output: + // b + // a +} +` + +// rbRegexSpec holds a heredoc whose `=begin` line is text, not a block +// comment, and `#` inside regex literals, which is not a comment either. +const rbRegexSpec = `describe C do + it "reads a heredoc" do + text = <<-DOC +=begin + DOC + expect(text).to match(/#/) + expect(text).to match(%r{#}) + end + + it "still counts" do + expect(1).to eq(1) + end +end +` + +const goMain = `package a + +import ( + "os" + "testing" +) + +func TestMain(m *testing.M) { + os.Exit(m.Run()) +} +` + +const pyTests = `import pytest + + +def test_one(): + assert 1 == 1 + + +def test_two(): + assert 2 == 2 +` + +const pyUnittest = `import unittest + + +class T(unittest.TestCase): + def test_one(self): + self.assertEqual(1, 1) +` + +const rbTests = `describe A do + it "one" do + expect(1).to eq(1) + end + + it "two" do + 2.should eq(2) + end +end +` + +// testsIntactBase is every fixture file; each case starts from all of them +// committed at the pinned base. +var testsIntactBase = map[string]string{ + "pkg/a_test.go": goTests, + "pkg/example_test.go": goExamples, + "pkg/output_test.go": goOutputExamples, + "tests/fixtures/example.go": goExamples, + "pkg/skip_test.go": goSkipped, + "pkg/main_test.go": goMain, + "pkg/open_test.go": goTests + "/* never closed\n", + "pkg/nul_test.go": goTests + "\x00", + "src/re.test.ts": jsRegexTests, + "spec/re_spec.rb": rbRegexSpec, + "pkg/a.go": "package a\n", + "src/a.test.ts": jsTests, + "tests/test_a.py": pyTests, + "tests/test_unit.py": pyUnittest, + "spec/a_spec.rb": rbTests, + "src/helpers.py": "def helper():\n return 1\n", +} + +type intactEdit struct { + write map[string]string + remove []string +} + +func testsIntactRepo(t *testing.T) (string, string) { + t.Helper() + dir := t.TempDir() + for name, content := range testsIntactBase { + writeIntactFile(t, dir, name, content) + } + intactGit(t, dir, "init", "--quiet") + intactGit(t, dir, "add", "-A") + intactGit(t, dir, "-c", "user.email=test@jig.local", "-c", "user.name=jig test", + "commit", "--quiet", "-m", "base") + return dir, strings.TrimSpace(intactGit(t, dir, "rev-parse", "HEAD")) +} + +// intactGit runs a fixture git command with auto gc and auto maintenance +// off, so no detached git is still writing the repository's .git when +// TempDir's cleanup removes it. +func intactGit(t *testing.T, dir string, args ...string) string { + t.Helper() + command := exec.Command("git", append([]string{"-c", "gc.auto=0", "-c", "maintenance.auto=false"}, args...)...) + command.Dir = dir + output, err := command.CombinedOutput() + if err != nil { + t.Fatalf("git %v: %v: %s", args, err, output) + } + return string(output) +} + +func writeIntactFile(t *testing.T, dir, name, content string) { + t.Helper() + full := filepath.Join(dir, name) + if err := os.MkdirAll(filepath.Dir(full), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(full, []byte(content), 0o644); err != nil { + t.Fatal(err) + } +} + +func applyIntactEdit(t *testing.T, dir string, edit *intactEdit) { + t.Helper() + if edit == nil { + return + } + for name, content := range edit.write { + writeIntactFile(t, dir, name, content) + } + for _, name := range edit.remove { + if err := os.Remove(filepath.Join(dir, name)); err != nil { + t.Fatal(err) + } + } +} + +// intactViolations reduces a report's failed checks to "path kind" pairs, +// so a case states WHICH file lost WHAT without pinning git's hunk choice. +func intactViolations(report protocol.GateReport) []string { + var out []string + for _, check := range report.Checks { + if check.Ok { + continue + } + file, _, _ := strings.Cut(check.Item, ":") + kind := "?" + for _, candidate := range []string{"unterminated", "uninspectable", "too large", "embedded", "index flag", "m.Run", + "deleted", "test function", "assertion", "skip"} { + if strings.Contains(check.Note, candidate) { + kind = candidate + break + } + } + out = append(out, file+" "+kind) + } + sort.Strings(out) + return out +} + +func TestTestsIntactFlagsWeakenedTestsAndPassesHonestChanges(t *testing.T) { + replace := func(name, old, new string) map[string]string { + return map[string]string{name: strings.Replace(testsIntactBase[name], old, new, 1)} + } + cases := []struct { + name string + committed *intactEdit + uncommitted *intactEdit + allow []string + want []string + }{ + // (a) deletion, seen whether or not it was committed. + {name: "an uncommitted deleted test file", + uncommitted: &intactEdit{remove: []string{"pkg/a_test.go"}}, + want: []string{"pkg/a_test.go deleted"}}, + {name: "a committed deleted test file", + committed: &intactEdit{remove: []string{"tests/test_a.py"}}, + want: []string{"tests/test_a.py deleted"}}, + // (b) net loss of test functions — a lost test takes its assertion. + {name: "a Go test removed", + uncommitted: &intactEdit{write: replace("pkg/a_test.go", + "\nfunc TestTwo(t *testing.T) {\n\tif 2 != 2 {\n\t\tt.Errorf(\"two\")\n\t}\n}\n", "")}, + want: []string{"pkg/a_test.go assertion", "pkg/a_test.go test function"}}, + {name: "a Go test commented out", + uncommitted: &intactEdit{write: replace("pkg/a_test.go", + "func TestTwo(t *testing.T) {\n\tif 2 != 2 {\n\t\tt.Errorf(\"two\")\n\t}\n}\n", + "// func TestTwo(t *testing.T) {\n// \tif 2 != 2 {\n// \t\tt.Errorf(\"two\")\n// \t}\n// }\n")}, + want: []string{"pkg/a_test.go assertion", "pkg/a_test.go test function"}}, + {name: "a Python test removed", + committed: &intactEdit{write: replace("tests/test_a.py", "\n\ndef test_two():\n assert 2 == 2\n", "")}, + want: []string{"tests/test_a.py assertion", "tests/test_a.py test function"}}, + {name: "an RSpec example turned into xit", + uncommitted: &intactEdit{write: replace("spec/a_spec.rb", ` it "two" do`, ` xit "two" do`)}, + want: []string{"spec/a_spec.rb skip", "spec/a_spec.rb test function"}}, + // (c) net loss of assertions. + {name: "a JS expect removed", + uncommitted: &intactEdit{write: replace("src/a.test.ts", " expect(1).toBe(1);\n", "")}, + want: []string{"src/a.test.ts assertion"}}, + {name: "a unittest assertion removed", + uncommitted: &intactEdit{write: replace("tests/test_unit.py", " self.assertEqual(1, 1)\n", " pass\n")}, + want: []string{"tests/test_unit.py assertion"}}, + {name: "an RSpec expectation removed", + uncommitted: &intactEdit{write: replace("spec/a_spec.rb", " expect(1).to eq(1)\n", "")}, + want: []string{"spec/a_spec.rb assertion"}}, + // (d) added skip and focus markers. + {name: "a Go t.Skip added", + uncommitted: &intactEdit{write: replace("pkg/a_test.go", + "func TestOne(t *testing.T) {\n", "func TestOne(t *testing.T) {\n\tt.Skip(\"flaky\")\n")}, + want: []string{"pkg/a_test.go skip"}}, + {name: "a JS describe.skip", + uncommitted: &intactEdit{write: replace("src/a.test.ts", `describe("a"`, `describe.skip("a"`)}, + want: []string{"src/a.test.ts skip"}}, + // A focused test is still a test: the focus is the finding. + {name: "a JS test.only", + uncommitted: &intactEdit{write: replace("src/a.test.ts", ` test("two"`, ` test.only("two"`)}, + want: []string{"src/a.test.ts skip"}}, + {name: "a pytest skip decorator", + committed: &intactEdit{write: replace("tests/test_a.py", + "def test_two():", "@pytest.mark.skip(reason=\"flaky\")\ndef test_two():")}, + want: []string{"tests/test_a.py skip"}}, + {name: "an RSpec pending", + uncommitted: &intactEdit{write: replace("spec/a_spec.rb", + " expect(1).to eq(1)\n", " pending \"flaky\"\n expect(1).to eq(1)\n")}, + want: []string{"spec/a_spec.rb skip"}}, + // Parameterized JS focus and skip, anywhere in the chain. + {name: "a JS test.only.each", + uncommitted: &intactEdit{write: replace("src/a.test.ts", "test.each([1, 2])", "test.only.each([1, 2])")}, + want: []string{"src/a.test.ts skip"}}, + {name: "a JS test.skip.each", + uncommitted: &intactEdit{write: replace("src/a.test.ts", "test.each([1, 2])", "test.skip.each([1, 2])")}, + want: []string{"src/a.test.ts skip"}}, + {name: "a JS it.only.each", + uncommitted: &intactEdit{write: replace("src/a.test.ts", ` it("one"`, ` it.only.each([1])("one"`)}, + want: []string{"src/a.test.ts skip"}}, + {name: "a JS describe.only.each", + uncommitted: &intactEdit{write: replace("src/a.test.ts", `describe("a"`, `describe.only.each([1])("a"`)}, + want: []string{"src/a.test.ts skip"}}, + {name: "a JS describe.skip.each", + uncommitted: &intactEdit{write: replace("src/a.test.ts", `describe("a"`, `describe.skip.each([1])("a"`)}, + want: []string{"src/a.test.ts skip"}}, + // Markers that drop a file or end the run, not just skip one test. + {name: "a go:build ignore constraint", + uncommitted: &intactEdit{write: map[string]string{"pkg/a_test.go": "//go:build ignore\n\n" + goTests}}, + want: []string{"pkg/a_test.go skip"}}, + {name: "a legacy +build constraint", + uncommitted: &intactEdit{write: map[string]string{"pkg/a_test.go": "// +build ignore\n\n" + goTests}}, + want: []string{"pkg/a_test.go skip"}}, + {name: "an added TestMain", + uncommitted: &intactEdit{write: map[string]string{"pkg/a_test.go": goTests + "\nfunc TestMain(m *testing.M) {}\n"}}, + want: []string{"pkg/a_test.go skip"}}, + {name: "an added os.Exit", + uncommitted: &intactEdit{write: replace("pkg/a_test.go", + "func TestOne(t *testing.T) {\n", "func TestOne(t *testing.T) {\n\tos.Exit(0)\n")}, + want: []string{"pkg/a_test.go skip"}}, + {name: "a module-level pytestmark", + uncommitted: &intactEdit{write: replace("tests/test_a.py", + "import pytest\n", "import pytest\n\npytestmark = pytest.mark.skip\n")}, + want: []string{"tests/test_a.py skip"}}, + {name: "a parenthesized pytest skipif", + uncommitted: &intactEdit{write: replace("tests/test_a.py", + "def test_two():", "@(pytest.mark.skipif(True, reason=\"x\"))\ndef test_two():")}, + want: []string{"tests/test_a.py skip"}}, + {name: "an aliased pytest mark", + uncommitted: &intactEdit{write: replace("tests/test_a.py", + "import pytest\n", "import pytest\nfrom pytest import mark\nskip_it = mark.skip\n")}, + want: []string{"tests/test_a.py skip"}}, + {name: "an xfail through pytest.param", + uncommitted: &intactEdit{write: replace("tests/test_a.py", "def test_two():", + "@pytest.mark.parametrize(\"n\", [pytest.param(1, marks=pytest.mark.xfail)])\ndef test_two():")}, + want: []string{"tests/test_a.py skip"}}, + {name: "a committed conftest collect_ignore", + committed: &intactEdit{write: map[string]string{"conftest.py": "collect_ignore = [\"tests/test_a.py\"]\n"}}, + want: []string{"conftest.py skip"}}, + {name: "an RSpec inline skip after do;", + uncommitted: &intactEdit{write: replace("spec/a_spec.rb", "\nend\n", "\n it \"three\" do; skip; end\nend\n")}, + want: []string{"spec/a_spec.rb skip"}}, + {name: "an RSpec skip in a one-line block", + uncommitted: &intactEdit{write: replace("spec/a_spec.rb", "\nend\n", "\n it(\"four\") { skip }\nend\n")}, + want: []string{"spec/a_spec.rb skip"}}, + // Untracked files: git diff never shows them. + {name: "an untracked test file with a build constraint", + uncommitted: &intactEdit{write: map[string]string{"pkg/c_test.go": "//go:build ignore\n\n" + goTests}}, + want: []string{"pkg/c_test.go skip"}}, + {name: "an untracked conftest collect_ignore", + uncommitted: &intactEdit{write: map[string]string{"tests/conftest.py": "collect_ignore = [\"test_a.py\"]\n"}}, + want: []string{"tests/conftest.py skip"}}, + {name: "an untracked test file without markers", + uncommitted: &intactEdit{write: map[string]string{"pkg/c_test.go": goTests, "src/c.spec.js": jsTests}}}, + {name: "an untracked test file the allow list covers", + uncommitted: &intactEdit{write: map[string]string{"pkg/c_test.go": "//go:build ignore\n\n" + goTests}}, + allow: []string{"pkg/c_test.go"}}, + // A -diff attribute must not hide a weakened file as binary. + {name: "an untracked -diff attribute", + uncommitted: &intactEdit{write: map[string]string{ + ".gitattributes": "*_test.go -diff\n", + "pkg/a_test.go": strings.Replace(goTests, "\t\tt.Fatal(\"one\")\n", "", 1)}}, + want: []string{"pkg/a_test.go assertion"}}, + {name: "a committed -diff attribute", + committed: &intactEdit{write: map[string]string{ + ".gitattributes": "*.ts binary\n", + "src/a.test.ts": strings.Replace(jsTests, " expect(1).toBe(1);\n", "", 1)}}, + want: []string{"src/a.test.ts assertion"}}, + // A reworded skip is the same skip. + {name: "a skip message edited", + uncommitted: &intactEdit{write: replace("pkg/skip_test.go", "old reason", "new reason")}}, + {name: "a skip moved to another test", + uncommitted: &intactEdit{write: map[string]string{"pkg/skip_test.go": strings.Replace(goSkipped, + "\tt.Skip(\"old reason\")\n}\n", "}\n\nfunc TestOther(t *testing.T) {\n\tt.Skip(\"other reason\")\n}\n", 1)}}}, + // Whole-file comparison: block comments, docstrings, and tokens + // split across lines. + {name: "a Go test wrapped in a block comment", + uncommitted: &intactEdit{write: map[string]string{"pkg/a_test.go": strings.Replace(goTests, + "func TestTwo", "/*\nfunc TestTwo", 1) + "*/\n"}}, + want: []string{"pkg/a_test.go assertion", "pkg/a_test.go test function"}}, + {name: "a skip after a closing block comment", + uncommitted: &intactEdit{write: replace("pkg/a_test.go", + "func TestOne(t *testing.T) {\n", "func TestOne(t *testing.T) {\n\t/* note */ t.Skip()\n")}, + want: []string{"pkg/a_test.go skip"}}, + {name: "a Go skip split across lines", + uncommitted: &intactEdit{write: replace("pkg/a_test.go", + "func TestOne(t *testing.T) {\n", "func TestOne(t *testing.T) {\n\tt.\n\t\tSkip(\"x\")\n")}, + want: []string{"pkg/a_test.go skip"}}, + {name: "a JS only split across lines", + uncommitted: &intactEdit{write: replace("src/a.test.ts", ` test("two"`, " test\n .only(\"two\"")}, + want: []string{"src/a.test.ts skip"}}, + {name: "a JS test converted to test.each", + uncommitted: &intactEdit{write: replace("src/a.test.ts", ` test("two", () => {`, ` test.each([2])("two", (n) => {`)}}, + {name: "a Python test wrapped in a string", + uncommitted: &intactEdit{write: replace("tests/test_a.py", + "def test_two():\n assert 2 == 2\n", "\"\"\"\ndef test_two():\n assert 2 == 2\n\"\"\"\n")}, + want: []string{"tests/test_a.py assertion", "tests/test_a.py test function"}}, + {name: "an RSpec example in =begin/=end", + uncommitted: &intactEdit{write: replace("spec/a_spec.rb", + " it \"two\" do\n 2.should eq(2)\n end\n", "=begin\n it \"two\" do\n 2.should eq(2)\n end\n=end\n")}, + want: []string{"spec/a_spec.rb assertion", "spec/a_spec.rb test function"}}, + // TestMain must keep running the suite. + {name: "os.Exit(m.Run()) replaced by os.Exit(0)", + uncommitted: &intactEdit{write: replace("pkg/main_test.go", "os.Exit(m.Run())", "os.Exit(0)")}, + want: []string{"pkg/main_test.go m.Run"}}, + // More Python and RSpec markers. + {name: "a pytest.importorskip", + uncommitted: &intactEdit{write: replace("tests/test_a.py", + "import pytest\n", "import pytest\n\nnp = pytest.importorskip(\"numpy\")\n")}, + want: []string{"tests/test_a.py skip"}}, + {name: "a module-level __test__ = False", + uncommitted: &intactEdit{write: replace("tests/test_a.py", "import pytest\n", "import pytest\n\n__test__ = False\n")}, + want: []string{"tests/test_a.py skip"}}, + {name: "an annotated pytestmark", + uncommitted: &intactEdit{write: replace("tests/test_a.py", + "import pytest\n", "import pytest\n\npytestmark: list = [marker]\n")}, + want: []string{"tests/test_a.py skip"}}, + {name: "a mark imported under an alias", + uncommitted: &intactEdit{write: replace("tests/test_a.py", + "import pytest\n", "import pytest\nfrom pytest import mark as m\n")}, + want: []string{"tests/test_a.py skip"}}, + {name: "a conftest pytest_ignore_collect", + uncommitted: &intactEdit{write: map[string]string{"tests/conftest.py": "def pytest_ignore_collect(collection_path):\n return True\n"}}, + want: []string{"tests/conftest.py skip"}}, + {name: "a conftest pytest_collection_modifyitems", + committed: &intactEdit{write: map[string]string{"conftest.py": "def pytest_collection_modifyitems(items):\n items.clear()\n"}}, + want: []string{"conftest.py skip"}}, + {name: "RSpec skip: true metadata", + uncommitted: &intactEdit{write: replace("spec/a_spec.rb", ` it "two" do`, ` it "two", skip: true do`)}, + want: []string{"spec/a_spec.rb skip"}}, + {name: "RSpec :skip metadata", + uncommitted: &intactEdit{write: replace("spec/a_spec.rb", ` it "two" do`, ` it "two", :skip do`)}, + want: []string{"spec/a_spec.rb skip"}}, + {name: "RSpec pending: true metadata", + uncommitted: &intactEdit{write: replace("spec/a_spec.rb", ` it "two" do`, ` it "two", pending: true do`)}, + want: []string{"spec/a_spec.rb skip"}}, + {name: "RSpec focus: true metadata", + uncommitted: &intactEdit{write: replace("spec/a_spec.rb", `describe A do`, `describe A, focus: true do`)}, + want: []string{"spec/a_spec.rb skip"}}, + // Gitignored files still run; installed dependencies are not ours. + {name: "a gitignored untracked test file with a marker", + uncommitted: &intactEdit{write: map[string]string{ + ".gitignore": "pkg/hidden_test.go\n", + "pkg/hidden_test.go": "//go:build ignore\n\n" + goTests}}, + want: []string{"pkg/hidden_test.go skip"}}, + {name: "a focused test inside node_modules", + uncommitted: &intactEdit{write: map[string]string{ + ".gitignore": "node_modules/\n", + "node_modules/dep/a.test.js": "describe.only(\"dep\", () => {});\n"}}}, + // Regex literals, heredocs, and %-literals are not comments. + {name: "an assertion removed after a JS regex holding /*", + uncommitted: &intactEdit{write: replace("src/re.test.ts", " expect(cls.test(\"*\")).toBe(true);\n", "")}, + want: []string{"src/re.test.ts assertion"}}, + {name: "an assertion removed after an arrow-returned regex holding /*", + uncommitted: &intactEdit{write: replace("src/re.test.ts", " expect(arrow().test(\"*\")).toBe(true);\n", "")}, + want: []string{"src/re.test.ts assertion"}}, + {name: "an assertion removed after a Ruby heredoc holding =begin", + uncommitted: &intactEdit{write: replace("spec/re_spec.rb", " expect(1).to eq(1)\n", "")}, + want: []string{"spec/re_spec.rb assertion"}}, + {name: "a pending hidden after # in a Ruby regex", + uncommitted: &intactEdit{write: replace("spec/a_spec.rb", + " expect(1).to eq(1)\n", " expect(\"#\").to match(/#/); pending\n expect(1).to eq(1)\n")}, + want: []string{"spec/a_spec.rb skip"}}, + {name: "a pending hidden after # in a Ruby %r literal", + uncommitted: &intactEdit{write: replace("spec/a_spec.rb", + " expect(1).to eq(1)\n", " expect(\"#\").to match(%r{#}); pending\n expect(1).to eq(1)\n")}, + want: []string{"spec/a_spec.rb skip"}}, + {name: "a Go block comment left open", + uncommitted: &intactEdit{write: map[string]string{"pkg/a_test.go": goTests + "/*\n"}}, + want: []string{"pkg/a_test.go unterminated"}}, + {name: "a JS template literal left open", + uncommitted: &intactEdit{write: map[string]string{"src/a.test.ts": jsTests + "const s = `open\n"}}, + want: []string{"src/a.test.ts unterminated"}}, + {name: "a Ruby heredoc left open", + uncommitted: &intactEdit{write: map[string]string{"spec/a_spec.rb": rbTests + "text = <<~EOS\nnever ends\n"}}, + want: []string{"spec/a_spec.rb unterminated"}}, + {name: "a base version left open", + uncommitted: &intactEdit{write: map[string]string{"pkg/open_test.go": goTests}}, + want: []string{"pkg/open_test.go unterminated"}}, + // Bare unittest skips. + {name: "a bare @skip from unittest", + uncommitted: &intactEdit{write: replace("tests/test_unit.py", + "import unittest\n", "import unittest\nfrom unittest import skip\n")}}, + {name: "a bare @skip decorator", + uncommitted: &intactEdit{write: replace("tests/test_unit.py", + " def test_one(self):", " @skip(\"flaky\")\n def test_one(self):")}, + want: []string{"tests/test_unit.py skip"}}, + {name: "a bare @skipIf decorator", + uncommitted: &intactEdit{write: replace("tests/test_unit.py", + " def test_one(self):", " @skipIf(True, \"flaky\")\n def test_one(self):")}, + want: []string{"tests/test_unit.py skip"}}, + {name: "a bare @skipUnless decorator", + uncommitted: &intactEdit{write: replace("tests/test_unit.py", + " def test_one(self):", " @skipUnless(False, \"flaky\")\n def test_one(self):")}, + want: []string{"tests/test_unit.py skip"}}, + {name: "a bare @expectedFailure decorator", + uncommitted: &intactEdit{write: replace("tests/test_unit.py", + " def test_one(self):", " @expectedFailure\n def test_one(self):")}, + want: []string{"tests/test_unit.py skip"}}, + {name: "raise unittest.SkipTest", + uncommitted: &intactEdit{write: replace("tests/test_unit.py", + " self.assertEqual(1, 1)\n", " raise unittest.SkipTest(\"x\")\n self.assertEqual(1, 1)\n")}, + want: []string{"tests/test_unit.py skip"}}, + {name: "raise a bare SkipTest", + uncommitted: &intactEdit{write: replace("tests/test_unit.py", + " self.assertEqual(1, 1)\n", " raise SkipTest(\"x\")\n self.assertEqual(1, 1)\n")}, + want: []string{"tests/test_unit.py skip"}}, + {name: "a unittest skip imported under an alias", + uncommitted: &intactEdit{write: replace("tests/test_unit.py", + "import unittest\n", "import unittest\nfrom unittest import skip as later\n")}, + want: []string{"tests/test_unit.py skip"}}, + // vitest chain modifiers. + {name: "a vitest test.skipIf", + uncommitted: &intactEdit{write: replace("src/a.test.ts", ` test("two"`, ` test.skipIf(true)("two"`)}, + want: []string{"src/a.test.ts skip"}}, + {name: "a vitest it.runIf", + uncommitted: &intactEdit{write: replace("src/a.test.ts", ` it("one"`, ` it.runIf(false)("one"`)}, + want: []string{"src/a.test.ts skip"}}, + {name: "a vitest test.fails", + uncommitted: &intactEdit{write: replace("src/a.test.ts", ` test("two"`, ` test.fails("two"`)}, + want: []string{"src/a.test.ts skip"}}, + {name: "a vitest it.concurrent.only", + uncommitted: &intactEdit{write: replace("src/a.test.ts", ` it("one"`, ` it.concurrent.only("one"`)}, + want: []string{"src/a.test.ts skip"}}, + // vitest options objects: the same markers as the chain, and still a test. + {name: "a vitest skip: true option", + uncommitted: &intactEdit{write: replace("src/a.test.ts", ` test("two", () => {`, ` test("two", { skip: true }, () => {`)}, + want: []string{"src/a.test.ts skip"}}, + {name: "a vitest only: true option", + uncommitted: &intactEdit{write: replace("src/a.test.ts", ` it("one", () => {`, ` it("one", { only: true }, () => {`)}, + want: []string{"src/a.test.ts skip"}}, + {name: "a vitest fails: true option", + uncommitted: &intactEdit{write: replace("src/a.test.ts", ` test("two", () => {`, ` test("two", { fails: true }, () => {`)}, + want: []string{"src/a.test.ts skip"}}, + {name: "a vitest todo: true option", + uncommitted: &intactEdit{write: replace("src/a.test.ts", ` test("two", () => {`, ` test("two", { todo: true }, () => {`)}, + want: []string{"src/a.test.ts skip"}}, + {name: "a vitest only beside concurrent", + uncommitted: &intactEdit{write: replace("src/a.test.ts", ` test("two", () => {`, + ` test("two", { concurrent: true, only: true }, () => {`)}, + want: []string{"src/a.test.ts skip"}}, + {name: "a vitest skip option on describe", + uncommitted: &intactEdit{write: replace("src/a.test.ts", `describe("a", () => {`, `describe("a", { skip: true }, () => {`)}, + want: []string{"src/a.test.ts skip"}}, + {name: "a vitest options object without a marker", + uncommitted: &intactEdit{write: replace("src/a.test.ts", ` test("two", () => {`, + ` test("two", { timeout: 5, skip: false }, () => {`)}}, + {name: "skip: true inside a test name", + uncommitted: &intactEdit{write: replace("src/a.test.ts", ` test("two", () => {`, ` test("two, { skip: true }", () => {`)}}, + // Go examples and fuzz targets are tests in a _test.go file only. + {name: "a Go Example removed", + uncommitted: &intactEdit{write: replace("pkg/example_test.go", "func ExampleOne() {\n\t// Output:\n}\n", "")}, + want: []string{"pkg/example_test.go assertion", "pkg/example_test.go test function"}}, + {name: "a Go fuzz target removed", + uncommitted: &intactEdit{write: replace("pkg/example_test.go", + "func FuzzOne(f *testing.F) {\n\tf.Fuzz(func(t *testing.T, s string) {})\n}\n", "")}, + want: []string{"pkg/example_test.go test function"}}, + {name: "a method named like an Example removed", + uncommitted: &intactEdit{write: replace("pkg/example_test.go", "func (helper) ExampleMethod() {}\n", "")}}, + {name: "an Example removed from a Go file that is not _test.go", + uncommitted: &intactEdit{write: replace("tests/fixtures/example.go", "func ExampleOne() {\n\t// Output:\n}\n", "")}}, + // NUL bytes and wide encodings cannot be read as source. + {name: "a NUL byte in a tracked worktree file", + uncommitted: &intactEdit{write: map[string]string{"pkg/a_test.go": goTests + "\x00"}}, + want: []string{"pkg/a_test.go uninspectable"}}, + {name: "a NUL byte in the base version", + uncommitted: &intactEdit{write: map[string]string{"pkg/nul_test.go": goTests}}, + want: []string{"pkg/nul_test.go uninspectable"}}, + {name: "a NUL byte in an untracked test file", + uncommitted: &intactEdit{write: map[string]string{"pkg/z_test.go": "package a\n\x00func TestZ(t *testing.T) { t.Skip() }\n"}}, + want: []string{"pkg/z_test.go uninspectable"}}, + {name: "a UTF-16 byte-order mark", + uncommitted: &intactEdit{write: map[string]string{"pkg/w_test.go": "\xFF\xFEpackage a\n"}}, + want: []string{"pkg/w_test.go uninspectable"}}, + {name: "a binary fixture under tests/", + uncommitted: &intactEdit{write: map[string]string{"tests/fixtures/image.bin": "\x89PNG\x00\x01"}}}, + {name: "committed and uncommitted losses are both seen", + committed: &intactEdit{write: replace("src/a.test.ts", " expect(1).toBe(1);\n", "")}, + uncommitted: &intactEdit{remove: []string{"spec/a_spec.rb"}}, + want: []string{"spec/a_spec.rb deleted", "src/a.test.ts assertion"}}, + // Renames: --no-renames makes the old path a deletion. + {name: "a renamed test file is a deletion", + committed: &intactEdit{remove: []string{"pkg/a_test.go"}, + write: map[string]string{"pkg/b_test.go": goTests}}, + want: []string{"pkg/a_test.go deleted"}}, + {name: "a renamed test file the allow list covers", + committed: &intactEdit{remove: []string{"pkg/a_test.go"}, + write: map[string]string{"pkg/b_test.go": goTests}}, + allow: []string{"pkg/a_test.go"}}, + {name: "allow exempts every loss in a matching path", + uncommitted: &intactEdit{remove: []string{"tests/test_a.py"}, + write: replace("tests/test_unit.py", " self.assertEqual(1, 1)\n", "")}, + allow: []string{"tests/test_*.py"}}, + {name: "allow exempts only what it matches", + uncommitted: &intactEdit{remove: []string{"tests/test_a.py", "spec/a_spec.rb"}}, + allow: []string{"tests/*"}, + want: []string{"spec/a_spec.rb deleted"}}, + // Honest changes pass. + {name: "an assertion replaced with a stronger one", + uncommitted: &intactEdit{write: replace("pkg/a_test.go", + "\tif 1 != 1 {\n\t\tt.Fatal(\"one\")\n\t}\n", "\trequire.Equal(t, 1, 1)\n")}}, + {name: "a test renamed within its file", + uncommitted: &intactEdit{write: replace("tests/test_a.py", "def test_two():", "def test_two_is_two():")}}, + {name: "a new test file", + committed: &intactEdit{write: map[string]string{"pkg/c_test.go": goTests, "src/c.spec.js": jsTests}}}, + {name: "new tests appended", + uncommitted: &intactEdit{write: map[string]string{"pkg/a_test.go": goTests + + "\nfunc TestThree(t *testing.T) {\n\tif 3 != 3 {\n\t\tt.Fatal(\"three\")\n\t}\n}\n"}}}, + {name: "a skip marker in a non-test file", + uncommitted: &intactEdit{write: map[string]string{ + "pkg/a.go": "package a\n\nfunc helper(t T) { t.Skip() }\n", + "src/helpers.py": "@pytest.mark.skip\ndef helper():\n return 1\n"}}}, + {name: "a commented-out skip marker", + uncommitted: &intactEdit{write: replace("pkg/a_test.go", + "func TestOne(t *testing.T) {\n", "func TestOne(t *testing.T) {\n\t// t.Skip(\"flaky\")\n")}}, + {name: "nothing changed"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + dir, base := testsIntactRepo(t) + if tc.committed != nil { + applyIntactEdit(t, dir, tc.committed) + intactGit(t, dir, "add", "-A") + intactGit(t, dir, "-c", "user.email=test@jig.local", "-c", "user.name=jig test", + "commit", "--quiet", "-m", "agent") + } + applyIntactEdit(t, dir, tc.uncommitted) + report := gateTestsIntact(gateContext{ctx: context.Background(), worktree: dir, + baseSHA: base, allow: tc.allow}) + got := intactViolations(report) + if strings.Join(got, "\n") != strings.Join(tc.want, "\n") { + t.Fatalf("violations = %q, want %q\nchecks: %+v", got, tc.want, report.Checks) + } + if len(report.Checks) == 0 { + t.Fatal("a green gate must still record what it checked (R9)") + } + }) + } +} + +// A `/` after `=>` opens a regex, so its `/*` is no comment; after a +// comparison's operand it is still division. +func TestTestsIntactLexesRegexAfterArrow(t *testing.T) { + cases := []struct{ name, src, want string }{ + {"an arrow-returned regex holding /*", "const re = () => /[/*]/;\n", `const re=()=>"";`}, + {"an arrow-returned regex in a call", "xs.map(x => /a*/.test(x));\n", `xs.map(x=>"".test(x));`}, + {"a comparison followed by division", "x = a > b / c / d;\n", `x=a>b/c/d;`}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got, open := langJS.normalize(tc.src) + if open != "" || got != tc.want { + t.Fatalf("normalize = %q (open %q), want %q", got, open, tc.want) + } + }) + } +} + +// intactRunner commits the base fixtures once and judges each later edit +// against them; one file is rewritten per call, so cases run in sequence. +func intactRunner(t *testing.T) func(t *testing.T, edit map[string]string) []string { + dir, base := testsIntactRepo(t) + return func(t *testing.T, edit map[string]string) []string { + t.Helper() + for name := range testsIntactBase { + writeIntactFile(t, dir, name, testsIntactBase[name]) + } + applyIntactEdit(t, dir, &intactEdit{write: edit}) + return intactViolations(gateTestsIntact(gateContext{ctx: context.Background(), worktree: dir, baseSHA: base})) + } +} + +// A vitest options object stops a test wherever it sits in the argument +// list, whatever the first argument is, and whether its key is quoted. +func TestTestsIntactOptionsObjectMarkers(t *testing.T) { + run := intactRunner(t) + forms := []string{ + `%s("x", { %s: true }, fn);`, + `%s("x", fn, { %s: true });`, + `%s(makeName(), { %s: true }, fn);`, + `%s("x", { "%s": true }, fn);`, + `%s("x", { '%s': true }, fn);`, + `%s.each([1])("x %%i", { %s: true }, fn);`, + "%s(\"x\", {\n timeout: 5,\n %s: 1,\n}, () => {\n expect(1).toBe(1);\n});", + } + for _, call := range []string{"test", "it", "describe"} { + for _, kind := range []string{"skip", "only", "fails", "todo"} { + for _, form := range forms { + line := fmt.Sprintf(form, call, kind) + t.Run(line, func(t *testing.T) { + got := run(t, map[string]string{"src/a.test.ts": jsTests + line + "\n"}) + if want := []string{"src/a.test.ts skip"}; strings.Join(got, "\n") != strings.Join(want, "\n") { + t.Fatalf("violations = %q, want %q", got, want) + } + }) + } + } + } +} + +// A falsy option leaves the test running, and option syntax inside a string +// is text. +func TestTestsIntactOptionsObjectFalsyAndStrings(t *testing.T) { + run := intactRunner(t) + for _, line := range []string{ + `test("x", { skip: false }, fn);`, + `test("x", { skip: 0 }, fn);`, + `test("x", { skip: null }, fn);`, + `test("x", { skip: undefined }, fn);`, + `test("x", { skip: "" }, fn);`, + `test("x", { skip: '' }, fn);`, + `test("x", fn, { "only": false });`, + `test("x, { skip: true }", fn);`, + `test("x", fn, "{ skip: true }");`, + `test("x", { name: "skip: true" }, fn);`, + `it("x", () => { const o = { skip: true }; expect(o).toBeTruthy(); });`, + } { + t.Run(line, func(t *testing.T) { + if got := run(t, map[string]string{"src/a.test.ts": jsTests + line + "\n"}); len(got) > 0 { + t.Fatalf("violations = %q, want none", got) + } + }) + } +} + +// An example's `// Output:` line is its assertion: deleting it leaves an +// example that is compiled and never run. +func TestTestsIntactGoExampleOutput(t *testing.T) { + run := intactRunner(t) + replace := func(old, new string) map[string]string { + edited := strings.Replace(goOutputExamples, old, new, 1) + if edited == goOutputExamples { + t.Fatalf("%q is not in the fixture", old) + } + return map[string]string{"pkg/output_test.go": edited} + } + cases := []struct { + name string + edit map[string]string + want []string + }{ + {"an Output line deleted", replace("\t// Output: hello\n", ""), + []string{"pkg/output_test.go assertion"}}, + {"an Unordered output line deleted", replace("\t// Unordered output:\n", ""), + []string{"pkg/output_test.go assertion"}}, + {"an example with output turned into an empty test", + replace("func ExampleHello() {\n\tfmt.Println(\"hello\")\n\t// Output: hello\n}", + "func TestHello(t *testing.T) {}"), + []string{"pkg/output_test.go assertion"}}, + {"the expected output edited, its line kept", + replace("\tfmt.Println(\"hello\")\n\t// Output: hello\n", "\tfmt.Println(\"hello, world\")\n\t// Output: hello, world\n"), + nil}, + {"the unordered output reordered", replace("\t// b\n\t// a\n", "\t// a\n\t// b\n"), nil}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + if got := run(t, tc.edit); strings.Join(got, "\n") != strings.Join(tc.want, "\n") { + t.Fatalf("violations = %q, want %q", got, tc.want) + } + }) + } +} + +// Findings name the worktree line, so the correction points the agent at +// the spot; the note says what to do about it. +func TestTestsIntactFindingsCarryTheLineAndTheFix(t *testing.T) { + dir, base := testsIntactRepo(t) + writeIntactFile(t, dir, "pkg/a_test.go", strings.Replace(goTests, + "func TestTwo(t *testing.T) {\n", "func TestTwo(t *testing.T) {\n\tt.Skip(\"flaky\")\n", 1)) + writeIntactFile(t, dir, "src/a.test.ts", strings.Replace(jsTests, " expect(1).toBe(1);\n", "", 1)) + report := gateTestsIntact(gateContext{ctx: context.Background(), worktree: dir, baseSHA: base}) + want := map[string]string{ + "pkg/a_test.go:12": `adds a skip/focus marker (t.Skip("flaky"))`, + "src/a.test.ts:3": "net loss of 1 assertion(s) (3 at base " + shortSHA(base) + ", 2 now)", + } + for _, check := range report.Checks { + if check.Ok { + continue + } + note, ok := want[check.Item] + if !ok || !strings.Contains(check.Note, note) { + t.Fatalf("unexpected failed check %+v", check) + } + delete(want, check.Item) + } + if len(want) > 0 { + t.Fatalf("missing findings %v in %+v", want, report.Checks) + } +} + +// A repository's diff.interHunkContext would merge nearby hunks and fill the +// gap with context lines; the gate forces it off and counts context anyway, +// so the second finding still names its own line. +func TestTestsIntactLineNumbersSurviveInterHunkContext(t *testing.T) { + dir, base := testsIntactRepo(t) + intactGit(t, dir, "config", "diff.interHunkContext", "20") + skipped := strings.Replace(goTests, "func TestOne(t *testing.T) {\n", + "func TestOne(t *testing.T) {\n\tt.Skip(\"one\")\n", 1) + skipped = strings.Replace(skipped, "func TestTwo(t *testing.T) {\n", + "func TestTwo(t *testing.T) {\n\tt.Skip(\"two\")\n", 1) + writeIntactFile(t, dir, "pkg/a_test.go", skipped) + report := gateTestsIntact(gateContext{ctx: context.Background(), worktree: dir, baseSHA: base}) + var items []string + for _, check := range report.Checks { + if !check.Ok { + items = append(items, check.Item) + } + } + if want := []string{"pkg/a_test.go:6", "pkg/a_test.go:13"}; strings.Join(items, ",") != strings.Join(want, ",") { + t.Fatalf("failed items = %q, want %q", items, want) + } + // The parser alone, on a hunk that carries context: the counter must + // advance over it. + files := parseUnifiedDiff("diff --git a/x_test.go b/x_test.go\n--- a/x_test.go\n+++ b/x_test.go\n" + + "@@ -5,3 +5,4 @@\n func TestA(t *testing.T) {\n+\tt.Skip()\n \tt.Fatal(\"a\")\n }\n") + if len(files) != 1 || len(files[0].added) != 1 || files[0].added[0].line != 6 { + t.Fatalf("parsed %+v, want one added line at 6", files) + } +} + +// Should git ever report a test file as binary despite --text, the gate +// refuses to vouch for what it cannot read. +func TestTestsIntactFailsABinaryTestFile(t *testing.T) { + files := parseUnifiedDiff("diff --git a/pkg/a_test.go b/pkg/a_test.go\nindex 1111111..2222222 100644\n" + + "Binary files a/pkg/a_test.go and b/pkg/a_test.go differ\n") + var report protocol.GateReport + (&intactJudge{report: &report, base: "base"}).judgeDiffFile(files[0]) + if report.Passed() || report.Checks[0].Item != "pkg/a_test.go" || + !strings.Contains(report.Checks[0].Note, "binary") { + t.Fatalf("checks = %+v, want a failed binary check", report.Checks) + } + report = protocol.GateReport{} + (&intactJudge{gc: gateContext{allow: []string{"pkg/*"}}, report: &report, base: "base"}).judgeDiffFile(files[0]) + if !report.Passed() { + t.Fatalf("checks = %+v, want allow to exempt it", report.Checks) + } +} + +// What the gate cannot read fails closed: an oversized file (never +// truncated), an embedded repository git shows as one opaque entry, and a +// tracked file whose index flags make git diff report it unchanged. +func TestTestsIntactFailsClosedOnWhatItCannotInspect(t *testing.T) { + padding := strings.Repeat("// padding line\n", maxTestFileBytes/16+1) + cases := []struct { + name string + setup func(t *testing.T, dir string) + want []string + }{ + {"an oversized tracked test file", func(t *testing.T, dir string) { + writeIntactFile(t, dir, "pkg/a_test.go", goTests+padding+"func init() { _ = 1 }\n") + }, []string{"pkg/a_test.go too large"}}, + {"an oversized untracked test file", func(t *testing.T, dir string) { + writeIntactFile(t, dir, "pkg/big_test.go", goTests+padding) + }, []string{"pkg/big_test.go too large"}}, + {"an embedded repository holding tests", func(t *testing.T, dir string) { + writeIntactFile(t, dir, "sub/x_test.go", goTests) + intactGit(t, filepath.Join(dir, "sub"), "init", "--quiet") + }, []string{"sub/ embedded"}}, + {"skip-worktree on a weakened test file", func(t *testing.T, dir string) { + intactGit(t, dir, "update-index", "--skip-worktree", "pkg/a_test.go") + writeIntactFile(t, dir, "pkg/a_test.go", strings.Replace(goTests, "\t\tt.Fatal(\"one\")\n", "", 1)) + }, []string{"pkg/a_test.go index flag"}}, + {"assume-unchanged on a weakened test file", func(t *testing.T, dir string) { + intactGit(t, dir, "update-index", "--assume-unchanged", "tests/test_a.py") + writeIntactFile(t, dir, "tests/test_a.py", strings.Replace(pyTests, " assert 2 == 2\n", " pass\n", 1)) + }, []string{"tests/test_a.py index flag"}}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + dir, base := testsIntactRepo(t) + tc.setup(t, dir) + report := gateTestsIntact(gateContext{ctx: context.Background(), worktree: dir, baseSHA: base}) + got := intactViolations(report) + if strings.Join(got, "\n") != strings.Join(tc.want, "\n") { + t.Fatalf("violations = %q, want %q\nchecks: %+v", got, tc.want, report.Checks) + } + }) + } +} + +func TestTestsIntactFailsClosedWithoutABase(t *testing.T) { + dir, _ := testsIntactRepo(t) + cases := []struct { + name, base, item string + }{ + {"no pinned base", "", "base commit"}, + {"a base git cannot resolve", strings.Repeat("0", 40), "git diff"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + report := gateTestsIntact(gateContext{ctx: context.Background(), worktree: dir, baseSHA: tc.base}) + if report.Passed() || report.Checks[0].Item != tc.item { + t.Fatalf("checks = %+v, want a failed %q check", report.Checks, tc.item) + } + }) + } +} + +// A path with spaces or characters git quotes must still be classified by +// its real name, or a deletion would slip past as a non-test path. +func TestTestsIntactReadsQuotedAndSpacedPaths(t *testing.T) { + dir, base := testsIntactRepo(t) + for _, name := range []string{"pkg/with space_test.go", "pkg/tab\there_test.go"} { + writeIntactFile(t, dir, name, goTests) + } + intactGit(t, dir, "add", "-A") + intactGit(t, dir, "-c", "user.email=test@jig.local", "-c", "user.name=jig test", + "commit", "--quiet", "-m", "more") + base = strings.TrimSpace(intactGit(t, dir, "rev-parse", "HEAD")) + for _, name := range []string{"pkg/with space_test.go", "pkg/tab\there_test.go"} { + if err := os.Remove(filepath.Join(dir, name)); err != nil { + t.Fatal(err) + } + } + report := gateTestsIntact(gateContext{ctx: context.Background(), worktree: dir, baseSHA: base}) + got := intactViolations(report) + want := []string{"pkg/tab\there_test.go deleted", "pkg/with space_test.go deleted"} + if strings.Join(got, "\n") != strings.Join(want, "\n") { + t.Fatalf("violations = %q, want %q", got, want) + } +} diff --git a/internal/protocol/definition.go b/internal/protocol/definition.go index a1c1622..0679922 100644 --- a/internal/protocol/definition.go +++ b/internal/protocol/definition.go @@ -3,6 +3,7 @@ package protocol import ( "bytes" "fmt" + "path" "sort" "strings" "time" @@ -19,6 +20,7 @@ const ( GateDiffMatchesClaims = "diff_matches_claims" GateVerdictConsistent = "verdict_consistent" GateTestsPass = "tests_pass" + GateTestsIntact = "tests_intact" ) // CheckAllPhasesPassed is the built-in acceptance-predicate check that every @@ -32,6 +34,7 @@ var builtinGates = map[string]bool{ GateDiffMatchesClaims: true, GateVerdictConsistent: true, GateTestsPass: true, + GateTestsIntact: true, } // Repair-edge continuation and exhaustion policies (KTD2). @@ -265,11 +268,14 @@ type PhaseSpec struct { // GateSpec configures one built-in gate on a phase. Budget is the // gate-correction retry budget; zero means DefaultPhaseRetryBudget. Command -// is required by tests_pass and meaningless to every other gate. +// is required by tests_pass and meaningless to every other gate. Allow is +// tests_intact's list of path.Match globs naming test files a definition +// authorizes the agent to delete or weaken; it is rejected on any other gate. type GateSpec struct { - Name string `yaml:"name"` - Command string `yaml:"command"` - Budget int `yaml:"budget"` + Name string `yaml:"name"` + Command string `yaml:"command"` + Budget int `yaml:"budget"` + Allow []string `yaml:"allow"` } // RepairEdge is the one declared loop construct (KTD2): @@ -645,6 +651,9 @@ func (spec *DefinitionSpec) validatePhase(phase PhaseSpec, phases map[string]Pha if gate.Budget < 0 { return fmt.Errorf("phase %q: gate %q budget must not be negative", phase.Name, gate.Name) } + if err := validateGateAllow(gate); err != nil { + return fmt.Errorf("phase %q: gate %q: %w", phase.Name, gate.Name, err) + } } return spec.validateRepairEdge(phase, phases) } @@ -798,6 +807,34 @@ func (spec *DefinitionSpec) RequiredEnvNames() []string { return names } +// validateGateAllow checks a gate's allow list at save time (R1): only +// tests_intact reads one, so on any other gate it is a misconfiguration that +// would otherwise sit there looking like an exemption. Each entry must be a +// well-formed path.Match glob. `**` is refused rather than accepted, because +// path.Match reads it as a plain `*` that stops at `/` — the writes +// allowlist's `**` crosses directories, and an allow entry that quietly +// meant less than it says would fail the agent it was written to exempt. +func validateGateAllow(gate GateSpec) error { + if gate.Allow == nil { + return nil + } + if gate.Name != GateTestsIntact { + return fmt.Errorf("allow is only meaningful on %q", GateTestsIntact) + } + for _, pattern := range gate.Allow { + if strings.TrimSpace(pattern) == "" { + return fmt.Errorf("allow entries must be non-empty globs") + } + if strings.Contains(pattern, "**") { + return fmt.Errorf("allow glob %q: `**` is not supported; allow uses path.Match, where `*` stops at `/`", pattern) + } + if _, err := path.Match(pattern, ""); err != nil { + return fmt.Errorf("allow glob %q is malformed: %w", pattern, err) + } + } + return nil +} + func builtinGateNames() []string { names := make([]string, 0, len(builtinGates)) for name := range builtinGates { diff --git a/internal/protocol/definition_test.go b/internal/protocol/definition_test.go index 4ca3499..3a73805 100644 --- a/internal/protocol/definition_test.go +++ b/internal/protocol/definition_test.go @@ -324,6 +324,47 @@ phases: `, `"build"`, `"tests_pass"`, "requires a command") } +func TestTestsIntactAllowIsValidatedAtSaveTime(t *testing.T) { + const base = ` +name: tests-intact +roster: + builder: {model: opus, system_prompt: s, user_prompt: u} +phases: + - name: build + kind: agent + owner: builder + gates: +` + spec := mustParse(t, base+` - {name: tests_intact, allow: ["legacy/*_test.go", "e2e/[ab]*.spec.ts"]}`+"\n") + if got := spec.Phases[0].Gates[0].Allow; len(got) != 2 || got[0] != "legacy/*_test.go" { + t.Fatalf("allow = %q, want both globs kept in order", got) + } + mustParse(t, base+" - {name: tests_intact}\n") + cases := []struct { + name string + gate string + want []string + }{ + {"allow on another gate", `{name: artifacts_exist, allow: ["*_test.go"]}`, + []string{`"build"`, `"artifacts_exist"`, `only meaningful on "tests_intact"`}}, + {"an empty allow entry", `{name: tests_intact, allow: [""]}`, + []string{`"tests_intact"`, "non-empty"}}, + {"a blank allow entry", `{name: tests_intact, allow: [" "]}`, + []string{`"tests_intact"`, "non-empty"}}, + {"an unclosed character class", `{name: tests_intact, allow: ["foo[_test.go"]}`, + []string{`"foo[_test.go"`, "malformed"}}, + {"a trailing escape", `{name: tests_intact, allow: ["foo\\"]}`, + []string{"malformed"}}, + {"a double star", `{name: tests_intact, allow: ["legacy/**"]}`, + []string{`"legacy/**"`, "not supported"}}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + mustReject(t, base+" - "+tc.gate+"\n", tc.want...) + }) + } +} + func TestUnknownYAMLFieldIsRejectedAtParseTime(t *testing.T) { mustReject(t, ` name: bad