admin_reason: missing-secondary-reviewer — self-authored overnight sweep; isolated adversarial review round completed (MEDIUM table-bleed finding fixed with RED/GREEN evidence) and sha-pinned bench 46331/0 on the merged head.
This commit is contained in:
Tom Boucher
5
.changeset/merry-lynx-climb.md
Normal file
5
.changeset/merry-lynx-climb.md
Normal file
@@ -0,0 +1,5 @@
|
||||
---
|
||||
type: Fixed
|
||||
pr: 4826
|
||||
---
|
||||
**Roadmap queries read past hard-wrapped Goal/Requirements fields** — the roadmapper soft-wraps long fields at ~85 chars, but five single-line field regexes truncated every wrapped Goal/Requirements at the first line: plan-phase's phase_req_ids silently dropped continuation-line REQ IDs (escaping the Requirements Coverage Gate), get-phase/analyze cut the goal mid-sentence, and phase complete left later REQs Pending with empty warnings. A shared multiline extractor now reads wrapped fields to the next field label. (#4731)
|
||||
26
src/init.cts
26
src/init.cts
@@ -141,7 +141,6 @@ const { resolveCapabilityRuntimeState } = capabilityStateMod;
|
||||
void stripShippedMilestones;
|
||||
|
||||
// Accept all bold/colon variants of the Requirements header (#2769)
|
||||
const REQUIREMENTS_HEADER_RE = /^\*\*Requirements:?\*\*[^\S\n]*:?[^\S\n]*([^\n]*)$/m;
|
||||
|
||||
// #2056/#2104: isForeignPrefixedPhaseQuery is imported from phase-id.cts
|
||||
// (the canonical predicate). parsePhasePrefix is no longer needed locally.
|
||||
@@ -939,9 +938,14 @@ function cmdInitExecutePhase(
|
||||
has_reviews: false,
|
||||
};
|
||||
});
|
||||
const reqMatch = (roadmapPhase?.['section'] as string | undefined)?.match(REQUIREMENTS_HEADER_RE);
|
||||
const reqExtracted = reqMatch
|
||||
? reqMatch[1].replace(/[\[\]]/g, '').split(',').map((s) => s.trim()).filter(Boolean).join(', ')
|
||||
// #4731: multiline-aware — the Requirements field may hard-wrap, so the
|
||||
// value is extracted past the line break before the ID scan.
|
||||
const phaseSection = roadmapPhase?.['section'] as string | undefined;
|
||||
const reqLine = phaseSection
|
||||
? roadmapParser.extractPhaseFieldMultiline(phaseSection, 'Requirements')
|
||||
: null;
|
||||
const reqExtracted = reqLine
|
||||
? reqLine.replace(/[\[\]]/g, '').split(',').map((s) => s.trim()).filter(Boolean).join(', ')
|
||||
: null;
|
||||
const phase_req_ids = reqExtracted && reqExtracted !== 'TBD' ? reqExtracted : null;
|
||||
|
||||
@@ -1109,9 +1113,14 @@ function cmdInitPlanPhase(
|
||||
has_reviews: false,
|
||||
};
|
||||
});
|
||||
const reqMatch = (roadmapPhase?.['section'] as string | undefined)?.match(REQUIREMENTS_HEADER_RE);
|
||||
const reqExtracted = reqMatch
|
||||
? reqMatch[1].replace(/[\[\]]/g, '').split(',').map((s) => s.trim()).filter(Boolean).join(', ')
|
||||
// #4731: multiline-aware — the Requirements field may hard-wrap, so the
|
||||
// value is extracted past the line break before the ID scan.
|
||||
const phaseSection = roadmapPhase?.['section'] as string | undefined;
|
||||
const reqLine = phaseSection
|
||||
? roadmapParser.extractPhaseFieldMultiline(phaseSection, 'Requirements')
|
||||
: null;
|
||||
const reqExtracted = reqLine
|
||||
? reqLine.replace(/[\[\]]/g, '').split(',').map((s) => s.trim()).filter(Boolean).join(', ')
|
||||
: null;
|
||||
const phase_req_ids = reqExtracted && reqExtracted !== 'TBD' ? reqExtracted : null;
|
||||
|
||||
@@ -2841,8 +2850,7 @@ function cmdInitManager(cwd: string, raw: boolean): void {
|
||||
: content.length;
|
||||
const section = content.slice(sectionStart, sectionEnd);
|
||||
|
||||
const goalMatch = section.match(/\*\*Goal(?::\*\*|\*\*:)\s*([^\n]+)/i);
|
||||
const goal = goalMatch ? goalMatch[1].trim() : null;
|
||||
const goal = roadmapParser.extractPhaseFieldMultiline(section, 'Goal');
|
||||
|
||||
const dependsMatch = section.match(/\*\*Depends on(?::\*\*|\*\*:)\s*([^\n]+)/i);
|
||||
const depends_on = dependsMatch ? dependsMatch[1].trim() : null;
|
||||
|
||||
@@ -3896,16 +3896,22 @@ function cmdPhaseComplete(cwd: string, phaseNum: string, raw: boolean): void {
|
||||
);
|
||||
|
||||
const sectionText = phaseSectionMatch ? phaseSectionMatch[1] : '';
|
||||
const reqMatch = sectionText.match(
|
||||
/\*\*Requirements:?\*\*[^\S\n]*:?[^\S\n]*([^\n]+)/i,
|
||||
);
|
||||
// #4731: multiline-aware — hard-wrapped Requirements read past the
|
||||
// line break before the ID scan. The shared extractor also stops at
|
||||
// headings and table rows, so a Requirements field followed by the
|
||||
// Traceability table cannot bleed other phases' REQ-IDs into the
|
||||
// citation scan (isolated-review MEDIUM on the inline lookahead,
|
||||
// whose lazy capture swallowed everything to section end).
|
||||
const reqLine = sectionText
|
||||
? roadmapParserMod.extractPhaseFieldMultiline(sectionText, 'Requirements')
|
||||
: null;
|
||||
|
||||
const originalReqContent = fs.readFileSync(reqPath, 'utf-8');
|
||||
let reqContent = originalReqContent;
|
||||
|
||||
// #2316: `citedReqIds` — the REQ-IDs ROADMAP's own **Requirements:**
|
||||
// line for this phase actually cites — is hoisted out of the
|
||||
// `if (reqMatch)` block (previously scoped only inside it) so the
|
||||
// `if (reqLine)` block (previously scoped only inside it) so the
|
||||
// ghost-ID cross-check below (~#2316-1) can consult it. `TBD` is the
|
||||
// literal placeholder `phase.add`/`-batch`/`-insert` seed
|
||||
// (`**Requirements**: TBD`, src/phase.cts:833,920,1078) — never a
|
||||
@@ -3918,18 +3924,18 @@ function cmdPhaseComplete(cwd: string, phaseNum: string, raw: boolean): void {
|
||||
// `else`, discarding this fact silently instead of surfacing it.
|
||||
const traceabilityWriteMisses: string[] = [];
|
||||
|
||||
if (reqMatch) {
|
||||
if (reqLine) {
|
||||
// #2334 HIGH 3 + #3697: selection and under-selection detection both
|
||||
// live in `analyzeRequirementsLine` (module scope, above), extracted in
|
||||
// round 3 so the parser is directly testable — a closure in here is
|
||||
// reachable only by spawning the CLI, which no fast-check property test
|
||||
// can do. `citedReqIds` is byte-identical to the expression that stood
|
||||
// here; nothing about what phase-complete MARKS has changed.
|
||||
const reqLineAnalysis = analyzeRequirementsLine(reqMatch[1]);
|
||||
const reqLineAnalysis = analyzeRequirementsLine(reqLine);
|
||||
citedReqIds = reqLineAnalysis.citedReqIds;
|
||||
const reqLineWarning = formatRequirementsLineWarning(
|
||||
phaseNum,
|
||||
reqMatch[1],
|
||||
reqLine,
|
||||
reqLineAnalysis,
|
||||
);
|
||||
if (reqLineWarning) {
|
||||
|
||||
@@ -2255,5 +2255,45 @@ export = {
|
||||
// #3641: the scope axis's phase-ENTRY predicate, exported so roadmap
|
||||
// validate's V004 document-level check routes through the same single
|
||||
// owner (and its convention gate) instead of a private inline copy.
|
||||
extractPhaseFieldMultiline,
|
||||
hasPhaseEntries,
|
||||
};
|
||||
|
||||
|
||||
/**
|
||||
* #4731 — extract a bold-labeled phase field (Goal, Requirements, ...) reading
|
||||
* past hard-wrapped continuation lines. The roadmapper soft-wraps long fields
|
||||
* at ~85 chars, so a single-line capture silently truncated every wrapped
|
||||
* Goal/Requirements. Stops at: another `**Field**` label line, a blank line,
|
||||
* or any heading. Wrapped lines are trimmed and joined with single spaces; a
|
||||
* single-line field is returned unchanged (trimmed). Returns null when the
|
||||
* label is absent.
|
||||
*/
|
||||
function extractPhaseFieldMultiline(section: string, label: string): string | null {
|
||||
// #2769 label shapes: `**X:**`, `**X**:`, and the spaced `**X** :`.
|
||||
const labelRe = new RegExp(
|
||||
'\\*\\*' + label + '(?::\\*\\*|\\*\\*\\s*:?)\\s*([^\\n]+)',
|
||||
'i',
|
||||
);
|
||||
const match = section.match(labelRe);
|
||||
if (!match) return null;
|
||||
const startIdx = match.index ?? 0;
|
||||
const after = section.slice(startIdx + match[0].length);
|
||||
const firstLine = match[1].trim();
|
||||
const contLines = [];
|
||||
const lines = after.split('\n');
|
||||
for (let li = 0; li < lines.length; li++) {
|
||||
const raw = lines[li];
|
||||
// The split's first element is the remainder of the captured first line's
|
||||
// own newline — an empty leading element is the line break, not a blank
|
||||
// continuation line.
|
||||
if (li === 0 && !raw.trim()) continue;
|
||||
if (!raw.trim()) break;
|
||||
if (/^\s*\*\*[A-Z][A-Za-z ]*:?(\*\*)?:?\s/.test(raw)) break;
|
||||
if (/^\s*#{1,4}\s/.test(raw)) break;
|
||||
if (/^\s*\|/.test(raw)) break;
|
||||
contLines.push(raw.trim());
|
||||
}
|
||||
return [firstLine, ...contLines].join(' ').trim() || null;
|
||||
}
|
||||
|
||||
|
||||
@@ -231,9 +231,9 @@ function searchPhaseInContent(content: string, escapedPhase: string, phaseNum: s
|
||||
|
||||
const section = content.slice(headerIndex, sectionEnd).trim();
|
||||
|
||||
// Extract goal if present (supports both **Goal:** and **Goal**: formats)
|
||||
const goalMatch = section.match(/\*\*Goal(?::\*\*|\*\*:)\s*([^\n]+)/i);
|
||||
const goal = goalMatch ? goalMatch[1].trim() : null;
|
||||
// Extract goal if present (supports both **Goal:** and **Goal**: formats).
|
||||
// #4731: multiline-aware — hard-wrapped Goals read past the line break.
|
||||
const goal = roadmapParserModule.extractPhaseFieldMultiline(section, 'Goal');
|
||||
|
||||
// Mode: vertical-MVP slice mode flag. Lowercased + trimmed for canonical
|
||||
// comparison; unrecognized values are preserved verbatim for forward-compat.
|
||||
@@ -509,8 +509,7 @@ function collectAnalyzePhases(
|
||||
const sectionEnd = nextHeader ? sectionStart + nextHeader.index! : content.length;
|
||||
const section = content.slice(sectionStart, sectionEnd);
|
||||
|
||||
const goalMatch = section.match(/\*\*Goal(?::\*\*|\*\*:)\s*([^\n]+)/i);
|
||||
const goal = goalMatch ? goalMatch[1].trim() : null;
|
||||
const goal = roadmapParserModule.extractPhaseFieldMultiline(section, 'Goal');
|
||||
|
||||
const modeMatch = section.match(/\*\*Mode(?::\*\*|\*\*:)\s*([^\n]+)/i);
|
||||
const mode = modeMatch ? modeMatch[1].trim().toLowerCase() : null;
|
||||
@@ -1645,6 +1644,7 @@ function cmdRoadmapAnnotateDependencies(cwd: string, phaseNum: string | null | u
|
||||
}, raw, updated ? `annotated ${waves.length} wave(s), ${crossCuttingTruths.length} constraint(s)` : 'skipped (already annotated or no plan list)');
|
||||
}
|
||||
|
||||
|
||||
export = {
|
||||
cmdRoadmapGetPhase,
|
||||
getRoadmapPhaseWithFallback,
|
||||
|
||||
@@ -5275,3 +5275,68 @@ describe('#4040 partial-init completeness fields', () => {
|
||||
'new-project gate must resume a partial bootstrap instead of erroring');
|
||||
});
|
||||
});
|
||||
|
||||
// ── #4731 — hard-wrapped Goal/Requirements fields read past the line break ───
|
||||
// The roadmapper soft-wraps long fields at ~85 chars; the five single-line
|
||||
// field regexes truncated every wrapped Goal/Requirements at the first line:
|
||||
// plan-phase's phase_req_ids silently dropped the IDs on continuation lines
|
||||
// (silently escaping the Requirements Coverage Gate) and get-phase/analyze
|
||||
// cut the goal mid-sentence.
|
||||
describe('init plan-phase — wrapped Goal/Requirements fields (#4731)', () => {
|
||||
let tmpDir;
|
||||
|
||||
beforeEach(() => {
|
||||
tmpDir = createTempDir('gsd-4731-');
|
||||
fs.mkdirSync(path.join(tmpDir, '.planning'), { recursive: true });
|
||||
fs.writeFileSync(path.join(tmpDir, '.planning', 'STATE.md'), '# State\n');
|
||||
fs.writeFileSync(
|
||||
path.join(tmpDir, '.planning', 'REQUIREMENTS.md'),
|
||||
['# Requirements', '', '- [ ] **REQ-01**: thing', '- [ ] **REQ-11**: thing'].join('\n'),
|
||||
);
|
||||
fs.writeFileSync(
|
||||
path.join(tmpDir, '.planning', 'ROADMAP.md'),
|
||||
[
|
||||
'# Roadmap',
|
||||
'',
|
||||
'## Phases',
|
||||
'',
|
||||
'- [ ] **Phase 1: Demo** - Goal',
|
||||
'',
|
||||
'## Phase Details',
|
||||
'',
|
||||
'### Phase 1: Demo',
|
||||
'',
|
||||
'**Goal:** Deliver a small demo feature that exercises the planning pipeline end to end with a',
|
||||
'goal sentence long enough to wrap onto a second line',
|
||||
'**Requirements**: REQ-01, REQ-02, REQ-03, REQ-04, REQ-05, REQ-06, REQ-07, REQ-08, REQ-09,',
|
||||
'REQ-10, REQ-11',
|
||||
'**Plans**: 1 plans',
|
||||
'',
|
||||
].join('\n'),
|
||||
);
|
||||
const phaseDir = path.join(tmpDir, '.planning', 'phases', '01-demo');
|
||||
fs.mkdirSync(phaseDir, { recursive: true });
|
||||
fs.writeFileSync(path.join(phaseDir, '01-01-PLAN.md'), '---\nphase: 01-demo\nplan: 01\n---\n# Plan');
|
||||
fs.writeFileSync(path.join(phaseDir, '01-01-SUMMARY.md'), '---\nphase: 01-demo\nplan: 01\n---\n# Summary');
|
||||
});
|
||||
|
||||
afterEach(() => cleanup(tmpDir));
|
||||
|
||||
test('wrapped Requirements yield all eleven IDs (#4731)', () => {
|
||||
const result = runGsdTools('init plan-phase 1 --pick phase_req_ids', tmpDir);
|
||||
assert.equal(
|
||||
result.output.trim(),
|
||||
'REQ-01, REQ-02, REQ-03, REQ-04, REQ-05, REQ-06, REQ-07, REQ-08, REQ-09, REQ-10, REQ-11',
|
||||
'continuation-line REQ-10/REQ-11 must not be silently dropped',
|
||||
);
|
||||
});
|
||||
|
||||
test('wrapped Goal is returned in full (#4731)', () => {
|
||||
const result = runGsdTools('query roadmap.get-phase 1 --pick goal', tmpDir);
|
||||
assert.equal(
|
||||
result.output.trim(),
|
||||
'Deliver a small demo feature that exercises the planning pipeline end to end with a goal sentence long enough to wrap onto a second line',
|
||||
'the goal must read past the hard wrap',
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -12385,6 +12385,140 @@ describe('issue #2334: ghost-REQ-ID classification must probe write surfaces, no
|
||||
);
|
||||
});
|
||||
|
||||
// ── #4731 review follow-up — the phase-complete citation scan must stop at the ─
|
||||
// section's own table. The inline lookahead the first cut used stopped only at
|
||||
// the next `**Bold**` label or end-of-section, so a Requirements field followed
|
||||
// by a table bled every ID-shaped cell after it into the citation scan — and
|
||||
// phase complete then ticked other phases' checkboxes and flipped their
|
||||
// Traceability rows. The shared extractor stops at headings, table rows, and
|
||||
// blank lines; these pins hold that boundary.
|
||||
describe('#4731 review follow-up: Requirements citation scan stops at the section table', () => {
|
||||
function build4731TableBleedFixture() {
|
||||
const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-4731-table-bleed-'));
|
||||
const planDir = path.join(tmpDir, '.planning');
|
||||
const phase1Dir = path.join(planDir, 'phases', '01-preset');
|
||||
const phase2Dir = path.join(planDir, 'phases', '02-next');
|
||||
fs.mkdirSync(phase1Dir, { recursive: true });
|
||||
fs.mkdirSync(phase2Dir, { recursive: true });
|
||||
|
||||
// REQ-99 belongs to phase 02 — it sits in phase 01's deliverables table and
|
||||
// is still Pending in REQUIREMENTS.md. Phase 01's own IDs are hard-wrapped
|
||||
// across three lines (the #4731 shape).
|
||||
fs.writeFileSync(path.join(planDir, 'REQUIREMENTS.md'), [
|
||||
'# Requirements',
|
||||
'',
|
||||
'## Active',
|
||||
'',
|
||||
'- [ ] **REQ-11**: first wrapped requirement',
|
||||
'- [ ] **REQ-12**: second wrapped requirement',
|
||||
'- [ ] **REQ-13**: third wrapped requirement',
|
||||
'- [ ] **REQ-99**: phase 02 requirement, still pending',
|
||||
'',
|
||||
'## Traceability',
|
||||
'',
|
||||
'| Requirement | Phase | Status |',
|
||||
'|-------------|-------|--------|',
|
||||
'| REQ-11 | Phase 1 | Pending |',
|
||||
'| REQ-12 | Phase 1 | Pending |',
|
||||
'| REQ-13 | Phase 1 | Pending |',
|
||||
'| REQ-99 | Phase 2 | Pending |',
|
||||
'',
|
||||
].join('\n'));
|
||||
|
||||
fs.writeFileSync(path.join(planDir, 'ROADMAP.md'), [
|
||||
'# Roadmap',
|
||||
'',
|
||||
'- [ ] Phase 01: Preset',
|
||||
'- [ ] Phase 02: Next',
|
||||
'',
|
||||
'### Phase 01: Preset',
|
||||
'**Goal:** Build preset',
|
||||
'**Requirements**: REQ-11,',
|
||||
'REQ-12,',
|
||||
'REQ-13',
|
||||
'',
|
||||
'| Deliverable | Requirement |',
|
||||
'|-------------|-------------|',
|
||||
'| Widget | REQ-99 |',
|
||||
'',
|
||||
'**Plans:** 1 plans',
|
||||
'',
|
||||
'### Phase 02: Next',
|
||||
'**Goal:** whatever',
|
||||
'**Requirements:** REQ-99',
|
||||
'**Plans:** 1 plans',
|
||||
'',
|
||||
'## Progress',
|
||||
'',
|
||||
'| Phase | Plans Complete | Status | Completed |',
|
||||
'|-------|----------------|--------|-----------|',
|
||||
'| 01. Preset | 0/1 | Not started | - |',
|
||||
'| 02. Next | 0/1 | Not started | - |',
|
||||
'',
|
||||
].join('\n'));
|
||||
|
||||
fs.writeFileSync(path.join(planDir, 'STATE.md'), [
|
||||
'---', 'milestone: v1.3', '---',
|
||||
'# State',
|
||||
'',
|
||||
'**Current Phase:** 01',
|
||||
'**Completed Phases:** 0',
|
||||
'**Total Phases:** 2',
|
||||
'**Progress:** 0%',
|
||||
'',
|
||||
].join('\n'));
|
||||
|
||||
for (const [dir, n] of [[phase1Dir, '01'], [phase2Dir, '02']]) {
|
||||
fs.writeFileSync(path.join(dir, `${n}-01-PLAN.md`), '# Plan\nDo the work.\n');
|
||||
fs.writeFileSync(path.join(dir, `${n}-01-SUMMARY.md`), '# Summary\nDone.\n');
|
||||
}
|
||||
|
||||
return tmpDir;
|
||||
}
|
||||
|
||||
test(
|
||||
'#4731-followup: a wrapped Requirements field followed by a table cites ONLY the wrapped IDs — the table\'s REQ-99 stays Pending',
|
||||
() => {
|
||||
const tmpDir = build4731TableBleedFixture();
|
||||
try {
|
||||
const { output } = runVerifiedPhaseComplete(['phase', 'complete', '1'], tmpDir);
|
||||
const parsed = JSON.parse(output);
|
||||
const warnings = parsed.warnings || [];
|
||||
const reqContent = fs.readFileSync(path.join(tmpDir, '.planning', 'REQUIREMENTS.md'), 'utf-8');
|
||||
for (const id of ['REQ-11', 'REQ-12', 'REQ-13']) {
|
||||
assert.ok(
|
||||
new RegExp(`-\\s*\\[x\\]\\s*\\*\\*${id}\\*\\*`, 'i').test(reqContent),
|
||||
`#4731-followup FAILED (fixture invariant): ${id} is cited by the wrapped Requirements field and must be ticked.\n${reqContent}`,
|
||||
);
|
||||
}
|
||||
// The discriminator: REQ-99 appears AFTER the field (in the section's
|
||||
// deliverables table) — the scan must have stopped at the table.
|
||||
assert.ok(
|
||||
/-\s*\[\s\]\s*\*\*REQ-99\*\*/i.test(reqContent),
|
||||
`#4731-followup FAILED: REQ-99 lives in the table AFTER the Requirements field (it belongs to phase 02) ` +
|
||||
`and must NOT have been ticked by phase 01's completion scan.\n${reqContent}`,
|
||||
);
|
||||
const req99Row = reqContent.split(/\r?\n/)
|
||||
.filter((l) => l.trim().startsWith('|'))
|
||||
.map((l) => splitTableRow(l))
|
||||
.find((cells) => cells[0] && cells[0].trim().toLowerCase() === 'req-99');
|
||||
assert.ok(
|
||||
req99Row && /^Pending$/i.test(req99Row[req99Row.length - 1].trim()),
|
||||
`#4731-followup FAILED: REQ-99's Traceability row must stay Pending — the citation scan bled into the table.\n${reqContent}`,
|
||||
);
|
||||
assert.ok(
|
||||
!warnings.some((w) => /not registered anywhere/i.test(w) && /REQ-99/i.test(w)),
|
||||
`#4731-followup FAILED: REQ-99 must not surface as a ghost — it is registered; the scan just must not reach it, ` +
|
||||
`got: ${JSON.stringify(warnings)}`,
|
||||
);
|
||||
assert.strictEqual(parsed.requirements_updated, true, "#4731-followup FAILED (fixture invariant): the phase's own IDs must have been written");
|
||||
} finally {
|
||||
cleanup(tmpDir);
|
||||
}
|
||||
},
|
||||
);
|
||||
});
|
||||
|
||||
// ─────────────────────────────────────────────────────────────────────────────
|
||||
// Regressions: issue #3697 — the `**Requirements**:` tokenizer UNDER-selects
|
||||
// silently. #2339 fixed OVER-selection (the shape filter) and added the
|
||||
|
||||
Reference in New Issue
Block a user