From f181fd3a4b4323fceb288f5872c556ab580bc9d2 Mon Sep 17 00:00:00 2001 From: Sholto McNeilage Date: Thu, 18 Jun 2026 11:16:17 +1000 Subject: [PATCH] fix(retro): language-agnostic test detection + anti-fabrication guard The retro generator's test-counting commands only matched JS/TS files (grep '\.(test|spec)\.'), returning 0 for Python (test_*.py), Terraform (*.tftest.hcl), and Bats (*.bats) suites. Combined with a repo-wide total count, the contradiction ('N total tests, 0 added') led the model to fabricate a non-existent 'bootstrap commit' to reconcile the numbers. - Commands 10/12/13: language-agnostic TEST pattern; split tests ADDED (--diff-filter=A) from TOUCHED; command 10 uses git ls-files. - New Step 1.5 plausibility guard: per-commit figures must come from 'git show --shortstat '; no invented bootstrap/foundation narrative; reconcile count contradictions by re-checking, not narrating. - Applied to both SKILL.md.tmpl (source) and SKILL.md (runtime). - regression-retro-test-detection.test.ts locks in both invariants. Note: SKILL.md line 1 'name: gstack-retro' is pre-existing install-time prefix drift (skill_prefix:true), not part of this fix. Co-Authored-By: Claude Opus 4.8 --- retro/SKILL.md | 65 ++++++++-- retro/SKILL.md.tmpl | 63 ++++++++-- test/regression-retro-test-detection.test.ts | 122 +++++++++++++++++++ 3 files changed, 233 insertions(+), 17 deletions(-) create mode 100644 test/regression-retro-test-detection.test.ts diff --git a/retro/SKILL.md b/retro/SKILL.md index cb65e98cf..70e51617a 100644 --- a/retro/SKILL.md +++ b/retro/SKILL.md @@ -1,5 +1,5 @@ --- -name: retro +name: gstack-retro preamble-tier: 2 version: 2.0.0 description: Weekly engineering retrospective. (gstack) @@ -972,7 +972,8 @@ git log origin/ --since="" --format="%H|%aN|%ae|%ai|%s" --short # 2. Per-commit test vs total LOC breakdown with author # Each commit block starts with COMMIT:|, followed by numstat lines. -# Separate test files (matching test/|spec/|__tests__/) from production files. +# Separate test files from production files using the language-agnostic TEST pattern +# below (commands 10/12) — NOT just JS/TS. Python test_*.py, *.tftest.hcl, *.bats count. git log origin/ --since="" --format="COMMIT:%H|%aN" --numstat # 3. Commit timestamps for session detection and hourly distribution (with author) @@ -996,19 +997,58 @@ cat ~/.gstack/greptile-history.md 2>/dev/null || true # 9. TODOS.md backlog (if available) cat TODOS.md 2>/dev/null || true -# 10. Test file count -find . -name '*.test.*' -o -name '*.spec.*' -o -name '*_test.*' -o -name '*_spec.*' 2>/dev/null | grep -v node_modules | wc -l +# --- Language-agnostic TEST file pattern (used by commands 10, 12, 13) --- +# Matches: Python test_*.py / *_test.py, JS/TS *.test.* / *.spec.*, Ruby *_spec.rb, +# Terraform *.tftest.hcl, Bats *.bats, and any path under tests/ __tests__/ spec/. +# Do NOT narrow this to '\.(test|spec)\.' — that misses every non-JS test suite. +# TEST_RE='(^|/)test_|_test\.|\.test\.|\.spec\.|_spec\.|\.tftest\.hcl$|\.bats$|(^|/)(tests?|__tests__|spec)/' +# VENDOR_RE='/(node_modules|\.venv|venv|dist|build|\.git)/' + +# 10. Total test file count (repo-wide; git ls-files respects .gitignore) +git ls-files | grep -ivE '/(node_modules|\.venv|venv|dist|build)/' \ + | grep -iE '(^|/)test_|_test\.|\.test\.|\.spec\.|_spec\.|\.tftest\.hcl$|\.bats$|(^|/)(tests?|__tests__|spec)/' \ + | sort -u | wc -l # 11. Regression test commits in window git log origin/ --since="" --oneline --grep="test(qa):" --grep="test(design):" --grep="test: coverage" -# 12. gstack skill usage telemetry (if available) -cat ~/.gstack/analytics/skill-usage.jsonl 2>/dev/null || true +# 12. Test files ADDED in window (this is "tests added this period") +git log origin/ --since="" --diff-filter=A --format="" --name-only \ + | grep -ivE '/(node_modules|\.venv|venv|dist|build)/' \ + | grep -iE '(^|/)test_|_test\.|\.test\.|\.spec\.|_spec\.|\.tftest\.hcl$|\.bats$|(^|/)(tests?|__tests__|spec)/' \ + | sort -u | wc -l -# 12. Test files changed in window -git log origin/ --since="" --format="" --name-only | grep -E '\.(test|spec)\.' | sort -u | wc -l +# 13. Test files TOUCHED in window (added OR modified — sanity cross-check vs command 12) +git log origin/ --since="" --format="" --name-only \ + | grep -ivE '/(node_modules|\.venv|venv|dist|build)/' \ + | grep -iE '(^|/)test_|_test\.|\.test\.|\.spec\.|_spec\.|\.tftest\.hcl$|\.bats$|(^|/)(tests?|__tests__|spec)/' \ + | sort -u | wc -l + +# 14. gstack skill usage telemetry (if available) +cat ~/.gstack/analytics/skill-usage.jsonl 2>/dev/null || true ``` +### Step 1.5: Per-commit plausibility guard (anti-fabrication) + +Before describing ANY single commit's size or any "big landing," obey these rules. They +exist because a contradiction between aggregate counts (e.g. "1,446 total tests exist" from +command 10) and a window count (e.g. "0 tests added" from command 12) was once reconciled by +inventing a non-existent "bootstrap commit" with fabricated file/LOC figures. + +- **Per-commit figures come from that commit only.** Any per-commit file count or LOC you + state MUST come from `git show --stat ` / `git show --shortstat ` for that exact + hash. Never infer a commit's size from aggregate totals, a PR number, or a commit subject. +- **Verify outliers.** If you are about to describe a commit as unusually large (>500 files + or >50k insertions), first run `git show --shortstat ` and quote the real numbers. If + the commit is small, do not call it large. +- **No invented "bootstrap"/"foundation"/"initial import" narrative.** Only describe such a + commit if a real commit in the window has a matching subject AND a verified stat showing it. + A mature repo (check `git log --reverse --format=%ai | head -1`) does not get re-bootstrapped. +- **Reconcile contradictions by re-checking, not narrating.** If command 10 (total) and + command 12 (added) seem to contradict each other, re-run command 12 (it is the usual culprit + — a test-pattern miss) before writing a single word about test activity. Never bridge two + numbers with an event you have not verified exists. + ### Step 2: Compute Metrics Calculate and present these metrics in a summary table: @@ -1379,9 +1419,16 @@ Narrative covering: ### Test Health - Total test files: N (from command 10) -- Tests added this period: M (from command 12 — test files changed) +- Tests added this period: M (from command 12 — test files ADDED). Also note tests TOUCHED + (command 13). If command 12 returns 0 but command 13 > 0, tests were modified not added — + say so; do NOT report "no test activity." - Regression test commits: list `test(qa):` and `test(design):` and `test: coverage` commits from command 11 - If prior retro exists and has `test_health`: show delta "Test count: {last} → {now} (+{delta})" +- **Sanity check before claiming "0 tests added":** new tests are frequently committed under + `fix:`/`feat:` (not a `test:` prefix), and command 11 only counts `test(...)`-prefixed + commits — never infer "0 new tests" from command 11. Trust command 12 (file-level). If + command 12 is 0 while many files changed, re-run it before concluding — a zero there is + more often a pattern miss than a real absence. - If test ratio < 20%: flag as growth area — "100% test coverage is the goal. Tests make vibe coding safe." ### Plan Completion diff --git a/retro/SKILL.md.tmpl b/retro/SKILL.md.tmpl index b0819c8a6..c5bb7e011 100644 --- a/retro/SKILL.md.tmpl +++ b/retro/SKILL.md.tmpl @@ -172,7 +172,8 @@ git log origin/ --since="" --format="%H|%aN|%ae|%ai|%s" --short # 2. Per-commit test vs total LOC breakdown with author # Each commit block starts with COMMIT:|, followed by numstat lines. -# Separate test files (matching test/|spec/|__tests__/) from production files. +# Separate test files from production files using the language-agnostic TEST pattern +# below (commands 10/12) — NOT just JS/TS. Python test_*.py, *.tftest.hcl, *.bats count. git log origin/ --since="" --format="COMMIT:%H|%aN" --numstat # 3. Commit timestamps for session detection and hourly distribution (with author) @@ -196,19 +197,58 @@ cat ~/.gstack/greptile-history.md 2>/dev/null || true # 9. TODOS.md backlog (if available) cat TODOS.md 2>/dev/null || true -# 10. Test file count -find . -name '*.test.*' -o -name '*.spec.*' -o -name '*_test.*' -o -name '*_spec.*' 2>/dev/null | grep -v node_modules | wc -l +# --- Language-agnostic TEST file pattern (used by commands 10, 12, 13) --- +# Matches: Python test_*.py / *_test.py, JS/TS *.test.* / *.spec.*, Ruby *_spec.rb, +# Terraform *.tftest.hcl, Bats *.bats, and any path under tests/ __tests__/ spec/. +# Do NOT narrow this to '\.(test|spec)\.' — that misses every non-JS test suite. +# TEST_RE='(^|/)test_|_test\.|\.test\.|\.spec\.|_spec\.|\.tftest\.hcl$|\.bats$|(^|/)(tests?|__tests__|spec)/' +# VENDOR_RE='/(node_modules|\.venv|venv|dist|build|\.git)/' + +# 10. Total test file count (repo-wide; git ls-files respects .gitignore) +git ls-files | grep -ivE '/(node_modules|\.venv|venv|dist|build)/' \ + | grep -iE '(^|/)test_|_test\.|\.test\.|\.spec\.|_spec\.|\.tftest\.hcl$|\.bats$|(^|/)(tests?|__tests__|spec)/' \ + | sort -u | wc -l # 11. Regression test commits in window git log origin/ --since="" --oneline --grep="test(qa):" --grep="test(design):" --grep="test: coverage" -# 12. gstack skill usage telemetry (if available) -cat ~/.gstack/analytics/skill-usage.jsonl 2>/dev/null || true +# 12. Test files ADDED in window (this is "tests added this period") +git log origin/ --since="" --diff-filter=A --format="" --name-only \ + | grep -ivE '/(node_modules|\.venv|venv|dist|build)/' \ + | grep -iE '(^|/)test_|_test\.|\.test\.|\.spec\.|_spec\.|\.tftest\.hcl$|\.bats$|(^|/)(tests?|__tests__|spec)/' \ + | sort -u | wc -l -# 12. Test files changed in window -git log origin/ --since="" --format="" --name-only | grep -E '\.(test|spec)\.' | sort -u | wc -l +# 13. Test files TOUCHED in window (added OR modified — sanity cross-check vs command 12) +git log origin/ --since="" --format="" --name-only \ + | grep -ivE '/(node_modules|\.venv|venv|dist|build)/' \ + | grep -iE '(^|/)test_|_test\.|\.test\.|\.spec\.|_spec\.|\.tftest\.hcl$|\.bats$|(^|/)(tests?|__tests__|spec)/' \ + | sort -u | wc -l + +# 14. gstack skill usage telemetry (if available) +cat ~/.gstack/analytics/skill-usage.jsonl 2>/dev/null || true ``` +### Step 1.5: Per-commit plausibility guard (anti-fabrication) + +Before describing ANY single commit's size or any "big landing," obey these rules. They +exist because a contradiction between aggregate counts (e.g. "1,446 total tests exist" from +command 10) and a window count (e.g. "0 tests added" from command 12) was once reconciled by +inventing a non-existent "bootstrap commit" with fabricated file/LOC figures. + +- **Per-commit figures come from that commit only.** Any per-commit file count or LOC you + state MUST come from `git show --stat ` / `git show --shortstat ` for that exact + hash. Never infer a commit's size from aggregate totals, a PR number, or a commit subject. +- **Verify outliers.** If you are about to describe a commit as unusually large (>500 files + or >50k insertions), first run `git show --shortstat ` and quote the real numbers. If + the commit is small, do not call it large. +- **No invented "bootstrap"/"foundation"/"initial import" narrative.** Only describe such a + commit if a real commit in the window has a matching subject AND a verified stat showing it. + A mature repo (check `git log --reverse --format=%ai | head -1`) does not get re-bootstrapped. +- **Reconcile contradictions by re-checking, not narrating.** If command 10 (total) and + command 12 (added) seem to contradict each other, re-run command 12 (it is the usual culprit + — a test-pattern miss) before writing a single word about test activity. Never bridge two + numbers with an event you have not verified exists. + ### Step 2: Compute Metrics Calculate and present these metrics in a summary table: @@ -556,9 +596,16 @@ Narrative covering: ### Test Health - Total test files: N (from command 10) -- Tests added this period: M (from command 12 — test files changed) +- Tests added this period: M (from command 12 — test files ADDED). Also note tests TOUCHED + (command 13). If command 12 returns 0 but command 13 > 0, tests were modified not added — + say so; do NOT report "no test activity." - Regression test commits: list `test(qa):` and `test(design):` and `test: coverage` commits from command 11 - If prior retro exists and has `test_health`: show delta "Test count: {last} → {now} (+{delta})" +- **Sanity check before claiming "0 tests added":** new tests are frequently committed under + `fix:`/`feat:` (not a `test:` prefix), and command 11 only counts `test(...)`-prefixed + commits — never infer "0 new tests" from command 11. Trust command 12 (file-level). If + command 12 is 0 while many files changed, re-run it before concluding — a zero there is + more often a pattern miss than a real absence. - If test ratio < 20%: flag as growth area — "100% test coverage is the goal. Tests make vibe coding safe." ### Plan Completion diff --git a/test/regression-retro-test-detection.test.ts b/test/regression-retro-test-detection.test.ts new file mode 100644 index 000000000..0e703792e --- /dev/null +++ b/test/regression-retro-test-detection.test.ts @@ -0,0 +1,122 @@ +/** + * Regression tests for the /retro test-detection + anti-fabrication fix. + * + * Background: a generated retro claimed "0 new tests added" and invented a + * non-existent 5,165-file "bootstrap commit" to reconcile that zero against a + * large total-test count. Two root causes in retro/SKILL.md(.tmpl): + * + * 1. The "tests changed in window" command used `grep -E '\.(test|spec)\.'`, + * which only matches JS/TS files (foo.test.ts / foo.spec.js). It returned 0 + * for Python (test_*.py), Terraform (*.tftest.hcl), and Bats (*.bats) suites. + * 2. There was no guard forbidding per-commit figures that aren't read from the + * commit itself, so the model fabricated a "bootstrap" narrative. + * + * These are static invariants against the skill body — they fail the build if the + * language-agnostic pattern is narrowed again or the plausibility guard is removed. + */ +import { describe, expect, test } from "bun:test"; +import * as fs from "node:fs"; +import * as path from "node:path"; + +const ROOT = path.resolve(import.meta.dir, ".."); +const RETRO_TMPL = path.join(ROOT, "retro", "SKILL.md.tmpl"); +const RETRO_MD = path.join(ROOT, "retro", "SKILL.md"); + +function readTmpl(): string { + return fs.readFileSync(RETRO_TMPL, "utf-8"); +} +function readMd(): string { + return fs.readFileSync(RETRO_MD, "utf-8"); +} + +// The test-detection pattern must recognise these representative paths. +const MUST_MATCH = [ + "tests/apps/engine_attribution/unit/test_register_tenant_context.py", // Python prefix + "libs/foo/bar_test.py", // Python suffix + "apps/dashboard/src/x.test.ts", // JS/TS infix + "apps/dashboard/src/x.spec.tsx", // JS/TS spec + "infrastructure/terraform/modules/cloud_run/tests/cloud_run_v2.tftest.hcl", // Terraform + "tests/infrastructure/test_assert_no_cloud_run_destroy.bats", // Bats +]; +// The pattern must NOT classify these production files as tests. +const MUST_NOT_MATCH = [ + "apps/engine_attribution/main.py", + "infrastructure/terraform/environments/production/main.tf", + "packages/components_ui/src/Button.tsx", +]; + +// The literal grep -iE pattern embedded in the skill (kept in sync with both files). +const TEST_RE = + /\(\^\|\/\)test_\|_test\\\.\|\\\.test\\\.\|\\\.spec\\\.\|_spec\\\.\|\\\.tftest\\\.hcl\$\|\\\.bats\$\|\(\^\|\/\)\(tests\?\|__tests__\|spec\)\//; + +describe("retro test-detection — language-agnostic pattern present in both layers", () => { + for (const [label, read] of [ + ["template", readTmpl], + ["compiled SKILL.md", readMd], + ] as const) { + test(`${label} embeds the language-agnostic TEST pattern (Python/TF/Bats, not just JS)`, () => { + const body = read(); + expect(body).toMatch(TEST_RE); + // belt-and-braces: the specific non-JS tokens must be there + expect(body).toContain("tftest\\.hcl$"); + expect(body).toContain("\\.bats$"); + expect(body).toContain("(^|/)test_"); + }); + + test(`${label} does NOT use the narrow JS-only grep as a counting command`, () => { + const body = read(); + // The narrow pattern may appear ONLY inside the explanatory "Do NOT narrow" comment. + const narrowOnCommandLine = body + .split("\n") + .filter((l) => l.includes("(test|spec)") && !l.includes("Do NOT narrow")); + expect(narrowOnCommandLine).toEqual([]); + }); + } +}); + +describe("retro test-detection — pattern behaviour (model the embedded regex in JS)", () => { + // Mirror of the embedded grep -iE pattern, as a JS RegExp, case-insensitive. + const re = /(^|\/)test_|_test\.|\.test\.|\.spec\.|_spec\.|\.tftest\.hcl$|\.bats$|(^|\/)(tests?|__tests__|spec)\//i; + + for (const p of MUST_MATCH) { + test(`matches test file: ${p}`, () => { + expect(re.test(p)).toBe(true); + }); + } + for (const p of MUST_NOT_MATCH) { + test(`does not match production file: ${p}`, () => { + expect(re.test(p)).toBe(false); + }); + } +}); + +describe("retro anti-fabrication — Step 1.5 plausibility guard present", () => { + for (const [label, read] of [ + ["template", readTmpl], + ["compiled SKILL.md", readMd], + ] as const) { + test(`${label} has the Step 1.5 plausibility guard`, () => { + const body = read(); + expect(body).toMatch(/### Step 1\.5: Per-commit plausibility guard/); + }); + + test(`${label} forbids inventing a bootstrap/foundation narrative`, () => { + const body = read(); + expect(body).toMatch(/No invented "bootstrap"\/"foundation"\/"initial import" narrative/); + }); + + test(`${label} requires per-commit figures from git show --shortstat`, () => { + const body = read(); + expect(body).toMatch(/git show --shortstat /); + }); + + test(`${label} guard sits between the command block and Step 2`, () => { + const body = read(); + const guard = body.indexOf("### Step 1.5: Per-commit plausibility guard"); + const step2 = body.indexOf("### Step 2: Compute Metrics"); + expect(guard).toBeGreaterThan(-1); + expect(step2).toBeGreaterThan(-1); + expect(guard).toBeLessThan(step2); + }); + } +});