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
5 changes: 5 additions & 0 deletions .changeset/reject-unread-delta-files.md
Original file line number Diff line number Diff line change
@@ -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/<capability-path>/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.
2 changes: 2 additions & 0 deletions docs-lab/reference/schemas/spec-driven/index.md
Original file line number Diff line number Diff line change
Expand Up @@ -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)):
Expand Down
19 changes: 15 additions & 4 deletions src/commands/workflow/instructions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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/<capability>.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>;
}): string[] {
}): Promise<string[]> {
const { state, schema, changeDir, changeName, skippedArtifacts } = input;
if (state === 'blocked') return [];

Expand All @@ -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
Expand All @@ -466,6 +476,7 @@ function collectApplyWarnings(input: {
// a placeholder rather than a guess.
const specTarget = specArtifacts.length === 1 ? specArtifacts[0].id : '<artifact-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.`,
Expand Down Expand Up @@ -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,
Expand Down
9 changes: 8 additions & 1 deletion src/core/archive.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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/<capability>.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
Expand Down
22 changes: 17 additions & 5 deletions src/core/validation/validator.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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/<capability>.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',
Expand Down Expand Up @@ -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) {
Expand Down
59 changes: 59 additions & 0 deletions src/utils/spec-discovery.ts
Original file line number Diff line number Diff line change
@@ -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"). */
Expand Down Expand Up @@ -75,6 +76,64 @@ export async function discoverSpecFiles(specsRoot: string): Promise<DiscoveredSp
return results.sort((a, b) => (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<UnreadDeltaFile[]> {
const results: UnreadDeltaFile[] = [];
const walk = async (dir: string, segments: string[]): Promise<void> => {
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
Expand Down
Loading
Loading