diff --git a/.changeset/validate-scenario-loss.md b/.changeset/validate-scenario-loss.md new file mode 100644 index 0000000000..a6c45180c4 --- /dev/null +++ b/.changeset/validate-scenario-loss.md @@ -0,0 +1,5 @@ +--- +"@fission-ai/openspec": patch +--- + +`openspec validate ` now reports a MODIFIED requirement that omits a scenario the main spec still has — the same loss archive already refuses to apply — so the change fails at authoring time instead of at archive time. A change carrying a stale MODIFIED block will start failing validation; it was already unarchivable, and the message names the scenarios to copy back in. diff --git a/docs/cli.md b/docs/cli.md index 04cb514d2d..66534ddc83 100644 --- a/docs/cli.md +++ b/docs/cli.md @@ -526,7 +526,7 @@ openspec show add-dark-mode --json ### `openspec validate` -Validate changes and specs for structural issues. +Validate changes and specs for structural issues, and check a change's MODIFIED requirements against the main specs they would replace. ``` openspec validate [item-name] [options] diff --git a/docs/troubleshooting.md b/docs/troubleshooting.md index 72f47a5e59..75a521f965 100644 --- a/docs/troubleshooting.md +++ b/docs/troubleshooting.md @@ -96,6 +96,14 @@ openspec validate --all --strict # stricter checks, good for CI Common causes are a missing required section (like a spec with no scenarios) or a malformed delta header. Fix the file and re-run. The [CLI reference](cli.md#openspec-validate) documents the output format. +One message deserves its own note: + +```text +MODIFIED "" omits scenario(s) the current spec still has: "" +``` + +A `MODIFIED` requirement replaces the whole requirement block, so it has to carry every scenario that survives the change, not only the ones you edited. Copy the named scenarios from `openspec/specs//spec.md` back into the delta. This often appears on an older change after someone else's change added a scenario to the same requirement — archive refuses that change either way, and validation now says so before you implement it. + ### The AI created incomplete or wrong artifacts The AI didn't have enough context. A few levers help: diff --git a/openspec/specs/cli-validate/spec.md b/openspec/specs/cli-validate/spec.md index 61afc7953b..5f213978c4 100644 --- a/openspec/specs/cli-validate/spec.md +++ b/openspec/specs/cli-validate/spec.md @@ -62,6 +62,35 @@ The CLI SHALL append a Next steps footer when the item is invalid and not using - **WHEN** a change validation fails - **THEN** print "Next steps" with 2-3 targeted bullets and suggest `openspec change show --json --deltas-only` +### Requirement: Change validation SHALL report scenarios a MODIFIED block would drop + +The `validate` command SHALL compare every `MODIFIED` requirement in a change against the main specs and report, as an error naming the delta file, each scenario the main spec still has that the `MODIFIED` block omits. A `MODIFIED` requirement replaces the whole requirement block, so archive refuses to apply one that drops a scenario; this is the same check, run without writing anything. + +The comparison SHALL match archive's operation order, comparing a `MODIFIED` that names the new header of a rename against the renamed requirement's scenarios. + +The check SHALL be silent when the main spec file or the requirement header is absent, because a `MODIFIED` written against a base that has not landed yet is a separate condition that archive gates. A main spec that exists but cannot be read SHALL be reported instead, since archive fails on it too. + +Validation run inside `openspec archive` SHALL NOT report these issues, because archive enforces the same check when it applies the deltas. + +#### Scenario: MODIFIED omits an existing scenario + +- **GIVEN** the main spec's requirement has scenarios "A" and "B" +- **WHEN** a change MODIFIES that requirement with only scenario "A" and `openspec validate ` runs +- **THEN** report an error naming the delta file and scenario "B" +- **AND** exit with code 1 + +#### Scenario: MODIFIED names the new header of a rename + +- **GIVEN** the main spec has requirement "A" with scenarios "S1" and "S2" +- **WHEN** a change renames "A" to "B" and MODIFIES "B" with only scenario "S1" +- **THEN** report an error naming scenario "S2" + +#### Scenario: MODIFIED header is not in the main spec + +- **GIVEN** a change MODIFIES a requirement header the main spec does not contain +- **WHEN** `openspec validate ` runs +- **THEN** do not report a dropped-scenario error for that requirement + ### Requirement: Top-level validate command The CLI SHALL provide a top-level `validate` command for validating changes and specs with flexible selection options. diff --git a/skills/openspec-sync-specs/SKILL.md b/skills/openspec-sync-specs/SKILL.md index 49e4612c2d..e907fc41c6 100644 --- a/skills/openspec-sync-specs/SKILL.md +++ b/skills/openspec-sync-specs/SKILL.md @@ -100,7 +100,7 @@ This is an **agent-driven** operation - you will read delta specs and directly e **MODIFIED Requirements:** - Find the requirement in main spec - Apply the changes - this can be: - - Adding new scenarios (don't need to copy existing ones) + - Adding new scenarios the main spec does not have yet - Modifying existing scenarios - Changing the requirement description - Preserve scenarios/content not mentioned in the delta @@ -149,6 +149,12 @@ The system SHALL do something new. ## MODIFIED Requirements ### Requirement: Existing Feature +The system SHALL keep doing the existing thing, now also handling A. + +#### Scenario: Scenario the main spec already has +- **WHEN** user does X +- **THEN** system does Y + #### Scenario: New scenario to add - **WHEN** user does A - **THEN** system does B @@ -185,9 +191,9 @@ The system SHALL do something new. **Key Principle: Intelligent Merging** -Unlike programmatic merging, you can apply **partial updates**: -- To add a scenario, just include that scenario under MODIFIED - don't copy existing scenarios -- The delta represents *intent*, not a wholesale replacement +Unlike programmatic merging, you merge rather than overwrite: +- A MODIFIED block carries the whole requirement - body plus every scenario that survives the change. `openspec validate` and `openspec archive` both reject one that drops a scenario the main spec still has. +- Keep anything the delta does not mention, in the main spec's existing order - Use your judgment to merge changes sensibly **Output On Success** diff --git a/src/commands/change.ts b/src/commands/change.ts index f9a1995496..23a23de5e5 100644 --- a/src/commands/change.ts +++ b/src/commands/change.ts @@ -256,7 +256,11 @@ export class ChangeCommand { } const validator = new Validator(options?.strict || false); - const report = await validator.validateChangeDeltaSpecs(changeDir); + const report = await validator.validateChangeDeltaSpecs(changeDir, { + // Derived from changesPath so the main specs come from the same root the + // change itself was resolved against. + mainSpecsDir: path.join(path.dirname(changesPath), 'specs'), + }); if (options?.json) { console.log(JSON.stringify(report, null, 2)); diff --git a/src/commands/validate.ts b/src/commands/validate.ts index eb44ede0f9..0e74722493 100644 --- a/src/commands/validate.ts +++ b/src/commands/validate.ts @@ -197,7 +197,7 @@ export class ValidateCommand { if (type === 'change') { const changeDir = path.join(root.changesDir, id); const start = Date.now(); - const report = await validator.validateChangeDeltaSpecs(changeDir); + const report = await validator.validateChangeDeltaSpecs(changeDir, { mainSpecsDir: root.specsDir }); const durationMs = Date.now() - start; this.printReport('change', id, report, durationMs, opts.json, root); // Non-zero exit if invalid (keeps enriched output test semantics) @@ -279,7 +279,7 @@ export class ValidateCommand { queue.push(async () => { const start = Date.now(); const changeDir = path.join(root.changesDir, id); - const report = await validator.validateChangeDeltaSpecs(changeDir); + const report = await validator.validateChangeDeltaSpecs(changeDir, { mainSpecsDir: root.specsDir }); const durationMs = Date.now() - start; return { id, type: 'change' as const, valid: report.valid, issues: report.issues, durationMs }; }); diff --git a/src/core/archive.ts b/src/core/archive.ts index f0f6013b3e..e7f3d943fa 100644 --- a/src/core/archive.ts +++ b/src/core/archive.ts @@ -334,6 +334,9 @@ export class ArchiveCommand { } catch {} } if (hasDeltaSpecs) { + // No mainSpecsDir here on purpose: the scenario-loss check standalone + // validate runs (#1477) is the same one buildUpdatedSpec enforces a few + // steps later, and reporting it here would relabel that failure. const deltaReport = await validator.validateChangeDeltaSpecs(changeDir); if (!deltaReport.valid) { hasValidationErrors = true; diff --git a/src/core/parsers/requirement-blocks.ts b/src/core/parsers/requirement-blocks.ts index cb0e79b75b..2f2c8a2004 100644 --- a/src/core/parsers/requirement-blocks.ts +++ b/src/core/parsers/requirement-blocks.ts @@ -323,3 +323,74 @@ function parseRenamedPairs(sectionBody: SectionBody): Array<{ from: string; to: } return pairs; } + +interface ScenarioBlock { + name: string; + raw: string; +} + +/** + * Scenario names the current requirement block has and the incoming + * (MODIFIED) block does not. A MODIFIED requirement replaces the whole block, + * so every name reported here would be dropped from the main spec. + * + * Shared by archive (which refuses to apply the block) and validate (which + * reports the same loss at authoring time, #1477), so the two cannot disagree + * about what counts as a dropped scenario. + */ +export function findMissingCurrentScenarios(current: RequirementBlock, incoming: RequirementBlock): string[] { + // Multiplicity-aware: a name present N times in current and M times in + // incoming means max(0, N - M) instances are missing. Set membership would + // treat N>M as fully covered and let archive silently drop duplicates + // (residual #1246 / duplicate-scenario-name blind spot). + const remainingIncoming = new Map(); + for (const scenario of parseScenarioBlocks(incoming.raw)) { + const name = scenario.name; + remainingIncoming.set(name, (remainingIncoming.get(name) ?? 0) + 1); + } + + const missing: string[] = []; + for (const scenario of parseScenarioBlocks(current.raw)) { + const name = scenario.name; + const remaining = remainingIncoming.get(name) ?? 0; + if (remaining > 0) { + remainingIncoming.set(name, remaining - 1); + } else { + missing.push(name); + } + } + return missing; +} + +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). + 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) { + index++; + continue; + } + + const start = index; + const name = headerMatch[1].trim(); + index++; + while (index < lines.length && (mask[index] || !/^####\s*Scenario:\s*(.+)\s*$/.test(lines[index]))) { + index++; + } + + scenarios.push({ + name, + raw: lines.slice(start, index).join('\n').trimEnd(), + }); + } + + return scenarios; +} diff --git a/src/core/specs-apply.ts b/src/core/specs-apply.ts index e8d2f3910f..2f9c1a5e3d 100644 --- a/src/core/specs-apply.ts +++ b/src/core/specs-apply.ts @@ -10,6 +10,7 @@ import path from 'path'; import chalk from 'chalk'; import { extractRequirementsSection, + findMissingCurrentScenarios, foldRequirementName, parseDeltaSpec, normalizeRequirementName, @@ -33,11 +34,6 @@ export interface SpecUpdate { exists: boolean; } -interface ScenarioBlock { - name: string; - raw: string; -} - // ----------------------------------------------------------------------------- // Public API // ----------------------------------------------------------------------------- @@ -572,60 +568,3 @@ export function buildSpecSkeleton(specFolderName: string, changeName: string, pu return `# ${titleBase} Specification\n\n## Purpose\n${purposeBody}\n\n## Requirements\n`; } -function findMissingCurrentScenarios(current: RequirementBlock, incoming: RequirementBlock): string[] { - // Multiplicity-aware: a name present N times in current and M times in - // incoming means max(0, N - M) instances are missing. Set membership would - // treat N>M as fully covered and let archive silently drop duplicates - // (residual #1246 / duplicate-scenario-name blind spot). - const remainingIncoming = new Map(); - for (const scenario of parseScenarioBlocks(incoming.raw)) { - const name = scenario.name; - remainingIncoming.set(name, (remainingIncoming.get(name) ?? 0) + 1); - } - - const missing: string[] = []; - for (const scenario of parseScenarioBlocks(current.raw)) { - const name = scenario.name; - const remaining = remainingIncoming.get(name) ?? 0; - if (remaining > 0) { - remainingIncoming.set(name, remaining - 1); - } else { - missing.push(name); - } - } - return missing; -} - -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). - 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) { - index++; - continue; - } - - const start = index; - const name = headerMatch[1].trim(); - index++; - while (index < lines.length && (mask[index] || !/^####\s*Scenario:\s*(.+)\s*$/.test(lines[index]))) { - index++; - } - - scenarios.push({ - name, - raw: lines.slice(start, index).join('\n').trimEnd(), - }); - } - - return scenarios; -} - diff --git a/src/core/templates/workflows/sync-specs.ts b/src/core/templates/workflows/sync-specs.ts index a172a7fc8c..164eefacde 100644 --- a/src/core/templates/workflows/sync-specs.ts +++ b/src/core/templates/workflows/sync-specs.ts @@ -102,7 +102,7 @@ ${STORE_SELECTION_GUIDANCE} **MODIFIED Requirements:** - Find the requirement in main spec - Apply the changes - this can be: - - Adding new scenarios (don't need to copy existing ones) + - Adding new scenarios the main spec does not have yet - Modifying existing scenarios - Changing the requirement description - Preserve scenarios/content not mentioned in the delta @@ -151,6 +151,12 @@ The system SHALL do something new. ## MODIFIED Requirements ### Requirement: Existing Feature +The system SHALL keep doing the existing thing, now also handling A. + +#### Scenario: Scenario the main spec already has +- **WHEN** user does X +- **THEN** system does Y + #### Scenario: New scenario to add - **WHEN** user does A - **THEN** system does B @@ -187,9 +193,9 @@ The system SHALL do something new. **Key Principle: Intelligent Merging** -Unlike programmatic merging, you can apply **partial updates**: -- To add a scenario, just include that scenario under MODIFIED - don't copy existing scenarios -- The delta represents *intent*, not a wholesale replacement +Unlike programmatic merging, you merge rather than overwrite: +- A MODIFIED block carries the whole requirement - body plus every scenario that survives the change. \`openspec validate\` and \`openspec archive\` both reject one that drops a scenario the main spec still has. +- Keep anything the delta does not mention, in the main spec's existing order - Use your judgment to merge changes sensibly **Output On Success** @@ -325,7 +331,7 @@ ${STORE_SELECTION_GUIDANCE} **MODIFIED Requirements:** - Find the requirement in main spec - Apply the changes - this can be: - - Adding new scenarios (don't need to copy existing ones) + - Adding new scenarios the main spec does not have yet - Modifying existing scenarios - Changing the requirement description - Preserve scenarios/content not mentioned in the delta @@ -374,6 +380,12 @@ The system SHALL do something new. ## MODIFIED Requirements ### Requirement: Existing Feature +The system SHALL keep doing the existing thing, now also handling A. + +#### Scenario: Scenario the main spec already has +- **WHEN** user does X +- **THEN** system does Y + #### Scenario: New scenario to add - **WHEN** user does A - **THEN** system does B @@ -410,9 +422,9 @@ The system SHALL do something new. **Key Principle: Intelligent Merging** -Unlike programmatic merging, you can apply **partial updates**: -- To add a scenario, just include that scenario under MODIFIED - don't copy existing scenarios -- The delta represents *intent*, not a wholesale replacement +Unlike programmatic merging, you merge rather than overwrite: +- A MODIFIED block carries the whole requirement - body plus every scenario that survives the change. \`openspec validate\` and \`openspec archive\` both reject one that drops a scenario the main spec still has. +- Keep anything the delta does not mention, in the main spec's existing order - Use your judgment to merge changes sensibly **Output On Success** diff --git a/src/core/validation/validator.ts b/src/core/validation/validator.ts index 0086c12766..280f309c36 100644 --- a/src/core/validation/validator.ts +++ b/src/core/validation/validator.ts @@ -10,7 +10,14 @@ import { MAX_REQUIREMENT_TEXT_LENGTH, VALIDATION_MESSAGES } from './constants.js'; -import { parseDeltaSpec, foldRequirementName, normalizeRequirementName, extractRequirementsSection } from '../parsers/requirement-blocks.js'; +import { + parseDeltaSpec, + foldRequirementName, + normalizeRequirementName, + extractRequirementsSection, + findMissingCurrentScenarios, + type RequirementBlock, +} from '../parsers/requirement-blocks.js'; import { extractRequirementBody as extractRequirementBodyShared, containsShallOrMust as containsShallOrMustShared, @@ -133,8 +140,16 @@ export class Validator { * - REMOVED: names only; no scenario/description required * - RENAMED: pairs well-formed * - No duplicates within sections; no cross-section conflicts per spec + * + * When `options.mainSpecsDir` is given, MODIFIED blocks are also checked + * against the current main specs for the scenario loss archive refuses to + * apply (#1477). Omitting it keeps the change-only checks, so callers with + * no main specs root (and existing library callers) behave as before. */ - async validateChangeDeltaSpecs(changeDir: string): Promise { + async validateChangeDeltaSpecs( + changeDir: string, + options: { mainSpecsDir?: string } = {} + ): Promise { const issues: ValidationIssue[] = []; const specsDir = path.join(changeDir, 'specs'); let totalDeltas = 0; @@ -148,7 +163,7 @@ export class Validator { // path silently skips (#1385). It finds spec.md at any depth, covering // both specs//spec.md and the nested multi-area // specs///spec.md layout (#1182b). - const specFiles = (await discoverSpecFiles(specsDir)).map(spec => spec.specFile); + const discoveredSpecs = await discoverSpecFiles(specsDir); // A spec.md directly at the specs/ root has no capability folder, so the // merge path drops it: without this error the change validates clean and @@ -166,7 +181,7 @@ export class Validator { }); } - for (const specFile of specFiles) { + for (const { id: specId, specFile } of discoveredSpecs) { let content: string | undefined; try { content = await fs.readFile(specFile, 'utf-8'); @@ -267,6 +282,19 @@ export class Validator { } } + // Run archive's scenario-loss check here too, so the change fails at + // authoring time instead of days later at archive time (#1477). + if (options.mainSpecsDir && plan.modified.length > 0) { + issues.push( + ...(await this.findScenarioLossIssues( + plan.modified, + plan.renamed, + path.join(options.mainSpecsDir, ...specId.split('/'), 'spec.md'), + entryPath + )) + ); + } + // Validate REMOVED (names only) for (const name of plan.removed) { const key = normalizeRequirementName(name); @@ -401,6 +429,101 @@ export class Validator { return this.createReport(issues); } + /** + * Report MODIFIED requirements whose block omits a scenario the main spec + * still carries. Uses the same comparison archive applies, so validate can + * only report what archive would refuse. + * + * Silent when the main spec or the requirement header is absent: applying a + * MODIFIED against a base that is not there yet is a different failure (a + * sister change still in flight is the legitimate case), and archive is the + * gate for it. A spec that exists but cannot be read is not absent, though — + * archive aborts on it, so reporting it beats calling the change valid. + */ + private async findScenarioLossIssues( + modified: RequirementBlock[], + renamed: Array<{ from: string; to: string }>, + mainSpecFile: string, + entryPath: string + ): Promise { + let mainContent: string; + try { + mainContent = await fs.readFile(mainSpecFile, 'utf-8'); + } catch (error) { + const code = (error as NodeJS.ErrnoException)?.code; + // Reported only for the codes that mean the file itself is unusable, and + // will be just as unusable when archive reads it. Everything else - + // ENOENT/ENOTDIR ("no main spec"), and transient resource errors like + // EMFILE that say nothing about the file - stays silent rather than + // failing a change that is fine. `validate --all` reads six changes at + // once, so a resource error must never become a verdict. + const UNUSABLE = new Set(['EACCES', 'EPERM', 'EISDIR', 'ELOOP', 'ENAMETOOLONG']); + if (!code || !UNUSABLE.has(code)) return []; + return [ + { + level: 'ERROR', + path: entryPath, + message: + `Could not read ${FileSystemUtils.toPosixPath(mainSpecFile)} to check the MODIFIED requirements against it ` + + `(${code}). Archive reads the same file, so fix the file before archiving.`, + }, + ]; + } + + const currentBlocks = new Map(); + for (const block of extractRequirementsSection(mainContent).bodyBlocks) { + currentBlocks.set(normalizeRequirementName(block.name), block); + } + // Archive applies RENAMED before MODIFIED, so a MODIFIED naming the new + // header is compared against the renamed block's scenarios. Fall back to + // the old header, or a rename-plus-modify pair would skip the check. + const renamedFrom = new Map( + renamed.map(({ from, to }) => [normalizeRequirementName(to), normalizeRequirementName(from)]) + ); + + // Walked, not looked up once: renames chain (A→B then B→C leaves C holding + // A's block), and the visited set stops a cycle from looping forever. Every + // name in a rename cycle is also a rename FROM, so the skip above already + // keeps the walk out of one; the guard stays because the cost of being + // wrong about that is a hung CLI, not a wrong message. + const currentBlockFor = (name: string): RequirementBlock | undefined => { + const visited = new Set(); + let key: string | undefined = name; + while (key !== undefined && !visited.has(key)) { + const block = currentBlocks.get(key); + if (block) return block; + visited.add(key); + key = renamedFrom.get(key); + } + return undefined; + }; + + // A MODIFIED naming a header the same delta renames away is already + // reported ("MODIFIED references old name from RENAMED"), and the block it + // would land on is not the one it names — so any scenario named here would + // send the author after the wrong requirement. + const renamedAway = new Set(renamed.map(({ from }) => normalizeRequirementName(from))); + + const issues: ValidationIssue[] = []; + for (const block of modified) { + const key = normalizeRequirementName(block.name); + if (renamedAway.has(key)) continue; + const current = currentBlockFor(key); + if (!current) continue; + const missing = findMissingCurrentScenarios(current, block); + if (missing.length === 0) continue; + issues.push({ + level: 'ERROR', + path: entryPath, + message: + `MODIFIED "${block.name}" omits scenario(s) the current spec still has: ` + + `${missing.map(name => `"${name}"`).join(', ')}. ` + + 'Copy them into the MODIFIED block (a MODIFIED requirement replaces the whole block, so archive refuses to drop them).', + }); + } + return issues; + } + private formatInvalidMarkerMessage(invalidReason: string): string { return `${VALIDATION_MESSAGES.CHANGE_SKIP_SPECS_INVALID_METADATA} (${invalidReason})`; } diff --git a/test/cli-e2e/validate-scenario-loss.test.ts b/test/cli-e2e/validate-scenario-loss.test.ts new file mode 100644 index 0000000000..cd0375bff4 --- /dev/null +++ b/test/cli-e2e/validate-scenario-loss.test.ts @@ -0,0 +1,104 @@ +import { afterAll, describe, it, expect, beforeAll } from 'vitest'; +import { promises as fs } from 'fs'; +import path from 'path'; +import { tmpdir } from 'os'; +import { runCLI } from '../helpers/run-cli.js'; + +/** + * The scenario-loss check (#1477) only runs when a command hands the validator + * its main specs root, so these exercise the wiring through the real CLI — + * every entry point, and the exit code each one reports. + */ +describe('openspec validate reports scenarios a MODIFIED block would drop (#1477)', () => { + const tempRoots: string[] = []; + let projectDir: string; + + const write = async (relative: string, content: string) => { + const file = path.join(projectDir, relative); + await fs.mkdir(path.dirname(file), { recursive: true }); + await fs.writeFile(file, content); + }; + + beforeAll(async () => { + const base = await fs.mkdtemp(path.join(tmpdir(), 'openspec-scenario-loss-e2e-')); + tempRoots.push(base); + projectDir = path.join(base, 'project'); + await fs.mkdir(projectDir, { recursive: true }); + + await write( + 'openspec/specs/widgets/spec.md', + `# widgets Specification\n\n## Purpose\nDefine widget behavior for the end-to-end check.\n\n## 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\n#### Scenario: Second scenario\n- **WHEN** idle\n- **THEN** idle is reported\n` + ); + await write( + 'openspec/changes/drops-a-scenario/proposal.md', + `# Drops a scenario\n\n## Why\nExercise the check.\n\n## What Changes\n- Rewrite one scenario\n` + ); + await write( + 'openspec/changes/drops-a-scenario/specs/widgets/spec.md', + `## 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` + ); + await write( + 'openspec/changes/keeps-every-scenario/proposal.md', + `# Keeps every scenario\n\n## Why\nControl case.\n\n## What Changes\n- Reword the requirement\n` + ); + await write( + 'openspec/changes/keeps-every-scenario/specs/widgets/spec.md', + `## MODIFIED Requirements\n\n### Requirement: Widget state\nThe system SHALL report the widget state promptly.\n\n#### Scenario: Existing scenario\n- **WHEN** queried\n- **THEN** the state is reported\n\n#### Scenario: Second scenario\n- **WHEN** idle\n- **THEN** idle is reported\n` + ); + }); + + afterAll(async () => { + await Promise.all(tempRoots.map((dir) => fs.rm(dir, { recursive: true, force: true }))); + }); + + it('fails `validate ` with exit code 1 and names the dropped scenario', async () => { + const result = await runCLI(['validate', '--type', 'change', 'drops-a-scenario'], { cwd: projectDir }); + + expect(result.exitCode).toBe(1); + expect(result.stderr).toContain('MODIFIED "Widget state" omits scenario(s)'); + expect(result.stderr).toContain('"Second scenario"'); + }); + + it('fails the same way under --strict, and reports it in --json', async () => { + const result = await runCLI( + ['validate', '--type', 'change', 'drops-a-scenario', '--strict', '--json'], + { cwd: projectDir } + ); + + expect(result.exitCode).toBe(1); + const report = JSON.parse(result.stdout); + const issue = report.items[0].issues.find((i: { message: string }) => + i.message.includes('omits scenario(s)') + ); + expect(issue.level).toBe('ERROR'); + expect(issue.path).toBe('widgets/spec.md'); + }); + + it('reports it in bulk `validate --changes`', async () => { + const result = await runCLI(['validate', '--changes', '--json'], { cwd: projectDir }); + + expect(result.exitCode).toBe(1); + const report = JSON.parse(result.stdout); + const byId = Object.fromEntries( + report.items.map((item: { id: string; valid: boolean }) => [item.id, item.valid]) + ); + expect(byId['drops-a-scenario']).toBe(false); + expect(byId['keeps-every-scenario']).toBe(true); + }); + + it('reports it through the deprecated `change validate` command', async () => { + const result = await runCLI(['change', 'validate', 'drops-a-scenario'], { cwd: projectDir }); + + expect(result.exitCode).toBe(1); + expect(result.stderr).toContain('omits scenario(s)'); + }); + + it('leaves a change that carries every scenario over passing', async () => { + const result = await runCLI(['validate', '--type', 'change', 'keeps-every-scenario', '--strict'], { + cwd: projectDir, + }); + + expect(result.exitCode).toBe(0); + expect(result.stdout).toContain("Change 'keeps-every-scenario' is valid"); + }); +}); diff --git a/test/core/templates/skill-templates-parity.test.ts b/test/core/templates/skill-templates-parity.test.ts index 04b1b1aef7..d4408433dd 100644 --- a/test/core/templates/skill-templates-parity.test.ts +++ b/test/core/templates/skill-templates-parity.test.ts @@ -42,7 +42,7 @@ const EXPECTED_FUNCTION_HASHES: Record = { getContinueChangeSkillTemplate: '676e7472977d2b6f4d922ce384db1f15020c195f94d6cd4ee71abcf0201e28a9', getApplyChangeSkillTemplate: '031cf8f8ffc2937fc4051651bd5e1fc6159bfd225605d8e4c3181054a4e52b38', getFfChangeSkillTemplate: '225a8eaf1b3769ac5d43e079297c5fa9cc20fc2e34fec9bb0d887c8c1fb0ea71', - getSyncSpecsSkillTemplate: '977a753b03daa33ddb8aa9bcc632e10d82062c02749a0c821ecc338311251186', + getSyncSpecsSkillTemplate: '6824990431141eba855c9560cded184c53a44985e14ba354032fe5deedd270b4', getOnboardSkillTemplate: '31dffc7c3b8d75ffbd59ed751d6a1550b885b20ef90e12d236262127ee4021e9', getOpsxExploreCommandTemplate: 'e9674ddace813e685b0e9fe37149140a3d33d48aa20b9ba2b0963a7c49c9aea7', getOpsxNewCommandTemplate: '652adc870f16bb260d54436356132b6ee051a9ed7cc0464603fb31f4db259762', @@ -51,7 +51,7 @@ const EXPECTED_FUNCTION_HASHES: Record = { getOpsxFfCommandTemplate: '678375642a21d255444f0ba717e659abb2cc2b7474981d52eae900a0793e3e4d', getArchiveChangeSkillTemplate: '7c1bf2170ba57833f111c79002ea56be3cca499e2b13b2ea8141c182351b1a3b', getBulkArchiveChangeSkillTemplate: 'de198c7b7c1472773b013b9af917de27773fd613083309f0e8e607c005c92d3d', - getOpsxSyncCommandTemplate: 'b1f3fea6a9d4e84f401f411a0fefe330ad9ee81cff065a578f4057386c5d81fa', + getOpsxSyncCommandTemplate: 'e30b1e1e7070da3521e3878065b400ced7b6260e532fd348df96df75d9d7f2e3', getVerifyChangeSkillTemplate: '917de96cc8341799107b0617979cdaf30e121c51676272f5caef143b090583f9', getOpsxArchiveCommandTemplate: 'fa0d2f4c1ff9b499353399ba040caaf2ba070154dac8b94cb4ca8e2568b1717a', getOpsxOnboardCommandTemplate: 'e69a5aa37749727290c05b687981dd69f3b17a55514a118d088c4124c5fd8505', @@ -70,7 +70,7 @@ const EXPECTED_GENERATED_SKILL_CONTENT_HASHES: Record = { 'openspec-continue-change': '2e1a7d17ec021949d115c72227729609bf9980ad1f23445af117c09834711121', 'openspec-apply-change': '49fc5772404e3033085384ee214c44488c93880a596a9a05dcad42f9ce86cf83', 'openspec-ff-change': '4228d75e3571097164f2360e2ad3063a5b88d44750078c3601b23a89e74c1de6', - 'openspec-sync-specs': 'db79c625bbfa3aaf948812fda5965eda876264973c9c5c4bbeac4a48df77f97d', + 'openspec-sync-specs': 'c7aff2b41cab0ba87257ea8a2b4892c34192f21f75e5924ab65490cfa924e66b', 'openspec-archive-change': '84b9d3a5690b8d64e1845b3c7368a4ad43369ea8549a76ef78912690d434363b', 'openspec-bulk-archive-change': '5ac320e2004e453c78541233f48e5f6e246cc674a44f1e427cecb7b2e9587f9b', 'openspec-verify-change': '1c3f73a36be691a18d3acb200d22e6874004d6d4a5d3e2e346ae95a7379e9da8', diff --git a/test/core/validation.scenario-loss.test.ts b/test/core/validation.scenario-loss.test.ts new file mode 100644 index 0000000000..32524e526b --- /dev/null +++ b/test/core/validation.scenario-loss.test.ts @@ -0,0 +1,402 @@ +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import { promises as fs } from 'fs'; +import os from 'os'; +import path from 'path'; +import { Validator } from '../../src/core/validation/validator.js'; +import { buildUpdatedSpec, findSpecUpdates } from '../../src/core/specs-apply.js'; + +/** + * validate reports the scenario loss archive refuses to apply (#1477). + * + * The point of these tests is parity: every case validate rejects must be one + * archive already rejects, and every case archive accepts must stay valid. + */ +describe('validate: MODIFIED blocks that would drop a main-spec scenario (#1477)', () => { + let testDir: string; + let changesDir: string; + let mainSpecsDir: string; + + /** Two scenarios in the main spec; the delta below keeps only the first. */ + const TWO_SCENARIO_REQUIREMENT = `### 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#### Scenario: Second scenario\n- **WHEN** idle\n- **THEN** idle is reported`; + const DELTA_KEEPING_ONE = `## 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 mainSpec = (body: string) => + `# widgets Specification\n\n## Purpose\nDefine widget behavior for these tests.\n\n## Requirements\n\n${body}\n`; + + const writeMainSpec = async (id: string, content: string) => { + const file = path.join(mainSpecsDir, ...id.split('/'), 'spec.md'); + await fs.mkdir(path.dirname(file), { recursive: true }); + await fs.writeFile(file, content); + }; + + const writeChange = async (changeName: string, specId: string, delta: string) => { + const changeDir = path.join(changesDir, changeName); + const specDir = path.join(changeDir, 'specs', ...specId.split('/')); + await fs.mkdir(specDir, { recursive: true }); + await fs.writeFile(path.join(specDir, 'spec.md'), delta); + return changeDir; + }; + + /** The scenario-loss issue, so assertions cannot pass on an unrelated error. */ + const lossIssue = (report: { issues: Array<{ level: string; path: string; message: string }> }) => + report.issues.find((i) => i.message.includes('omits scenario(s)')); + + const validate = (changeDir: string) => + new Validator(true).validateChangeDeltaSpecs(changeDir, { mainSpecsDir }); + + /** + * What archive would do with the same change: null when it applies cleanly. + * It shares the comparison itself with the validator (that is the point of the + * refactor), so what it cross-checks is the layer above: spec discovery, which + * requirement block the MODIFIED lands on, and archive's operation order. + */ + const archiveError = async (changeDir: string): Promise => { + const updates = await findSpecUpdates(changeDir, mainSpecsDir); + for (const update of updates) { + try { + await buildUpdatedSpec(update, path.basename(changeDir), { silent: true }); + } catch (error) { + return error instanceof Error ? error.message : String(error); + } + } + return null; + }; + + beforeEach(async () => { + testDir = await fs.mkdtemp(path.join(os.tmpdir(), 'openspec-scenario-loss-')); + changesDir = path.join(testDir, 'openspec', 'changes'); + mainSpecsDir = path.join(testDir, 'openspec', 'specs'); + await fs.mkdir(changesDir, { recursive: true }); + await fs.mkdir(mainSpecsDir, { recursive: true }); + }); + + afterEach(async () => { + await fs.rm(testDir, { recursive: true, force: true }); + }); + + it('errors when the MODIFIED block omits a scenario the main spec still has', async () => { + await writeMainSpec( + 'widgets', + mainSpec(TWO_SCENARIO_REQUIREMENT) + ); + const changeDir = await writeChange( + 'rename-scenario', + 'widgets', + DELTA_KEEPING_ONE + ); + + const report = await validate(changeDir); + + expect(report.valid).toBe(false); + const issue = report.issues.find((i) => i.message.includes('omits scenario(s)')); + expect(issue?.level).toBe('ERROR'); + expect(issue?.path).toBe('widgets/spec.md'); + expect(issue?.message).toContain('MODIFIED "Widget state"'); + expect(issue?.message).toContain('"Second scenario"'); + // Parity: archive refuses this change today, naming the same scenario. + expect(await archiveError(changeDir)).toContain('Second scenario'); + }); + + it('counts repeated scenario names, so keeping one of two duplicates still errors', async () => { + await writeMainSpec( + 'widgets', + mainSpec( + `### Requirement: Widget state\nThe system SHALL report the widget state.\n\n#### Scenario: Repeated\n- **WHEN** queried once\n- **THEN** the state is reported\n\n#### Scenario: Repeated\n- **WHEN** queried twice\n- **THEN** the state is reported again` + ) + ); + const changeDir = await writeChange( + 'drop-duplicate', + 'widgets', + `## MODIFIED Requirements\n\n### Requirement: Widget state\nThe system SHALL report the widget state.\n\n#### Scenario: Repeated\n- **WHEN** queried once\n- **THEN** the state is reported\n` + ); + + const report = await validate(changeDir); + + expect(report.valid).toBe(false); + expect(lossIssue(report)?.message).toContain('"Repeated"'); + expect(await archiveError(changeDir)).toContain('Repeated'); + }); + + it('accepts a MODIFIED block that carries every current scenario over', async () => { + 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` + ) + ); + const changeDir = await writeChange( + 'keeps-all', + 'widgets', + `## MODIFIED Requirements\n\n### Requirement: Widget state\nThe system SHALL report the widget state promptly.\n\n#### Scenario: Existing scenario\n- **WHEN** queried\n- **THEN** the state is reported\n\n#### Scenario: New scenario\n- **WHEN** it errors\n- **THEN** the error is reported\n` + ); + + const report = await validate(changeDir); + + expect(report.valid).toBe(true); + expect(await archiveError(changeDir)).toBeNull(); + }); + + it('stays silent when the requirement header is not in the main spec (sister change in flight)', async () => { + 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` + ) + ); + const changeDir = await writeChange( + 'cross-change', + 'widgets', + `## MODIFIED Requirements\n\n### Requirement: Widget colour\nThe system SHALL report the widget colour.\n\n#### Scenario: Colour queried\n- **WHEN** queried\n- **THEN** the colour is reported\n` + ); + + const report = await validate(changeDir); + + expect(report.valid).toBe(true); + }); + + it('stays silent when the main spec file does not exist yet', async () => { + const changeDir = await writeChange( + 'greenfield', + 'gadgets', + `## MODIFIED Requirements\n\n### Requirement: Gadget state\nThe system SHALL report the gadget state.\n\n#### Scenario: Gadget queried\n- **WHEN** queried\n- **THEN** the state is reported\n` + ); + + const report = await validate(changeDir); + + expect(report.valid).toBe(true); + }); + + it('ignores a #### Scenario: sample inside a fenced block in the main spec', async () => { + await writeMainSpec( + 'widgets', + mainSpec( + `### Requirement: Widget state\nThe system SHALL report the widget state.\n\n#### Scenario: Real scenario\n- **WHEN** queried\n- **THEN** the state is reported\n\n\`\`\`markdown\n#### Scenario: Sample inside a fence\n- **WHEN** copied\n- **THEN** it is only an example\n\`\`\`` + ) + ); + const changeDir = await writeChange( + 'fenced-sample', + 'widgets', + `## MODIFIED Requirements\n\n### Requirement: Widget state\nThe system SHALL report the widget state clearly.\n\n#### Scenario: Real scenario\n- **WHEN** queried\n- **THEN** the state is reported\n` + ); + + const report = await validate(changeDir); + + expect(report.valid).toBe(true); + expect(await archiveError(changeDir)).toBeNull(); + }); + + it('resolves nested capability layouts against the matching main spec', async () => { + await writeMainSpec( + 'platform/session', + mainSpec( + `### Requirement: Session start\nThe system SHALL start a session.\n\n#### Scenario: Started\n- **WHEN** requested\n- **THEN** a session starts\n\n#### Scenario: Resumed\n- **WHEN** resumed\n- **THEN** the session continues` + ) + ); + const changeDir = await writeChange( + 'nested-drop', + 'platform/session', + `## MODIFIED Requirements\n\n### Requirement: Session start\nThe system SHALL start a session quickly.\n\n#### Scenario: Started\n- **WHEN** requested\n- **THEN** a session starts\n` + ); + + const report = await validate(changeDir); + + expect(report.valid).toBe(false); + const issue = report.issues.find((i) => i.message.includes('omits scenario(s)')); + expect(issue?.path).toBe('platform/session/spec.md'); + expect(issue?.message).toContain('"Resumed"'); + }); + + it('checks a MODIFIED that names the new header of a rename in the same delta', async () => { + await writeMainSpec( + 'widgets', + mainSpec( + `### Requirement: Old name\nThe system SHALL do the old thing.\n\n#### Scenario: Kept\n- **WHEN** invoked\n- **THEN** it works\n\n#### Scenario: Dropped\n- **WHEN** retried\n- **THEN** it still works` + ) + ); + const changeDir = await writeChange( + 'rename-then-modify', + 'widgets', + `## RENAMED Requirements\n\n- FROM: \`### Requirement: Old name\`\n- TO: \`### Requirement: New name\`\n\n## MODIFIED Requirements\n\n### Requirement: New name\nThe system SHALL do the new thing.\n\n#### Scenario: Kept\n- **WHEN** invoked\n- **THEN** it works\n` + ); + + const report = await validate(changeDir); + + expect(report.valid).toBe(false); + expect(lossIssue(report)?.message).toContain('"Dropped"'); + expect(await archiveError(changeDir)).toContain('Dropped'); + }); + + it('follows a chain of renames back to the block the main spec still holds', async () => { + await writeMainSpec( + 'widgets', + mainSpec( + `### Requirement: Alpha\nThe system SHALL do the alpha thing.\n\n#### Scenario: Kept\n- **WHEN** invoked\n- **THEN** it works\n\n#### Scenario: Dropped\n- **WHEN** retried\n- **THEN** it still works` + ) + ); + const changeDir = await writeChange( + 'rename-chain', + 'widgets', + `## RENAMED Requirements\n\n- FROM: \`### Requirement: Alpha\`\n- TO: \`### Requirement: Bravo\`\n- FROM: \`### Requirement: Bravo\`\n- TO: \`### Requirement: Charlie\`\n\n## MODIFIED Requirements\n\n### Requirement: Charlie\nThe system SHALL do the charlie thing.\n\n#### Scenario: Kept\n- **WHEN** invoked\n- **THEN** it works\n` + ); + + const report = await validate(changeDir); + + expect(report.valid).toBe(false); + expect(lossIssue(report)?.message).toContain('"Dropped"'); + expect(await archiveError(changeDir)).toContain('Dropped'); + }); + + it('reads a CRLF main spec the same way archive does', async () => { + await writeMainSpec( + 'widgets', + mainSpec(TWO_SCENARIO_REQUIREMENT).replace(/\n/g, '\r\n') + ); + const changeDir = await writeChange( + 'crlf-drop', + 'widgets', + `## MODIFIED Requirements\r\n\r\n### Requirement: Widget state\r\nThe system SHALL report the widget state.\r\n\r\n#### Scenario: Existing scenario\r\n- **WHEN** queried\r\n- **THEN** the state is reported\r\n` + ); + + const report = await validate(changeDir); + + expect(report.valid).toBe(false); + expect(lossIssue(report)?.message).toContain('"Second scenario"'); + expect(await archiveError(changeDir)).toContain('Second scenario'); + }); + + it('runs no main-spec check when the caller passes no main specs directory', async () => { + await writeMainSpec( + 'widgets', + mainSpec(TWO_SCENARIO_REQUIREMENT) + ); + const changeDir = await writeChange( + 'no-root', + 'widgets', + DELTA_KEEPING_ONE + ); + + const report = await new Validator(true).validateChangeDeltaSpecs(changeDir); + + expect(report.valid).toBe(true); + }); + + it('fails the change in the default (non-strict) mode too', async () => { + // --strict is opt-in, so the shipped default is the mode that matters most. + await writeMainSpec('widgets', mainSpec(TWO_SCENARIO_REQUIREMENT)); + const changeDir = await writeChange('non-strict', 'widgets', DELTA_KEEPING_ONE); + + const report = await new Validator(false).validateChangeDeltaSpecs(changeDir, { mainSpecsDir }); + + expect(report.valid).toBe(false); + expect(lossIssue(report)?.level).toBe('ERROR'); + }); + + it('terminates on a rename cycle instead of walking it forever', async () => { + // Two guards keep the rename walk out of a cycle (the rename-away skip and + // the visited set). A hang here is unrecoverable — it blocks the event loop, + // so no test timeout can interrupt it — which is why the input is pinned. + await writeMainSpec( + 'widgets', + mainSpec( + `### Requirement: Untouched\nThe system SHALL do the untouched thing.\n\n#### Scenario: Only\n- **WHEN** invoked\n- **THEN** it works` + ) + ); + const changeDir = await writeChange( + 'rename-cycle', + 'widgets', + `## RENAMED Requirements\n\n- FROM: \`### Requirement: Alpha\`\n- TO: \`### Requirement: Bravo\`\n- FROM: \`### Requirement: Bravo\`\n- TO: \`### Requirement: Alpha\`\n\n## MODIFIED Requirements\n\n### Requirement: Alpha\nThe system SHALL do the alpha thing.\n\n#### Scenario: Only\n- **WHEN** invoked\n- **THEN** it works\n` + ); + + const report = await validate(changeDir); + + expect(report).toBeDefined(); + expect(lossIssue(report)).toBeUndefined(); + }); + + it('ignores a fenced scenario sample inside the MODIFIED block itself', async () => { + 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#### Scenario: Second scenario\n- **WHEN** idle\n- **THEN** idle is reported` + ) + ); + // The delta quotes "Second scenario" inside a fence; a fenced sample is not + // a scenario, so it must not satisfy the requirement to carry it over. + const changeDir = await writeChange( + 'fenced-in-delta', + '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\n\`\`\`markdown\n#### Scenario: Second scenario\n- **WHEN** idle\n- **THEN** idle is reported\n\`\`\`\n` + ); + + const report = await validate(changeDir); + + expect(report.valid).toBe(false); + expect(lossIssue(report)?.message).toContain('"Second scenario"'); + expect(await archiveError(changeDir)).toContain('Second scenario'); + }); + + it('says so when the main spec exists but cannot be read', async () => { + // A directory where spec.md belongs reads as EISDIR: not absent, and archive + // aborts on it, so reporting beats calling the change valid. + await fs.mkdir(path.join(mainSpecsDir, 'widgets', 'spec.md'), { recursive: true }); + const changeDir = await writeChange('unreadable-main-spec', 'widgets', DELTA_KEEPING_ONE); + + const report = await validate(changeDir); + + expect(report.valid).toBe(false); + const issue = report.issues.find((i) => i.message.includes('Could not read')); + expect(issue?.level).toBe('ERROR'); + expect(issue?.message).toContain('widgets/spec.md'); + expect(issue?.message).toContain('EISDIR'); + expect(await archiveError(changeDir)).not.toBeNull(); + }); + + it('stays silent on a read error that says nothing about the file', async () => { + // A resource error (EMFILE and friends) means the process is busy, not that + // the change is wrong - `validate --all` reads six changes at once, so it + // must not turn one into a verdict. + await writeMainSpec('widgets', mainSpec(TWO_SCENARIO_REQUIREMENT)); + const changeDir = await writeChange('transient-read-error', 'widgets', DELTA_KEEPING_ONE); + // Only the main spec read fails: the delta must still be read, or the check + // never runs and the test proves nothing. + const mainSpecFile = path.join(mainSpecsDir, 'widgets', 'spec.md'); + const readFile = fs.readFile; + const spy = vi.spyOn(fs, 'readFile').mockImplementation(async (file, ...rest) => { + if (String(file) === mainSpecFile) { + throw Object.assign(new Error('EMFILE: too many open files'), { code: 'EMFILE' }); + } + return (readFile as unknown as typeof fs.readFile)(file, ...(rest as [])); + }); + + try { + const report = await validate(changeDir); + expect(spy.mock.calls.some(([file]) => String(file) === mainSpecFile)).toBe(true); + expect(report.issues.some((i) => i.message.includes('Could not read'))).toBe(false); + } finally { + spy.mockRestore(); + } + }); + + 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. + // The contradiction itself is still reported by the RENAMED/MODIFIED check. + await writeMainSpec( + 'widgets', + mainSpec( + `### Requirement: Old name\nThe system SHALL do the old thing.\n\n#### Scenario: Kept\n- **WHEN** invoked\n- **THEN** it works\n\n#### Scenario: Dropped\n- **WHEN** retried\n- **THEN** it still works` + ) + ); + const changeDir = await writeChange( + 'modifies-renamed-away', + 'widgets', + `## RENAMED Requirements\n\n- FROM: \`### Requirement: Old name\`\n- TO: \`### Requirement: New name\`\n\n## MODIFIED Requirements\n\n### Requirement: Old name\nThe system SHALL do the old thing.\n\n#### Scenario: Kept\n- **WHEN** invoked\n- **THEN** it works\n` + ); + + const report = await validate(changeDir); + + expect(report.valid).toBe(false); + expect(lossIssue(report)).toBeUndefined(); + expect(report.issues.map((i) => i.message).join('\n')).toContain('MODIFIED references old name from RENAMED'); + }); +});