From 61d6a2ba52323bcdca3ab83660b353beecd61b15 Mon Sep 17 00:00:00 2001 From: Clay Good Date: Mon, 20 Jul 2026 11:49:15 -0500 Subject: [PATCH 1/2] fix(validate): reject a delta spec at the change's specs/ root (#1385) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A `spec.md` written directly under a change's `specs/` directory was accepted by `validate` — including `--strict` — but skipped by the apply/archive merge, which only reads capability folders. The change validated clean, archived successfully, and its requirements never reached `openspec/specs/`. Point the validator at the shared `discoverSpecFiles` helper so it applies exactly the merge path's rules, and report a root-level `specs/spec.md` as an error naming the capability-folder convention. Archive's delta-detection gate now also sees that file, so validation runs and blocks the archive instead of completing with the delta dropped. Co-Authored-By: Claude Opus 4.8 (1M context) --- .changeset/root-level-delta-spec.md | 7 ++++ src/core/archive.ts | 10 +++++- src/core/validation/validator.ts | 53 +++++++++++------------------ test/core/archive.test.ts | 31 +++++++++++++++++ test/core/validation.test.ts | 51 +++++++++++++++++++++++++++ 5 files changed, 118 insertions(+), 34 deletions(-) create mode 100644 .changeset/root-level-delta-spec.md 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..1adb2f093 100644 --- a/src/core/archive.ts +++ b/src/core/archive.ts @@ -275,7 +275,15 @@ 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)) { + // The root-level specs/spec.md is not a mergeable delta, but it must + // still trigger validation: otherwise a change whose only delta sits + // there skips this gate and archives with its requirements dropped + // (#1385). Validation reports it as an error and blocks the archive. + const deltaCandidates = [ + ...(await discoverSpecFiles(changeSpecsDir)).map(spec => spec.specFile), + path.join(changeSpecsDir, 'spec.md'), + ]; + for (const specFile of deltaCandidates) { 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..ec782742e 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; @@ -125,11 +126,25 @@ export class Validator { 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). + if (await FileSystemUtils.fileExists(path.join(specsDir, 'spec.md'))) { + 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 { @@ -310,34 +325,6 @@ export class Validator { 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..f3e944e66 100644 --- a/test/core/archive.test.ts +++ b/test/core/archive.test.ts @@ -1263,6 +1263,37 @@ 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 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..439933205 100644 --- a/test/core/validation.test.ts +++ b/test/core/validation.test.ts @@ -506,6 +506,57 @@ 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); + }); + + 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'); From d6a30c584d6a044ec7ce350e854f353786370c7b Mon Sep 17 00:00:00 2001 From: Clay Good Date: Mon, 20 Jul 2026 12:08:10 -0500 Subject: [PATCH 2/2] fix(archive): trip the delta gate on any root-level spec.md MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to the same divergence class as the parent commit, found while re-reviewing it. Archive's gate only ran validation when a candidate file carried delta headers, so a root-level `specs/spec.md` written in main-spec shape (`## Requirements`) still archived with exit 0 while `validate` reported an error — the two commands disagreed again. The file is never merged whatever its shape, so existence alone now trips the gate. Also stop reporting a *directory* named `specs/spec.md` as misplaced: that is an ordinary capability folder the merge path reads normally, and `fileExists` matched it. Both sites now require a regular file. Finally, suppress the generic "No deltas found" error when the root-level error already fired: it contradicted the precise message by claiming there were no deltas in the very file just named. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/core/archive.ts | 18 ++++++++---------- src/core/validation/validator.ts | 12 ++++++++++-- test/core/archive.test.ts | 32 ++++++++++++++++++++++++++++++++ test/core/validation.test.ts | 27 +++++++++++++++++++++++++++ 4 files changed, 77 insertions(+), 12 deletions(-) diff --git a/src/core/archive.ts b/src/core/archive.ts index 1adb2f093..869db1814 100644 --- a/src/core/archive.ts +++ b/src/core/archive.ts @@ -274,16 +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; - // The root-level specs/spec.md is not a mergeable delta, but it must - // still trigger validation: otherwise a change whose only delta sits - // there skips this gate and archives with its requirements dropped - // (#1385). Validation reports it as an error and blocks the archive. - const deltaCandidates = [ - ...(await discoverSpecFiles(changeSpecsDir)).map(spec => spec.specFile), - path.join(changeSpecsDir, 'spec.md'), - ]; - for (const specFile of deltaCandidates) { + // 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 ec782742e..4dcc9a2fd 100644 --- a/src/core/validation/validator.ts +++ b/src/core/validation/validator.ts @@ -122,6 +122,7 @@ 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[] }> = []; @@ -136,7 +137,11 @@ export class Validator { // 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). - if (await FileSystemUtils.fileExists(path.join(specsDir, 'spec.md'))) { + // 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', @@ -318,7 +323,10 @@ 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) }); } diff --git a/test/core/archive.test.ts b/test/core/archive.test.ts index f3e944e66..975970ca2 100644 --- a/test/core/archive.test.ts +++ b/test/core/archive.test.ts @@ -1294,6 +1294,38 @@ The system SHALL record request metrics. 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 439933205..a443e4ab0 100644 --- a/test/core/validation.test.ts +++ b/test/core/validation.test.ts @@ -532,6 +532,33 @@ The system SHALL record request metrics. 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 () => {