From 66c16a61f729c12a36465c7e412c7814eee272e2 Mon Sep 17 00:00:00 2001 From: Dwin Gharibi Date: Fri, 11 Sep 2026 20:27:30 +0330 Subject: [PATCH 1/2] fix(archive): refuse delta files the merge path never reads validate and archive read a change's deltas only from specs//spec.md, but the spec-driven artifact graph counts any markdown file under specs/ as written. A delta at specs/user-auth.md was reported done by status and ready by apply, rejected by validate only as having no deltas, and then archived with exit 0 and nothing merged. Name every markdown file under specs/ that carries delta sections but is not a capability's spec.md. validate reports it as an error with the spec.md its requirements belong in, archive runs that validation and refuses the change, and apply lists it in its warnings. --no-validate and changes with no spec files behave as before. --- .changeset/reject-unread-delta-files.md | 5 + docs/writing-specs.md | 2 +- src/commands/workflow/instructions.ts | 19 +- src/core/archive.ts | 9 +- src/core/validation/validator.ts | 22 +- src/utils/spec-discovery.ts | 59 ++++ test/core/misplaced-delta-files.test.ts | 427 ++++++++++++++++++++++++ 7 files changed, 532 insertions(+), 11 deletions(-) create mode 100644 .changeset/reject-unread-delta-files.md create mode 100644 test/core/misplaced-delta-files.test.ts diff --git a/.changeset/reject-unread-delta-files.md b/.changeset/reject-unread-delta-files.md new file mode 100644 index 0000000000..3051a95145 --- /dev/null +++ b/.changeset/reject-unread-delta-files.md @@ -0,0 +1,5 @@ +--- +'@fission-ai/openspec': patch +--- + +Stop archiving a change whose delta was written somewhere `archive` never reads. `validate` and `archive` read a change's deltas only from `specs//spec.md`, but the spec-driven artifact graph counts any markdown file under `specs/` as the specs being written, so a delta at `specs/user-auth.md`, or in a second file beside a capability's `spec.md`, was reported done by `status` and ready by `instructions apply` with no warning, rejected by `validate` only as "no deltas found", and then archived with exit 0 and nothing merged into `openspec/specs/`. A markdown file that carries delta sections but is not a capability's `spec.md` is now a validation error naming the file and the `spec.md` its requirements belong in; `archive` runs that validation and refuses the change instead of archiving it unmerged, and `instructions apply` lists each such file in its `warnings`. `--no-validate` still archives as before, a change with no spec files still archives, and notes without delta sections under `specs/` are not affected. diff --git a/docs/writing-specs.md b/docs/writing-specs.md index 1e9883a7c5..c919b4984b 100644 --- a/docs/writing-specs.md +++ b/docs/writing-specs.md @@ -58,7 +58,7 @@ A change describes its edits to the specs with three section types. Using the ri On archive, ADDED gets appended to the main spec, MODIFIED replaces the old version, and REMOVED is dropped from it. Remove the last requirement a capability has and you retire it: rather than leave a spec with nothing in it, archive deletes `openspec/specs//spec.md`. Because that is the one archive step that removes a file, it has to be asked for — add `retire_capabilities: true` to the change's `.openspec.yaml`, alongside the `schema:` that file already needs. Without it the archive aborts and tells you so. Retirement deletes the whole file, so it is also refused while the spec holds anything outside its title, `## Purpose`, and its requirement blocks — a `## Notes` section, a comment under a requirement. The abort names those lines; move them into `## Purpose` or a requirement, or delete the spec by hand. For a spec in the caller's checkout, the archive output also names the `git checkout` that restores a committed file; selected stores receive checkout-scoped recovery guidance instead. If you mark a real change as ADDED, you end up with two competing requirements; if you describe new behavior as MODIFIED, there's nothing to replace. When in doubt, open the current spec and see whether the requirement is already there. -One more section is worth knowing about. When your delta creates a capability that doesn't exist yet, open it with `## Purpose` — a sentence or two on what the capability is for. Archive uses it as the Purpose of the main spec it creates; skip it and you get a `TBD` placeholder to fill in by hand. An existing spec already has a Purpose, so a delta's is ignored there — edit `openspec/specs//spec.md` directly to change one. Here, `` is the directory relative to `specs/`, such as `user-auth` in a flat project or `identity/user-auth` in a project organized by domain. +One more section is worth knowing about. When your delta creates a capability that doesn't exist yet, open it with `## Purpose` — a sentence or two on what the capability is for. Archive uses it as the Purpose of the main spec it creates; skip it and you get a `TBD` placeholder to fill in by hand. An existing spec already has a Purpose, so a delta's is ignored there — edit `openspec/specs//spec.md` directly to change one. Here, `` is the directory relative to `specs/`, such as `user-auth` in a flat project or `identity/user-auth` in a project organized by domain. Inside a change, each capability's delta is the `spec.md` in that directory; `openspec validate` and `openspec archive` reject delta sections written in any other file under `specs/`, such as `specs/user-auth.md`, because archive would never merge them. ## Right-size the change diff --git a/src/commands/workflow/instructions.ts b/src/commands/workflow/instructions.ts index cc2b143bb2..aff8968c1f 100644 --- a/src/commands/workflow/instructions.ts +++ b/src/commands/workflow/instructions.ts @@ -17,6 +17,7 @@ import { type ArtifactInstructions, } from '../../core/artifact-graph/index.js'; import { isSpecsArtifactPath } from '../../core/artifact-graph/outputs.js'; +import { findUnreadDeltaFiles } from '../../utils/spec-discovery.js'; import { getChangeDir, resolveCurrentPlanningHomeSync, @@ -436,14 +437,18 @@ function collectMissingPrerequisites(input: { * reached tasks yet, the missing specs are the next step rather than a warning. * Schemas that declare no spec-producing artifact carry `skip_specs` from * creation, so this never fires on them. + * + * A delta file the merge path never reads (specs/.md, a note + * beside spec.md) still satisfies the specs glob, so it reads as written here + * while validate rejects it and archive would drop it. Each one is named. */ -function collectApplyWarnings(input: { +async function collectApplyWarnings(input: { state: ApplyInstructions['state']; schema: { artifacts: { id: string; generates: string }[] }; changeDir: string; changeName: string; skippedArtifacts?: Set; -}): string[] { +}): Promise { const { state, schema, changeDir, changeName, skippedArtifacts } = input; if (state === 'blocked') return []; @@ -452,10 +457,15 @@ function collectApplyWarnings(input: { ); if (specArtifacts.length === 0) return []; if (specArtifacts.some((artifact) => skippedArtifacts?.has(artifact.id))) return []; + const warnings = (await findUnreadDeltaFiles(path.join(changeDir, 'specs'))).map( + (file) => + `specs/${file.path} is not a capability's spec.md, so \`openspec validate ${changeName}\` rejects it and archive never merges it. ` + + `Move its requirements into specs/${file.expected}.` + ); const hasDeltas = specArtifacts.some( (artifact) => resolveArtifactOutputs(changeDir, artifact.generates).length > 0 ); - if (hasDeltas) return []; + if (hasDeltas) return warnings; const metadataPath = path.join(changeDir, METADATA_FILENAME); // The command names the artifact this schema actually declares, never the @@ -466,6 +476,7 @@ function collectApplyWarnings(input: { // a placeholder rather than a guess. const specTarget = specArtifacts.length === 1 ? specArtifacts[0].id : ''; return [ + ...warnings, `This change has no delta specs and does not declare \`skip_specs: true\`, so \`openspec validate ${changeName}\` fails on it. ` + `Write the delta specs before implementing (\`openspec instructions ${specTarget} --change ${changeName}\`), ` + `or add \`skip_specs: true\` to ${metadataPath} if this change really changes no specified behavior.`, @@ -608,7 +619,7 @@ export async function generateApplyInstructions( instruction = schemaInstruction?.trim() ?? 'Read context files, work through pending tasks, mark complete as you go.\nPause if you hit blockers or need clarification.'; } - const warnings = collectApplyWarnings({ + const warnings = await collectApplyWarnings({ state, schema, changeDir, diff --git a/src/core/archive.ts b/src/core/archive.ts index 888a6135a6..61c0921ec6 100644 --- a/src/core/archive.ts +++ b/src/core/archive.ts @@ -23,7 +23,7 @@ import { finalizeRetiredSpec, type SpecUpdate, } from './specs-apply.js'; -import { discoverSpecFiles, hasAnyFileUnder } from '../utils/spec-discovery.js'; +import { discoverSpecFiles, findUnreadDeltaFiles, hasAnyFileUnder } from '../utils/spec-discovery.js'; import { METADATA_FILENAME, readRetireCapabilitiesMarker, readSkipSpecsMarker } from '../utils/change-metadata.js'; import { confirmPrompt, isNonInteractivePromptError } from '../utils/interactive.js'; import { FileSystemUtils } from '../utils/file-system.js'; @@ -1225,6 +1225,13 @@ export class ArchiveCommand { // folder, so only a regular file counts. const rootSpecStat = await fs.stat(path.join(changeSpecsDir, 'spec.md')).catch(() => null); let hasDeltaSpecs = rootSpecStat?.isFile() === true; + // Likewise for delta sections in any other file the merge path does not + // read (specs/.md, a note beside spec.md): without this the + // zero-delta leniency below archives the change as done with nothing + // merged, although validate rejects it. + if (!hasDeltaSpecs) { + hasDeltaSpecs = (await findUnreadDeltaFiles(changeSpecsDir)).length > 0; + } // A change that declares skip_specs must not carry any file under // specs/ — validate reports that as a conflict, so archive has to run // the same check instead of skipping validation because the files diff --git a/src/core/validation/validator.ts b/src/core/validation/validator.ts index a36e4e75c8..f4e63f8933 100644 --- a/src/core/validation/validator.ts +++ b/src/core/validation/validator.ts @@ -26,7 +26,7 @@ import { } from '../parsers/requirement-text.js'; import { findMainSpecStructureIssues } from '../parsers/spec-structure.js'; import { FileSystemUtils } from '../../utils/file-system.js'; -import { discoverSpecFiles, hasAnyFileUnder } from '../../utils/spec-discovery.js'; +import { discoverSpecFiles, findUnreadDeltaFiles, hasAnyFileUnder } from '../../utils/spec-discovery.js'; import { METADATA_FILENAME, readSkipSpecsMarker, @@ -426,6 +426,18 @@ export class Validator { } } + // The same drop happens to delta sections in any other file the merge + // path does not read (specs/.md, a note beside spec.md), + // while the artifact graph's specs/**/*.md glob counts it as written. + const unreadDeltaFiles = await findUnreadDeltaFiles(specsDir); + for (const file of unreadDeltaFiles) { + issues.push({ + level: 'ERROR', + path: file.path, + message: `Delta spec found at specs/${file.path}. Delta specs must be a spec.md inside a capability folder — this file is ignored when the change is applied or archived. Move its requirements into specs/${file.expected}.`, + }); + } + for (const { path: specPath, sections } of emptySectionSpecs) { issues.push({ level: 'ERROR', @@ -467,10 +479,10 @@ export class Validator { issues.push({ level: 'ERROR', path: 'file', message: VALIDATION_MESSAGES.CHANGE_SKIP_SPECS_CONFLICT }); } - // 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) { + // The root-level and unread-file errors already name the file and the fix; + // adding "No deltas found" on top would contradict them, since the deltas + // are sitting in the files just reported. + if (totalDeltas === 0 && !hasRootLevelSpec && unreadDeltaFiles.length === 0) { if (skipSpecs && !specsDirHasFiles) { issues.push({ level: 'INFO', path: 'file', message: VALIDATION_MESSAGES.CHANGE_SKIP_SPECS_ACCEPTED }); } else if (!skipSpecs) { diff --git a/src/utils/spec-discovery.ts b/src/utils/spec-discovery.ts index ab6b8a5eb3..d1d2d35d2a 100644 --- a/src/utils/spec-discovery.ts +++ b/src/utils/spec-discovery.ts @@ -1,6 +1,7 @@ import { promises as fs } from 'fs'; import path from 'path'; import { FileSystemUtils } from './file-system.js'; +import { parseDeltaSpec } from '../core/parsers/requirement-blocks.js'; export interface DiscoveredSpec { /** Spec id relative to the specs root, forward-slash separated on every platform (e.g. "web" or "platform/session-layout"). */ @@ -75,6 +76,64 @@ export async function discoverSpecFiles(specsRoot: string): Promise (a.id < b.id ? -1 : a.id > b.id ? 1 : 0)); } +export interface UnreadDeltaFile { + /** File path relative to the specs root, forward-slash separated. */ + path: string; + /** The spec.md the merge path reads for it, relative to the specs root. */ + expected: string; +} + +/** + * Markdown files under a change's specs/ that carry delta sections but are not + * a capability's `spec.md`, so discoverSpecFiles, and with it validate and + * archive, never reads them: `specs/user-auth.md`, or `specs/user-auth/delta.md` + * beside or instead of the capability's spec.md. The artifact graph's + * recursive specs/ markdown glob does match them, so status and apply report + * the specs as written while archive has nothing to merge. A `spec.md` at the + * specs/ root has its own check (#1385) and is not repeated here. Notes with + * no delta section are not deltas and are not reported. The walk matches + * discoverSpecFiles: dot entries are skipped, symlinked directories are not + * followed, and a dangling link is skipped. A missing root yields an empty + * list; any other read failure is thrown. Results are sorted by path. + */ +export async function findUnreadDeltaFiles(specsRoot: string): Promise { + const results: UnreadDeltaFile[] = []; + const walk = async (dir: string, segments: string[]): Promise => { + let entries; + try { + entries = await fs.readdir(dir, { withFileTypes: true }); + } catch (err: any) { + if (err?.code === 'ENOENT' || err?.code === 'ENOTDIR') return; + throw err; + } + for (const entry of entries) { + if (entry.name.startsWith('.')) continue; + if (entry.isDirectory()) { + await walk(path.join(dir, entry.name), [...segments, entry.name]); + continue; + } + if (!entry.isFile() && !entry.isSymbolicLink()) continue; + if (entry.name === 'spec.md' || !entry.name.toLowerCase().endsWith('.md')) continue; + const filePath = path.join(dir, entry.name); + let content: string; + try { + if (entry.isSymbolicLink() && !(await fs.stat(filePath)).isFile()) continue; + content = await fs.readFile(filePath, 'utf-8'); + } catch (err: any) { + // A dangling link is not content; anything else fails loudly. + if (err?.code === 'ENOENT') continue; + throw err; + } + if (!Object.values(parseDeltaSpec(content).sectionPresence).some(Boolean)) continue; + const capability = + segments.length > 0 ? segments.join('/') : entry.name.slice(0, -'.md'.length); + results.push({ path: [...segments, entry.name].join('/'), expected: `${capability}/spec.md` }); + } + }; + await walk(specsRoot, []); + return results.sort((a, b) => (a.path < b.path ? -1 : a.path > b.path ? 1 : 0)); +} + /** * True when any regular non-dot file exists anywhere under the given * directory. Used by validate/archive to detect content under a change's diff --git a/test/core/misplaced-delta-files.test.ts b/test/core/misplaced-delta-files.test.ts new file mode 100644 index 0000000000..39eb200f37 --- /dev/null +++ b/test/core/misplaced-delta-files.test.ts @@ -0,0 +1,427 @@ +import { describe, it, expect, beforeEach, afterEach, afterAll, vi } from 'vitest'; +import { promises as fs } from 'fs'; +import path from 'path'; +import os from 'os'; +import { findUnreadDeltaFiles } from '../../src/utils/spec-discovery.js'; +import { Validator } from '../../src/core/validation/validator.js'; +import { ArchiveCommand } from '../../src/core/archive.js'; +import { generateApplyInstructions } from '../../src/commands/workflow/instructions.js'; +import { runCLI } from '../helpers/run-cli.js'; + +vi.mock('@inquirer/prompts', () => ({ + select: vi.fn(), + confirm: vi.fn(), +})); + +vi.mock('../../src/utils/interactive.js', async (importOriginal) => { + const actual = await importOriginal(); + return { ...actual, confirmPrompt: vi.fn() }; +}); + +/** + * validate and archive read a change's deltas only from + * specs//spec.md, but the spec-driven artifact graph counts + * any specs/**.md as the specs being written. A delta written anywhere else, + * such as specs/user-auth.md, was reported done by status and ready by apply, + * rejected by validate as "no deltas", and then archived with exit 0 and + * nothing merged into openspec/specs/. + */ +const DELTA = [ + '## ADDED Requirements', + '', + '### Requirement: Password Login', + 'The system SHALL let a user sign in with a password.', + '', + '#### Scenario: Valid password', + '- **WHEN** a user submits a valid password', + '- **THEN** a session is created', + '', +].join('\n'); + +async function write(root: string, segments: string[], content: string): Promise { + const file = path.join(root, ...segments); + await fs.mkdir(path.dirname(file), { recursive: true }); + await fs.writeFile(file, content); +} + +const exists = (p: string) => fs.access(p).then(() => true, () => false); + +describe('findUnreadDeltaFiles', () => { + let tempDir: string; + let specsDir: string; + + beforeEach(async () => { + tempDir = await fs.mkdtemp(path.join(os.tmpdir(), 'openspec-unread-deltas-')); + specsDir = path.join(tempDir, 'specs'); + }); + + afterEach(async () => { + await fs.rm(tempDir, { recursive: true, force: true }); + }); + + it('reports a delta written as specs/.md', async () => { + await write(specsDir, ['user-auth.md'], DELTA); + expect(await findUnreadDeltaFiles(specsDir)).toEqual([ + { path: 'user-auth.md', expected: 'user-auth/spec.md' }, + ]); + }); + + it('reports a delta in a capability folder under another name', async () => { + await write(specsDir, ['user-auth', 'delta.md'], DELTA); + expect(await findUnreadDeltaFiles(specsDir)).toEqual([ + { path: 'user-auth/delta.md', expected: 'user-auth/spec.md' }, + ]); + }); + + it('reports a stray delta beside a capability spec.md', async () => { + await write(specsDir, ['user-auth', 'spec.md'], DELTA); + await write(specsDir, ['user-auth', 'more.md'], DELTA); + expect(await findUnreadDeltaFiles(specsDir)).toEqual([ + { path: 'user-auth/more.md', expected: 'user-auth/spec.md' }, + ]); + }); + + it('reports one inside a nested area folder', async () => { + await write(specsDir, ['platform', 'session', 'changes.md'], DELTA); + expect(await findUnreadDeltaFiles(specsDir)).toEqual([ + { path: 'platform/session/changes.md', expected: 'platform/session/spec.md' }, + ]); + }); + + it('reports a spec file whose name differs only in case', async () => { + await write(specsDir, ['user-auth', 'SPEC.md'], DELTA); + expect(await findUnreadDeltaFiles(specsDir)).toEqual([ + { path: 'user-auth/SPEC.md', expected: 'user-auth/spec.md' }, + ]); + }); + + it('ignores the flat and nested layouts the merge path reads', async () => { + await write(specsDir, ['user-auth', 'spec.md'], DELTA); + await write(specsDir, ['platform', 'session', 'spec.md'], DELTA); + expect(await findUnreadDeltaFiles(specsDir)).toEqual([]); + }); + + it('leaves a specs/-root spec.md to its own check', async () => { + await write(specsDir, ['spec.md'], DELTA); + expect(await findUnreadDeltaFiles(specsDir)).toEqual([]); + }); + + it('ignores notes with no delta section', async () => { + await write(specsDir, ['README.md'], '# Notes\n\nWhy these specs are organized this way.\n'); + await write(specsDir, ['user-auth', 'spec.md'], DELTA); + await write(specsDir, ['user-auth', 'notes.md'], '# Notes\n\nOpen questions for review.\n'); + expect(await findUnreadDeltaFiles(specsDir)).toEqual([]); + }); + + it('ignores delta headers that only appear inside a code fence', async () => { + await write( + specsDir, + ['guide.md'], + '# How to write a delta\n\n```markdown\n## ADDED Requirements\n### Requirement: Example\n```\n' + ); + expect(await findUnreadDeltaFiles(specsDir)).toEqual([]); + }); + + it('skips dot entries and non-markdown files', async () => { + await write(specsDir, ['.drafts', 'user-auth.md'], DELTA); + await write(specsDir, ['.hidden.md'], DELTA); + await write(specsDir, ['user-auth.txt'], DELTA); + expect(await findUnreadDeltaFiles(specsDir)).toEqual([]); + }); + + it('returns results sorted by path', async () => { + await write(specsDir, ['zeta.md'], DELTA); + await write(specsDir, ['alpha', 'delta.md'], DELTA); + expect((await findUnreadDeltaFiles(specsDir)).map((file) => file.path)).toEqual([ + 'alpha/delta.md', + 'zeta.md', + ]); + }); + + it('returns nothing for a change with no specs folder', async () => { + expect(await findUnreadDeltaFiles(specsDir)).toEqual([]); + }); + + it.skipIf(process.platform === 'win32')('skips a dangling symlink', async () => { + await fs.mkdir(specsDir, { recursive: true }); + await fs.symlink(path.join(tempDir, 'missing.md'), path.join(specsDir, 'ghost.md')); + expect(await findUnreadDeltaFiles(specsDir)).toEqual([]); + }); +}); + +describe('validate with an unread delta file', () => { + let tempDir: string; + let changeDir: string; + + beforeEach(async () => { + tempDir = await fs.mkdtemp(path.join(os.tmpdir(), 'openspec-unread-validate-')); + changeDir = path.join(tempDir, 'change'); + }); + + afterEach(async () => { + await fs.rm(tempDir, { recursive: true, force: true }); + }); + + it('rejects specs/.md, naming the file and where it belongs', async () => { + await write(changeDir, ['specs', 'user-auth.md'], DELTA); + + const report = await new Validator(true).validateChangeDeltaSpecs(changeDir); + + expect(report.valid).toBe(false); + const issue = report.issues.find((i) => i.path === 'user-auth.md'); + expect(issue?.level).toBe('ERROR'); + expect(issue?.message).toContain('specs/user-auth.md'); + expect(issue?.message).toContain('specs/user-auth/spec.md'); + // The precise error replaces the generic one, which would say "No deltas + // found" about deltas sitting in the file it just named. + expect(report.issues.some((i) => i.message.includes('No deltas found'))).toBe(false); + }); + + it('rejects a stray delta file even when the capability spec.md is valid', async () => { + await write(changeDir, ['specs', 'user-auth', 'spec.md'], DELTA); + await write(changeDir, ['specs', 'user-auth', 'more.md'], DELTA); + + const report = await new Validator(true).validateChangeDeltaSpecs(changeDir); + + expect(report.valid).toBe(false); + expect(report.issues.find((i) => i.path === 'user-auth/more.md')?.level).toBe('ERROR'); + }); + + it('control: accepts the nested layout specs///spec.md', async () => { + await write(changeDir, ['specs', 'platform', 'session', 'spec.md'], DELTA); + + const report = await new Validator(true).validateChangeDeltaSpecs(changeDir); + + expect(report.valid).toBe(true); + }); + + it('control: accepts notes beside a valid delta', async () => { + await write(changeDir, ['specs', 'user-auth', 'spec.md'], DELTA); + await write(changeDir, ['specs', 'README.md'], '# Notes\n\nPlain notes.\n'); + + const report = await new Validator(true).validateChangeDeltaSpecs(changeDir); + + expect(report.valid).toBe(true); + }); +}); + +describe('archive with an unread delta file', () => { + let tempDir: string; + let archiveCommand: ArchiveCommand; + const originalCwd = process.cwd(); + const originalConsoleLog = console.log; + const originalExitCode = process.exitCode; + const originalXdgDataHome = process.env.XDG_DATA_HOME; + + beforeEach(async () => { + tempDir = await fs.mkdtemp(path.join(os.tmpdir(), 'openspec-unread-archive-')); + process.chdir(tempDir); + process.env.XDG_DATA_HOME = path.join(tempDir, 'xdg-data'); + await fs.mkdir(path.join(tempDir, 'openspec', 'specs'), { recursive: true }); + await fs.mkdir(path.join(tempDir, 'openspec', 'changes', 'archive'), { recursive: true }); + console.log = vi.fn(); + process.exitCode = undefined; + archiveCommand = new ArchiveCommand(); + }); + + afterEach(async () => { + console.log = originalConsoleLog; + process.exitCode = originalExitCode; + if (originalXdgDataHome === undefined) { + delete process.env.XDG_DATA_HOME; + } else { + process.env.XDG_DATA_HOME = originalXdgDataHome; + } + process.chdir(originalCwd); + vi.clearAllMocks(); + await fs.rm(tempDir, { recursive: true, force: true }); + }); + + async function change(name: string, specs: Array<[string[], string]>): Promise { + const changeDir = path.join(tempDir, 'openspec', 'changes', name); + await write(changeDir, ['tasks.md'], '- [x] Task 1\n'); + for (const [segments, content] of specs) { + await write(changeDir, ['specs', ...segments], content); + } + return changeDir; + } + + const archived = async (name: string) => + (await fs.readdir(path.join(tempDir, 'openspec', 'changes', 'archive'))).some((entry) => + entry.endsWith(name) + ); + const mainSpec = () => path.join(tempDir, 'openspec', 'specs', 'user-auth', 'spec.md'); + + it('refuses to archive when the only delta is specs/.md', async () => { + const changeDir = await change('flat-delta', [[['user-auth.md'], DELTA]]); + + await archiveCommand.execute('flat-delta', { yes: true }); + + expect(process.exitCode).toBe(1); + expect(console.log).toHaveBeenCalledWith(expect.stringContaining('Validation failed')); + expect(console.log).toHaveBeenCalledWith(expect.stringContaining('specs/user-auth/spec.md')); + expect(await archived('flat-delta')).toBe(false); + expect(await exists(changeDir)).toBe(true); + expect(await exists(mainSpec())).toBe(false); + }); + + it('refuses to archive when a stray delta sits beside a valid spec.md', async () => { + await change('stray-delta', [ + [['user-auth', 'spec.md'], DELTA], + [['user-auth', 'more.md'], DELTA.replace('Password Login', 'Passkey Login')], + ]); + + await archiveCommand.execute('stray-delta', { yes: true }); + + expect(process.exitCode).toBe(1); + expect(await archived('stray-delta')).toBe(false); + expect(await exists(mainSpec())).toBe(false); + }); + + it('still archives with --no-validate, the documented escape hatch', async () => { + await change('flat-no-validate', [[['user-auth.md'], DELTA]]); + + await archiveCommand.execute('flat-no-validate', { yes: true, noValidate: true }); + + expect(process.exitCode).toBeUndefined(); + expect(await archived('flat-no-validate')).toBe(true); + }); + + it('control: a change with no spec files still archives', async () => { + await change('tooling-only', []); + + await archiveCommand.execute('tooling-only', { yes: true }); + + expect(process.exitCode).toBeUndefined(); + expect(await archived('tooling-only')).toBe(true); + }); + + it('control: specs//spec.md archives and merges', async () => { + await change('nested-delta', [[['user-auth', 'spec.md'], DELTA]]); + + await archiveCommand.execute('nested-delta', { yes: true }); + + expect(process.exitCode).toBeUndefined(); + expect(await archived('nested-delta')).toBe(true); + expect(await fs.readFile(mainSpec(), 'utf-8')).toContain('Password Login'); + }); +}); + +describe('instructions apply with an unread delta file', () => { + let tempDir: string; + let changeDir: string; + + beforeEach(async () => { + tempDir = await fs.mkdtemp(path.join(os.tmpdir(), 'openspec-unread-apply-')); + changeDir = path.join(tempDir, 'openspec', 'changes', 'my-change'); + await write(changeDir, ['.openspec.yaml'], 'schema: spec-driven\n'); + await write(changeDir, ['proposal.md'], '## Why\nx\n'); + await write(changeDir, ['tasks.md'], '## 1. Implementation\n- [x] 1.1 Write the code\n'); + }); + + afterEach(async () => { + await fs.rm(tempDir, { recursive: true, force: true }); + }); + + it('warns about specs/.md, exactly when the validator rejects it', async () => { + await write(changeDir, ['specs', 'user-auth.md'], DELTA); + + const instructions = await generateApplyInstructions(tempDir, 'my-change'); + const report = await new Validator().validateChangeDeltaSpecs(changeDir); + + expect(instructions.state).toBe('all_done'); + expect(instructions.warnings).toHaveLength(1); + expect(instructions.warnings?.[0]).toContain('specs/user-auth.md'); + expect(instructions.warnings?.[0]).toContain('specs/user-auth/spec.md'); + expect(instructions.warnings?.[0]).toContain('openspec validate my-change'); + expect(report.valid).toBe(false); + }); + + it('names a stray delta beside a valid spec.md', async () => { + await write(changeDir, ['specs', 'user-auth', 'spec.md'], DELTA); + await write(changeDir, ['specs', 'user-auth', 'more.md'], DELTA); + + const instructions = await generateApplyInstructions(tempDir, 'my-change'); + + expect(instructions.warnings).toHaveLength(1); + expect(instructions.warnings?.[0]).toContain('specs/user-auth/more.md'); + }); + + it('control: stays quiet for specs//spec.md', async () => { + await write(changeDir, ['specs', 'user-auth', 'spec.md'], DELTA); + + const instructions = await generateApplyInstructions(tempDir, 'my-change'); + const report = await new Validator().validateChangeDeltaSpecs(changeDir); + + expect(instructions.warnings).toBeUndefined(); + expect(report.valid).toBe(true); + }); +}); + +describe('end to end: a delta written as specs/.md', () => { + const temps: string[] = []; + afterAll(async () => { + await Promise.all(temps.map((dir) => fs.rm(dir, { recursive: true, force: true }))); + }); + const T = 120_000; + + /** A fully planned change created through the CLI, its delta at the given path. */ + async function loginChange(deltaPath: string[]) { + const base = await fs.mkdtemp(path.join(os.tmpdir(), 'openspec-unread-e2e-')); + temps.push(base); + const home = path.join(base, 'home'); + const project = path.join(base, 'project'); + await fs.mkdir(home, { recursive: true }); + await fs.mkdir(project, { recursive: true }); + const env = { + HOME: home, + XDG_CONFIG_HOME: path.join(home, '.config'), + XDG_DATA_HOME: path.join(home, '.local', 'share'), + OPENSPEC_NO_ANIMATION: '1', + }; + const cli = (args: string[]) => runCLI(args, { cwd: project, env, timeoutMs: 60_000 }); + expect((await cli(['init', '--tools', 'claude'])).exitCode).toBe(0); + expect((await cli(['new', 'change', 'add-login'])).exitCode).toBe(0); + const dir = path.join(project, 'openspec', 'changes', 'add-login'); + await write( + dir, + ['proposal.md'], + '# Add login\n\n## Why\nUsers need to sign in so that their data is private to them and auditable.\n\n## What Changes\n- **user-auth**: adds login\n' + ); + await write(dir, ['design.md'], '# Design\n\nSession cookies.\n'); + await write(dir, ['tasks.md'], '## 1. Work\n- [x] 1.1 Implement login\n'); + await write(dir, ['specs', ...deltaPath], DELTA); + const mainSpec = path.join(project, 'openspec', 'specs', 'user-auth', 'spec.md'); + return { cli, dir, mainSpec }; + } + + it('apply warns, validate rejects, and archive refuses instead of archiving it unmerged', async () => { + const c = await loginChange(['user-auth.md']); + + const apply = JSON.parse( + (await c.cli(['instructions', 'apply', '--change', 'add-login', '--json'])).stdout + ); + expect(apply.warnings?.join('\n')).toContain('specs/user-auth/spec.md'); + + const validated = await c.cli(['validate', 'add-login']); + expect(validated.exitCode).not.toBe(0); + expect(validated.stdout + validated.stderr).toContain('specs/user-auth.md'); + + const archived = await c.cli(['archive', 'add-login', '--yes']); + expect(archived.exitCode).not.toBe(0); + expect(await exists(c.dir)).toBe(true); + expect(await exists(c.mainSpec)).toBe(false); + }, T); + + it('control: specs//spec.md validates, archives, and merges', async () => { + const c = await loginChange(['user-auth', 'spec.md']); + + const apply = JSON.parse( + (await c.cli(['instructions', 'apply', '--change', 'add-login', '--json'])).stdout + ); + expect(apply.warnings).toBeUndefined(); + expect((await c.cli(['validate', 'add-login'])).exitCode).toBe(0); + expect((await c.cli(['archive', 'add-login', '--yes'])).exitCode).toBe(0); + expect(await fs.readFile(c.mainSpec, 'utf-8')).toContain('Password Login'); + }, T); +}); From b699b7ece4110e957d7f1e387d32b2781490cb09 Mon Sep 17 00:00:00 2001 From: Clay Good Date: Tue, 15 Sep 2026 07:58:58 -0500 Subject: [PATCH 2/2] docs(schemas): move delta file placement note to docs-lab The legacy docs/ tree is frozen; the canonical page for the spec-driven delta layout is docs-lab/reference/schemas/spec-driven/index.md. Co-Authored-By: Claude Opus 5 --- docs-lab/reference/schemas/spec-driven/index.md | 2 ++ docs/writing-specs.md | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/docs-lab/reference/schemas/spec-driven/index.md b/docs-lab/reference/schemas/spec-driven/index.md index be7f1c4c61..e16dec2dc8 100644 --- a/docs-lab/reference/schemas/spec-driven/index.md +++ b/docs-lab/reference/schemas/spec-driven/index.md @@ -122,6 +122,8 @@ This is the foundation - specs, design, and tasks all build on this. Defines what behavior changes, with one delta spec per capability the proposal lists. +Each delta spec is the `spec.md` inside its capability folder. `openspec validate` and `openspec archive` reject delta sections written in any other file under `specs/`, such as `specs/user-auth.md`, because archive never merges them. + ### Structure The template the agent receives as the output format ([templates/spec.md](https://github.com/Fission-AI/OpenSpec/blob/main/schemas/spec-driven/templates/spec.md)): diff --git a/docs/writing-specs.md b/docs/writing-specs.md index c919b4984b..1e9883a7c5 100644 --- a/docs/writing-specs.md +++ b/docs/writing-specs.md @@ -58,7 +58,7 @@ A change describes its edits to the specs with three section types. Using the ri On archive, ADDED gets appended to the main spec, MODIFIED replaces the old version, and REMOVED is dropped from it. Remove the last requirement a capability has and you retire it: rather than leave a spec with nothing in it, archive deletes `openspec/specs//spec.md`. Because that is the one archive step that removes a file, it has to be asked for — add `retire_capabilities: true` to the change's `.openspec.yaml`, alongside the `schema:` that file already needs. Without it the archive aborts and tells you so. Retirement deletes the whole file, so it is also refused while the spec holds anything outside its title, `## Purpose`, and its requirement blocks — a `## Notes` section, a comment under a requirement. The abort names those lines; move them into `## Purpose` or a requirement, or delete the spec by hand. For a spec in the caller's checkout, the archive output also names the `git checkout` that restores a committed file; selected stores receive checkout-scoped recovery guidance instead. If you mark a real change as ADDED, you end up with two competing requirements; if you describe new behavior as MODIFIED, there's nothing to replace. When in doubt, open the current spec and see whether the requirement is already there. -One more section is worth knowing about. When your delta creates a capability that doesn't exist yet, open it with `## Purpose` — a sentence or two on what the capability is for. Archive uses it as the Purpose of the main spec it creates; skip it and you get a `TBD` placeholder to fill in by hand. An existing spec already has a Purpose, so a delta's is ignored there — edit `openspec/specs//spec.md` directly to change one. Here, `` is the directory relative to `specs/`, such as `user-auth` in a flat project or `identity/user-auth` in a project organized by domain. Inside a change, each capability's delta is the `spec.md` in that directory; `openspec validate` and `openspec archive` reject delta sections written in any other file under `specs/`, such as `specs/user-auth.md`, because archive would never merge them. +One more section is worth knowing about. When your delta creates a capability that doesn't exist yet, open it with `## Purpose` — a sentence or two on what the capability is for. Archive uses it as the Purpose of the main spec it creates; skip it and you get a `TBD` placeholder to fill in by hand. An existing spec already has a Purpose, so a delta's is ignored there — edit `openspec/specs//spec.md` directly to change one. Here, `` is the directory relative to `specs/`, such as `user-auth` in a flat project or `identity/user-auth` in a project organized by domain. ## Right-size the change