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
9 changes: 9 additions & 0 deletions .changeset/archive-phantom-proposal-warnings.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
---
"@fission-ai/openspec": patch
---

Fix phantom requirements parsed from delta specs, which made `openspec archive` warn about problems `openspec validate` never reported.

A header inside a delta section that is not a `### Requirement:` header — a divider such as `### Documentation Requirements` — was read as a requirement with no scenario. `openspec archive` warned that it was missing a scenario, and `openspec show <change> --json` and `openspec change list` counted it as an extra delta. The change parser now ignores those headers, matching the delta reader, so the phantom is gone from the warnings and from the JSON. Main spec parsing is unchanged.

`openspec archive` also no longer repeats requirement-level issues from the delta specs in its non-blocking "Proposal warnings in proposal.md" block. Each defect was printed twice there, and a `## REMOVED Requirements` entry — names-only by design — was reported as missing a scenario on every correct removal. Delta spec validation still reports and blocks on genuine defects, and proposal-level warnings are unchanged.
9 changes: 9 additions & 0 deletions openspec/specs/cli-archive/spec.md
Original file line number Diff line number Diff line change
Expand Up @@ -193,6 +193,15 @@ The archive command SHALL validate changes before applying them to ensure data i
- **AND** only proceed if validation passes
- **AND** show validation errors if it fails

#### Scenario: Proposal warnings stay proposal-level

- **WHEN** archiving a change
- **THEN** the non-blocking proposal warnings SHALL NOT repeat requirement-level
issues reached through the delta specs
- **AND** a requirement removed by a `## REMOVED Requirements` delta SHALL NOT be
reported as missing a scenario
- **AND** proposal-level issues SHALL still be reported

#### Scenario: Force archive without validation

- **WHEN** executing `openspec archive change-name --no-validate`
Expand Down
19 changes: 16 additions & 3 deletions src/core/archive.ts
Original file line number Diff line number Diff line change
Expand Up @@ -259,10 +259,23 @@ export class ArchiveCommand {
try {
await fs.access(changeFile);
const changeReport = await validator.validateChange(changeFile);
// Proposal validation is informative only (do not block archive)
if (!changeReport.valid) {
// Proposal validation is informative only (do not block archive).
// `validateChange` parses the change together with its delta specs,
// so it also raises requirement-level issues under
// `deltas.<n>.requirement(s)`. Those
// are not proposal problems, and reporting them here was noisy and
// sometimes wrong (#498): the change parser records every requirement
// under both `requirement` and `requirements`, so each defect was
// printed twice, and REMOVED requirements — names-only by design —
// produced a "missing scenario" warning for a correct removal.
// Genuine delta defects are still caught below, by the delta spec
// validation and by the rebuilt-spec check that runs before any write.
const proposalIssues = changeReport.issues.filter(
(issue) => !/^deltas\.\d+\.requirements?\./.test(issue.path)
);
if (!changeReport.valid && proposalIssues.length > 0) {
console.log(chalk.yellow(`\nProposal warnings in proposal.md (non-blocking):`));
for (const issue of changeReport.issues) {
for (const issue of proposalIssues) {
const symbol = issue.level === 'ERROR' ? '⚠' : (issue.level === 'WARNING' ? '⚠' : 'ℹ');
console.log(chalk.yellow(` ${symbol} ${issue.message}`));
}
Expand Down
23 changes: 23 additions & 0 deletions src/core/parsers/change-parser.ts
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,29 @@ export class ChangeParser extends MarkdownParser {
return deltas;
}

/**
* Read requirements from a delta section, ignoring headers that are not
* `### Requirement: <name>`.
*
* A delta section often carries divider headers such as
* `### Documentation Requirements`. The base parser treats every child header
* as a requirement, which invented a scenario-less requirement that does not
* exist (#498): archive warned about a missing scenario, and `show --json`
* reported an extra delta. The delta reader already skips these headers and
* notes them, so this keeps the two readers in agreement.
*
* Overriding here rather than in MarkdownParser keeps main spec parsing —
* `view`, `list`, `spec --json`, spec validation — untouched.
*/
protected parseRequirements(section: Section): Requirement[] {
return super.parseRequirements({
...section,
children: section.children.filter((child) =>
/^Requirement:\s*\S/i.test(child.title.trim())
),
});
}

private parseSpecDeltas(specName: string, content: string): Delta[] {
const deltas: Delta[] = [];
const sections = this.parseSectionsFromContent(content);
Expand Down
178 changes: 178 additions & 0 deletions test/core/archive.test.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest';
import { ArchiveCommand } from '../../src/core/archive.js';
import { Validator } from '../../src/core/validation/validator.js';
import { VALIDATION_MESSAGES } from '../../src/core/validation/constants.js';
import { formatLocalDate } from '../../src/utils/date.js';
import { promises as fs } from 'fs';
import path from 'path';
Expand Down Expand Up @@ -1548,4 +1549,181 @@ The system SHALL do the thing differently.
await expect(fs.access(changeDir)).resolves.not.toThrow();
});
});

describe('proposal warnings (#498)', () => {
const LONG_WHY =
'This change exists to document AI application patterns thoroughly for the team, which is long enough.';

async function createChange(
changeName: string,
why: string,
deltaSpec: string
): Promise<string> {
const changeDir = path.join(tempDir, 'openspec', 'changes', changeName);
await fs.mkdir(path.join(changeDir, 'specs', 'docs'), { recursive: true });
await fs.writeFile(
path.join(changeDir, 'proposal.md'),
`# Proposal\n\n## Why\n${why}\n\n## What Changes\n- Add docs.\n`
);
await fs.writeFile(path.join(changeDir, 'tasks.md'), '- [x] Task 1\n');
await fs.writeFile(path.join(changeDir, 'specs', 'docs', 'spec.md'), deltaSpec);
return changeDir;
}

function loggedLines(): string[] {
return (console.log as unknown as ReturnType<typeof vi.fn>).mock.calls.map(
(call) => String(call[0])
);
}

// A stray non-`### Requirement:` header inside a delta section used to be
// parsed as a requirement, so archive blamed a requirement that does not
// exist while `openspec validate` reported the change as valid (#498).
it('does not report phantom requirement warnings for a stray delta header', async () => {
const changeName = 'stray-header';
await createChange(
changeName,
LONG_WHY,
[
'# Docs Delta',
'',
'## ADDED Requirements',
'',
'### Documentation Requirements',
'',
'### Requirement: AI Application Documentation',
'Teams building AI applications SHALL document agent definitions.',
'',
'#### Scenario: Agent Definition Documentation',
'- **WHEN** a team ships an agent',
'- **THEN** the agent definition is documented',
'',
].join('\n')
);

await archiveCommand.execute(changeName, { yes: true });

const output = loggedLines().join('\n');
expect(output).not.toContain('Proposal warnings in proposal.md');
expect(output).not.toContain('Requirement must have at least one scenario');

// The change still archives, exactly as `validate` predicted.
const archives = await fs.readdir(path.join(tempDir, 'openspec', 'changes', 'archive'));
expect(archives).toEqual([expect.stringMatching(new RegExp(`\\d{4}-\\d{2}-\\d{2}-${changeName}`))]);
});

// REMOVED requirements are names-only by design, so delta spec validation
// exempts them. The proposal report did not, and warned about a missing
// scenario on every correct removal.
it('does not warn about missing scenarios for REMOVED requirements', async () => {
const changeName = 'removal';
const changeDir = path.join(tempDir, 'openspec', 'changes', changeName);
await fs.mkdir(path.join(changeDir, 'specs', 'docs'), { recursive: true });
await fs.writeFile(
path.join(changeDir, 'proposal.md'),
`# Proposal\n\n## Why\n${LONG_WHY}\n\n## What Changes\n- Remove docs.\n`
);
await fs.writeFile(path.join(changeDir, 'tasks.md'), '- [x] Task 1\n');
await fs.writeFile(
path.join(changeDir, 'specs', 'docs', 'spec.md'),
'# Docs Delta\n\n## REMOVED Requirements\n\n### Requirement: Old Thing\n'
);
// The removal needs a main spec to remove the requirement from.
const mainSpecDir = path.join(tempDir, 'openspec', 'specs', 'docs');
await fs.mkdir(mainSpecDir, { recursive: true });
await fs.writeFile(
path.join(mainSpecDir, 'spec.md'),
'# docs Specification\n\n## Purpose\nDocs.\n\n## Requirements\n### Requirement: Old Thing\nThe system SHALL do the old thing.\n\n#### Scenario: Old\n- **WHEN** invoked\n- **THEN** it happens\n'
);

await archiveCommand.execute(changeName, { yes: true });

const output = loggedLines().join('\n');
expect(output).not.toContain('Proposal warnings in proposal.md');
expect(output).not.toContain('Requirement must have at least one scenario');
});

it('still reports genuine proposal-level warnings', async () => {
const changeName = 'short-why';
await createChange(
changeName,
'Short.',
[
'# Docs Delta',
'',
'## ADDED Requirements',
'',
'### Requirement: Real Requirement',
'The system SHALL do a thing.',
'',
'#### Scenario: It works',
'- **WHEN** invoked',
'- **THEN** it works',
'',
].join('\n')
);

await archiveCommand.execute(changeName, { yes: true });

const output = loggedLines().join('\n');
expect(output).toContain('Proposal warnings in proposal.md');
expect(output).toContain('Why section must be at least 50 characters');
});

// The filter is anchored to the dot-joined Zod paths
// (`deltas.<n>.requirement(s).…`). Rules in applyChangeRules use bracket
// notation (`deltas[<n>].description`) and describe simple deltas parsed
// from `## What Changes`, which are proposal-level. They must survive.
it('keeps proposal-level warnings about simple deltas from What Changes', async () => {
const changeName = 'simple-deltas';
const changeDir = path.join(tempDir, 'openspec', 'changes', changeName);
await fs.mkdir(changeDir, { recursive: true });
await fs.writeFile(
path.join(changeDir, 'proposal.md'),
'# Proposal\n\n## Why\nShort.\n\n## What Changes\n- **docs:** add x\n'
);
await fs.writeFile(path.join(changeDir, 'tasks.md'), '- [x] Task 1\n');

await archiveCommand.execute(changeName, { yes: true });

const output = loggedLines().join('\n');
expect(output).toContain('Proposal warnings in proposal.md');
expect(output).toContain(VALIDATION_MESSAGES.DELTA_DESCRIPTION_TOO_BRIEF);
expect(output).toContain(`ADDED ${VALIDATION_MESSAGES.DELTA_MISSING_REQUIREMENTS}`);
});

// Real delta defects are still caught. A missing scenario used to be
// reported three times (twice as proposal warnings, once by the delta
// report) and is now reported once, by the delta report.
it('still blocks the archive on real delta requirement errors, reported once', async () => {
const changeName = 'bad-delta';
const changeDir = await createChange(
changeName,
LONG_WHY,
[
'# Docs Delta',
'',
'## ADDED Requirements',
'',
'### Requirement: Missing Scenario',
'The system SHALL do a thing.',
'',
].join('\n')
);

await archiveCommand.execute(changeName, { yes: true });

const lines = loggedLines();
const output = lines.join('\n');
expect(output).toContain('Validation errors in change delta specs');
expect(output).toContain('must include at least one scenario');
expect(output).not.toContain('Proposal warnings in proposal.md');
expect(
lines.filter((line) => line.includes('must include at least one scenario'))
).toHaveLength(1);

// The change was not archived.
await expect(fs.access(changeDir)).resolves.not.toThrow();
});
});
});
69 changes: 69 additions & 0 deletions test/core/parsers/change-parser.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -69,4 +69,73 @@ describe('ChangeParser', () => {
expect(change.deltas[0].operation).toBe('ADDED');
});
});

// A divider header inside a delta section used to be parsed as a requirement,
// inventing a scenario-less delta that does not exist (#498).
it('ignores delta headers that are not "### Requirement:" (#498)', async () => {
await withTempDir(async (dir) => {
const specDir = path.join(dir, 'specs', 'docs');
await fs.mkdir(specDir, { recursive: true });

const content = `# Test Change\n\n## Why\nWe need it because reasons that are sufficiently long.\n\n## What Changes\n- Add docs`;
const deltaSpec = [
'# Docs Delta',
'',
'## ADDED Requirements',
'',
'### Documentation Requirements',
'',
'### Requirement: AI Application Documentation',
'Teams building AI applications SHALL document agent definitions.',
'',
'#### Scenario: Agent Definition Documentation',
'- **WHEN** a team ships an agent',
'- **THEN** the agent definition is documented',
].join('\n');

await fs.writeFile(path.join(specDir, 'spec.md'), deltaSpec, 'utf8');

const parser = new ChangeParser(content, dir);
const change = await parser.parseChangeWithDeltas('test-change');

expect(change.deltas.length).toBe(1);
expect(change.deltas[0].requirement?.text).toBe(
'Teams building AI applications SHALL document agent definitions.'
);
expect(change.deltas[0].requirement?.scenarios.length).toBe(1);
});
});

// A nameless "### Requirement:" header carries no requirement to validate,
// and the delta reader skips it too.
it('ignores a nameless "### Requirement:" delta header (#498)', async () => {
await withTempDir(async (dir) => {
const specDir = path.join(dir, 'specs', 'docs');
await fs.mkdir(specDir, { recursive: true });

const content = `# Test Change\n\n## Why\nWe need it because reasons that are sufficiently long.\n\n## What Changes\n- Add docs`;
const deltaSpec = [
'# Docs Delta',
'',
'## ADDED Requirements',
'',
'### Requirement:',
'',
'### Requirement: Real One',
'The system SHALL do a thing.',
'',
'#### Scenario: It works',
'- **WHEN** invoked',
'- **THEN** it works',
].join('\n');

await fs.writeFile(path.join(specDir, 'spec.md'), deltaSpec, 'utf8');

const parser = new ChangeParser(content, dir);
const change = await parser.parseChangeWithDeltas('test-change');

expect(change.deltas.length).toBe(1);
expect(change.deltas[0].requirement?.text).toBe('The system SHALL do a thing.');
});
});
});
Loading