diff --git a/.changeset/fix-scenario-loss-parity.md b/.changeset/fix-scenario-loss-parity.md new file mode 100644 index 0000000000..bcd2981b84 --- /dev/null +++ b/.changeset/fix-scenario-loss-parity.md @@ -0,0 +1,7 @@ +--- +"@fission-ai/openspec": patch +--- + +### Bug Fixes + +- **Stop silently dropping unlabeled scenarios on archive** — `openspec validate` and `openspec archive` now recognize every level-4 (`####` followed by whitespace) child of a requirement as a scenario, matching how the spec is counted elsewhere. Before, the scenario-loss guard only recognized headers written exactly as `#### Scenario:`, so a `MODIFIED` requirement that dropped a differently-labeled child (for example `#### Edge case`) passed validation and was then permanently deleted by archive with no warning. Both paths now agree, so the loss is caught at authoring time. Scenario names are normalized when comparing (an optional `Scenario:` prefix and a CommonMark closing `#` run are ignored), so simply relabeling a scenario is not mistaken for dropping one. diff --git a/src/core/parsers/requirement-blocks.ts b/src/core/parsers/requirement-blocks.ts index 2f2c8a2004..b861cb4332 100644 --- a/src/core/parsers/requirement-blocks.ts +++ b/src/core/parsers/requirement-blocks.ts @@ -1,4 +1,4 @@ -import { buildCodeFenceMask } from './requirement-text.js'; +import { buildCodeFenceMask, SCENARIO_HEADER } from './requirement-text.js'; export interface RequirementBlock { headerLine: string; // e.g., '### Requirement: Something' @@ -362,27 +362,61 @@ export function findMissingCurrentScenarios(current: RequirementBlock, incoming: return missing; } +/** + * Any non-fenced level-4 header on the given (masked) line. Reuses the spec + * path's SCENARIO_HEADER so the two counters cannot drift apart. + */ +function scenarioHeaderAt(lines: string[], mask: boolean[], index: number): boolean { + return !mask[index] && SCENARIO_HEADER.test(lines[index]); +} + +/** + * The scenario name for a `#### ` header, matching the label the author reads: + * the header text with the leading `####`, an optional CommonMark closing `#` + * run (`#### Foo ####` renders as `Foo`), and an optional `Scenario:` prefix + * stripped. Both the current and incoming blocks run through here, so the + * comparison in findMissingCurrentScenarios stays internally consistent + * regardless of label — and two headers that render to the same title (one + * ATX-closed, one not) are not mistaken for a dropped scenario. + */ +function scenarioNameAt(line: string): string { + return line + .replace(SCENARIO_HEADER, '') + // Optional ATX closing sequence. CommonMark only treats a trailing `#` run + // as a close when it is preceded by a space or tab — not any Unicode space — + // so this uses `[ \t]`, not `\s`. A looser `\s` could strip a `#` run after + // an exotic space (e.g. NBSP) that CommonMark keeps, folding two distinct + // scenario names into one and masking a real loss. `[ \t]` keeps the fold + // faithful to how the header actually renders. + .replace(/[ \t]+#+[ \t]*$/, '') + .replace(/^Scenario:\s*/i, '') + .trim(); +} + function parseScenarioBlocks(requirementRaw: string): ScenarioBlock[] { const lines = requirementRaw.replace(/\r\n?/g, '\n').split('\n'); - // A `#### Scenario:` inside a fenced example is not a real scenario. The - // validator's countScenarios already ignores fenced lines; the drift check - // must agree with it, or a fenced sample can false-abort an archive (or - // mask a genuinely dropped scenario). + // A scenario is ANY non-fenced `#### ` header, matching the spec path's + // SCENARIO_HEADER / countScenarios (requirement-text.ts) exactly — not only + // `#### Scenario:`. The two MUST agree: a level-4 child whose header is not + // literally `Scenario:` (e.g. `#### Edge case`) is still a scenario the spec + // path counts, so a MODIFIED block that drops it would otherwise slip past + // this loss check and be deleted by archive with no error (the parity the + // SCENARIO_HEADER comment warns not to break). A `####` inside a fenced + // example is masked out, matching countScenarios. const mask = buildCodeFenceMask(lines); const scenarios: ScenarioBlock[] = []; let index = 0; while (index < lines.length) { - const headerMatch = mask[index] ? null : lines[index].match(/^####\s*Scenario:\s*(.+)\s*$/); - if (!headerMatch) { + if (!scenarioHeaderAt(lines, mask, index)) { index++; continue; } const start = index; - const name = headerMatch[1].trim(); + const name = scenarioNameAt(lines[index]); index++; - while (index < lines.length && (mask[index] || !/^####\s*Scenario:\s*(.+)\s*$/.test(lines[index]))) { + while (index < lines.length && !scenarioHeaderAt(lines, mask, index)) { index++; } diff --git a/src/core/parsers/requirement-text.ts b/src/core/parsers/requirement-text.ts index 9841e3ddcf..a750430dc7 100644 --- a/src/core/parsers/requirement-text.ts +++ b/src/core/parsers/requirement-text.ts @@ -23,10 +23,11 @@ const HEADER_LINE = /^#{1,6}\s/; /** * A level-4 header. Deliberately matches ANY `####` header, not only * `#### Scenario:` — the spec path treats every level-4 child of a requirement - * as a scenario, so the delta counter must too (parity). Don't tighten this to - * `Scenario:` without changing both paths together. + * as a scenario, so the delta counter must too (parity). The delta/loss path + * reuses this exact constant via `scenarioHeaderAt` in requirement-blocks.ts; + * keep both paths on it rather than reintroducing a separate `Scenario:` regex. */ -const SCENARIO_HEADER = /^####\s+/; +export const SCENARIO_HEADER = /^####\s+/; /** * The one predicate for normative-keyword detection. Matches `SHALL` or `MUST` diff --git a/test/core/parsers/requirement-blocks.test.ts b/test/core/parsers/requirement-blocks.test.ts index d0f9712cfe..323324ea08 100644 --- a/test/core/parsers/requirement-blocks.test.ts +++ b/test/core/parsers/requirement-blocks.test.ts @@ -1,5 +1,10 @@ import { describe, it, expect } from 'vitest'; -import { extractRequirementsSection, parseDeltaSpec } from '../../../src/core/parsers/requirement-blocks.js'; +import { + extractRequirementsSection, + parseDeltaSpec, + findMissingCurrentScenarios, + type RequirementBlock, +} from '../../../src/core/parsers/requirement-blocks.js'; describe('extractRequirementsSection', () => { it('parses canonical ### Requirement: headers', () => { @@ -131,3 +136,81 @@ describe('extractRequirementsSection (fenced code blocks)', () => { expect(result.bodyBlocks.map((b) => b.name)).toEqual(['Real requirement']); }); }); + +describe('findMissingCurrentScenarios: level-4 header parity', () => { + // Only .raw is read; a minimal block keeps these focused on scenario parsing. + const block = (raw: string): RequirementBlock => ({ headerLine: raw.split('\n')[0], name: '', raw }); + const req = (...scenarios: string[]) => + block(['### Requirement: Widget state', 'The system SHALL report it.', '', ...scenarios].join('\n')); + + it('does not treat a level-5 (#####) header as a dropped scenario', () => { + // `#### ` requires exactly four hashes then whitespace, matching countScenarios; + // `##### Deep detail` is body, not a scenario, so dropping it is no loss. + const current = req('#### Scenario: Kept', '- **WHEN** a', '- **THEN** b', '', '##### Deep detail', '- a nested note'); + const incoming = req('#### Scenario: Kept', '- **WHEN** a', '- **THEN** b'); + expect(findMissingCurrentScenarios(current, incoming)).toEqual([]); + }); + + it('does not treat an unlabeled #### header inside a fence as a scenario', () => { + const current = req( + '#### Scenario: Real', + '- **WHEN** a', + '- **THEN** b', + '', + '```markdown', + '#### Edge case', + '- only an example', + '```' + ); + const incoming = req('#### Scenario: Real', '- **WHEN** a', '- **THEN** b'); + expect(findMissingCurrentScenarios(current, incoming)).toEqual([]); + }); + + it('normalizes an optional Scenario: label, so relabeling is not a loss', () => { + const current = req('#### Edge case', '- **WHEN** a', '- **THEN** b'); + const incoming = req('#### Scenario: Edge case', '- **WHEN** a', '- **THEN** b'); + expect(findMissingCurrentScenarios(current, incoming)).toEqual([]); + }); + + it('counts unlabeled scenarios by multiplicity, like labeled ones', () => { + const current = req( + '#### Edge case', + '- **WHEN** a', + '- **THEN** b', + '', + '#### Edge case', + '- **WHEN** c', + '- **THEN** d' + ); + const incoming = req('#### Edge case', '- **WHEN** a', '- **THEN** b'); + expect(findMissingCurrentScenarios(current, incoming)).toEqual(['Edge case']); + }); + + it('does not let a fenced #### in the incoming block satisfy a real scenario', () => { + // Symmetry with the current-side fence test: masking must apply to the + // incoming block too, or a fenced sample in the MODIFIED delta would count + // as coverage and hide a genuine drop (validate passes, archive deletes). + const current = req('#### Scenario: Real', '- **WHEN** a', '- **THEN** b'); + const incoming = req( + '```markdown', + '#### Scenario: Real', + '- only an example', + '```' + ); + expect(findMissingCurrentScenarios(current, incoming)).toEqual(['Real']); + }); + + it('normalizes a lowercase scenario: label the same as Scenario:', () => { + const current = req('#### Scenario: Edge case', '- **WHEN** a', '- **THEN** b'); + const incoming = req('#### scenario: Edge case', '- **WHEN** a', '- **THEN** b'); + expect(findMissingCurrentScenarios(current, incoming)).toEqual([]); + }); + + it('treats an ATX-closed header as the same scenario as its open form', () => { + // `#### Foo ####` renders as `Foo` in CommonMark; the loss guard must fold + // the two so relabeling one side does not read as a dropped scenario. + const current = req('#### Scenario: Edge case', '- **WHEN** a', '- **THEN** b'); + const incoming = req('#### Scenario: Edge case ####', '- **WHEN** a', '- **THEN** b'); + expect(findMissingCurrentScenarios(current, incoming)).toEqual([]); + }); +}); diff --git a/test/core/validation.scenario-loss.test.ts b/test/core/validation.scenario-loss.test.ts index 32524e526b..522f6969d8 100644 --- a/test/core/validation.scenario-loss.test.ts +++ b/test/core/validation.scenario-loss.test.ts @@ -377,6 +377,53 @@ describe('validate: MODIFIED blocks that would drop a main-spec scenario (#1477) } }); + it('detects a dropped level-4 scenario whose header is not labeled "Scenario:"', async () => { + // The spec path (SCENARIO_HEADER / countScenarios) counts EVERY `#### ` + // child of a requirement as a scenario, so archive replaces the whole block + // and drops an unlabeled `#### Edge case`. The loss check must see it too, + // or the drop is silent (validate passes, archive deletes it with no error). + await writeMainSpec( + 'widgets', + mainSpec( + `### Requirement: Widget state\nThe system SHALL report the widget state.\n\n#### Scenario: Existing scenario\n- **WHEN** queried\n- **THEN** the state is reported\n\n#### Edge case\n- **WHEN** disabled\n- **THEN** nothing is reported` + ) + ); + const changeDir = await writeChange( + 'drop-unlabeled-scenario', + 'widgets', + `## MODIFIED Requirements\n\n### Requirement: Widget state\nThe system SHALL report the widget state.\n\n#### Scenario: Existing scenario\n- **WHEN** queried\n- **THEN** the state is reported\n` + ); + + const report = await validate(changeDir); + + expect(report.valid).toBe(false); + expect(lossIssue(report)?.message).toContain('"Edge case"'); + // Parity: archive refuses the same change, naming the same scenario. + expect(await archiveError(changeDir)).toContain('Edge case'); + }); + + it('detects a dropped labeled scenario even when an unlabeled sibling is kept', async () => { + // The reverse of the case above: labels and non-labels are counted the same + // way, in both directions, so dropping the labeled one is still caught. + await writeMainSpec( + 'widgets', + mainSpec( + `### Requirement: Widget state\nThe system SHALL report the widget state.\n\n#### Scenario: Labeled\n- **WHEN** queried\n- **THEN** the state is reported\n\n#### Unlabeled\n- **WHEN** idle\n- **THEN** idle is reported` + ) + ); + const changeDir = await writeChange( + 'drop-labeled-keep-unlabeled', + 'widgets', + `## MODIFIED Requirements\n\n### Requirement: Widget state\nThe system SHALL report the widget state.\n\n#### Unlabeled\n- **WHEN** idle\n- **THEN** idle is reported\n` + ); + + const report = await validate(changeDir); + + expect(report.valid).toBe(false); + expect(lossIssue(report)?.message).toContain('"Labeled"'); + expect(await archiveError(changeDir)).toContain('Labeled'); + }); + it('does not name scenarios for a MODIFIED the same delta renames away', async () => { // The block this MODIFIED would land on is not the one it names, so any // scenario reported here would send the author after the wrong requirement.