From 5b675c8845aacd4b8917256df26c4e7e95e0a0c1 Mon Sep 17 00:00:00 2001 From: Clay Good Date: Wed, 29 Jul 2026 09:48:19 -0500 Subject: [PATCH 1/6] fix(validate): report scenarios a MODIFIED requirement would drop MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `openspec validate ` accepted a MODIFIED requirement that omits a scenario the main spec still has, even with --strict. Archive refuses to apply that block (a MODIFIED replaces the whole requirement, so the omitted scenario would be lost), so the change could pass validation, be implemented and reviewed, and fail only days later at archive time (#1477). Validate now runs the same non-mutating check against the main specs and reports each omitted scenario, naming the delta file. The comparison itself moved to the parser module so archive and validate share one implementation and cannot drift. The check is silent when the main spec file or the requirement header is absent — a MODIFIED written against a sister change still in flight is a separate condition archive gates — so validate can only report what archive already refuses. Co-Authored-By: Claude Opus 5 (1M context) --- .changeset/validate-scenario-loss.md | 5 + openspec/specs/cli-validate/spec.md | 19 ++ src/commands/change.ts | 4 +- src/commands/validate.ts | 4 +- src/core/parsers/requirement-blocks.ts | 71 ++++++ src/core/specs-apply.ts | 63 +---- src/core/validation/validator.ts | 95 +++++++- test/core/validation.scenario-loss.test.ts | 255 +++++++++++++++++++++ 8 files changed, 447 insertions(+), 69 deletions(-) create mode 100644 .changeset/validate-scenario-loss.md create mode 100644 test/core/validation.scenario-loss.test.ts diff --git a/.changeset/validate-scenario-loss.md b/.changeset/validate-scenario-loss.md new file mode 100644 index 0000000000..c7c87169b5 --- /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. diff --git a/openspec/specs/cli-validate/spec.md b/openspec/specs/cli-validate/spec.md index 61afc7953b..bf8f046067 100644 --- a/openspec/specs/cli-validate/spec.md +++ b/openspec/specs/cli-validate/spec.md @@ -62,6 +62,25 @@ 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 + +A `MODIFIED` requirement replaces the whole requirement block, so archive refuses to apply one that omits a scenario the main spec still has. Change validation SHALL run the same non-mutating check against the main specs and report each omitted scenario as an error, naming the delta file and the 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. + +#### 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 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/src/commands/change.ts b/src/commands/change.ts index f9a1995496..5c45013902 100644 --- a/src/commands/change.ts +++ b/src/commands/change.ts @@ -256,7 +256,9 @@ export class ChangeCommand { } const validator = new Validator(options?.strict || false); - const report = await validator.validateChangeDeltaSpecs(changeDir); + const report = await validator.validateChangeDeltaSpecs(changeDir, { + mainSpecsDir: path.join(process.cwd(), 'openspec', '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/parsers/requirement-blocks.ts b/src/core/parsers/requirement-blocks.ts index cb0e79b75b..e062869d79 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; } + +export 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; +} + +export 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/validation/validator.ts b/src/core/validation/validator.ts index 0086c12766..78c288c3fa 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,21 @@ export class Validator { } } + // MODIFIED blocks that drop a scenario the main spec still has are + // rejected by archive, which refuses to overwrite the requirement and + // lose it. Run the same non-mutating check here 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 +431,63 @@ 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. + */ + 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 { + return []; + } + + 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. Re-key the + // same way or a rename-plus-modify pair would skip the check entirely. + for (const { from, to } of renamed) { + const fromKey = normalizeRequirementName(from); + const toKey = normalizeRequirementName(to); + const block = currentBlocks.get(fromKey); + if (!block || currentBlocks.has(toKey)) continue; + currentBlocks.delete(fromKey); + currentBlocks.set(toKey, block); + } + + const issues: ValidationIssue[] = []; + for (const block of modified) { + const current = currentBlocks.get(normalizeRequirementName(block.name)); + 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/core/validation.scenario-loss.test.ts b/test/core/validation.scenario-loss.test.ts new file mode 100644 index 0000000000..9227aec074 --- /dev/null +++ b/test/core/validation.scenario-loss.test.ts @@ -0,0 +1,255 @@ +import { describe, it, expect, beforeEach, afterEach } 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; + + 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; + }; + + const validate = (changeDir: string) => + new Validator(true).validateChangeDeltaSpecs(changeDir, { mainSpecsDir }); + + /** What archive would do with the same change: null when it applies cleanly. */ + 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( + `### 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 changeDir = await writeChange( + 'rename-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); + 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 rejects exactly this change today. + 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(report.issues.map((i) => i.message).join('\n')).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(report.issues.map((i) => i.message).join('\n')).toContain('"Dropped"'); + expect(await archiveError(changeDir)).toContain('Dropped'); + }); + + it('reads a CRLF main spec the same way archive does', 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` + ).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(report.issues.map((i) => i.message).join('\n')).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( + `### 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 changeDir = await writeChange( + 'no-root', + '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 new Validator(true).validateChangeDeltaSpecs(changeDir); + + expect(report.valid).toBe(true); + }); +}); From f5f48c9dc8f29ee7690c540d19fb998cb1480941 Mon Sep 17 00:00:00 2001 From: Clay Good Date: Wed, 29 Jul 2026 09:55:46 -0500 Subject: [PATCH 2/6] refactor(validate): tighten the scenario-loss check after review Keep the moved scenario parser module-private, derive change validate's main specs root from the changes root it already resolved, replace the rename re-keying with a lookup fallback, and say at archive's call site why it does not opt in. Co-Authored-By: Claude Opus 5 (1M context) --- src/commands/change.ts | 4 +++- src/core/archive.ts | 3 +++ src/core/parsers/requirement-blocks.ts | 4 ++-- src/core/validation/validator.ts | 22 ++++++++-------------- test/core/validation.scenario-loss.test.ts | 20 +++++++++----------- 5 files changed, 25 insertions(+), 28 deletions(-) diff --git a/src/commands/change.ts b/src/commands/change.ts index 5c45013902..23a23de5e5 100644 --- a/src/commands/change.ts +++ b/src/commands/change.ts @@ -257,7 +257,9 @@ export class ChangeCommand { const validator = new Validator(options?.strict || false); const report = await validator.validateChangeDeltaSpecs(changeDir, { - mainSpecsDir: path.join(process.cwd(), 'openspec', 'specs'), + // 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) { 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 e062869d79..2f2c8a2004 100644 --- a/src/core/parsers/requirement-blocks.ts +++ b/src/core/parsers/requirement-blocks.ts @@ -324,7 +324,7 @@ function parseRenamedPairs(sectionBody: SectionBody): Array<{ from: string; to: return pairs; } -export interface ScenarioBlock { +interface ScenarioBlock { name: string; raw: string; } @@ -362,7 +362,7 @@ export function findMissingCurrentScenarios(current: RequirementBlock, incoming: return missing; } -export function parseScenarioBlocks(requirementRaw: string): ScenarioBlock[] { +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 diff --git a/src/core/validation/validator.ts b/src/core/validation/validator.ts index 78c288c3fa..a3a8ed9606 100644 --- a/src/core/validation/validator.ts +++ b/src/core/validation/validator.ts @@ -282,9 +282,7 @@ export class Validator { } } - // MODIFIED blocks that drop a scenario the main spec still has are - // rejected by archive, which refuses to overwrite the requirement and - // lose it. Run the same non-mutating check here so the change fails at + // 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( @@ -459,20 +457,16 @@ export class Validator { 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. Re-key the - // same way or a rename-plus-modify pair would skip the check entirely. - for (const { from, to } of renamed) { - const fromKey = normalizeRequirementName(from); - const toKey = normalizeRequirementName(to); - const block = currentBlocks.get(fromKey); - if (!block || currentBlocks.has(toKey)) continue; - currentBlocks.delete(fromKey); - currentBlocks.set(toKey, block); - } + // 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)]) + ); const issues: ValidationIssue[] = []; for (const block of modified) { - const current = currentBlocks.get(normalizeRequirementName(block.name)); + const key = normalizeRequirementName(block.name); + const current = currentBlocks.get(key) ?? currentBlocks.get(renamedFrom.get(key) ?? ''); if (!current) continue; const missing = findMissingCurrentScenarios(current, block); if (missing.length === 0) continue; diff --git a/test/core/validation.scenario-loss.test.ts b/test/core/validation.scenario-loss.test.ts index 9227aec074..586b845bbe 100644 --- a/test/core/validation.scenario-loss.test.ts +++ b/test/core/validation.scenario-loss.test.ts @@ -16,6 +16,10 @@ describe('validate: MODIFIED blocks that would drop a main-spec scenario (#1477) 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`; @@ -64,14 +68,12 @@ describe('validate: MODIFIED blocks that would drop a main-spec scenario (#1477) it('errors when the MODIFIED block omits a scenario the main spec still has', 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` - ) + mainSpec(TWO_SCENARIO_REQUIREMENT) ); const changeDir = await writeChange( 'rename-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` + DELTA_KEEPING_ONE ); const report = await validate(changeDir); @@ -218,9 +220,7 @@ describe('validate: MODIFIED blocks that would drop a main-spec scenario (#1477) it('reads a CRLF main spec the same way archive does', 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` - ).replace(/\n/g, '\r\n') + mainSpec(TWO_SCENARIO_REQUIREMENT).replace(/\n/g, '\r\n') ); const changeDir = await writeChange( 'crlf-drop', @@ -238,14 +238,12 @@ describe('validate: MODIFIED blocks that would drop a main-spec scenario (#1477) it('runs no main-spec check when the caller passes no main specs directory', 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` - ) + mainSpec(TWO_SCENARIO_REQUIREMENT) ); const changeDir = await writeChange( 'no-root', '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` + DELTA_KEEPING_ONE ); const report = await new Validator(true).validateChangeDeltaSpecs(changeDir); From 96b0c845b987c113468d2ca719e97674199e2da3 Mon Sep 17 00:00:00 2001 From: Clay Good Date: Wed, 29 Jul 2026 10:09:31 -0500 Subject: [PATCH 3/6] fix(validate): follow rename chains when checking for dropped scenarios A delta that renames A to B and then B to C leaves C holding A's block at archive time. Walk the rename map instead of looking it up once, so the chained case reports the same loss archive refuses. Co-Authored-By: Claude Opus 5 (1M context) --- src/core/validation/validator.ts | 17 +++++++++++++++-- test/core/validation.scenario-loss.test.ts | 20 ++++++++++++++++++++ 2 files changed, 35 insertions(+), 2 deletions(-) diff --git a/src/core/validation/validator.ts b/src/core/validation/validator.ts index a3a8ed9606..a14d2bfd67 100644 --- a/src/core/validation/validator.ts +++ b/src/core/validation/validator.ts @@ -463,10 +463,23 @@ export class Validator { 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. + 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; + }; + const issues: ValidationIssue[] = []; for (const block of modified) { - const key = normalizeRequirementName(block.name); - const current = currentBlocks.get(key) ?? currentBlocks.get(renamedFrom.get(key) ?? ''); + const current = currentBlockFor(normalizeRequirementName(block.name)); if (!current) continue; const missing = findMissingCurrentScenarios(current, block); if (missing.length === 0) continue; diff --git a/test/core/validation.scenario-loss.test.ts b/test/core/validation.scenario-loss.test.ts index 586b845bbe..4f3cb3629c 100644 --- a/test/core/validation.scenario-loss.test.ts +++ b/test/core/validation.scenario-loss.test.ts @@ -217,6 +217,26 @@ describe('validate: MODIFIED blocks that would drop a main-spec scenario (#1477) 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(report.issues.map((i) => i.message).join('\n')).toContain('"Dropped"'); + expect(await archiveError(changeDir)).toContain('Dropped'); + }); + it('reads a CRLF main spec the same way archive does', async () => { await writeMainSpec( 'widgets', From 9b49084e99c8985e8bf1dbaaa852dfe8a07d3b64 Mon Sep 17 00:00:00 2001 From: Clay Good Date: Wed, 29 Jul 2026 10:41:14 -0500 Subject: [PATCH 4/6] fix(validate): close the gaps five adversarial reviews found MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Code: - An unreadable main spec was swallowed, so a change archive aborts on validated clean. Only ENOENT/ENOTDIR mean "no main spec" now; anything else is reported. - A MODIFIED naming a header the same delta renames away no longer names scenarios from the block it would not land on. That contradiction is already reported on its own, and the scenario list pointed at the wrong requirement. Guidance: the sync-specs skill told agents a MODIFIED block may carry only the changed scenario, and its format reference showed one. Both validate and archive reject that shape, so the template, the generated skill, and the golden hashes are updated to match the schema's own rule. Tests: the CLI wiring had no coverage at all — removing the argument that turns the check on broke nothing. Adds end-to-end coverage of every entry point and exit code, plus the non-strict default, a fenced scenario in the delta, an unreadable main spec, the rename-away case, and a rename cycle. Loose assertions now pin the scenario-loss issue itself. Docs: a troubleshooting entry for the new message, and the changeset says that a stale change will newly fail. Co-Authored-By: Claude Opus 5 (1M context) --- .changeset/validate-scenario-loss.md | 2 +- docs/cli.md | 2 +- docs/troubleshooting.md | 8 ++ openspec/specs/cli-validate/spec.md | 14 ++- skills/openspec-sync-specs/SKILL.md | 14 ++- src/core/templates/workflows/sync-specs.ts | 28 +++-- src/core/validation/validator.ts | 34 +++++- test/cli-e2e/validate-scenario-loss.test.ts | 104 ++++++++++++++++ .../templates/skill-templates-parity.test.ts | 6 +- test/core/validation.scenario-loss.test.ts | 114 +++++++++++++++++- 10 files changed, 296 insertions(+), 30 deletions(-) create mode 100644 test/cli-e2e/validate-scenario-loss.test.ts diff --git a/.changeset/validate-scenario-loss.md b/.changeset/validate-scenario-loss.md index c7c87169b5..a6c45180c4 100644 --- a/.changeset/validate-scenario-loss.md +++ b/.changeset/validate-scenario-loss.md @@ -2,4 +2,4 @@ "@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. +`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..0540e4c173 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: + +``` +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 bf8f046067..5f213978c4 100644 --- a/openspec/specs/cli-validate/spec.md +++ b/openspec/specs/cli-validate/spec.md @@ -64,9 +64,13 @@ The CLI SHALL append a Next steps footer when the item is invalid and not using ### Requirement: Change validation SHALL report scenarios a MODIFIED block would drop -A `MODIFIED` requirement replaces the whole requirement block, so archive refuses to apply one that omits a scenario the main spec still has. Change validation SHALL run the same non-mutating check against the main specs and report each omitted scenario as an error, naming the delta file and the scenarios. +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 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. +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 @@ -75,6 +79,12 @@ The check SHALL be silent when the main spec file or the requirement header is a - **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 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/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 a14d2bfd67..421b7ddd65 100644 --- a/src/core/validation/validator.ts +++ b/src/core/validation/validator.ts @@ -437,7 +437,8 @@ export class Validator { * 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. + * 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[], @@ -448,8 +449,20 @@ export class Validator { let mainContent: string; try { mainContent = await fs.readFile(mainSpecFile, 'utf-8'); - } catch { - return []; + } catch (error) { + const code = (error as NodeJS.ErrnoException)?.code; + // ENOTDIR joins ENOENT as "no such spec": a file sitting where a parent + // folder would be means the capability has no main spec either. + if (code === 'ENOENT' || code === 'ENOTDIR') return []; + return [ + { + level: 'ERROR', + path: entryPath, + message: + `Could not read ${FileSystemUtils.toPosixPath(mainSpecFile)} to check the MODIFIED requirements against it ` + + `(${code ?? 'unknown error'}). Archive reads the same file, so fix the file before archiving.`, + }, + ]; } const currentBlocks = new Map(); @@ -464,7 +477,10 @@ export class Validator { ); // 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. + // 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; @@ -477,9 +493,17 @@ export class Validator { 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 current = currentBlockFor(normalizeRequirementName(block.name)); + 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; 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 index 4f3cb3629c..e32df98b53 100644 --- a/test/core/validation.scenario-loss.test.ts +++ b/test/core/validation.scenario-loss.test.ts @@ -37,10 +37,19 @@ describe('validate: MODIFIED blocks that would drop a main-spec scenario (#1477) 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. */ + /** + * 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) { @@ -84,7 +93,7 @@ describe('validate: MODIFIED blocks that would drop a main-spec scenario (#1477) expect(issue?.path).toBe('widgets/spec.md'); expect(issue?.message).toContain('MODIFIED "Widget state"'); expect(issue?.message).toContain('"Second scenario"'); - // Parity: archive rejects exactly this change today. + // Parity: archive refuses this change today, naming the same scenario. expect(await archiveError(changeDir)).toContain('Second scenario'); }); @@ -104,7 +113,7 @@ describe('validate: MODIFIED blocks that would drop a main-spec scenario (#1477) const report = await validate(changeDir); expect(report.valid).toBe(false); - expect(report.issues.map((i) => i.message).join('\n')).toContain('"Repeated"'); + expect(lossIssue(report)?.message).toContain('"Repeated"'); expect(await archiveError(changeDir)).toContain('Repeated'); }); @@ -213,7 +222,7 @@ describe('validate: MODIFIED blocks that would drop a main-spec scenario (#1477) const report = await validate(changeDir); expect(report.valid).toBe(false); - expect(report.issues.map((i) => i.message).join('\n')).toContain('"Dropped"'); + expect(lossIssue(report)?.message).toContain('"Dropped"'); expect(await archiveError(changeDir)).toContain('Dropped'); }); @@ -233,7 +242,7 @@ describe('validate: MODIFIED blocks that would drop a main-spec scenario (#1477) const report = await validate(changeDir); expect(report.valid).toBe(false); - expect(report.issues.map((i) => i.message).join('\n')).toContain('"Dropped"'); + expect(lossIssue(report)?.message).toContain('"Dropped"'); expect(await archiveError(changeDir)).toContain('Dropped'); }); @@ -251,7 +260,7 @@ describe('validate: MODIFIED blocks that would drop a main-spec scenario (#1477) const report = await validate(changeDir); expect(report.valid).toBe(false); - expect(report.issues.map((i) => i.message).join('\n')).toContain('"Second scenario"'); + expect(lossIssue(report)?.message).toContain('"Second scenario"'); expect(await archiveError(changeDir)).toContain('Second scenario'); }); @@ -270,4 +279,97 @@ describe('validate: MODIFIED blocks that would drop a main-spec scenario (#1477) 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(await archiveError(changeDir)).not.toBeNull(); + }); + + 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'); + }); }); From 2dbd8394b602e33995cb3131ad9e32602ff3c13e Mon Sep 17 00:00:00 2001 From: Clay Good Date: Wed, 29 Jul 2026 10:59:50 -0500 Subject: [PATCH 5/6] fix(validate): never turn a transient read error into a verdict The unreadable-main-spec report added in the last commit fired for any errno that was not ENOENT/ENOTDIR, which includes resource errors like EMFILE that say nothing about the file. `validate --all` reads six changes at once, so a busy process could have failed a change that is fine. Reported now only for the codes that mean the file itself is unusable and will be just as unusable when archive reads it. Co-Authored-By: Claude Opus 5 (1M context) --- src/core/validation/validator.ts | 13 +++++++--- test/core/validation.scenario-loss.test.ts | 29 +++++++++++++++++++++- 2 files changed, 37 insertions(+), 5 deletions(-) diff --git a/src/core/validation/validator.ts b/src/core/validation/validator.ts index 421b7ddd65..280f309c36 100644 --- a/src/core/validation/validator.ts +++ b/src/core/validation/validator.ts @@ -451,16 +451,21 @@ export class Validator { mainContent = await fs.readFile(mainSpecFile, 'utf-8'); } catch (error) { const code = (error as NodeJS.ErrnoException)?.code; - // ENOTDIR joins ENOENT as "no such spec": a file sitting where a parent - // folder would be means the capability has no main spec either. - if (code === 'ENOENT' || code === 'ENOTDIR') return []; + // 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 ?? 'unknown error'}). Archive reads the same file, so fix the file before archiving.`, + `(${code}). Archive reads the same file, so fix the file before archiving.`, }, ]; } diff --git a/test/core/validation.scenario-loss.test.ts b/test/core/validation.scenario-loss.test.ts index e32df98b53..32524e526b 100644 --- a/test/core/validation.scenario-loss.test.ts +++ b/test/core/validation.scenario-loss.test.ts @@ -1,4 +1,4 @@ -import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; import { promises as fs } from 'fs'; import os from 'os'; import path from 'path'; @@ -347,9 +347,36 @@ describe('validate: MODIFIED blocks that would drop a main-spec scenario (#1477) 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. From 89bd416155cdbadca322dae0e54fb554145e11a3 Mon Sep 17 00:00:00 2001 From: Clay Good Date: Wed, 29 Jul 2026 11:22:24 -0500 Subject: [PATCH 6/6] docs(troubleshooting): label the example fence (MD040) Every other fence in the file names its language. Co-Authored-By: Claude Opus 5 (1M context) --- docs/troubleshooting.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/troubleshooting.md b/docs/troubleshooting.md index 0540e4c173..75a521f965 100644 --- a/docs/troubleshooting.md +++ b/docs/troubleshooting.md @@ -98,7 +98,7 @@ Common causes are a missing required section (like a spec with no scenarios) or One message deserves its own note: -``` +```text MODIFIED "" omits scenario(s) the current spec still has: "" ```