Skip to content

Commit 805169b

Browse files
clay-goodclaude
andcommitted
fix(validate): reject a delta spec at the change's specs/ root (#1385)
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) <noreply@anthropic.com>
1 parent 46a4d78 commit 805169b

5 files changed

Lines changed: 118 additions & 34 deletions

File tree

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
---
2+
"@fission-ai/openspec": patch
3+
---
4+
5+
### Fixed
6+
7+
- 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/<capability>/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.

src/core/archive.ts

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -267,7 +267,15 @@ export class ArchiveCommand {
267267
// Validate delta-formatted spec files under the change directory if present
268268
const changeSpecsDir = path.join(changeDir, 'specs');
269269
let hasDeltaSpecs = false;
270-
for (const { specFile } of await discoverSpecFiles(changeSpecsDir)) {
270+
// The root-level specs/spec.md is not a mergeable delta, but it must
271+
// still trigger validation: otherwise a change whose only delta sits
272+
// there skips this gate and archives with its requirements dropped
273+
// (#1385). Validation reports it as an error and blocks the archive.
274+
const deltaCandidates = [
275+
...(await discoverSpecFiles(changeSpecsDir)).map(spec => spec.specFile),
276+
path.join(changeSpecsDir, 'spec.md'),
277+
];
278+
for (const specFile of deltaCandidates) {
271279
try {
272280
const content = await fs.readFile(specFile, 'utf-8');
273281
if (/^##\s+(ADDED|MODIFIED|REMOVED|RENAMED)\s+Requirements/m.test(content)) {

src/core/validation/validator.ts

Lines changed: 20 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ import {
1818
} from '../parsers/requirement-text.js';
1919
import { findMainSpecStructureIssues } from '../parsers/spec-structure.js';
2020
import { FileSystemUtils } from '../../utils/file-system.js';
21+
import { discoverSpecFiles } from '../../utils/spec-discovery.js';
2122

2223
export class Validator {
2324
private strictMode: boolean;
@@ -125,11 +126,25 @@ export class Validator {
125126
const emptySectionSpecs: Array<{ path: string; sections: string[] }> = [];
126127

127128
try {
128-
// Discover delta specs at any depth so the nested multi-area layout
129-
// (specs/<area>/<capability>/spec.md) is validated, not just the
130-
// one-level specs/<capability>/spec.md layout (#1182b). The spec-driven
131-
// specs glob is specs/**/*.md; delta files are always named spec.md.
132-
const specFiles = await this.findDeltaSpecFiles(specsDir);
129+
// Discover delta specs through the same helper the change parser, show,
130+
// apply, and archive use, so validate never accepts a layout the merge
131+
// path silently skips (#1385). It finds spec.md at any depth, covering
132+
// both specs/<capability>/spec.md and the nested multi-area
133+
// specs/<area>/<capability>/spec.md layout (#1182b).
134+
const specFiles = (await discoverSpecFiles(specsDir)).map(spec => spec.specFile);
135+
136+
// A spec.md directly at the specs/ root has no capability folder, so the
137+
// merge path drops it: without this error the change validates clean and
138+
// archives while its requirements never reach openspec/specs/ (#1385).
139+
if (await FileSystemUtils.fileExists(path.join(specsDir, 'spec.md'))) {
140+
issues.push({
141+
level: 'ERROR',
142+
path: 'spec.md',
143+
message:
144+
'Delta spec found at specs/spec.md. Delta specs must live in a capability folder (e.g. specs/<capability>/spec.md) — a file at the specs/ root is ignored when the change is applied or archived.',
145+
});
146+
}
147+
133148
for (const specFile of specFiles) {
134149
let content: string | undefined;
135150
try {
@@ -310,34 +325,6 @@ export class Validator {
310325
return this.createReport(issues);
311326
}
312327

313-
/**
314-
* Recursively collect every delta `spec.md` under a change's specs directory,
315-
* so both the one-level (specs/<capability>/spec.md) and nested multi-area
316-
* (specs/<area>/<capability>/spec.md) layouts are discovered (#1182b).
317-
* Returns absolute paths, sorted for deterministic issue ordering.
318-
*/
319-
private async findDeltaSpecFiles(specsDir: string): Promise<string[]> {
320-
const results: string[] = [];
321-
const walk = async (dir: string): Promise<void> => {
322-
let entries;
323-
try {
324-
entries = await fs.readdir(dir, { withFileTypes: true });
325-
} catch {
326-
return;
327-
}
328-
for (const entry of entries) {
329-
const full = path.join(dir, entry.name);
330-
if (entry.isDirectory()) {
331-
await walk(full);
332-
} else if (entry.isFile() && entry.name === 'spec.md') {
333-
results.push(full);
334-
}
335-
}
336-
};
337-
await walk(specsDir);
338-
return results.sort();
339-
}
340-
341328
private convertZodErrors(error: ZodError): ValidationIssue[] {
342329
return error.issues.map(err => {
343330
let message = err.message;

test/core/archive.test.ts

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1062,6 +1062,37 @@ The system will log all events.
10621062
expect(archives.some(a => a.includes(changeName))).toBe(false);
10631063
});
10641064

1065+
it('sets exit code 1 when the only delta spec sits at the specs/ root (#1385)', async () => {
1066+
const changeName = 'exit-root-delta';
1067+
const changeDir = path.join(tempDir, 'openspec', 'changes', changeName);
1068+
const changeSpecsDir = path.join(changeDir, 'specs');
1069+
await fs.mkdir(changeSpecsDir, { recursive: true });
1070+
1071+
// No capability folder: the merge path skips this file, so archiving it
1072+
// used to succeed while dropping the requirement.
1073+
const specContent = `## ADDED Requirements
1074+
1075+
### Requirement: Request metrics
1076+
The system SHALL record request metrics.
1077+
1078+
#### Scenario: Request is counted
1079+
- **WHEN** a request completes
1080+
- **THEN** a counter is incremented`;
1081+
await fs.writeFile(path.join(changeSpecsDir, 'spec.md'), specContent);
1082+
await fs.writeFile(path.join(changeDir, 'tasks.md'), '- [x] Task 1\n');
1083+
1084+
await archiveCommand.execute(changeName, { yes: true });
1085+
1086+
expect(process.exitCode).toBe(1);
1087+
expect(console.log).toHaveBeenCalledWith(
1088+
expect.stringContaining('Validation failed')
1089+
);
1090+
1091+
const archiveDir = path.join(tempDir, 'openspec', 'changes', 'archive');
1092+
const archives = await fs.readdir(archiveDir);
1093+
expect(archives.some(a => a.includes(changeName))).toBe(false);
1094+
});
1095+
10651096
it('sets exit code 1 when spec rebuild fails (MODIFIED on new spec)', async () => {
10661097
const changeName = 'exit-rebuild-fail';
10671098
const changeDir = path.join(tempDir, 'openspec', 'changes', changeName);

test/core/validation.test.ts

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -506,6 +506,57 @@ The system SHALL handle all errors gracefully.
506506
expect(report.summary.errors).toBe(0);
507507
});
508508

509+
it('should fail when a delta spec.md sits directly under specs/', async () => {
510+
// #1385: the merge path only reads specs/<capability>/spec.md, so a
511+
// root-level file used to validate clean and then archive with its
512+
// requirements silently dropped.
513+
const changeDir = path.join(testDir, 'test-change-root-delta');
514+
const specsDir = path.join(changeDir, 'specs');
515+
await fs.mkdir(specsDir, { recursive: true });
516+
517+
const deltaSpec = `## ADDED Requirements
518+
519+
### Requirement: Request metrics
520+
The system SHALL record request metrics.
521+
522+
#### Scenario: Request is counted
523+
- **WHEN** a request completes
524+
- **THEN** a counter is incremented`;
525+
526+
await fs.writeFile(path.join(specsDir, 'spec.md'), deltaSpec);
527+
528+
const validator = new Validator(true);
529+
const report = await validator.validateChangeDeltaSpecs(changeDir);
530+
531+
expect(report.valid).toBe(false);
532+
expect(
533+
report.issues.some(i => i.message.includes('Delta spec found at specs/spec.md'))
534+
).toBe(true);
535+
});
536+
537+
it('should still validate a nested capability layout', async () => {
538+
const changeDir = path.join(testDir, 'test-change-nested-delta');
539+
const specsDir = path.join(changeDir, 'specs', 'platform', 'metrics');
540+
await fs.mkdir(specsDir, { recursive: true });
541+
542+
const deltaSpec = `## ADDED Requirements
543+
544+
### Requirement: Request metrics
545+
The system SHALL record request metrics.
546+
547+
#### Scenario: Request is counted
548+
- **WHEN** a request completes
549+
- **THEN** a counter is incremented`;
550+
551+
await fs.writeFile(path.join(specsDir, 'spec.md'), deltaSpec);
552+
553+
const validator = new Validator(true);
554+
const report = await validator.validateChangeDeltaSpecs(changeDir);
555+
556+
expect(report.valid).toBe(true);
557+
expect(report.summary.errors).toBe(0);
558+
});
559+
509560
it('should fail when requirement text lacks SHALL/MUST', async () => {
510561
const changeDir = path.join(testDir, 'test-change-3');
511562
const specsDir = path.join(changeDir, 'specs', 'test-spec');

0 commit comments

Comments
 (0)