From 86dd9e1b09ab03a79ea2bc3aa384c0e896903f83 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Sun, 12 Apr 2026 08:10:30 -0400 Subject: [PATCH] fix(2134): fix code-review SUMMARY.md parser section-reset for top-level keys (#2142) * test(2134): add failing test for code-review SUMMARY.md YAML parser section reset Demonstrates bug #2134: the section-reset regex in the inline node parser in get-shit-done/workflows/code-review.md uses \s+ (requires leading whitespace), so top-level YAML keys at column 0 (decisions:, metrics:, tags:) never reset inSection, causing their list items to be mis-classified as key_files.modified entries. RED test asserts that the buggy parser contaminates the file list with decision strings. GREEN test and additional tests verify correct behaviour with the fix. Co-Authored-By: Claude Sonnet 4.6 * fix(2134): fix YAML parser section reset to handle top-level keys (\s* not \s+) The inline node parser in compute_file_scope (Tier 2) used \s+ in the section-reset regex, requiring leading whitespace. Top-level YAML keys at column 0 (decisions:, metrics:, tags:) never matched, so inSection was never cleared and their list items were mis-classified as key_files.modified entries. Fix: change \s+ to \s* in both the reset check and its dash-guard companion so any key at any indentation level (including column 0) resets inSection. Before: /^\s+\w+:/.test(line) && !/^\s+-/.test(line) After: /^\s*\w+:/.test(line) && !/^\s*-/.test(line) Closes #2134 Co-Authored-By: Claude Sonnet 4.6 --------- Co-authored-by: Claude Sonnet 4.6 --- get-shit-done/workflows/code-review.md | 2 +- tests/code-review-summary-parser.test.cjs | 139 ++++++++++++++++++++++ 2 files changed, 140 insertions(+), 1 deletion(-) create mode 100644 tests/code-review-summary-parser.test.cjs diff --git a/get-shit-done/workflows/code-review.md b/get-shit-done/workflows/code-review.md index d464bbdd1..bf07b8606 100644 --- a/get-shit-done/workflows/code-review.md +++ b/get-shit-done/workflows/code-review.md @@ -172,7 +172,7 @@ 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()); } diff --git a/tests/code-review-summary-parser.test.cjs b/tests/code-review-summary-parser.test.cjs new file mode 100644 index 000000000..72c541c22 --- /dev/null +++ b/tests/code-review-summary-parser.test.cjs @@ -0,0 +1,139 @@ +'use strict'; + +const { describe, it } = require('node:test'); +const assert = require('node:assert/strict'); + +// Replicates the inline node -e parser from get-shit-done/workflows/code-review.md +// step compute_file_scope, Tier 2 (lines ~172-181). +// +// Bug #2134: the section-reset regex uses \s+ (requires leading whitespace), so +// top-level YAML keys at column 0 (e.g. `decisions:`) never reset inSection. +// Items from subsequent top-level lists are therefore mis-classified as +// key_files.modified entries. + +/** + * Extracts files from SUMMARY.md YAML frontmatter using the CURRENT (buggy) logic + * copied verbatim from code-review.md. + */ +function parseFilesWithBuggyLogic(frontmatterYaml) { + const files = []; + let inSection = null; + for (const line of frontmatterYaml.split('\n')) { + if (/^\s+created:/.test(line)) { inSection = 'created'; continue; } + if (/^\s+modified:/.test(line)) { inSection = 'modified'; continue; } + // BUG: \s+ requires leading whitespace — top-level keys like `decisions:` don't match + 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()); + } + } + return files; +} + +/** + * Extracts files using the FIXED logic (\s* instead of \s+). + */ +function parseFilesWithFixedLogic(frontmatterYaml) { + const files = []; + let inSection = null; + for (const line of frontmatterYaml.split('\n')) { + if (/^\s+created:/.test(line)) { inSection = 'created'; continue; } + if (/^\s+modified:/.test(line)) { inSection = 'modified'; continue; } + // FIX: \s* allows zero leading whitespace — handles top-level YAML keys + 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()); + } + } + return files; +} + +// SUMMARY.md YAML frontmatter that mirrors a realistic post-execution artifact. +// key_files.modified has ONE real file; decisions has TWO entries that must NOT +// appear in the extracted file list. +const FRONTMATTER = [ + 'type: summary', + 'phase: "02"', + 'key_files:', + ' modified:', + ' - src/real-file.js', + ' created:', + ' - src/new-file.js', + 'decisions:', + ' - Used async/await over callbacks', + ' - Kept error handling inline', + 'metrics:', + ' lines_changed: 42', + 'tags:', + ' - refactor', + ' - async', +].join('\n'); + +describe('code-review SUMMARY.md YAML parser', () => { + it('RED: buggy parser mis-classifies decisions entries as files (demonstrates the bug)', () => { + const files = parseFilesWithBuggyLogic(FRONTMATTER); + + // With the bug, `decisions:` at column 0 never resets inSection, so the + // two decision strings are incorrectly captured as modified files. + // This assertion documents the broken behavior we are fixing. + const hasDecisionContamination = files.some( + (f) => f === 'Used async/await over callbacks' || f === 'Kept error handling inline' + ); + assert.ok( + hasDecisionContamination, + 'Expected buggy parser to include decision entries in file list, but it did not — ' + + 'the bug may already be fixed or the test replication is wrong. Got: ' + + JSON.stringify(files) + ); + }); + + it('GREEN: fixed parser returns only the actual file paths', () => { + const files = parseFilesWithFixedLogic(FRONTMATTER); + + assert.deepStrictEqual( + files.sort(), + ['src/new-file.js', 'src/real-file.js'], + 'Fixed parser should return only the two real file paths, not decision strings' + ); + }); + + it('fixed parser: modified-only frontmatter with top-level sibling keys', () => { + const yaml = [ + 'key_files:', + ' modified:', + ' - src/a.ts', + ' - src/b.ts', + 'decisions:', + ' - Some decision', + 'metrics:', + ' count: 2', + ].join('\n'); + + const files = parseFilesWithFixedLogic(yaml); + assert.deepStrictEqual(files.sort(), ['src/a.ts', 'src/b.ts']); + }); + + it('fixed parser: created-only frontmatter with top-level sibling keys', () => { + const yaml = [ + 'key_files:', + ' created:', + ' - src/brand-new.ts', + 'tags:', + ' - feature', + ].join('\n'); + + const files = parseFilesWithFixedLogic(yaml); + assert.deepStrictEqual(files, ['src/brand-new.ts']); + }); + + it('fixed parser: no key_files section returns empty array', () => { + const yaml = [ + 'type: summary', + 'decisions:', + ' - A decision', + ].join('\n'); + + const files = parseFilesWithFixedLogic(yaml); + assert.deepStrictEqual(files, []); + }); +});