From ac518646214f199cd62268109af16be012571f65 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Fri, 8 May 2026 23:53:32 -0400 Subject: [PATCH] fix(3263): harden code-review SUMMARY parser; accept BL-/blocker as Critical-tier across pipeline (#3274) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(3263): harden code-review SUMMARY parser; accept BL-/blocker as Critical-tier across pipeline Bug 1: compute_file_scope Node script used ^\s*\w+: boundary regex, which excluded hyphens and left inSection sticky after key-decisions:/patterns-established:/ requirements-completed: blocks. Prose bullets were captured as file paths. Fixed to [\w-]+ boundary and added em-dash/parenthetical stripping with a path validity guard so only path-shaped strings are emitted. Bug 2: present_results grep matched only critical: in frontmatter. When reviewer emitted blocker:, CRITICAL was silently empty. Fixed grep to accept both keys via -E "^\s*(critical |blocker):". Top-issues preview also missed BL-* headings; fixed to include ### BL-\ in the grep pattern. Bug 3: gsd-code-fixer finding_parser documented CR-\d+ only. BL-* findings from a drifted reviewer were silently dropped from critical_warning scope. Updated ID alphabet, severity description, filter sets, and sort order to treat BL-* as Critical-tier-equivalent to CR-*. Reviewer contract: gsd-code-reviewer write_review step now declares blocker:/BL- as accepted tier-equivalent alternatives to critical:/CR-, so the contract acknowledges the reality the workflow defenses accept. Regression tests: tests/code-review-pipeline-regression.test.cjs (18 tests) covers all three bugs behaviourally (pure-function parsers) plus docs-parity assertions on the workflow and agent .md files. Co-Authored-By: Claude Sonnet 4.6 * changeset: add fragment for PR 3274 (fix(3263) code-review parser) Co-Authored-By: Claude Sonnet 4.6 * fix(workflow): use POSIX [[:space:]] instead of \s in grep -E (CR finding 1) BSD grep on macOS does not support \s in ERE; replace with the POSIX [[:space:]] character class so the critical/blocker grep works on both GNU and BSD grep. Also update the corresponding docs-parity test assertion. Co-Authored-By: Claude Sonnet 4.6 * test: tighten em-dash and grep docs-parity assertions (CR finding 2) - Replace `includes('split(/\\s+')` with `includes('split(/\\s+—\\s')` so the assertion actually enforces the em-dash narrative strip and cannot be satisfied by a bare whitespace split. - Update the present_results grep assertion to expect [[:space:]] after the workflow portability fix. Co-Authored-By: Claude Sonnet 4.6 --------- Co-authored-by: Claude Sonnet 4.6 --- .changeset/humble-tunas-leap.md | 5 + agents/gsd-code-fixer.md | 10 +- agents/gsd-code-reviewer.md | 2 + get-shit-done/workflows/code-review.md | 14 +- .../code-review-pipeline-regression.test.cjs | 349 ++++++++++++++++++ 5 files changed, 371 insertions(+), 9 deletions(-) create mode 100644 .changeset/humble-tunas-leap.md create mode 100644 tests/code-review-pipeline-regression.test.cjs diff --git a/.changeset/humble-tunas-leap.md b/.changeset/humble-tunas-leap.md new file mode 100644 index 000000000..ec1ea76a1 --- /dev/null +++ b/.changeset/humble-tunas-leap.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 3274 +--- +code-review SUMMARY parser no longer silently discards critical/blocker counts on macOS (BSD grep \s portability); BL-/blocker entries are now correctly treated as Critical-tier diff --git a/agents/gsd-code-fixer.md b/agents/gsd-code-fixer.md index 1960f4a86..8ffb9a6cd 100644 --- a/agents/gsd-code-fixer.md +++ b/agents/gsd-code-fixer.md @@ -153,7 +153,7 @@ Each finding starts with: ### {ID}: {Title} ``` -Where ID matches: `CR-\d+` (Critical), `WR-\d+` (Warning), or `IN-\d+` (Info) +Where ID matches: `CR-\d+` or `BL-\d+` (Critical-tier-equivalent), `WR-\d+` (Warning), or `IN-\d+` (Info) **Required Fields:** @@ -387,7 +387,7 @@ Read `./CLAUDE.md` and check for `.claude/skills/` or `.agents/skills/` (as desc For each finding, extract: - `id`: Finding identifier (e.g., CR-01, WR-03, IN-12) -- `severity`: Critical (CR-*), Warning (WR-*), Info (IN-*) +- `severity`: Critical (CR-* or BL-*), Warning (WR-*), Info (IN-*) - `title`: Issue title from `### ` heading - `file`: Primary file path from **File:** line - `files`: ALL file paths referenced in finding (including in Fix section) — for multi-file fixes @@ -396,11 +396,11 @@ For each finding, extract: - `fix`: Full fix content from **Fix:** section (may be multi-line, may contain code fences) **2. Filter by fix_scope:** -- If `fix_scope == "critical_warning"`: include only CR-* and WR-* findings -- If `fix_scope == "all"`: include CR-*, WR-*, and IN-* findings +- If `fix_scope == "critical_warning"`: include only CR-*, BL-*, and WR-* findings +- If `fix_scope == "all"`: include CR-*, BL-*, WR-*, and IN-* findings **3. Sort findings by severity:** -- Critical first, then Warning, then Info +- Critical (CR-* and BL-*) first, then Warning, then Info - Within same severity, maintain document order **4. Count findings in scope:** diff --git a/agents/gsd-code-reviewer.md b/agents/gsd-code-reviewer.md index 64fa1b26f..83d615251 100644 --- a/agents/gsd-code-reviewer.md +++ b/agents/gsd-code-reviewer.md @@ -269,6 +269,8 @@ status: clean | issues_found --- ``` +**Label equivalence:** The canonical frontmatter key is `critical:`. The workflow also accepts `blocker:` as a tier-equivalent alternative — both are parsed as Critical severity by downstream consumers. Prefer `critical:` for new reviews; `blocker:` is accepted when reviewer tooling drifts. Similarly, finding IDs beginning with `BL-` are treated as Critical-tier-equivalent to `CR-` IDs by the fixer and pipeline; prefer `CR-` as the canonical prefix. + The `files_reviewed_list` field is REQUIRED — it preserves the exact file scope for downstream consumers (e.g., --auto re-review in code-review-fix workflow). List every file that was reviewed, one per line in YAML list format. **3. Body structure:** diff --git a/get-shit-done/workflows/code-review.md b/get-shit-done/workflows/code-review.md index 43d34c78a..fefb810f8 100644 --- a/get-shit-done/workflows/code-review.md +++ b/get-shit-done/workflows/code-review.md @@ -172,9 +172,15 @@ if [ -z "$FILES_OVERRIDE" ]; then for (const line of yaml.split('\n')) { if (/^\s+created:/.test(line)) { inSection = 'created'; continue; } if (/^\s+modified:/.test(line)) { inSection = 'modified'; continue; } - if (/^\s*\w+:/.test(line) && !/^\s*-/.test(line)) { inSection = null; continue; } + if (/^\s*[\w-]+:/.test(line) && !/^\s*-/.test(line)) { inSection = null; continue; } if (inSection && /^\s+-\s+(.+)/.test(line)) { - files.push(line.match(/^\s+-\s+(.+)/)[1].trim()); + let raw = line.match(/^\s+-\s+(.+)/)[1].trim(); + raw = raw.replace(/^['"]|['"]$/g, ''); + raw = raw.replace(/\s+\([^)]*\)\s*$/, ''); + raw = raw.split(/\s+—\s/)[0].trim(); + if (/\//.test(raw) && /\.[A-Za-z0-9]+$/.test(raw)) { + files.push(raw); + } } } if (files.length) console.log(files.join('\n')); @@ -429,7 +435,7 @@ FRONTMATTER=$(REVIEW_PATH="${REVIEW_PATH}" node -e " # Parse fields from frontmatter only (not full file) STATUS=$(echo "$FRONTMATTER" | grep "^status:" | cut -d: -f2 | xargs) FILES_REVIEWED=$(echo "$FRONTMATTER" | grep "^files_reviewed:" | cut -d: -f2 | xargs) -CRITICAL=$(echo "$FRONTMATTER" | grep "critical:" | head -1 | cut -d: -f2 | xargs) +CRITICAL=$(echo "$FRONTMATTER" | grep -E "^[[:space:]]*(critical|blocker):" | head -1 | cut -d: -f2 | xargs) WARNING=$(echo "$FRONTMATTER" | grep "warning:" | head -1 | cut -d: -f2 | xargs) INFO=$(echo "$FRONTMATTER" | grep "info:" | head -1 | cut -d: -f2 | xargs) TOTAL=$(echo "$FRONTMATTER" | grep "total:" | head -1 | cut -d: -f2 | xargs) @@ -478,7 +484,7 @@ Next steps: If critical > 0 or warning > 0, list top 3 issues inline: ```bash echo "Top issues:" -grep -A 3 "^### CR-\|^### WR-" "${REVIEW_PATH}" | head -n 12 +grep -A 3 "^### CR-\|^### BL-\|^### WR-" "${REVIEW_PATH}" | head -n 12 ``` **Note on tests:** Automated tests for this command and workflow are planned for Phase 4 (Pipeline Integration & Testing, requirement INFR-03). Phase 2 focuses on correct implementation; Phase 4 adds regression coverage across platforms. diff --git a/tests/code-review-pipeline-regression.test.cjs b/tests/code-review-pipeline-regression.test.cjs new file mode 100644 index 000000000..613c02bf3 --- /dev/null +++ b/tests/code-review-pipeline-regression.test.cjs @@ -0,0 +1,349 @@ +// allow-test-rule: source-text-is-the-product +// The workflow and agent .md files ARE the product: their text is loaded and +// executed/interpreted at runtime by the agent host. Testing that specific +// strings exist within these files tests the deployed contract, not an +// implementation detail. No runtime API exists to enumerate the label accept- +// list or filter-set definitions — the text IS the specification. +// +// Bug 1 (compute_file_scope) — The inline Node.js script embedded in the +// workflow .md is the parser. The test implements the identical parse logic as +// a pure JS function (mirroring lines 172-184 of code-review.md exactly) and +// asserts on its structured output. A separate docs-parity assertion checks +// that the workflow .md contains the hyphen-aware boundary regex and the +// em-dash/parenthetical stripping — both of which are the deployed contract. +// +// Bug 2 (present_results) — Tested both behaviourally (pure JS helper that +// mimics the grep|cut pipeline) and via docs-parity on the workflow .md text. +// +// Bugs 3 and reviewer contract — docs-parity only on agents/*.md: the filter- +// set definition and label-equivalence contract exist only as text in those +// files; there is no runtime enumeration API. + +'use strict'; + +const { describe, test } = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('node:fs'); +const path = require('node:path'); + +const ROOT = path.resolve(__dirname, '..'); +const WORKFLOW_PATH = path.join(ROOT, 'get-shit-done', 'workflows', 'code-review.md'); +const FIXER_PATH = path.join(ROOT, 'agents', 'gsd-code-fixer.md'); +const REVIEWER_PATH = path.join(ROOT, 'agents', 'gsd-code-reviewer.md'); + +// --------------------------------------------------------------------------- +// Pure-function implementation of the compute_file_scope Node script body. +// This mirrors the logic in code-review.md lines 172-184 exactly. +// If those lines change, this function must be updated in tandem (and the +// docs-parity assertions below will catch a mismatch at the regex level). +// --------------------------------------------------------------------------- +function parseKeyFiles(yaml) { + const files = []; + let inSection = null; + for (const line of yaml.split('\n')) { + if (/^\s+created:/.test(line)) { inSection = 'created'; continue; } + if (/^\s+modified:/.test(line)) { inSection = 'modified'; continue; } + // Hyphen-aware boundary: reset inSection for ANY key: line (including key-decisions:, etc.) + if (/^\s*[\w-]+:/.test(line) && !/^\s*-/.test(line)) { inSection = null; continue; } + if (inSection && /^\s+-\s+(.+)/.test(line)) { + let raw = line.match(/^\s+-\s+(.+)/)[1].trim(); + raw = raw.replace(/^['"]|['"]$/g, ''); + // Order matters: parens BEFORE em-dash because em-dashes can appear inside parens + raw = raw.replace(/\s+\([^)]*\)\s*$/, ''); + raw = raw.split(/\s+—\s/)[0].trim(); + if (/\//.test(raw) && /\.[A-Za-z0-9]+$/.test(raw)) { + files.push(raw); + } + } + } + return files; +} + +// --------------------------------------------------------------------------- +// Pure-function implementation of the present_results severity-label parser. +// Mirrors the grep -E "^\s*(critical|blocker):" | head -1 | cut -d: -f2 | xargs +// pipeline from code-review.md. +// --------------------------------------------------------------------------- +function parseFrontmatterCritical(frontmatter) { + const lines = frontmatter.split('\n'); + const match = lines.find((l) => /^\s*(critical|blocker):/.test(l)); + if (!match) return { critical: 0 }; + const value = match.split(':').slice(1).join(':').trim(); + return { critical: parseInt(value, 10) || 0 }; +} + +// --------------------------------------------------------------------------- +// BUG 1 — SUMMARY parser: compute_file_scope must not bleed prose from +// hyphenated sections (key-decisions:, patterns-established:, etc.) into the +// file list, and must strip em-dash descriptions and parentheticals. +// --------------------------------------------------------------------------- +describe('Bug 1 — compute_file_scope SUMMARY parser', () => { + test('extracts only key-files.created and key-files.modified entries', () => { + const yaml = [ + 'key-files:', + ' created:', + ' - app/foo.tsx', + ' modified:', + ' - lib/bar.ts', + 'key-decisions:', + ' - We chose RSC for performance reasons', + 'patterns-established:', + ' - Always validate at the boundary', + 'requirements-completed:', + ' - REQ-01 done', + ].join('\n'); + + const files = parseKeyFiles(yaml); + assert.deepStrictEqual(files.sort(), ['app/foo.tsx', 'lib/bar.ts'].sort()); + }); + + test('strips em-dash narrative from bullet: "app/foo.tsx — RSC catalogue with filters"', () => { + const yaml = [ + 'key-files:', + ' created:', + ' - app/foo.tsx — RSC catalogue with topic/mode/date filters', + ].join('\n'); + + const files = parseKeyFiles(yaml); + assert.deepStrictEqual(files, ['app/foo.tsx']); + }); + + test('strips parenthetical from bullet: "tests/bar.test.ts (122 lines — 17 assertions)"', () => { + const yaml = [ + 'key-files:', + ' created:', + ' - tests/bar.test.ts (122 lines — 17 assertions)', + ].join('\n'); + + const files = parseKeyFiles(yaml); + assert.deepStrictEqual(files, ['tests/bar.test.ts']); + }); + + test('hyphenated sections in any order produce identical results', () => { + const yamlA = [ + 'key-decisions:', + ' - Some decision', + 'key-files:', + ' created:', + ' - src/index.ts', + 'patterns-established:', + ' - Some pattern', + ].join('\n'); + + const yamlB = [ + 'patterns-established:', + ' - Some pattern', + 'key-files:', + ' created:', + ' - src/index.ts', + 'key-decisions:', + ' - Some decision', + ].join('\n'); + + assert.deepStrictEqual(parseKeyFiles(yamlA), parseKeyFiles(yamlB)); + assert.deepStrictEqual(parseKeyFiles(yamlA), ['src/index.ts']); + }); + + test('prose-only bullets from key-decisions are never included in file list', () => { + const yaml = [ + 'key-decisions:', + ' - We chose RSC for performance reasons', + ' - Deferred auth to Phase 3', + 'key-files:', + ' created:', + ' - app/page.tsx', + ].join('\n'); + + const files = parseKeyFiles(yaml); + assert.deepStrictEqual(files, ['app/page.tsx']); + }); + + // Docs-parity: the workflow .md must contain the hyphen-aware boundary regex + // so what we tested above is actually what is deployed. + test('code-review.md contains hyphen-aware boundary regex [\\w-]+', () => { + const src = fs.readFileSync(WORKFLOW_PATH, 'utf8'); + // Locate the Node script block in the compute_file_scope step + const scriptStart = src.indexOf('const files = [];'); + assert.ok(scriptStart !== -1, 'compute_file_scope script must contain "const files = [];"'); + const scriptEnd = src.indexOf('if (files.length)', scriptStart); + const scriptSection = src.slice(scriptStart, scriptEnd); + // Must use [\\w-]+ (hyphen-aware) not \\w+ only + const hasHyphenAwareRegex = scriptSection.includes('[\\\\w-]') || scriptSection.includes('[\\w-]'); + assert.ok( + hasHyphenAwareRegex, + 'compute_file_scope boundary regex must be hyphen-aware ([\\w-]+), found section:\n' + scriptSection + ); + }); + + // Docs-parity: the workflow .md must contain the em-dash and parenthetical stripping. + test('code-review.md contains em-dash split and parenthetical strip in script body', () => { + const src = fs.readFileSync(WORKFLOW_PATH, 'utf8'); + const scriptStart = src.indexOf('const files = [];'); + const scriptEnd = src.indexOf('if (files.length)', scriptStart); + const scriptSection = src.slice(scriptStart, scriptEnd); + assert.ok( + scriptSection.includes('replace(/\\s+\\([^)]*\\)\\s*$/, \'\')'), + 'Script must strip parentheticals with replace(/\\s+\\([^)]*\\)\\s*$/, \'\')' + ); + assert.ok( + scriptSection.includes('split(/\\s+—\\s'), + 'Script must split on em-dash to strip narrative' + ); + }); +}); + +// --------------------------------------------------------------------------- +// BUG 2 — severity-label parser: present_results must accept both `critical:` +// and `blocker:` as Critical-tier frontmatter keys. +// --------------------------------------------------------------------------- +describe('Bug 2 — present_results severity-label parser', () => { + test('frontmatter with blocker: 8 is parsed as critical: 8', () => { + const frontmatter = [ + 'phase: 03-courses', + 'reviewed: 2025-01-01T00:00:00Z', + 'findings:', + ' blocker: 8', + ' warning: 2', + ' info: 0', + ' total: 10', + 'status: issues_found', + ].join('\n'); + + const result = parseFrontmatterCritical(frontmatter); + assert.strictEqual(result.critical, 8); + }); + + test('frontmatter with critical: 5 is parsed as critical: 5', () => { + const frontmatter = [ + 'phase: 03-courses', + 'reviewed: 2025-01-01T00:00:00Z', + 'findings:', + ' critical: 5', + ' warning: 1', + ' info: 0', + ' total: 6', + 'status: issues_found', + ].join('\n'); + + const result = parseFrontmatterCritical(frontmatter); + assert.strictEqual(result.critical, 5); + }); + + test('frontmatter with neither critical nor blocker returns 0', () => { + const frontmatter = [ + 'phase: 03-courses', + 'findings:', + ' warning: 3', + ' info: 1', + ' total: 4', + 'status: issues_found', + ].join('\n'); + + const result = parseFrontmatterCritical(frontmatter); + assert.strictEqual(result.critical, 0); + }); + + // Docs-parity: the workflow .md must contain the updated grep pattern. + test('code-review.md present_results grep accepts both critical and blocker labels', () => { + const src = fs.readFileSync(WORKFLOW_PATH, 'utf8'); + assert.ok( + src.includes('grep -E "^[[:space:]]*(critical|blocker):"'), + 'code-review.md present_results must grep for both critical: and blocker: labels' + ); + }); + + // Docs-parity: the workflow .md must contain the updated grep for BL- headings. + test('code-review.md present_results grep includes BL- headings alongside CR- and WR-', () => { + const src = fs.readFileSync(WORKFLOW_PATH, 'utf8'); + assert.ok( + src.includes('### BL-') && src.includes('### CR-') && src.includes('### WR-'), + 'code-review.md present_results must grep for BL- alongside CR- and WR- headings' + ); + }); +}); + +// --------------------------------------------------------------------------- +// BUG 3 — fixer agent ID alphabet and filter sets must include BL-* alongside CR-*. +// --------------------------------------------------------------------------- +describe('Bug 3 — gsd-code-fixer BL-* inclusion in filter sets', () => { + test('finding_parser documents BL-\\d+ as Critical-tier-equivalent', () => { + const src = fs.readFileSync(FIXER_PATH, 'utf8'); + const parserStart = src.indexOf(''); + const parserEnd = src.indexOf(''); + assert.ok(parserStart !== -1, 'gsd-code-fixer.md must have a block'); + const parserSection = src.slice(parserStart, parserEnd); + assert.ok( + parserSection.includes('BL-'), + 'finding_parser block must document BL-* as a Critical-tier-equivalent ID prefix' + ); + }); + + test('parse_findings step documents severity as "Critical (CR-* or BL-*)"', () => { + const src = fs.readFileSync(FIXER_PATH, 'utf8'); + const stepStart = src.indexOf(''); + const stepEnd = src.indexOf('', stepStart); + assert.ok(stepStart !== -1, 'gsd-code-fixer.md must have a parse_findings step'); + const stepSection = src.slice(stepStart, stepEnd); + assert.ok( + stepSection.includes('CR-* or BL-*') || stepSection.includes('CR-* and BL-*'), + 'parse_findings step must describe Critical severity as "CR-* or BL-*"' + ); + }); + + test('critical_warning filter set includes BL-* alongside CR-* and WR-*', () => { + const src = fs.readFileSync(FIXER_PATH, 'utf8'); + const stepStart = src.indexOf(''); + const stepEnd = src.indexOf('', stepStart); + const stepSection = src.slice(stepStart, stepEnd); + + const critWarningIdx = stepSection.indexOf('critical_warning'); + assert.ok(critWarningIdx !== -1, 'parse_findings must define critical_warning filter'); + const lineStart = stepSection.lastIndexOf('\n', critWarningIdx); + const lineEnd = stepSection.indexOf('\n', critWarningIdx); + const filterLine = stepSection.slice(lineStart, lineEnd); + assert.ok( + filterLine.includes('BL-'), + 'critical_warning filter line must include BL-*: ' + filterLine.trim() + ); + }); + + test('sort order description mentions both CR-* and BL-* for Critical tier', () => { + const src = fs.readFileSync(FIXER_PATH, 'utf8'); + const stepStart = src.indexOf(''); + const stepEnd = src.indexOf('', stepStart); + const stepSection = src.slice(stepStart, stepEnd); + assert.ok( + stepSection.includes('BL-'), + 'parse_findings sort-order description must mention BL-* as Critical-tier alongside CR-*' + ); + }); +}); + +// --------------------------------------------------------------------------- +// REVIEWER CONTRACT — gsd-code-reviewer.md must acknowledge BL-/blocker: as +// an accepted alternative to CR-/critical: (tier-equivalent). +// --------------------------------------------------------------------------- +describe('Reviewer contract — gsd-code-reviewer.md label-equivalence', () => { + test('write_review step documents blocker: as accepted alternative to critical:', () => { + const src = fs.readFileSync(REVIEWER_PATH, 'utf8'); + const stepStart = src.indexOf(''); + const stepEnd = src.indexOf('', stepStart); + assert.ok(stepStart !== -1, 'gsd-code-reviewer.md must have a write_review step'); + const stepSection = src.slice(stepStart, stepEnd); + assert.ok( + stepSection.includes('blocker'), + 'write_review step must acknowledge blocker: as a tier-equivalent alternative to critical:' + ); + }); + + test('write_review step acknowledges BL- finding ID prefix as Critical-tier-equivalent', () => { + const src = fs.readFileSync(REVIEWER_PATH, 'utf8'); + const stepStart = src.indexOf(''); + const stepEnd = src.indexOf('', stepStart); + const stepSection = src.slice(stepStart, stepEnd); + assert.ok( + stepSection.includes('BL-'), + 'write_review step must acknowledge BL- as a Critical-tier-equivalent finding ID prefix' + ); + }); +});