mirror of
https://github.com/glittercowboy/get-shit-done
synced 2026-04-25 17:25:23 +02:00
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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
This commit is contained in:
Tom Boucher
@@ -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());
|
||||
}
|
||||
|
||||
139
tests/code-review-summary-parser.test.cjs
Normal file
139
tests/code-review-summary-parser.test.cjs
Normal file
@@ -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, []);
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user