fix(validate): reject a scenario header with no body

.changeset/validate-rejects-empty-scenarios.md

@@ -0,0 +1,5 @@
+---
+'@fission-ai/openspec': patch
+---
+
+Stop `validate` accepting a requirement whose only scenario is a bare header. The delta scenario counter counted every `####` header, while the spec path that archive uses to validate the rebuilt spec keeps a scenario only when its body has content, so `validate` called such a change valid and `archive` then refused it with a generic "Requirement must have at least one scenario" that did not name the requirement. Both paths now share one rule, `hasScenarioBody`, and read a scenario's body up to the same boundary, so `validate` rejects exactly what archive rejects, naming the requirement and saying that a header with no body under it does not count. A scenario whose body is only a fenced block or a deeper header still counts, a requirement with one real scenario is still accepted even when another is empty, and main-spec validation is unchanged.

src/core/parsers/markdown-parser.ts

@@ -1,5 +1,5 @@
 import { Spec, Change, Requirement, Scenario, Delta, DeltaOperation } from '../schemas/index.js';
-import { buildCodeFenceMask, extractRequirementText } from './requirement-text.js';
+import { buildCodeFenceMask, extractRequirementText, hasScenarioBody } from './requirement-text.js';
 
 export interface Section {
   level: number;
@@ -172,8 +172,9 @@ export class MarkdownParser {
     const scenarios: Scenario[] = [];
 
     for (const scenarioSection of requirementSection.children) {
-      // Store the raw text content of the scenario section
-      if (scenarioSection.content.trim()) {
+      // Store the raw text content of the scenario section. A header with no
+      // body is not a scenario; the delta counter applies the same rule.
+      if (hasScenarioBody(scenarioSection.content)) {
         scenarios.push({
           rawText: scenarioSection.content
         });

src/core/parsers/requirement-text.ts

@@ -26,9 +26,24 @@ const HEADER_LINE = /^#{1,6}\s/;
  * as a scenario, so the delta counter must too (parity). The delta/loss path
  * reuses this exact constant via `scenarioHeaderAt` in requirement-blocks.ts;
  * keep both paths on it rather than reintroducing a separate `Scenario:` regex.
+ * A header alone is not yet a scenario on either path: see hasScenarioBody.
  */
 export const SCENARIO_HEADER = /^####\s+/;
 
+/** A header at scenario level or above (`#` to `####`): where a scenario body ends. */
+const SCENARIO_BODY_END = /^#{1,4}\s/;
+
+/**
+ * Whether a scenario's body has content. The spec path
+ * (`MarkdownParser.parseScenarios`) drops a scenario whose body is empty, so
+ * the delta counter must too (parity): otherwise `validate` accepts a
+ * requirement whose only scenario is a bare header, and archive rejects it when
+ * it validates the rebuilt spec.
+ */
+export function hasScenarioBody(body: string): boolean {
+  return body.trim().length > 0;
+}
+
 /**
  * The one predicate for normative-keyword detection. Matches `SHALL` or `MUST`
  * as whole words so the change-delta reader and the schema-based reader accept
@@ -88,15 +103,31 @@ export function extractRequirementText(headerTitle: string, bodyLines: string[])
 
 /**
  * Count the real scenarios in a requirement block: `#### ` headers on non-fenced
- * lines. A `#### Scenario:` that lives inside a fenced example is not a real
- * scenario and is not counted.
+ * lines whose body has content. A `#### Scenario:` that lives inside a fenced
+ * example is not a real scenario and is not counted.
  */
 export function countScenarios(bodyLines: string[]): number {
+  return readScenarioBodies(bodyLines).filter(hasScenarioBody).length;
+}
+
+/** Count the `#### ` headers in a requirement block that have no body under them. */
+export function countEmptyScenarios(bodyLines: string[]): number {
+  return readScenarioBodies(bodyLines).filter((body) => !hasScenarioBody(body)).length;
+}
+
+/**
+ * The body of each scenario in a requirement block. A body runs to the next
+ * non-fenced header of level 4 or above, the boundary the spec path uses, so a
+ * fenced block or a deeper `#####` header is part of it.
+ */
+function readScenarioBodies(bodyLines: string[]): string[] {
   const mask = buildCodeFenceMask(bodyLines);
-  let count = 0;
+  const bodies: string[] = [];
   for (let i = 0; i < bodyLines.length; i++) {
-    if (mask[i]) continue;
-    if (SCENARIO_HEADER.test(bodyLines[i])) count++;
+    if (mask[i] || !SCENARIO_HEADER.test(bodyLines[i])) continue;
+    let end = i + 1;
+    while (end < bodyLines.length && (mask[end] || !SCENARIO_BODY_END.test(bodyLines[end]))) end++;
+    bodies.push(bodyLines.slice(i + 1, end).join('\n'));
   }
-  return count;
+  return bodies;
 }

src/core/validation/validator.ts

@@ -23,6 +23,7 @@ import {
   extractRequirementBody as extractRequirementBodyShared,
   containsShallOrMust as containsShallOrMustShared,
   countScenarios as countScenariosShared,
+  countEmptyScenarios,
 } from '../parsers/requirement-text.js';
 import { findMainSpecStructureIssues } from '../parsers/spec-structure.js';
 import { FileSystemUtils } from '../../utils/file-system.js';
@@ -271,7 +272,7 @@ export class Validator {
           }
           const scenarioCount = this.countScenarios(block.raw);
           if (scenarioCount < 1) {
-            issues.push({ level: 'ERROR', path: entryPath, message: `ADDED "${block.name}" must include at least one scenario` });
+            issues.push({ level: 'ERROR', path: entryPath, message: `ADDED "${block.name}" must include at least one scenario${this.emptyScenarioHint(block.raw)}` });
           }
         }
 
@@ -306,7 +307,7 @@ export class Validator {
           }
           const scenarioCount = this.countScenarios(block.raw);
           if (scenarioCount < 1) {
-            issues.push({ level: 'ERROR', path: entryPath, message: `MODIFIED "${block.name}" must include at least one scenario` });
+            issues.push({ level: 'ERROR', path: entryPath, message: `MODIFIED "${block.name}" must include at least one scenario${this.emptyScenarioHint(block.raw)}` });
           }
         }
 
@@ -929,6 +930,16 @@ export class Validator {
     return countScenariosShared(blockRaw.split('\n').slice(1));
   }
 
+  /**
+   * Why a requirement with a visible scenario header still has no scenario:
+   * the header has no body, and the spec path archive validates against does
+   * not count it. Empty when the block has no bare scenario header.
+   */
+  private emptyScenarioHint(blockRaw: string): string {
+    if (countEmptyScenarios(blockRaw.split('\n').slice(1)) === 0) return '';
+    return ' (a scenario header with no body under it does not count; add its steps, e.g. "- **WHEN** ..." and "- **THEN** ...")';
+  }
+
   private formatSectionList(sections: string[]): string {
     if (sections.length === 0) return '';
     if (sections.length === 1) return sections[0];

test/core/validation.scenario-body-parity.test.ts

@@ -0,0 +1,205 @@
+import { describe, it, expect, afterAll } from 'vitest';
+import { promises as fs } from 'fs';
+import os from 'os';
+import path from 'path';
+import { countScenarios } from '../../src/core/parsers/requirement-text.js';
+import { MarkdownParser } from '../../src/core/parsers/markdown-parser.js';
+import { Validator } from '../../src/core/validation/validator.js';
+import { runCLI } from '../helpers/run-cli.js';
+
+/**
+ * A scenario header with nothing under it used to pass `validate` and then fail
+ * `archive`. The delta counter (countScenarios) counted every `####` header,
+ * while the spec path (MarkdownParser.parseScenarios) keeps a scenario only
+ * when its body has content. Archive validates the rebuilt spec on the spec
+ * path, so it refused a change `validate` had just called valid.
+ *
+ * The point of these tests is parity: for every scenario shape, both counters
+ * return the same number, and `validate` accepts exactly what `archive` applies.
+ */
+
+const REQUIREMENT_TEXT = 'The system SHALL issue a credit note when an invoice is voided.';
+
+const CASES: Array<{ name: string; scenarios: string[]; expected: number }> = [
+  {
+    name: 'a scenario with steps',
+    scenarios: ['#### Scenario: Invoice voided', '- **WHEN** an invoice is voided', '- **THEN** a credit note is issued'],
+    expected: 1,
+  },
+  { name: 'a scenario header with no body', scenarios: ['#### Scenario: Invoice voided'], expected: 0 },
+  {
+    name: 'a scenario whose body is only blank and whitespace lines',
+    scenarios: ['#### Scenario: Invoice voided', '', '   ', '\t'],
+    expected: 0,
+  },
+  {
+    name: 'two scenarios, one of them empty',
+    scenarios: [
+      '#### Scenario: Not written yet',
+      '',
+      '#### Scenario: Invoice voided',
+      '- **WHEN** an invoice is voided',
+      '- **THEN** a credit note is issued',
+    ],
+    expected: 1,
+  },
+  {
+    name: 'a scenario whose body is only a fenced block',
+    scenarios: ['#### Scenario: Invoice voided', '```text', 'WHEN voided THEN a credit note is issued', '```'],
+    expected: 1,
+  },
+  { name: 'a closed scenario header with no body', scenarios: ['#### Scenario: Invoice voided ####'], expected: 0 },
+  {
+    name: 'a closed scenario header with steps',
+    scenarios: ['#### Scenario: Invoice voided ####', '- **WHEN** an invoice is voided', '- **THEN** a credit note is issued'],
+    expected: 1,
+  },
+  {
+    name: 'a scenario whose body is only a deeper header',
+    scenarios: ['#### Scenario: Invoice voided', '##### Notes'],
+    expected: 1,
+  },
+  {
+    name: 'a scenario header inside a fenced example',
+    scenarios: ['```markdown', '#### Scenario: Example', '- **WHEN** x', '```'],
+    expected: 0,
+  },
+];
+
+/** The requirement block's lines after its `### Requirement:` header. */
+const deltaBodyLines = (scenarios: string[]) => [REQUIREMENT_TEXT, '', ...scenarios];
+
+/** How many scenarios the spec path keeps for the same requirement. */
+function specPathScenarioCount(scenarios: string[]): number {
+  const spec = [
+    '# billing Specification',
+    '',
+    '## Purpose',
+    'Define how billing documents are issued for these tests.',
+    '',
+    '## Requirements',
+    '',
+    '### Requirement: Credit Notes',
+    ...deltaBodyLines(scenarios),
+    '',
+  ].join('\n');
+  return new MarkdownParser(spec).parseSpec('billing').requirements[0].scenarios.length;
+}
+
+describe('scenario counting: delta path and spec path agree', () => {
+  for (const { name, scenarios, expected } of CASES) {
+    it(`counts ${name} the same on both paths`, () => {
+      expect(specPathScenarioCount(scenarios)).toBe(expected);
+      expect(countScenarios(deltaBodyLines(scenarios))).toBe(expected);
+    });
+  }
+});
+
+const temps: string[] = [];
+afterAll(async () => {
+  await Promise.all(temps.map((dir) => fs.rm(dir, { recursive: true, force: true })));
+});
+
+async function tempDir(prefix: string): Promise<string> {
+  const dir = await fs.mkdtemp(path.join(os.tmpdir(), prefix));
+  temps.push(dir);
+  return dir;
+}
+
+const deltaSpec = (section: 'ADDED' | 'MODIFIED', scenarios: string[]) =>
+  [`## ${section} Requirements`, '### Requirement: Credit Notes', ...deltaBodyLines(scenarios), ''].join('\n');
+
+describe('validate rejects a requirement whose only scenario is empty', () => {
+  async function validateDelta(delta: string) {
+    const changeDir = await tempDir('openspec-scenario-body-');
+    await fs.mkdir(path.join(changeDir, 'specs', 'billing'), { recursive: true });
+    await fs.writeFile(path.join(changeDir, 'specs', 'billing', 'spec.md'), delta);
+    return new Validator().validateChangeDeltaSpecs(changeDir);
+  }
+
+  const scenarioErrors = (report: { issues: Array<{ level: string; message: string }> }) =>
+    report.issues.filter((issue) => issue.level === 'ERROR' && issue.message.includes('scenario'));
+
+  it('control: accepts a scenario with steps', async () => {
+    const report = await validateDelta(deltaSpec('ADDED', CASES[0].scenarios));
+    expect(report.valid).toBe(true);
+  });
+
+  for (const section of ['ADDED', 'MODIFIED'] as const) {
+    it(`rejects a ${section} requirement whose only scenario has no body, naming it`, async () => {
+      const report = await validateDelta(deltaSpec(section, ['#### Scenario: Invoice voided']));
+      expect(report.valid).toBe(false);
+      const errors = scenarioErrors(report);
+      expect(errors).toHaveLength(1);
+      expect(errors[0].message).toContain(`${section} "Credit Notes"`);
+      // Say why a header the author can see does not count.
+      expect(errors[0].message).toMatch(/no body|nothing under/i);
+    });
+  }
+
+  it('rejects a scenario whose body is only whitespace', async () => {
+    const report = await validateDelta(deltaSpec('ADDED', ['#### Scenario: Invoice voided', '   ', '\t']));
+    expect(report.valid).toBe(false);
+  });
+
+  it('accepts two scenarios when only one is empty, as archive does', async () => {
+    const report = await validateDelta(deltaSpec('ADDED', CASES[3].scenarios));
+    expect(scenarioErrors(report)).toEqual([]);
+  });
+});
+
+/**
+ * A real project, created with `openspec init` and `openspec new change`, whose
+ * change `edit` adds a Credit Notes requirement with the given scenarios.
+ */
+async function projectWithScenarios(scenarios: string[]) {
+  const base = await tempDir('openspec-scenario-body-e2e-');
+  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,
+    USERPROFILE: 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', 'edit'])).exitCode).toBe(0);
+  const dir = path.join(project, 'openspec', 'changes', 'edit');
+  await fs.mkdir(path.join(dir, 'specs', 'billing'), { recursive: true });
+  await fs.writeFile(
+    path.join(dir, 'proposal.md'),
+    '# Credit notes\n\n## Why\nVoided invoices need a matching credit note so the ledger always balances.\n\n## What Changes\n- **billing**: adds credit notes\n'
+  );
+  await fs.writeFile(path.join(dir, 'tasks.md'), '## 1. Work\n- [x] 1.1 Done\n');
+  await fs.writeFile(path.join(dir, 'specs', 'billing', 'spec.md'), deltaSpec('ADDED', scenarios));
+
+  return {
+    verdicts: async () => {
+      const validated = JSON.parse((await cli(['validate', 'edit', '--json'])).stdout);
+      const archived = await cli(['archive', 'edit', '--yes']);
+      return { valid: validated.items[0].valid as boolean, archived: archived.exitCode === 0 };
+    },
+  };
+}
+
+describe('validate and archive give the same verdict (e2e)', () => {
+  it('control: a scenario with steps is valid and archives', async () => {
+    const project = await projectWithScenarios(CASES[0].scenarios);
+    expect(await project.verdicts()).toEqual({ valid: true, archived: true });
+  }, 120_000);
+
+  it('a scenario header with no body is rejected by both', async () => {
+    const project = await projectWithScenarios(['#### Scenario: Invoice voided']);
+    expect(await project.verdicts()).toEqual({ valid: false, archived: false });
+  }, 120_000);
+
+  it('two scenarios where one is empty are accepted by both', async () => {
+    const project = await projectWithScenarios(CASES[3].scenarios);
+    expect(await project.verdicts()).toEqual({ valid: true, archived: true });
+  }, 120_000);
+});