diff --git a/.changeset/root-level-delta-spec.md b/.changeset/root-level-delta-spec.md new file mode 100644 index 000000000..bb5567f94 --- /dev/null +++ b/.changeset/root-level-delta-spec.md @@ -0,0 +1,7 @@ +--- +"@fission-ai/openspec": patch +--- + +### Fixed + +- Stop a delta spec written directly at a change's `specs/` root from being silently dropped. `validate` accepted `specs/spec.md` and counted its deltas, but the apply/archive merge only reads capability folders (`specs//spec.md`), so the change could pass validation and be archived while its requirements never reached `openspec/specs/`. `validate` now uses the same discovery rules as the merge path and reports the misplaced file with a fix hint, and `archive` blocks instead of completing. diff --git a/src/core/archive.ts b/src/core/archive.ts index 6c777bc7e..869db1814 100644 --- a/src/core/archive.ts +++ b/src/core/archive.ts @@ -274,8 +274,14 @@ export class ArchiveCommand { // Validate delta-formatted spec files under the change directory if present const changeSpecsDir = path.join(changeDir, 'specs'); - let hasDeltaSpecs = false; - for (const { specFile } of await discoverSpecFiles(changeSpecsDir)) { + // A spec.md at the specs/ root is never merged, so archiving a change + // that has one drops its content whether or not it carries delta headers + // (#1385). Its existence alone must run validation, which reports it and + // blocks the archive. A directory named spec.md is a normal capability + // folder, so only a regular file counts. + const rootSpecStat = await fs.stat(path.join(changeSpecsDir, 'spec.md')).catch(() => null); + let hasDeltaSpecs = rootSpecStat?.isFile() === true; + for (const { specFile } of hasDeltaSpecs ? [] : await discoverSpecFiles(changeSpecsDir)) { try { const content = await fs.readFile(specFile, 'utf-8'); if (/^##\s+(ADDED|MODIFIED|REMOVED|RENAMED)\s+Requirements/m.test(content)) { diff --git a/src/core/validation/validator.ts b/src/core/validation/validator.ts index 511662f29..4dcc9a2fd 100644 --- a/src/core/validation/validator.ts +++ b/src/core/validation/validator.ts @@ -18,6 +18,7 @@ import { } from '../parsers/requirement-text.js'; import { findMainSpecStructureIssues } from '../parsers/spec-structure.js'; import { FileSystemUtils } from '../../utils/file-system.js'; +import { discoverSpecFiles } from '../../utils/spec-discovery.js'; export class Validator { private strictMode: boolean; @@ -121,15 +122,34 @@ export class Validator { const issues: ValidationIssue[] = []; const specsDir = path.join(changeDir, 'specs'); let totalDeltas = 0; + let hasRootLevelSpec = false; const missingHeaderSpecs: string[] = []; const emptySectionSpecs: Array<{ path: string; sections: string[] }> = []; try { - // Discover delta specs at any depth so the nested multi-area layout - // (specs///spec.md) is validated, not just the - // one-level specs//spec.md layout (#1182b). The spec-driven - // specs glob is specs/**/*.md; delta files are always named spec.md. - const specFiles = await this.findDeltaSpecFiles(specsDir); + // Discover delta specs through the same helper the change parser, show, + // apply, and archive use, so validate never accepts a layout the merge + // 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); + + // 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 + // archives while its requirements never reach openspec/specs/ (#1385). + // Only a regular file counts — a *directory* named spec.md is a capability + // folder like any other, and discoverSpecFiles reads it normally. + const rootSpecStat = await fs.stat(path.join(specsDir, 'spec.md')).catch(() => null); + hasRootLevelSpec = rootSpecStat?.isFile() === true; + if (hasRootLevelSpec) { + issues.push({ + level: 'ERROR', + path: 'spec.md', + message: + 'Delta spec found at specs/spec.md. Delta specs must live in a capability folder (e.g. specs//spec.md) — a file at the specs/ root is ignored when the change is applied or archived.', + }); + } + for (const specFile of specFiles) { let content: string | undefined; try { @@ -303,41 +323,16 @@ export class Validator { }); } - if (totalDeltas === 0) { + // The root-level error already names the file and the fix; adding "No + // deltas found" on top would contradict it, since the deltas are sitting in + // the file just reported. + if (totalDeltas === 0 && !hasRootLevelSpec) { issues.push({ level: 'ERROR', path: 'file', message: this.enrichTopLevelError('change', VALIDATION_MESSAGES.CHANGE_NO_DELTAS) }); } return this.createReport(issues); } - /** - * Recursively collect every delta `spec.md` under a change's specs directory, - * so both the one-level (specs//spec.md) and nested multi-area - * (specs///spec.md) layouts are discovered (#1182b). - * Returns absolute paths, sorted for deterministic issue ordering. - */ - private async findDeltaSpecFiles(specsDir: string): Promise { - const results: string[] = []; - const walk = async (dir: string): Promise => { - let entries; - try { - entries = await fs.readdir(dir, { withFileTypes: true }); - } catch { - return; - } - for (const entry of entries) { - const full = path.join(dir, entry.name); - if (entry.isDirectory()) { - await walk(full); - } else if (entry.isFile() && entry.name === 'spec.md') { - results.push(full); - } - } - }; - await walk(specsDir); - return results.sort(); - } - private convertZodErrors(error: ZodError): ValidationIssue[] { return error.issues.map(err => { let message = err.message; diff --git a/test/core/archive.test.ts b/test/core/archive.test.ts index 9f939d083..975970ca2 100644 --- a/test/core/archive.test.ts +++ b/test/core/archive.test.ts @@ -1263,6 +1263,69 @@ The system will log all events. expect(archives.some(a => a.includes(changeName))).toBe(false); }); + it('sets exit code 1 when the only delta spec sits at the specs/ root (#1385)', async () => { + const changeName = 'exit-root-delta'; + const changeDir = path.join(tempDir, 'openspec', 'changes', changeName); + const changeSpecsDir = path.join(changeDir, 'specs'); + await fs.mkdir(changeSpecsDir, { recursive: true }); + + // No capability folder: the merge path skips this file, so archiving it + // used to succeed while dropping the requirement. + const specContent = `## ADDED Requirements + +### Requirement: Request metrics +The system SHALL record request metrics. + +#### Scenario: Request is counted +- **WHEN** a request completes +- **THEN** a counter is incremented`; + await fs.writeFile(path.join(changeSpecsDir, 'spec.md'), specContent); + await fs.writeFile(path.join(changeDir, 'tasks.md'), '- [x] Task 1\n'); + + await archiveCommand.execute(changeName, { yes: true }); + + expect(process.exitCode).toBe(1); + expect(console.log).toHaveBeenCalledWith( + expect.stringContaining('Validation failed') + ); + + const archiveDir = path.join(tempDir, 'openspec', 'changes', 'archive'); + const archives = await fs.readdir(archiveDir); + expect(archives.some(a => a.includes(changeName))).toBe(false); + }); + + it('sets exit code 1 for a root-level specs/spec.md without delta headers (#1385)', async () => { + const changeName = 'exit-root-plain'; + const changeDir = path.join(tempDir, 'openspec', 'changes', changeName); + const changeSpecsDir = path.join(changeDir, 'specs'); + await fs.mkdir(changeSpecsDir, { recursive: true }); + + // Main-spec shape rather than delta shape: still never merged, so the + // gate must trip on the file existing, not on its headers. + const specContent = `# Metrics + +## Purpose +Metrics for requests. + +## Requirements + +### Requirement: Request metrics +The system SHALL record request metrics. + +#### Scenario: Request is counted +- **WHEN** a request completes +- **THEN** a counter is incremented`; + await fs.writeFile(path.join(changeSpecsDir, 'spec.md'), specContent); + await fs.writeFile(path.join(changeDir, 'tasks.md'), '- [x] Task 1\n'); + + await archiveCommand.execute(changeName, { yes: true }); + + expect(process.exitCode).toBe(1); + const archiveDir = path.join(tempDir, 'openspec', 'changes', 'archive'); + const archives = await fs.readdir(archiveDir); + expect(archives.some(a => a.includes(changeName))).toBe(false); + }); + it('sets exit code 1 when spec rebuild fails (MODIFIED on new spec)', async () => { const changeName = 'exit-rebuild-fail'; const changeDir = path.join(tempDir, 'openspec', 'changes', changeName); diff --git a/test/core/validation.test.ts b/test/core/validation.test.ts index ebdc90e97..a443e4ab0 100644 --- a/test/core/validation.test.ts +++ b/test/core/validation.test.ts @@ -506,6 +506,84 @@ The system SHALL handle all errors gracefully. expect(report.summary.errors).toBe(0); }); + it('should fail when a delta spec.md sits directly under specs/', async () => { + // #1385: the merge path only reads specs//spec.md, so a + // root-level file used to validate clean and then archive with its + // requirements silently dropped. + const changeDir = path.join(testDir, 'test-change-root-delta'); + const specsDir = path.join(changeDir, 'specs'); + await fs.mkdir(specsDir, { recursive: true }); + + const deltaSpec = `## ADDED Requirements + +### Requirement: Request metrics +The system SHALL record request metrics. + +#### Scenario: Request is counted +- **WHEN** a request completes +- **THEN** a counter is incremented`; + + await fs.writeFile(path.join(specsDir, 'spec.md'), deltaSpec); + + const validator = new Validator(true); + const report = await validator.validateChangeDeltaSpecs(changeDir); + + expect(report.valid).toBe(false); + expect( + report.issues.some(i => i.message.includes('Delta spec found at specs/spec.md')) + ).toBe(true); + // The precise error replaces the generic one, which would otherwise say + // "No deltas found" about a file it just named. + expect(report.issues.some(i => i.message.includes('No deltas found'))).toBe(false); + }); + + it('should accept a capability folder that is literally named spec.md', async () => { + const changeDir = path.join(testDir, 'test-change-spec-md-folder'); + const specsDir = path.join(changeDir, 'specs', 'spec.md'); + await fs.mkdir(specsDir, { recursive: true }); + + const deltaSpec = `## ADDED Requirements + +### Requirement: Request metrics +The system SHALL record request metrics. + +#### Scenario: Request is counted +- **WHEN** a request completes +- **THEN** a counter is incremented`; + + await fs.writeFile(path.join(specsDir, 'spec.md'), deltaSpec); + + const validator = new Validator(true); + const report = await validator.validateChangeDeltaSpecs(changeDir); + + // specs/spec.md is a directory here, so nothing is dropped by the merge. + expect(report.valid).toBe(true); + expect(report.summary.errors).toBe(0); + }); + + it('should still validate a nested capability layout', async () => { + const changeDir = path.join(testDir, 'test-change-nested-delta'); + const specsDir = path.join(changeDir, 'specs', 'platform', 'metrics'); + await fs.mkdir(specsDir, { recursive: true }); + + const deltaSpec = `## ADDED Requirements + +### Requirement: Request metrics +The system SHALL record request metrics. + +#### Scenario: Request is counted +- **WHEN** a request completes +- **THEN** a counter is incremented`; + + await fs.writeFile(path.join(specsDir, 'spec.md'), deltaSpec); + + const validator = new Validator(true); + const report = await validator.validateChangeDeltaSpecs(changeDir); + + expect(report.valid).toBe(true); + expect(report.summary.errors).toBe(0); + }); + it('should fail when requirement text lacks SHALL/MUST', async () => { const changeDir = path.join(testDir, 'test-change-3'); const specsDir = path.join(changeDir, 'specs', 'test-spec');