diff --git a/.changeset/sunny-ravens-leap.md b/.changeset/sunny-ravens-leap.md new file mode 100644 index 000000000..262b817cc --- /dev/null +++ b/.changeset/sunny-ravens-leap.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 4174 +--- +**TDD Audit no longer reads a git trailer token git cannot parse** — the trailer token is renamed gate_status → gate-status, so the per-commit gate trail becomes machine-readable the moment a producer starts writing it; previously every commit read as missing and the section self-suppressed silently. (#3962) diff --git a/docs/ship-pr-body-sections.md b/docs/ship-pr-body-sections.md index dedad1158..7f4eede4f 100644 --- a/docs/ship-pr-body-sections.md +++ b/docs/ship-pr-body-sections.md @@ -19,12 +19,12 @@ Custom sections are append-only. They render after `Key Decisions` (and before t ### TDD Audit section -The `TDD Audit` section is always appended last. It walks the commits in the `merge-base..HEAD` range (merges excluded), reads each commit's `gate_status:` Git trailer (`skill` | `fallback` | `exempt`), and pairs each `test:` commit with its following `feat:`/`fix:` implementation commit in a table. Commits that carry no recognized trailer are counted as `missing`. +The `TDD Audit` section is always appended last. It walks the commits in the `merge-base..HEAD` range (merges excluded), reads each commit's `gate-status:` Git trailer (`skill` | `fallback` | `exempt`), and pairs each `test:` commit with its following `feat:`/`fix:` implementation commit in a table. Commits that carry no recognized trailer are counted as `missing`. The section closes with a single aggregate trailer line that a GitHub squash-merge carries into the base branch: ``` -gate_status: skill=3, fallback=1, exempt=0, missing=0 +gate-status: skill=3, fallback=1, exempt=0, missing=0 ``` ## Configure Sections During Onboarding diff --git a/gsd-core/workflows/ship.md b/gsd-core/workflows/ship.md index 09293868f..2cfde4b49 100644 --- a/gsd-core/workflows/ship.md +++ b/gsd-core/workflows/ship.md @@ -309,16 +309,16 @@ Example configured sections: **8. TDD Audit section:** -Reconstruct the per-commit TDD gate trail before squash-merge discards it. Walk the PR branch's own commits (merges excluded) and read each commit's `gate_status:` trailer with Git's native trailer machinery — never a raw `%B` grep, which would also match the string written in prose: +Reconstruct the per-commit TDD gate trail before squash-merge discards it. Walk the PR branch's own commits (merges excluded) and read each commit's `gate-status:` trailer with Git's native trailer machinery — never a raw `%B` grep, which would also match the string written in prose: ```bash # Anchor on the merge-base so a stale local ${BASE_BRANCH} ref cannot over-count. RANGE_BASE=$(git merge-base "${BASE_BRANCH}" HEAD) git log "${RANGE_BASE}..HEAD" --no-merges --reverse \ - --format='%H%x1f%s%x1f%(trailers:key=gate_status,valueonly,separator=%x2c)%x1e' + --format='%H%x1f%s%x1f%(trailers:key=gate-status,valueonly,separator=%x2c)%x1e' ``` -Records are separated by `\x1e`; the fields inside each are `\x1f`-separated — ``, ``, ``. +Records are separated by `\x1e`; the fields inside each are `\x1f`-separated — ``, ``, ``. Pair commits by their conventional-commit type (the `type:` prefix of the subject): @@ -326,16 +326,16 @@ Pair commits by their conventional-commit type (the `type:` prefix of the subjec - A `refactor:`, `docs:`, or `chore:` commit that is not consumed as an Impl pairing is a standalone row with Impl commit `—`. - A `feat:`/`fix:` commit with no preceding unpaired `test:` is a standalone row. -Surface each commit's `gate_status:` value, normalized to exactly one of `skill`, `fallback`, `exempt`, or `missing` — never the raw trailer text. A commit whose trailer is absent, whose value is none of the first three, or which carries more than one `gate_status:` trailer (ambiguous) is counted as **missing** and still listed. This section is informational; it never blocks the ship. +Surface each commit's `gate-status:` value, normalized to exactly one of `skill`, `fallback`, `exempt`, or `missing` — never the raw trailer text. A commit whose trailer is absent, whose value is none of the first three, or which carries more than one `gate-status:` trailer (ambiguous) is counted as **missing** and still listed. This section is informational; it never blocks the ship. -**Self-suppress when every commit is missing (#2431):** the execute pipeline only writes `gate_status:` trailers when TDD mode is active. If every commit in the scan normalizes to `missing`, skip this section and the aggregate trailer (step 9) entirely — a 100%-missing table is pure noise. Only emit when at least one commit carries a real value (`skill`, `fallback`, or `exempt`). +**Self-suppress when every commit is missing (#2431):** the execute pipeline only writes `gate-status:` trailers when TDD mode is active. If every commit in the scan normalizes to `missing`, skip this section and the aggregate trailer (step 9) entirely — a 100%-missing table is pure noise. Only emit when at least one commit carries a real value (`skill`, `fallback`, or `exempt`). -Harden every table cell against injection, not just subjects: escape `|` as `\|` and strip `\r`/`\n` from both commit subjects and the rendered `gate_status` value. Prefer NUL (`-z` / `%x00`) record separation, and reject any record whose fields contain the `\x1f`/`\x1e` delimiters, so an adversarial commit message cannot corrupt record or field boundaries. +Harden every table cell against injection, not just subjects: escape `|` as `\|` and strip `\r`/`\n` from both commit subjects and the rendered `gate-status` value. Prefer NUL (`-z` / `%x00`) record separation, and reject any record whose fields contain the `\x1f`/`\x1e` delimiters, so an adversarial commit message cannot corrupt record or field boundaries. ```markdown ## TDD Audit -| Test commit | Impl commit | gate_status | +| Test commit | Impl commit | gate-status | |---|---|---| | `a1b2c3d` test: failing parser test | `e4f5g6h` feat: implement parser | skill | | `i7j8k9l` test: failing export test | `m0n1o2p` feat: implement export | fallback | @@ -346,12 +346,12 @@ Aggregate: 2 skill, 1 fallback, 1 exempt — 0 missing. This `## TDD Audit` section is the final body section — it renders after the configured `pr_body_sections`, immediately before the aggregate trailer — so the frozen core sections and the append-only configured sections both keep their existing order. -**9. Aggregate gate_status trailer (final line)** (only when step 8 was emitted — i.e., at least one real `gate_status` value exists): +**9. Aggregate gate-status trailer (final line)** (only when step 8 was emitted — i.e., at least one real `gate-status` value exists): After every other section — including any configured `pr_body_sections` — emit the audit aggregate as a single Git trailer on the **final line** of the PR body, preceded by a blank line so it parses as a valid trailer: ``` -gate_status: skill=2, fallback=1, exempt=1, missing=0 +gate-status: skill=2, fallback=1, exempt=1, missing=0 ``` Use the exact key order `skill=`, `fallback=`, `exempt=`, `missing=` so downstream tooling parses it stably. Keeping it last means a GitHub squash-merge that defaults its commit message to the PR description carries the aggregate into `${BASE_BRANCH}`, preserving the audit footprint in `git log` after the PR branch is deleted. (Best-effort: it depends on the repo's squash-message default; the in-body `## TDD Audit` section is the source of truth regardless.) diff --git a/tests/workflow-compat.test.cjs b/tests/workflow-compat.test.cjs index 7ab90034b..3bffcd376 100644 --- a/tests/workflow-compat.test.cjs +++ b/tests/workflow-compat.test.cjs @@ -76,7 +76,7 @@ describe('workflow CLI compatibility (#1759)', () => { 'use strict'; // feat(#41): /gsd-ship generate_pr_body emits a TDD Audit table + an aggregate -// `gate_status:` trailer so the per-commit TDD gate trail survives squash-merge. +// `gate-status:` trailer so the per-commit TDD gate trail survives squash-merge. // These assertions pin the shipped workflow prose in gsd-core/workflows/ship.md. const fs = require('node:fs'); @@ -89,15 +89,42 @@ function readRepoFile(relativePath) { return fs.readFileSync(path.join(repoRoot, relativePath), 'utf8'); } -describe('feat-41: ship.md TDD Audit gate_status extraction', () => { +describe('feat-41: ship.md TDD Audit gate-status extraction', () => { const workflow = readRepoFile('gsd-core/workflows/ship.md'); test('adds a "## TDD Audit" section to the generated PR body', () => { assert.match(workflow, /## TDD Audit/); }); - test('extracts gate_status via Git native trailer machinery, not a raw body grep', () => { - assert.match(workflow, /trailers:key=gate_status/); + test('shipped TDD-Audit trailer token round-trips through git (#3962)', (t) => { + // `_` is not a valid git trailer token character, so a `gate-status:` + // trailer is invisible to interpret-trailers and %(trailers:key=...) -- the + // audit read returned empty for every commit and self-suppression hid it. + // Extract the token the shipped workflow actually reads, then prove REAL + // git can match a commit carrying that exact trailer. + const { createTempGitProject, cleanup } = require('./helpers.cjs'); + const { runGit } = require('./helpers/process-seam.cjs'); + const tokenMatch = workflow.match(/trailers:key=([A-Za-z-]+),/); + assert.ok(tokenMatch, 'ship.md must read %(trailers:key=,...) with a simple token'); + const token = tokenMatch[1]; + assert.ok(!token.includes('_'), + `trailer token "${token}" contains "_" -- git ignores such trailers entirely (#3962)`); + + const tmpDir = createTempGitProject('gsd-3962-'); + t.after(() => cleanup(tmpDir)); + const msg = `test(2-01): probe\n\n${token}: skill\n`; + const c = runGit(['commit', '--allow-empty', '-m', msg], { cwd: tmpDir }); + assert.equal(c.outcome, 'exited', `probe commit must exit cleanly: ${c.stderr}`); + const r = runGit( + ['log', '-1', `--format=%(trailers:key=${token},valueonly)`], + { cwd: tmpDir }, + ); + assert.equal(r.stdout.trim(), 'skill', + `a commit carrying "${token}: skill" must read back as "skill" via the shipped token`); + }); + + test('extracts gate-status via Git native trailer machinery, not a raw body grep', () => { + assert.match(workflow, /trailers:key=gate-status/); }); test('scopes the scan to the merge-base..HEAD range', () => { @@ -110,8 +137,8 @@ describe('feat-41: ship.md TDD Audit gate_status extraction', () => { assert.match(workflow, /--no-merges/); }); - test('renders a Test commit / Impl commit / gate_status table', () => { - assert.match(workflow, /Test commit[\s\S]*Impl commit[\s\S]*gate_status/); + test('renders a Test commit / Impl commit / gate-status table', () => { + assert.match(workflow, /Test commit[\s\S]*Impl commit[\s\S]*gate-status/); }); test('pairs conventional-commit test: rows with their impl commit', () => { @@ -123,7 +150,7 @@ describe('feat-41: ship.md TDD Audit gate_status extraction', () => { assert.match(workflow, /[Ee]scape[\s\S]{0,60}\|/); }); - test('counts commits lacking a recognized gate_status trailer as missing', () => { + test('counts commits lacking a recognized gate-status trailer as missing', () => { assert.match(workflow, /missing/); }); @@ -134,7 +161,7 @@ describe('feat-41: ship.md TDD Audit gate_status extraction', () => { test('emits the aggregate trailer in the exact, stable key order', () => { assert.match( workflow, - /gate_status:\s*skill=[^,]*,\s*fallback=[^,]*,\s*exempt=[^,]*,\s*missing=/, + /gate-status:\s*skill=[^,]*,\s*fallback=[^,]*,\s*exempt=[^,]*,\s*missing=/, ); }); @@ -145,7 +172,7 @@ describe('feat-41: ship.md TDD Audit gate_status extraction', () => { // ─── #2431: TDD Audit section self-suppresses when all commits are missing ─ // - // The execute pipeline only writes `gate_status:` git trailers when TDD mode + // The execute pipeline only writes `gate-status:` git trailers when TDD mode // is active. Without TDD mode, every commit is `missing` and the section is // pure noise. The fix adds a self-suppress instruction: skip the section // entirely when every commit normalizes to `missing`. This is data-driven @@ -159,16 +186,16 @@ describe('feat-41: ship.md TDD Audit gate_status extraction', () => { }); test('#2431: step 9 (aggregate trailer) is also gated on real values existing', () => { - // The aggregate gate_status trailer is the companion to the TDD Audit - // section; both must be skipped together when no real gate_status exists. + // The aggregate gate-status trailer is the companion to the TDD Audit + // section; both must be skipped together when no real gate-status exists. // `\z` is a Perl/Ruby end-of-input anchor with NO meaning in JavaScript — it // matched a literal `z`, so the lazy span silently stopped at the first `z` // whenever `**10.` was absent, truncating the captured step. `$` (no /m flag) // is the JS end-of-input anchor. - const step9 = workflow.match(/\*\*9\.\s*Aggregate gate_status trailer[\s\S]*?(?=\*\*10\.|$)/); + const step9 = workflow.match(/\*\*9\.\s*Aggregate gate-status trailer[\s\S]*?(?=\*\*10\.|$)/); assert.ok(step9, 'step 9 must exist in the workflow'); assert.match(step9[0], /step 8|at least one|real/i, - 'step 9 must reference step 8 or require at least one real gate_status value (#2431)'); + 'step 9 must reference step 8 or require at least one real gate-status value (#2431)'); }); test('#2431: does NOT read workflow.tdd_mode inline (ADR-857 Phase 6 compliant)', () => { @@ -191,13 +218,13 @@ describe('feat-41: ship.md TDD Audit gate_status extraction', () => { assert.match(workflow, /skipping[\s\S]{0,80}(refactor|docs|chore)/i); }); - test('normalizes the gate_status cell to a known token, never raw trailer text', () => { + test('normalizes the gate-status cell to a known token, never raw trailer text', () => { assert.match(workflow, /normaliz[a-z]*[\s\S]{0,120}missing/i); assert.match(workflow, /never the raw/i); }); - test('treats a commit with multiple gate_status trailers as missing', () => { - assert.match(workflow, /more than one[\s\S]{0,40}gate_status/i); + test('treats a commit with multiple gate-status trailers as missing', () => { + assert.match(workflow, /more than one[\s\S]{0,40}gate-status/i); }); test('hardens every table cell against pipe/newline injection', () => {