Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions .changeset/root-level-delta-spec.md
Original file line number Diff line number Diff line change
@@ -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/<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.
10 changes: 8 additions & 2 deletions src/core/archive.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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)) {
Expand Down
63 changes: 29 additions & 34 deletions src/core/validation/validator.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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/<area>/<capability>/spec.md) is validated, not just the
// one-level specs/<capability>/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/<capability>/spec.md and the nested multi-area
// specs/<area>/<capability>/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/<capability>/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 {
Expand Down Expand Up @@ -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/<capability>/spec.md) and nested multi-area
* (specs/<area>/<capability>/spec.md) layouts are discovered (#1182b).
* Returns absolute paths, sorted for deterministic issue ordering.
*/
private async findDeltaSpecFiles(specsDir: string): Promise<string[]> {
const results: string[] = [];
const walk = async (dir: string): Promise<void> => {
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;
Expand Down
63 changes: 63 additions & 0 deletions test/core/archive.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
78 changes: 78 additions & 0 deletions test/core/validation.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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/<capability>/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');
Expand Down
Loading