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/validate-scenario-loss.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@fission-ai/openspec": patch
---

`openspec validate <change>` now reports a MODIFIED requirement that omits a scenario the main spec still has — the same loss archive already refuses to apply — so the change fails at authoring time instead of at archive time. A change carrying a stale MODIFIED block will start failing validation; it was already unarchivable, and the message names the scenarios to copy back in.
2 changes: 1 addition & 1 deletion docs/cli.md
Original file line number Diff line number Diff line change
Expand Up @@ -526,7 +526,7 @@ openspec show add-dark-mode --json

### `openspec validate`

Validate changes and specs for structural issues.
Validate changes and specs for structural issues, and check a change's MODIFIED requirements against the main specs they would replace.

```
openspec validate [item-name] [options]
Expand Down
8 changes: 8 additions & 0 deletions docs/troubleshooting.md
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,14 @@ openspec validate --all --strict # stricter checks, good for CI

Common causes are a missing required section (like a spec with no scenarios) or a malformed delta header. Fix the file and re-run. The [CLI reference](cli.md#openspec-validate) documents the output format.

One message deserves its own note:

```text
MODIFIED "<requirement>" omits scenario(s) the current spec still has: "<scenario>"
```

A `MODIFIED` requirement replaces the whole requirement block, so it has to carry every scenario that survives the change, not only the ones you edited. Copy the named scenarios from `openspec/specs/<capability>/spec.md` back into the delta. This often appears on an older change after someone else's change added a scenario to the same requirement — archive refuses that change either way, and validation now says so before you implement it.

Comment thread
coderabbitai[bot] marked this conversation as resolved.
### The AI created incomplete or wrong artifacts

The AI didn't have enough context. A few levers help:
Expand Down
29 changes: 29 additions & 0 deletions openspec/specs/cli-validate/spec.md
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,35 @@ The CLI SHALL append a Next steps footer when the item is invalid and not using
- **WHEN** a change validation fails
- **THEN** print "Next steps" with 2-3 targeted bullets and suggest `openspec change show <id> --json --deltas-only`

### Requirement: Change validation SHALL report scenarios a MODIFIED block would drop

The `validate` command SHALL compare every `MODIFIED` requirement in a change against the main specs and report, as an error naming the delta file, each scenario the main spec still has that the `MODIFIED` block omits. A `MODIFIED` requirement replaces the whole requirement block, so archive refuses to apply one that drops a scenario; this is the same check, run without writing anything.

The comparison SHALL match archive's operation order, comparing a `MODIFIED` that names the new header of a rename against the renamed requirement's scenarios.

The check SHALL be silent when the main spec file or the requirement header is absent, because a `MODIFIED` written against a base that has not landed yet is a separate condition that archive gates. A main spec that exists but cannot be read SHALL be reported instead, since archive fails on it too.

Validation run inside `openspec archive` SHALL NOT report these issues, because archive enforces the same check when it applies the deltas.

#### Scenario: MODIFIED omits an existing scenario

- **GIVEN** the main spec's requirement has scenarios "A" and "B"
- **WHEN** a change MODIFIES that requirement with only scenario "A" and `openspec validate <change>` runs
- **THEN** report an error naming the delta file and scenario "B"
- **AND** exit with code 1

#### Scenario: MODIFIED names the new header of a rename

- **GIVEN** the main spec has requirement "A" with scenarios "S1" and "S2"
- **WHEN** a change renames "A" to "B" and MODIFIES "B" with only scenario "S1"
- **THEN** report an error naming scenario "S2"

#### Scenario: MODIFIED header is not in the main spec

- **GIVEN** a change MODIFIES a requirement header the main spec does not contain
- **WHEN** `openspec validate <change>` runs
- **THEN** do not report a dropped-scenario error for that requirement

### Requirement: Top-level validate command

The CLI SHALL provide a top-level `validate` command for validating changes and specs with flexible selection options.
Expand Down
14 changes: 10 additions & 4 deletions skills/openspec-sync-specs/SKILL.md
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ This is an **agent-driven** operation - you will read delta specs and directly e
**MODIFIED Requirements:**
- Find the requirement in main spec
- Apply the changes - this can be:
- Adding new scenarios (don't need to copy existing ones)
- Adding new scenarios the main spec does not have yet
- Modifying existing scenarios
- Changing the requirement description
- Preserve scenarios/content not mentioned in the delta
Expand Down Expand Up @@ -149,6 +149,12 @@ The system SHALL do something new.
## MODIFIED Requirements

### Requirement: Existing Feature
The system SHALL keep doing the existing thing, now also handling A.

#### Scenario: Scenario the main spec already has
- **WHEN** user does X
- **THEN** system does Y

#### Scenario: New scenario to add
- **WHEN** user does A
- **THEN** system does B
Expand Down Expand Up @@ -185,9 +191,9 @@ The system SHALL do something new.

**Key Principle: Intelligent Merging**

Unlike programmatic merging, you can apply **partial updates**:
- To add a scenario, just include that scenario under MODIFIED - don't copy existing scenarios
- The delta represents *intent*, not a wholesale replacement
Unlike programmatic merging, you merge rather than overwrite:
- A MODIFIED block carries the whole requirement - body plus every scenario that survives the change. `openspec validate` and `openspec archive` both reject one that drops a scenario the main spec still has.
- Keep anything the delta does not mention, in the main spec's existing order
- Use your judgment to merge changes sensibly

**Output On Success**
Expand Down
6 changes: 5 additions & 1 deletion src/commands/change.ts
Original file line number Diff line number Diff line change
Expand Up @@ -256,7 +256,11 @@ export class ChangeCommand {
}

const validator = new Validator(options?.strict || false);
const report = await validator.validateChangeDeltaSpecs(changeDir);
const report = await validator.validateChangeDeltaSpecs(changeDir, {
// Derived from changesPath so the main specs come from the same root the
// change itself was resolved against.
mainSpecsDir: path.join(path.dirname(changesPath), 'specs'),
});

if (options?.json) {
console.log(JSON.stringify(report, null, 2));
Expand Down
4 changes: 2 additions & 2 deletions src/commands/validate.ts
Original file line number Diff line number Diff line change
Expand Up @@ -197,7 +197,7 @@ export class ValidateCommand {
if (type === 'change') {
const changeDir = path.join(root.changesDir, id);
const start = Date.now();
const report = await validator.validateChangeDeltaSpecs(changeDir);
const report = await validator.validateChangeDeltaSpecs(changeDir, { mainSpecsDir: root.specsDir });
const durationMs = Date.now() - start;
this.printReport('change', id, report, durationMs, opts.json, root);
// Non-zero exit if invalid (keeps enriched output test semantics)
Expand Down Expand Up @@ -279,7 +279,7 @@ export class ValidateCommand {
queue.push(async () => {
const start = Date.now();
const changeDir = path.join(root.changesDir, id);
const report = await validator.validateChangeDeltaSpecs(changeDir);
const report = await validator.validateChangeDeltaSpecs(changeDir, { mainSpecsDir: root.specsDir });
const durationMs = Date.now() - start;
return { id, type: 'change' as const, valid: report.valid, issues: report.issues, durationMs };
});
Expand Down
3 changes: 3 additions & 0 deletions src/core/archive.ts
Original file line number Diff line number Diff line change
Expand Up @@ -334,6 +334,9 @@ export class ArchiveCommand {
} catch {}
}
if (hasDeltaSpecs) {
// No mainSpecsDir here on purpose: the scenario-loss check standalone
// validate runs (#1477) is the same one buildUpdatedSpec enforces a few
// steps later, and reporting it here would relabel that failure.
const deltaReport = await validator.validateChangeDeltaSpecs(changeDir);
if (!deltaReport.valid) {
hasValidationErrors = true;
Expand Down
71 changes: 71 additions & 0 deletions src/core/parsers/requirement-blocks.ts
Original file line number Diff line number Diff line change
Expand Up @@ -323,3 +323,74 @@ function parseRenamedPairs(sectionBody: SectionBody): Array<{ from: string; to:
}
return pairs;
}

interface ScenarioBlock {
name: string;
raw: string;
}

/**
* Scenario names the current requirement block has and the incoming
* (MODIFIED) block does not. A MODIFIED requirement replaces the whole block,
* so every name reported here would be dropped from the main spec.
*
* Shared by archive (which refuses to apply the block) and validate (which
* reports the same loss at authoring time, #1477), so the two cannot disagree
* about what counts as a dropped scenario.
*/
export function findMissingCurrentScenarios(current: RequirementBlock, incoming: RequirementBlock): string[] {
// Multiplicity-aware: a name present N times in current and M times in
// incoming means max(0, N - M) instances are missing. Set membership would
// treat N>M as fully covered and let archive silently drop duplicates
// (residual #1246 / duplicate-scenario-name blind spot).
const remainingIncoming = new Map<string, number>();
for (const scenario of parseScenarioBlocks(incoming.raw)) {
const name = scenario.name;
remainingIncoming.set(name, (remainingIncoming.get(name) ?? 0) + 1);
}

const missing: string[] = [];
for (const scenario of parseScenarioBlocks(current.raw)) {
const name = scenario.name;
const remaining = remainingIncoming.get(name) ?? 0;
if (remaining > 0) {
remainingIncoming.set(name, remaining - 1);
} else {
missing.push(name);
}
}
return missing;
}

function parseScenarioBlocks(requirementRaw: string): ScenarioBlock[] {
const lines = requirementRaw.replace(/\r\n?/g, '\n').split('\n');
// A `#### Scenario:` inside a fenced example is not a real scenario. The
// validator's countScenarios already ignores fenced lines; the drift check
// must agree with it, or a fenced sample can false-abort an archive (or
// mask a genuinely dropped scenario).
const mask = buildCodeFenceMask(lines);
const scenarios: ScenarioBlock[] = [];
let index = 0;

while (index < lines.length) {
const headerMatch = mask[index] ? null : lines[index].match(/^####\s*Scenario:\s*(.+)\s*$/);
if (!headerMatch) {
index++;
continue;
}

const start = index;
const name = headerMatch[1].trim();
index++;
while (index < lines.length && (mask[index] || !/^####\s*Scenario:\s*(.+)\s*$/.test(lines[index]))) {
index++;
}

scenarios.push({
name,
raw: lines.slice(start, index).join('\n').trimEnd(),
});
}

return scenarios;
}
63 changes: 1 addition & 62 deletions src/core/specs-apply.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import path from 'path';
import chalk from 'chalk';
import {
extractRequirementsSection,
findMissingCurrentScenarios,
foldRequirementName,
parseDeltaSpec,
normalizeRequirementName,
Expand All @@ -33,11 +34,6 @@ export interface SpecUpdate {
exists: boolean;
}

interface ScenarioBlock {
name: string;
raw: string;
}

// -----------------------------------------------------------------------------
// Public API
// -----------------------------------------------------------------------------
Expand Down Expand Up @@ -572,60 +568,3 @@ export function buildSpecSkeleton(specFolderName: string, changeName: string, pu
return `# ${titleBase} Specification\n\n## Purpose\n${purposeBody}\n\n## Requirements\n`;
}

function findMissingCurrentScenarios(current: RequirementBlock, incoming: RequirementBlock): string[] {
// Multiplicity-aware: a name present N times in current and M times in
// incoming means max(0, N - M) instances are missing. Set membership would
// treat N>M as fully covered and let archive silently drop duplicates
// (residual #1246 / duplicate-scenario-name blind spot).
const remainingIncoming = new Map<string, number>();
for (const scenario of parseScenarioBlocks(incoming.raw)) {
const name = scenario.name;
remainingIncoming.set(name, (remainingIncoming.get(name) ?? 0) + 1);
}

const missing: string[] = [];
for (const scenario of parseScenarioBlocks(current.raw)) {
const name = scenario.name;
const remaining = remainingIncoming.get(name) ?? 0;
if (remaining > 0) {
remainingIncoming.set(name, remaining - 1);
} else {
missing.push(name);
}
}
return missing;
}

function parseScenarioBlocks(requirementRaw: string): ScenarioBlock[] {
const lines = requirementRaw.replace(/\r\n?/g, '\n').split('\n');
// A `#### Scenario:` inside a fenced example is not a real scenario. The
// validator's countScenarios already ignores fenced lines; the drift check
// must agree with it, or a fenced sample can false-abort an archive (or
// mask a genuinely dropped scenario).
const mask = buildCodeFenceMask(lines);
const scenarios: ScenarioBlock[] = [];
let index = 0;

while (index < lines.length) {
const headerMatch = mask[index] ? null : lines[index].match(/^####\s*Scenario:\s*(.+)\s*$/);
if (!headerMatch) {
index++;
continue;
}

const start = index;
const name = headerMatch[1].trim();
index++;
while (index < lines.length && (mask[index] || !/^####\s*Scenario:\s*(.+)\s*$/.test(lines[index]))) {
index++;
}

scenarios.push({
name,
raw: lines.slice(start, index).join('\n').trimEnd(),
});
}

return scenarios;
}

28 changes: 20 additions & 8 deletions src/core/templates/workflows/sync-specs.ts
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@ ${STORE_SELECTION_GUIDANCE}
**MODIFIED Requirements:**
- Find the requirement in main spec
- Apply the changes - this can be:
- Adding new scenarios (don't need to copy existing ones)
- Adding new scenarios the main spec does not have yet
- Modifying existing scenarios
- Changing the requirement description
- Preserve scenarios/content not mentioned in the delta
Expand Down Expand Up @@ -151,6 +151,12 @@ The system SHALL do something new.
## MODIFIED Requirements

### Requirement: Existing Feature
The system SHALL keep doing the existing thing, now also handling A.

#### Scenario: Scenario the main spec already has
- **WHEN** user does X
- **THEN** system does Y

#### Scenario: New scenario to add
- **WHEN** user does A
- **THEN** system does B
Expand Down Expand Up @@ -187,9 +193,9 @@ The system SHALL do something new.

**Key Principle: Intelligent Merging**

Unlike programmatic merging, you can apply **partial updates**:
- To add a scenario, just include that scenario under MODIFIED - don't copy existing scenarios
- The delta represents *intent*, not a wholesale replacement
Unlike programmatic merging, you merge rather than overwrite:
- A MODIFIED block carries the whole requirement - body plus every scenario that survives the change. \`openspec validate\` and \`openspec archive\` both reject one that drops a scenario the main spec still has.
- Keep anything the delta does not mention, in the main spec's existing order
- Use your judgment to merge changes sensibly

**Output On Success**
Expand Down Expand Up @@ -325,7 +331,7 @@ ${STORE_SELECTION_GUIDANCE}
**MODIFIED Requirements:**
- Find the requirement in main spec
- Apply the changes - this can be:
- Adding new scenarios (don't need to copy existing ones)
- Adding new scenarios the main spec does not have yet
- Modifying existing scenarios
- Changing the requirement description
- Preserve scenarios/content not mentioned in the delta
Expand Down Expand Up @@ -374,6 +380,12 @@ The system SHALL do something new.
## MODIFIED Requirements

### Requirement: Existing Feature
The system SHALL keep doing the existing thing, now also handling A.

#### Scenario: Scenario the main spec already has
- **WHEN** user does X
- **THEN** system does Y

#### Scenario: New scenario to add
- **WHEN** user does A
- **THEN** system does B
Expand Down Expand Up @@ -410,9 +422,9 @@ The system SHALL do something new.

**Key Principle: Intelligent Merging**

Unlike programmatic merging, you can apply **partial updates**:
- To add a scenario, just include that scenario under MODIFIED - don't copy existing scenarios
- The delta represents *intent*, not a wholesale replacement
Unlike programmatic merging, you merge rather than overwrite:
- A MODIFIED block carries the whole requirement - body plus every scenario that survives the change. \`openspec validate\` and \`openspec archive\` both reject one that drops a scenario the main spec still has.
- Keep anything the delta does not mention, in the main spec's existing order
- Use your judgment to merge changes sensibly

**Output On Success**
Expand Down
Loading
Loading