From ed819aa4d642b2511f3811a0fc0c32cabfd8e7e9 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Mon, 14 Sep 2026 00:57:08 -0400 Subject: [PATCH] fix(#4379): make the TDD RED-commit pathspec language-agnostic (#4715) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(#4379): make the TDD RED pathspec language-agnostic The pathspec IS this gate's definition of "a test file", and it listed only JS/TS conventions. Go's *_test.go matches none of them, so a commit adding a failing Go test was invisible, RED_COMMIT came back empty, and every behaviour-adding task halted with TDD GATE TRIPPED. references/tdd.md already advertises `go test ./...` and `cargo test` as supported, so the gate was refusing to see tests the docs promised to support. Two corrections, both measured against a seeded repo rather than reasoned: - cover the conventions tdd.md advertises: *_test.go, test_*.py, *_test.py, *_test.exs, *_spec.rb, *_test.rb. - drop the `**/` prefix. It does NOT match a path with no directory component, so a root-level foo.test.js was invisible even in the language the gate did support -- a second defect the report did not mention. A bare glob matches at every depth. Deliberately not widened to ordinary source: a pathspec matching implementation files would make the gate pass on any in-scope commit, which is worse than tripping wrongly. Rust is a known gap for that exact reason and is now documented rather than silently broken. Driving the shipped pathspec against a seeded repo: before, 0 of 7 language/root conventions matched; after, 7 of 7, with src/impl.go and src/lib.rs correctly unmatched in both. Emitted-Drift-Ack-Growth: execute-phase.md — the widened pathspec plus the comment recording why it must not cover ordinary source Co-Authored-By: Claude Opus 5 * test(#4379): drive the shipped RED pathspec against a real repo The existing row pinned the pathspec as a literal string, which the fix makes stale. Re-point it, and add behavioural coverage that EXTRACTS the pathspec from the shipped workflow and runs git log with it against a seeded repo -- re-typing the pattern into the test would only assert that two copies of a string agree. Rows: every advertised convention is visible; a root-level test file is not invisible (the half the report missed); existing JS/TS still matches; implementation files never match, so the gate can still trip; and Rust inline #[test] stays out of reach, asserted rather than left silent. Co-Authored-By: Claude Opus 5 * chore(#4379): add changeset fragment Co-Authored-By: Claude Opus 5 * fix(#4379): be honest about the widened pathspec's cost Adversarial review: the rationale comment claimed the change was safe without naming what it gives up. `*.spec.*` can match a non-test file carrying the word (api.spec.json, openapi.spec.yaml), which lets the gate pass on a commit touching only that. Not new -- `**/*.spec.*` already matched those at any nested path, so dropping `**/` extends the same class to the root -- but the comment should say so rather than imply the widening is free. Also: the tdd.md list named Ruby and Elixir as recognised while the detection step above it enumerates only Node/Python/Go/Rust. Say explicitly that the gate's pathspec is wider than the detected project types, and why that is deliberate. Co-Authored-By: Claude Opus 5 * test(#4379): drop a vacuous row, fold its point into a real one Adversarial review: the "rust inline #[test] remains out of reach" row asserted src/lib.rs never matches -- the identical assertion to the "implementation files never match" row directly below it. It exercised nothing about #[test] semantics and would have passed against almost any fix, so it was coverage theatre. Delete it and move its rationale into the row that already carries the assertion, where it explains WHY the Rust gap follows from that row holding: the two cannot both be satisfied by a path-based gate. Co-Authored-By: Claude Opus 5 * fix(#4379): move the pathspec rationale out of a size-capped file execute-phase.md sits under a FROZEN byte ceiling (ADR-857 Phase 6, #1168: < 93600). The 20-line rationale comment I added pushed it to 93933 and tripped seven tests, all the same ceiling. Base was 92371, so the budget was 1229 bytes and the comment spent 1481. Keep six lines at the call site -- what the pathspec is, why it is not wider, where to read more -- and move the trade-off detail to references/tdd.md, which has no ceiling. That is the right home anyway: the workflow is loaded into context on every run, the reference is read on demand. 92882 bytes, 718 under. Pathspec line byte-identical; re-proved behaviour after the trim: 0/7 conventions before, 7/7 after, no implementation files matched either way. Co-Authored-By: Claude Opus 5 * chore(#4379): refresh the compact-content benchmark baseline execute-phase.md changed size, so the committed baseline drifted. The script's own contract makes it a report that exits 0, but the test asserts the committed baseline is up to date -- refresh via --write, which is what the drift message instructs. Co-Authored-By: Claude Opus 5 * chore(#4379): backfill the changeset PR number Co-Authored-By: Claude Opus 5 --------- Co-authored-by: sim Co-authored-by: Claude Opus 5 --- .changeset/eager-moles-squeak.md | 5 + gsd-core/references/tdd.md | 26 ++++- gsd-core/workflows/execute-phase.md | 9 +- .../compact-content-benchmark-baseline.json | 12 +-- tests/safe-resume-gate-anchoring.test.cjs | 99 ++++++++++++++++++- 5 files changed, 141 insertions(+), 10 deletions(-) create mode 100644 .changeset/eager-moles-squeak.md diff --git a/.changeset/eager-moles-squeak.md b/.changeset/eager-moles-squeak.md new file mode 100644 index 000000000..adc62fd62 --- /dev/null +++ b/.changeset/eager-moles-squeak.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 4715 +--- +**TDD mode no longer trips on every task in Go, Ruby, Elixir and Python projects** — the RED-commit gate looked only for `*.test.*`, `*.spec.*` and `tests/`, so a commit adding `foo_test.go` was invisible and every behaviour-adding task halted with `TDD GATE TRIPPED: missing RED commit`. It now recognises the conventions the TDD reference already advertises, and matches at the repo root as well as in subdirectories. Rust stays a documented gap: `#[test]` lives in the implementation file, so no path-based gate can see it. (#4379) diff --git a/gsd-core/references/tdd.md b/gsd-core/references/tdd.md index 5915d99bc..6ec87fa65 100644 --- a/gsd-core/references/tdd.md +++ b/gsd-core/references/tdd.md @@ -178,10 +178,32 @@ cargo test # Rust ``` **5. Create first test file:** -Follow project conventions for test location: -- `*.test.ts` / `*.spec.ts` next to source +Follow project conventions for test location. The RED-commit gate +(`workflows/execute-phase.md`) recognises the patterns below at any depth, repo root +included. Note the gate's pathspec is deliberately **wider than the project types the +detection step above enumerates** — it costs nothing to recognise a convention the +onboarding flow does not yet auto-detect, and a project using one should not have its +RED commits go unseen: +- `*.test.*` / `*.spec.*` next to source — JS/TS and anything sharing the convention - `__tests__/` directory - `tests/` directory at root +- `*_test.go` — Go +- `test_*.py` / `*_test.py` — Python (in addition to `tests/`) +- `*_test.exs` — Elixir +- `*_spec.rb` / `*_test.rb` — Ruby + +**Cost of the broad pathspec (#4379).** `*.spec.*` can match a non-test file that happens to carry +the word — `api.spec.json`, `openapi.spec.yaml` — which lets the RED gate pass on a commit touching +only that. This is not new: the previous `**/*.spec.*` already matched those at any nested path, so +dropping the `**/` prefix only extends the same false-positive class to the repo root. It is +accepted rather than narrowed, because narrowing it is a behaviour change to the +currently-supported case and not part of making other languages visible. + +**Known gap — Rust (#4379).** `#[test]` conventionally lives inside the implementation file, so a +Rust RED commit touches `src/*.rs` and no path-based gate can distinguish it from ordinary source. +Widening the pathspec to cover it would match all source and make the gate meaningless. `cargo test` +works; the RED-*commit* gate cannot see it, so a Rust project using `workflow.tdd_mode` should +expect the gate to trip. Framework setup is a one-time cost included in the first TDD plan's RED phase. diff --git a/gsd-core/workflows/execute-phase.md b/gsd-core/workflows/execute-phase.md index eb9464bd8..01c80d2fa 100644 --- a/gsd-core/workflows/execute-phase.md +++ b/gsd-core/workflows/execute-phase.md @@ -223,7 +223,14 @@ if [ "$TDD_MODE" = "true" ]; then PLAN_N=$((10#${PLAN_ID})) PLAN_SCOPE_RE="^[a-z]+\((0*${PHASE_N})-(0*${PLAN_N})\):" # TDD gate's own scope check TDD_MILESTONE_BASE=$(git describe --tags --abbrev=0 2>/dev/null || echo "") - RED_COMMIT=$(git log --oneline -E ${TDD_MILESTONE_BASE:+"$TDD_MILESTONE_BASE..HEAD"} --grep="${PLAN_SCOPE_RE}" -- "**/*.test.*" "**/*.spec.*" "tests/" | head -1) + # #4379: this pathspec IS the gate's definition of "a test file". It was + # JS/TS-only, so Go tripped on every task; and `**/` never matches a + # root-level path, so a root `foo.test.js` was invisible too. Bare globs + # match at every depth. NOT widened to ordinary source — that would make + # the gate pass on any in-scope commit. Trade-offs and the Rust gap: + # references/tdd.md § Create first test file. + + RED_COMMIT=$(git log --oneline -E ${TDD_MILESTONE_BASE:+"$TDD_MILESTONE_BASE..HEAD"} --grep="${PLAN_SCOPE_RE}" -- "*.test.*" "*.spec.*" "tests/" "__tests__/" "*_test.go" "test_*.py" "*_test.py" "*_test.exs" "*_spec.rb" "*_test.rb" | head -1) if [ -z "$RED_COMMIT" ]; then gsd_run query state.update last_gate_trip "${PLAN_ID}/${TASK_ID}" || true echo "TDD GATE TRIPPED: missing RED commit for ${PLAN_ID}/${TASK_ID}" diff --git a/tests/fixtures/compact-content-benchmark-baseline.json b/tests/fixtures/compact-content-benchmark-baseline.json index 77715c550..f80617f3c 100644 --- a/tests/fixtures/compact-content-benchmark-baseline.json +++ b/tests/fixtures/compact-content-benchmark-baseline.json @@ -18,9 +18,9 @@ "reductionPct": 16.51 }, "execute-phase": { - "offTokens": 25952, - "onTokens": 23701, - "reductionPct": 8.67 + "offTokens": 26103, + "onTokens": 23852, + "reductionPct": 8.62 }, "new-project": { "offTokens": 14308, @@ -39,8 +39,8 @@ } }, "aggregate": { - "offTokens": 107440, - "onTokens": 90792, - "reductionPct": 15.5 + "offTokens": 107591, + "onTokens": 90943, + "reductionPct": 15.47 } } diff --git a/tests/safe-resume-gate-anchoring.test.cjs b/tests/safe-resume-gate-anchoring.test.cjs index 87f4acfde..91d1a6fb1 100644 --- a/tests/safe-resume-gate-anchoring.test.cjs +++ b/tests/safe-resume-gate-anchoring.test.cjs @@ -63,7 +63,7 @@ describe('#4003 — safe_resume_gate commit-scope greps', () => { assert.ok(w.includes('PHASE_INT=${PHASE_NUMBER%%.*}; PHASE_FRAC=${PHASE_NUMBER#"$PHASE_INT"}') && w.includes('PHASE_N="$((10#$PHASE_INT))${PHASE_FRAC//./\\\\.}"') && w.includes('PLAN_N=$((10#${PLAN_ID}))'), 'the TDD block derives zero-stripped components (#4619: leading integer segment only, decimal/N-segment tolerant)'); - assert.ok(w.includes('RED_COMMIT=$(git log --oneline -E ${TDD_MILESTONE_BASE:+"$TDD_MILESTONE_BASE..HEAD"} --grep="${PLAN_SCOPE_RE}" -- "**/*.test.*"'), + assert.ok(w.includes('RED_COMMIT=$(git log --oneline -E ${TDD_MILESTONE_BASE:+"$TDD_MILESTONE_BASE..HEAD"} --grep="${PLAN_SCOPE_RE}" -- "*.test.*"'), 'the RED grep must use the same anchored padding-tolerant scope, milestone-bounded'); assert.ok(!w.includes('--grep="^test(${PHASE_NUMBER}-${PLAN_ID})"'), 'the padded-literal RED grep must not remain'); @@ -137,3 +137,100 @@ describe('#4003 — safe_resume_gate commit-scope greps', () => { assert.ok(!unbounded.some((l) => /in prose/.test(l)), 'the anchor still holds without a tag'); }); }); + +// ─── #4379: the RED pathspec is language-agnostic, and matches at root ────── +// +// The pathspec IS this gate's definition of "a test file". It was JS/TS-only, so +// a commit adding `foo_test.go` was invisible and every behaviour-adding task in +// a Go project halted with `TDD GATE TRIPPED: missing RED commit` — while +// references/tdd.md:175-177 advertises `go test ./...` as supported. +// +// These rows drive the REAL pathspec, extracted from the shipped workflow, against +// a real git repo. Re-typing the pattern into the test would only assert that two +// copies of a string agree; running it answers the question the gate actually asks. +describe('#4379 — the TDD RED pathspec is language-agnostic', () => { + // Pull the pathspec out of the workflow rather than restating it: the thing + // under test is what SHIPS, not a copy maintained alongside it. + function shippedRedPathspec() { + const w = fs.readFileSync(WORKFLOW, 'utf8'); + const line = w.split(/\r?\n/).find((l) => l.includes('RED_COMMIT=$(git log')); + assert.ok(line, 'the RED_COMMIT line must exist in execute-phase.md'); + const tail = line.slice(line.indexOf('-- ') + 3, line.lastIndexOf('| head -1)')); + const specs = (tail.match(/"[^"]{1,200}"/g) || []).map((s) => s.slice(1, -1)); + assert.ok(specs.length > 0, `expected quoted pathspecs, parsed none from: ${tail}`); + return specs; + } + + // One seeded repo, one commit, reused by every row below. + function seedRepo(t) { + const dir = createTempGitProject('gsd-4379-'); + t.after(() => cleanup(dir)); + const files = [ + 'foo_test.go', 'pkg/bar_test.go', // Go — incl. ROOT level + 'root.test.js', 'src/a.test.ts', // JS/TS — incl. ROOT level + 'tests/c.py', '__tests__/d.js', // directory conventions + 'test_mod.py', 'mod_test.py', // Python, outside tests/ + 'lib_spec.rb', 'x_test.exs', // Ruby, Elixir + 'src/impl.go', 'src/lib.rs', // NOT tests — must never match + ]; + for (const rel of files) { + const abs = path.join(dir, rel); + fs.mkdirSync(path.dirname(abs), { recursive: true }); + fs.writeFileSync(abs, 'x\n'); + } + gitOrThrow(['add', '-A'], { cwd: dir }); + gitOrThrow(['commit', '-m', 'test(1-01): seed every convention'], { cwd: dir }); + return dir; + } + + function matched(dir, specs) { + const out = gitOrThrow(['log', '--format=', '--name-only', '--', ...specs], { cwd: dir }); + return new Set(out.split(/\r?\n/).map((l) => l.trim()).filter(Boolean)); + } + + test('every convention tdd.md advertises is visible to the RED detector', (t) => { + const dir = seedRepo(t); + const hits = matched(dir, shippedRedPathspec()); + for (const rel of [ + 'foo_test.go', 'pkg/bar_test.go', + 'test_mod.py', 'mod_test.py', + 'lib_spec.rb', 'x_test.exs', + ]) { + assert.ok(hits.has(rel), `${rel} must satisfy the RED detector (#4379); matched: ${[...hits].sort().join(', ')}`); + } + }); + + test('a ROOT-level test file is not invisible', (t) => { + // The pre-#4379 pathspec was `**/`-prefixed, and `**/` does not match a path + // with no directory component — so a root `foo.test.js` was missed even in + // the language the gate did support. This is the half the issue did not report. + const dir = seedRepo(t); + const hits = matched(dir, shippedRedPathspec()); + assert.ok(hits.has('root.test.js'), 'a root-level .test.js must match'); + assert.ok(hits.has('foo_test.go'), 'a root-level _test.go must match'); + }); + + test('the existing JS/TS conventions still match (no regression)', (t) => { + const dir = seedRepo(t); + const hits = matched(dir, shippedRedPathspec()); + for (const rel of ['src/a.test.ts', 'tests/c.py', '__tests__/d.js']) { + assert.ok(hits.has(rel), `${rel} must still match after widening`); + } + }); + + test('implementation files never match — the gate must still be able to trip', (t) => { + // A pathspec broad enough to catch ordinary source would make the gate pass on + // ANY in-scope commit, which is worse than the bug being fixed. + // + // `src/lib.rs` carries the Rust consequence: `#[test]` conventionally lives + // INSIDE the implementation file, so a Rust RED commit touches only source and + // no path-based gate can see it. That gap is a direct consequence of this row + // holding — the two cannot both be satisfied — which is why it is documented in + // references/tdd.md rather than "fixed" by widening the pathspec. + const dir = seedRepo(t); + const hits = matched(dir, shippedRedPathspec()); + for (const rel of ['src/impl.go', 'src/lib.rs']) { + assert.ok(!hits.has(rel), `${rel} must NOT be treated as a test file`); + } + }); +});