From 0b07106a3054b9eefc4c75ba3518280d9de82362 Mon Sep 17 00:00:00 2001 From: Clay Good Date: Mon, 29 Jun 2026 18:25:09 -0500 Subject: [PATCH 1/8] docs(openspec): propose spec parser reading fidelity (fixes #361, #498, #312) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The requirement-parsing layer silently misreads valid Markdown: - #361: requirement-body extraction returns only the first non-blank line, so a SHALL/MUST that wraps onto line 2 fails `validate --strict`. - #498: `validate` (delta-block parser) and `archive` (full-spec parser) recognize requirements by different rules, so a stray `###` header passes validate but becomes a phantom requirement that blocks archive. - #312 (residual): the requirement-body loop breaks on any `#` line without consulting the code-fence mask, truncating bodies that contain fenced code with `#` comments. Proposal: one shared, multi-line, fence-aware requirement-body extractor used by both the validator and the markdown parser; recognize only `### Requirement:`-prefixed level-3 headers; guarantee validate/archive parity. Adds regression + parity tests. #559 investigated and deferred (ambiguous root cause — see design.md). Co-Authored-By: Claude Opus 4.8 (1M context) --- .../fix-spec-parser-fidelity/.openspec.yaml | 2 + .../fix-spec-parser-fidelity/design.md | 57 +++++++++++++++++++ .../fix-spec-parser-fidelity/proposal.md | 39 +++++++++++++ .../specs/cli-archive/spec.md | 14 +++++ .../specs/cli-validate/spec.md | 22 +++++++ .../specs/openspec-conventions/spec.md | 14 +++++ .../changes/fix-spec-parser-fidelity/tasks.md | 24 ++++++++ 7 files changed, 172 insertions(+) create mode 100644 openspec/changes/fix-spec-parser-fidelity/.openspec.yaml create mode 100644 openspec/changes/fix-spec-parser-fidelity/design.md create mode 100644 openspec/changes/fix-spec-parser-fidelity/proposal.md create mode 100644 openspec/changes/fix-spec-parser-fidelity/specs/cli-archive/spec.md create mode 100644 openspec/changes/fix-spec-parser-fidelity/specs/cli-validate/spec.md create mode 100644 openspec/changes/fix-spec-parser-fidelity/specs/openspec-conventions/spec.md create mode 100644 openspec/changes/fix-spec-parser-fidelity/tasks.md diff --git a/openspec/changes/fix-spec-parser-fidelity/.openspec.yaml b/openspec/changes/fix-spec-parser-fidelity/.openspec.yaml new file mode 100644 index 0000000000..34f9314d22 --- /dev/null +++ b/openspec/changes/fix-spec-parser-fidelity/.openspec.yaml @@ -0,0 +1,2 @@ +schema: spec-driven +created: 2026-06-29 diff --git a/openspec/changes/fix-spec-parser-fidelity/design.md b/openspec/changes/fix-spec-parser-fidelity/design.md new file mode 100644 index 0000000000..4e8b8b640c --- /dev/null +++ b/openspec/changes/fix-spec-parser-fidelity/design.md @@ -0,0 +1,57 @@ +# Design: Spec parser reading fidelity + +## Context + +Two independent code paths extract "the requirement text" and then check it for `SHALL`/`MUST`: + +| Path | Entry point | Used by | +|------|-------------|---------| +| Delta-block parser | `Validator.extractRequirementText(blockRaw)` over `### Requirement:` blocks under `## ADDED/MODIFIED Requirements` | `openspec validate` | +| Full-spec parser | `MarkdownParser.parseRequirements` over the rebuilt main spec | `openspec archive` (via `validateSpecContent` → `parseSpec`) | + +Both currently capture **only the first non-blank body line**, and they recognize requirements by **different rules**. That combination produces the three bugs. + +## Root causes + +### 1. Single-line body capture (#361) + +`Validator.extractRequirementText` returns the first substantial line and stops (`return trimmed`). `MarkdownParser.parseRequirements` likewise selects `directContent.split('\n').find(l => l.trim())` — the first non-empty line only. A requirement whose normative keyword wraps onto line 2: + +```markdown +### Requirement: Quest Instance Realtime Updates + +Quest-related operations (creation, claiming, completion, approval, denial) +SHALL propagate to all family members' dashboards in real-time. +``` + +yields captured text `Quest-related operations (...)` with no `SHALL` → false `must contain SHALL or MUST` error. + +### 2. Divergent requirement recognition (#498) + +`parseRequirements` treats **every** level-3 child of the Requirements section as a requirement, including dividers like `### Documentation Requirements`. The delta-block parser only ever sees `### Requirement:`-prefixed blocks. So a stray header passes `validate` (not a delta requirement) but fails `archive` (a phantom requirement with no `SHALL`/scenario in the rebuilt spec). + +### 3. Fence mask not consulted in the body loop (#312) + +`parseRequirements` walks `child.content` and breaks on `line.trim().startsWith('#')` to stop at scenarios — but it never consults `codeFenceLineMask`. A `#`-comment inside a fenced code block in the requirement body truncates the captured text. The fix for #1 must also be fence-aware here. + +## Approach + +**One shared extractor.** Introduce a single function that, given a requirement block's raw lines and the fence mask, returns the full requirement body text (all lines from after the header to the first `#### Scenario:` header, skipping fenced regions and `**metadata**:` lines, joined with spaces/newlines). Both `Validator.extractRequirementText` and `MarkdownParser.parseRequirements` call it, so they cannot drift again. `SHALL`/`MUST` detection runs over the full returned body. + +**Recognize only `### Requirement:` headers.** `parseRequirements` filters level-3 children to those whose title begins with `Requirement:` (case-insensitive, after normalization). Non-matching level-3 headers are not requirements. This aligns the full-spec parser with the delta parser and the documented convention, closing the #498 divergence at the source rather than by adding a second validation surface. + +**Parity guarantee.** Because `archive`'s rebuilt-spec validation now recognizes requirements by the same rule `validate` uses, a change that passes `validate --strict` cannot newly fail validation at `archive` for requirement-recognition reasons. A parity test asserts this over the bug fixtures. + +## Alternatives considered + +- *Patch each extractor separately.* Rejected — duplicated logic is exactly how the two paths drifted; a shared extractor is the durable fix. +- *Make `archive` warn-only on phantom headers.* Rejected — it hides the inconsistency instead of removing it, and leaves `validate`/`archive` semantics different. + +## Out of scope: #559 (folder name vs. title) + +Investigated and deferred. The reproduction transcript shows the agent reading `changes//proposal.md` (unqualified) and getting `ENOENT`, then succeeding at `openspec/changes//proposal.md`. That is a missing-`openspec/`-prefix path resolution, not a demonstrated folder-vs-title divergence. Folding a speculative fix into a parser-fidelity change would blur its scope. Recommend a separate change once the intended behavior (warn on mismatch? canonicalize unqualified paths?) is confirmed with the reporter. + +## Risks + +- Multi-line capture could change `requirement.text` used elsewhere (e.g. display). Mitigation: keep a short single-line `text` for display if needed, but run `SHALL`/`MUST` detection over the full body; tests assert display output is unchanged for single-line requirements. +- Filtering non-`Requirement:` headers could drop content authors intended as requirements. Mitigation: this matches the documented convention; the regression suite includes a fixture confirming legitimate requirements are unaffected. diff --git a/openspec/changes/fix-spec-parser-fidelity/proposal.md b/openspec/changes/fix-spec-parser-fidelity/proposal.md new file mode 100644 index 0000000000..9e7ab23fa4 --- /dev/null +++ b/openspec/changes/fix-spec-parser-fidelity/proposal.md @@ -0,0 +1,39 @@ +## Why + +OpenSpec's promise is that the spec is the source of truth. That promise breaks when the parser silently *misreads* valid Markdown. Three confirmed defects in the requirement-parsing layer cause spec content to be dropped or judged inconsistently: + +- **Wrapped `SHALL`/`MUST` is invisible (#361).** Requirement-text extraction returns only the *first* non-blank body line. Both extractors do this: the validator's `extractRequirementText` ([validator.ts](../../../src/core/validation/validator.ts) returns the first substantial line) and `MarkdownParser.parseRequirements` ([markdown-parser.ts](../../../src/core/parsers/markdown-parser.ts) takes `firstLine`). When an author wraps a requirement across two lines and the normative keyword lands on line 2, `openspec validate --strict` reports `must contain SHALL or MUST` for a requirement that plainly contains it. Users are forced to reformat valid prose to satisfy the tool. + +- **`validate` passes but `archive` fails (#498).** The two commands recognize requirements by different rules. `openspec validate` inspects delta blocks split on `### Requirement:` (a non-`Requirement:` `###` header is simply not a requirement). `openspec archive` rebuilds the full spec and re-parses it with `MarkdownParser.parseRequirements`, which treats **every** level-3 child of the Requirements section as a requirement. A stray divider like `### Documentation Requirements` is ignored by `validate` but becomes a phantom requirement (no `SHALL`, no scenarios) at `archive` time, blocking the archive after validation already passed. + +- **Fenced code blocks leak into requirement text (#312, residual).** The parser added a code-fence mask for section detection, but the requirement-body loop in `parseRequirements` still breaks on any line starting with `#` ([markdown-parser.ts:213](../../../src/core/parsers/markdown-parser.ts)) without consulting the mask. A `#`-comment inside a fenced code block in a requirement body truncates the captured text. + +These are deterministic, reproducible, and currently unaddressed by any open PR. They undermine confidence in `validate`/`archive` as a gate. + +## What Changes + +- Make requirement-body text extraction **multi-line and fence-aware** in both the validator and the markdown parser, sharing one implementation so the two paths cannot drift again. The captured requirement text spans all body lines from the header down to the first `#### Scenario:` header, skips fenced code blocks, and skips `**metadata**:` lines — then `SHALL`/`MUST` detection runs over the whole body. +- Make `MarkdownParser.parseRequirements` recognize a requirement **only** when its level-3 header matches `### Requirement:`. Non-matching level-3 headers inside the Requirements section are no longer treated as phantom requirements, eliminating the `validate`/`archive` divergence in #498. +- Guarantee **`validate`/`archive` parity**: the rebuilt-spec validation performed during `archive` applies the same requirement-recognition rules as `openspec validate`, so a change that passes `validate --strict` cannot newly fail validation at `archive`. +- Add regression tests covering each reproduction (#361 wrapped keyword, #498 stray header, #312 fenced `#`), plus a parity test asserting `validate` and `archive` agree on the same fixtures. + +Out of scope (investigated, deferred): #559 (folder-name vs. title confusion). Its reproduction transcript shows an agent dereferencing an unqualified `changes/...` path (missing the `openspec/` prefix) rather than a pure name/title mismatch; the root cause is ambiguous and warrants its own change once clarified. See `design.md`. + +## Capabilities + +### New Capabilities + +_None._ + +### Modified Capabilities + +- `cli-validate`: requirement-text extraction becomes multi-line and fence-aware; `SHALL`/`MUST` detection runs over the full requirement body. +- `cli-archive`: rebuilt-spec validation recognizes requirements using the same rules as `openspec validate` (parity guarantee). +- `openspec-conventions`: only `### Requirement:`-prefixed level-3 headers identify requirements; other level-3 headers under Requirements are not requirements. + +## Impact + +- `src/core/parsers/markdown-parser.ts` — multi-line, fence-aware requirement-body extraction; recognize only `### Requirement:` headers. +- `src/core/validation/validator.ts` — `extractRequirementText` captures the full body; share extraction logic with the parser. +- `test/core/parsers/*`, `test/core/validation/*` — regression + parity tests. +- Fixes #361, #498, #312. Related: #559 (deferred), and the archive data-integrity work in #1112/#1246/#1277 (this change hardens the *reader* those rely on). diff --git a/openspec/changes/fix-spec-parser-fidelity/specs/cli-archive/spec.md b/openspec/changes/fix-spec-parser-fidelity/specs/cli-archive/spec.md new file mode 100644 index 0000000000..be3264202e --- /dev/null +++ b/openspec/changes/fix-spec-parser-fidelity/specs/cli-archive/spec.md @@ -0,0 +1,14 @@ +## ADDED Requirements + +### Requirement: Archive rebuilt-spec validation SHALL match validate semantics +The rebuilt-spec validation performed during `openspec archive` SHALL recognize requirements using the same rules as `openspec validate`. A change that passes `openspec validate --strict` SHALL NOT newly fail validation at archive time due to requirement-recognition differences between the delta-block parser and the full-spec parser. + +#### Scenario: Stray non-requirement header does not block archive +- **GIVEN** a change whose spec deltas pass `openspec validate --strict` and whose Requirements section contains a stray level-3 header that is not a `### Requirement:` header +- **WHEN** running `openspec archive ` +- **THEN** the rebuilt-spec validation SHALL NOT treat the stray header as a phantom requirement and SHALL NOT report `must contain SHALL or MUST` or `must have at least one scenario` for it + +#### Scenario: Genuinely invalid spec still fails consistently +- **GIVEN** a change whose spec contains a real `### Requirement:` block with no `SHALL`/`MUST` and no scenario +- **WHEN** running both `openspec validate --strict` and `openspec archive ` +- **THEN** both commands SHALL report the same requirement as invalid diff --git a/openspec/changes/fix-spec-parser-fidelity/specs/cli-validate/spec.md b/openspec/changes/fix-spec-parser-fidelity/specs/cli-validate/spec.md new file mode 100644 index 0000000000..8802bffead --- /dev/null +++ b/openspec/changes/fix-spec-parser-fidelity/specs/cli-validate/spec.md @@ -0,0 +1,22 @@ +## ADDED Requirements + +### Requirement: Multi-line requirement bodies SHALL be parsed for normative keywords +The validator SHALL detect `SHALL`/`MUST` across the entire requirement body, not only the first body line. Requirement-text extraction SHALL capture every body line from after the `### Requirement:` header up to the first `#### Scenario:` header (skipping blank lines and `**metadata**:` lines), and normative-keyword detection SHALL run over the full captured body. + +#### Scenario: Normative keyword on the second wrapped line +- **GIVEN** a requirement whose descriptive text wraps across two lines and whose `SHALL` keyword is on the second line +- **WHEN** running `openspec validate --strict` +- **THEN** validation SHALL recognize the requirement as containing `SHALL` and NOT report `must contain SHALL or MUST` + +#### Scenario: Single-line requirement unaffected +- **GIVEN** a requirement whose `SHALL` statement is on a single body line +- **WHEN** running `openspec validate --strict` +- **THEN** validation behavior and messages SHALL be unchanged from before this change + +### Requirement: Fenced code blocks SHALL be ignored during requirement-text extraction +The validator and markdown parser SHALL ignore lines inside fenced code blocks (` ``` ` or `~~~`) when extracting requirement body text. A `#`-prefixed line inside a fenced code block in a requirement body SHALL NOT truncate the captured text or be mistaken for a section header. + +#### Scenario: Hash comment inside a fenced block in the requirement body +- **GIVEN** a requirement body that contains a fenced code block with lines beginning with `#` (for example a shell comment) +- **WHEN** the spec is parsed for validation +- **THEN** the full requirement body SHALL be captured, the requirement count SHALL be correct, and scenarios SHALL parse normally diff --git a/openspec/changes/fix-spec-parser-fidelity/specs/openspec-conventions/spec.md b/openspec/changes/fix-spec-parser-fidelity/specs/openspec-conventions/spec.md new file mode 100644 index 0000000000..ea0016d701 --- /dev/null +++ b/openspec/changes/fix-spec-parser-fidelity/specs/openspec-conventions/spec.md @@ -0,0 +1,14 @@ +## ADDED Requirements + +### Requirement: Only Requirement-prefixed headers SHALL identify requirements +When parsing a spec's Requirements section, the parser SHALL treat a level-3 header as a requirement only when its title begins with `Requirement:` (case-insensitive, after normalization). Other level-3 headers within the Requirements section SHALL NOT be treated as requirements. + +#### Scenario: Stray level-3 divider is not a requirement +- **GIVEN** a Requirements section containing `### Documentation Requirements` followed by a valid `### Requirement: AI Application Documentation` block +- **WHEN** the spec is parsed +- **THEN** only `### Requirement: AI Application Documentation` SHALL be counted as a requirement +- **AND** `### Documentation Requirements` SHALL NOT produce a phantom requirement that fails `SHALL`/scenario validation + +#### Scenario: Recognition is consistent across commands +- **WHEN** the same spec content is processed by `openspec validate` and by the rebuilt-spec validation in `openspec archive` +- **THEN** both SHALL identify the same set of requirements diff --git a/openspec/changes/fix-spec-parser-fidelity/tasks.md b/openspec/changes/fix-spec-parser-fidelity/tasks.md new file mode 100644 index 0000000000..b1dd2ae422 --- /dev/null +++ b/openspec/changes/fix-spec-parser-fidelity/tasks.md @@ -0,0 +1,24 @@ +## 1. Shared requirement-body extraction (#361, #312) + +- [ ] 1.1 Add a shared `extractRequirementBody(lines, fenceMask, startIndex)` helper (in `src/core/parsers/`) that returns the full requirement body: all lines after the header up to the first `#### Scenario:` header, skipping fence-masked lines and `**metadata**:` lines. +- [ ] 1.2 Rewrite `MarkdownParser.parseRequirements` to use the helper for body capture (replacing the first-line-only `firstLine` logic at `markdown-parser.ts`), and consult `codeFenceLineMask` so `#` inside fenced blocks no longer truncates the body. +- [ ] 1.3 Rewrite `Validator.extractRequirementText` to use the same helper (or delegate to it), returning the full body rather than the first substantial line. +- [ ] 1.4 Run `SHALL`/`MUST` detection over the full captured body in both paths. + +## 2. Requirement recognition + parity (#498) + +- [ ] 2.1 In `MarkdownParser.parseRequirements`, treat a level-3 child as a requirement only when its title matches `### Requirement:` (case-insensitive after normalization); ignore other level-3 headers. +- [ ] 2.2 Confirm `archive`'s rebuilt-spec validation (`validateSpecContent` → `parseSpec`) now recognizes requirements identically to `openspec validate`. + +## 3. Tests + +- [ ] 3.1 Regression (#361): a requirement with `SHALL` wrapped onto the second body line passes `validate --strict`. +- [ ] 3.2 Regression (#312): a requirement body containing a fenced code block with `#`-comment lines captures the full text and parses scenarios/counts correctly. +- [ ] 3.3 Regression (#498): a spec with a stray `### Documentation Requirements` divider passes `validate` AND `archive` (no phantom requirement). +- [ ] 3.4 Parity test: `validate` and `archive` agree (pass/fail and messages) over the #361/#498/#312 fixtures. +- [ ] 3.5 Guard test: legitimate single-line requirements are unaffected (display text and counts unchanged). +- [ ] 3.6 Cross-platform: fixtures and assertions use `path.join()`; multi-line capture works for LF and CRLF inputs. + +## 4. Release + +- [ ] 4.1 Add a changeset describing the parser-fidelity fixes (Fixes #361, #498, #312). From 6d9f44db26844329fb2db31a4ceb6270d26c2362 Mon Sep 17 00:00:00 2001 From: Clay Good Date: Mon, 29 Jun 2026 18:43:42 -0500 Subject: [PATCH 2/8] docs(openspec): bulletproof parser-fidelity proposal with empirical evidence Hardened the proposal after reproducing every claim against main with the bundled CLI and correcting two inaccuracies: - #498 reframed: archive does NOT hard-fail. validate passes; archive emits NON-BLOCKING phantom "Proposal warnings in proposal.md" because validateChange/parseRequirements counts every level-3 header as a requirement, while the delta-block parser (validate) and specs-apply (rebuild) only recognize canonical `### Requirement:`. It is a consistency bug, not data loss. Verified the rebuilt spec is clean. - #312 reframed: the original repro is already fixed by codeFenceLineMask (requirement count verified correct). The residual is a regression hazard: the body loop is fence-unaware, harmless only while first-line-only, so the multi-line fix must be fence-aware from the start. Also: unify recognition on the canonical REQUIREMENT_HEADER_REGEX (/^###\s*Requirement:\s*(.+)$/i, case-insensitive); surfaced a third latent inconsistency (Zod substring includes('SHALL') vs delta word-boundary \b(SHALL|MUST)\b) and added a single-predicate requirement; verified zero non-Requirement level-3 headers in repo specs (CI-safe); added edge-case scenarios (multi-line spec+delta paths, fenced scenario-looking lines, REMOVED/RENAMED unaffected, display vs detection); replaced broken relative links with plain paths. Proposal passes `openspec validate --strict`. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../fix-spec-parser-fidelity/design.md | 67 +++++++++++-------- .../fix-spec-parser-fidelity/proposal.md | 63 ++++++++++++----- .../specs/cli-archive/spec.md | 17 ++--- .../specs/cli-validate/spec.md | 31 ++++++--- .../specs/openspec-conventions/spec.md | 11 ++- .../changes/fix-spec-parser-fidelity/tasks.md | 38 ++++++----- 6 files changed, 147 insertions(+), 80 deletions(-) diff --git a/openspec/changes/fix-spec-parser-fidelity/design.md b/openspec/changes/fix-spec-parser-fidelity/design.md index 4e8b8b640c..3edbdf8ae2 100644 --- a/openspec/changes/fix-spec-parser-fidelity/design.md +++ b/openspec/changes/fix-spec-parser-fidelity/design.md @@ -1,57 +1,68 @@ # Design: Spec parser reading fidelity -## Context +## Verified call graph (against `main`) -Two independent code paths extract "the requirement text" and then check it for `SHALL`/`MUST`: +Two requirement extractors exist, reached by different commands: -| Path | Entry point | Used by | -|------|-------------|---------| -| Delta-block parser | `Validator.extractRequirementText(blockRaw)` over `### Requirement:` blocks under `## ADDED/MODIFIED Requirements` | `openspec validate` | -| Full-spec parser | `MarkdownParser.parseRequirements` over the rebuilt main spec | `openspec archive` (via `validateSpecContent` → `parseSpec`) | +| Extractor | Recognition rule | `SHALL`/`MUST` check | Reached by | +|-----------|------------------|----------------------|------------| +| `Validator.extractRequirementText` (+ `countScenarios`) over delta blocks | canonical `REQUIREMENT_HEADER_REGEX` `/^###\s*Requirement:\s*(.+)$/i` (via `parseRequirementBlocksFromSection`) | `containsShallOrMust` → `/\b(SHALL\|MUST)\b/` | `openspec validate ` (`validateChangeDeltaSpecs`) | +| `MarkdownParser.parseRequirements` → `req.text` | **every** level-3 child of the Requirements/ADDED section | `RequirementSchema.refine` → `text.includes('SHALL')` | `openspec validate ` (`validateSpec`), `openspec archive` (`validateChange` on proposal.md **and** `validateSpecContent` on the rebuilt spec) | -Both currently capture **only the first non-blank body line**, and they recognize requirements by **different rules**. That combination produces the three bugs. +`ChangeParser extends MarkdownParser` and calls `this.parseRequirements(...)`, so it is the *same* extractor — there is no third implementation. `specs-apply` (the archive rebuild) parses with `parseRequirementBlocksFromSection`, i.e. the *canonical* rule, so rebuilt main specs are already clean. -## Root causes +Both extractors share two defects: **(a)** they capture only the first body line, and **(b)** they recognize requirements by different rules. That combination produces the bugs. -### 1. Single-line body capture (#361) +## Root causes (each reproduced; outputs in proposal.md) -`Validator.extractRequirementText` returns the first substantial line and stops (`return trimmed`). `MarkdownParser.parseRequirements` likewise selects `directContent.split('\n').find(l => l.trim())` — the first non-empty line only. A requirement whose normative keyword wraps onto line 2: +### 1. Single-line body capture (#361) -```markdown -### Requirement: Quest Instance Realtime Updates +`extractRequirementText` returns the first substantial line and stops (`return trimmed`). `parseRequirements` selects `directContent.split('\n').find(l => l.trim())` — also the first non-empty line. A normative keyword on body line 2 is never seen. Confirmed false-negative on **both** the delta path and the main-spec path. -Quest-related operations (creation, claiming, completion, approval, denial) -SHALL propagate to all family members' dashboards in real-time. -``` +### 2. Divergent requirement recognition (#498) -yields captured text `Quest-related operations (...)` with no `SHALL` → false `must contain SHALL or MUST` error. +`parseRequirements` treats every level-3 child as a requirement, including dividers like `### Documentation Requirements`. The delta-block parser only ever sees canonical `### Requirement:` blocks. So: -### 2. Divergent requirement recognition (#498) +- `openspec validate ` → passes (divider is not a requirement). +- `openspec archive` → `validateChange(proposal.md)` counts the divider as a phantom requirement → non-blocking `Proposal warnings in proposal.md` for a "requirement" the author never wrote. +- `openspec validate ` (main spec with the same divider) → the phantom surfaces as a **blocking** error. -`parseRequirements` treats **every** level-3 child of the Requirements section as a requirement, including dividers like `### Documentation Requirements`. The delta-block parser only ever sees `### Requirement:`-prefixed blocks. So a stray header passes `validate` (not a delta requirement) but fails `archive` (a phantom requirement with no `SHALL`/scenario in the rebuilt spec). +The archive does **not** hard-fail here, because `specs-apply` filters to canonical headers when rebuilding, so `validateSpecContent(rebuilt)` passes and the write succeeds. The bug is the inconsistent, confusing signal across commands, not data loss. -### 3. Fence mask not consulted in the body loop (#312) +### 3. Fence-mask not consulted in the body loop (#312, regression hazard) -`parseRequirements` walks `child.content` and breaks on `line.trim().startsWith('#')` to stop at scenarios — but it never consults `codeFenceLineMask`. A `#`-comment inside a fenced code block in the requirement body truncates the captured text. The fix for #1 must also be fence-aware here. +The original #312 is fixed: `codeFenceLineMask` makes `parseSections` skip fenced lines, so requirement counts are correct today (verified). But the body loop in `parseRequirements` breaks on `line.trim().startsWith('#')` without consulting the mask. Harmless now (only the first line is used), but fix #1 captures the full body, at which point a fenced `#` line would truncate it — reintroducing #312. The new extractor must be fence-aware. ## Approach -**One shared extractor.** Introduce a single function that, given a requirement block's raw lines and the fence mask, returns the full requirement body text (all lines from after the header to the first `#### Scenario:` header, skipping fenced regions and `**metadata**:` lines, joined with spaces/newlines). Both `Validator.extractRequirementText` and `MarkdownParser.parseRequirements` call it, so they cannot drift again. `SHALL`/`MUST` detection runs over the full returned body. +**One shared extractor.** A single helper takes the requirement block's lines plus the fence mask and returns the full body: all lines from after the header to the first `#### Scenario:` header (the scenario boundary is detected only on **non-fenced** lines), skipping fence-masked lines and `**metadata**:` lines, joined preserving readable text. Both `Validator.extractRequirementText` and `MarkdownParser.parseRequirements` delegate to it, so they cannot drift again. + +**Canonical recognition.** `parseRequirements` filters level-3 children through the exported `REQUIREMENT_HEADER_REGEX` (case-insensitive), identical to the delta parser and `specs-apply`. `## REMOVED`/`## RENAMED` requirements are parsed by separate functions (`parseRemovedNames`, `parseRenamedPairs`) and are unaffected. + +**One keyword predicate.** The shared extractor exposes one `containsShallOrMust` used by the Zod refine and the delta path, replacing the substring/word-boundary split. + +**Parity guarantee.** Because every command recognizes requirements by the same rule and detects keywords over the same full body, `validate` (change and spec) and `archive` agree on which requirements exist and whether each is well-formed. A parity test asserts this over the bug fixtures. -**Recognize only `### Requirement:` headers.** `parseRequirements` filters level-3 children to those whose title begins with `Requirement:` (case-insensitive, after normalization). Non-matching level-3 headers are not requirements. This aligns the full-spec parser with the delta parser and the documented convention, closing the #498 divergence at the source rather than by adding a second validation surface. +## Edge cases the implementation and tests must cover -**Parity guarantee.** Because `archive`'s rebuilt-spec validation now recognizes requirements by the same rule `validate` uses, a change that passes `validate --strict` cannot newly fail validation at `archive` for requirement-recognition reasons. A parity test asserts this over the bug fixtures. +- **Display vs. detection.** `req.text` feeds display and the `MAX_REQUIREMENT_TEXT_LENGTH` INFO check. Capturing the full body must not spuriously trip length INFO for legitimately multi-line requirements (the INFO is non-blocking, but tests assert single-line output/counts are byte-for-byte unchanged). +- **Metadata-only body.** A requirement with only `**ID**:`-style lines before its scenarios still yields no normative keyword → still correctly flagged. +- **Fenced scenario-looking lines.** A `#### Scenario:` or `#`-comment line *inside* a fenced block in the body must not end body capture or fabricate a scenario boundary. +- **Line endings.** Capture operates on `normalizeContent`-normalized text (LF/CRLF/CR), consistent with the existing "Parser SHALL handle cross-platform line endings" requirement. +- **Fence variants.** ` ``` ` and `~~~`, fences of length ≥3, and leading-whitespace fences are all masked (existing `buildCodeFenceMask` behavior); the body extractor relies on that mask rather than re-detecting fences. +- **No reclassification of valid specs.** Verified: zero non-`Requirement:` level-3 headers exist under Requirements in the repo's own specs, so the recognition change is behavior-preserving for all valid specs and CI fixtures. ## Alternatives considered -- *Patch each extractor separately.* Rejected — duplicated logic is exactly how the two paths drifted; a shared extractor is the durable fix. -- *Make `archive` warn-only on phantom headers.* Rejected — it hides the inconsistency instead of removing it, and leaves `validate`/`archive` semantics different. +- *Patch each extractor separately.* Rejected — duplicated logic is exactly how the paths drifted; a shared extractor is the durable fix. +- *Make `archive`/`validate ` warn-only on phantom headers.* Rejected — it adds a second validation surface and keeps semantics divergent instead of removing the divergence. +- *Treat a stray level-3 header as an error everywhere.* Rejected for this change — it would newly fail specs that today pass `validate `; aligning on the canonical rule (divider is not a requirement) is the least-surprising, backward-compatible choice. A separate lint for stray headers could be proposed later. ## Out of scope: #559 (folder name vs. title) -Investigated and deferred. The reproduction transcript shows the agent reading `changes//proposal.md` (unqualified) and getting `ENOENT`, then succeeding at `openspec/changes//proposal.md`. That is a missing-`openspec/`-prefix path resolution, not a demonstrated folder-vs-title divergence. Folding a speculative fix into a parser-fidelity change would blur its scope. Recommend a separate change once the intended behavior (warn on mismatch? canonicalize unqualified paths?) is confirmed with the reporter. +Investigated and deferred. The reproduction transcript shows the agent reading `changes//proposal.md` (unqualified) and getting `ENOENT`, then succeeding at `openspec/changes//proposal.md` — a missing-`openspec/`-prefix path resolution, not a demonstrated folder-vs-title divergence. Folding a speculative fix into a parser-fidelity change would blur its scope. Recommend a separate change once the intended behavior (warn on mismatch? canonicalize unqualified paths?) is confirmed with the reporter. ## Risks -- Multi-line capture could change `requirement.text` used elsewhere (e.g. display). Mitigation: keep a short single-line `text` for display if needed, but run `SHALL`/`MUST` detection over the full body; tests assert display output is unchanged for single-line requirements. -- Filtering non-`Requirement:` headers could drop content authors intended as requirements. Mitigation: this matches the documented convention; the regression suite includes a fixture confirming legitimate requirements are unaffected. +- **Behavioral change to `view`/`show` counts.** Filtering non-`Requirement:` headers changes counts only for specs that violate the convention; none exist in-repo. Mitigation: guard test over repo specs; changelog note. +- **Full-body text in downstream display.** Mitigation: keep display text concise (first line) while running detection over the full body, or document the widened `text`; tests pin single-line behavior. diff --git a/openspec/changes/fix-spec-parser-fidelity/proposal.md b/openspec/changes/fix-spec-parser-fidelity/proposal.md index 9e7ab23fa4..d740e3c158 100644 --- a/openspec/changes/fix-spec-parser-fidelity/proposal.md +++ b/openspec/changes/fix-spec-parser-fidelity/proposal.md @@ -1,23 +1,51 @@ ## Why -OpenSpec's promise is that the spec is the source of truth. That promise breaks when the parser silently *misreads* valid Markdown. Three confirmed defects in the requirement-parsing layer cause spec content to be dropped or judged inconsistently: +OpenSpec's promise is that the spec is the source of truth, and `validate`/`archive` are the gate that protects it. That gate is undermined by a fragmented requirement-parsing layer: there are **two** requirement extractors that disagree with each other and with the canonical delta parser. Each defect below was reproduced against `main` with the bundled CLI; outputs are quoted verbatim. -- **Wrapped `SHALL`/`MUST` is invisible (#361).** Requirement-text extraction returns only the *first* non-blank body line. Both extractors do this: the validator's `extractRequirementText` ([validator.ts](../../../src/core/validation/validator.ts) returns the first substantial line) and `MarkdownParser.parseRequirements` ([markdown-parser.ts](../../../src/core/parsers/markdown-parser.ts) takes `firstLine`). When an author wraps a requirement across two lines and the normative keyword lands on line 2, `openspec validate --strict` reports `must contain SHALL or MUST` for a requirement that plainly contains it. Users are forced to reformat valid prose to satisfy the tool. +### 1. Wrapped `SHALL`/`MUST` is invisible (#361) — confirmed live, both paths -- **`validate` passes but `archive` fails (#498).** The two commands recognize requirements by different rules. `openspec validate` inspects delta blocks split on `### Requirement:` (a non-`Requirement:` `###` header is simply not a requirement). `openspec archive` rebuilds the full spec and re-parses it with `MarkdownParser.parseRequirements`, which treats **every** level-3 child of the Requirements section as a requirement. A stray divider like `### Documentation Requirements` is ignored by `validate` but becomes a phantom requirement (no `SHALL`, no scenarios) at `archive` time, blocking the archive after validation already passed. +Requirement-text extraction captures only the **first** non-blank body line, then checks that line for a normative keyword. When an author wraps a requirement across two lines and the keyword lands on line 2, validation falsely rejects it. -- **Fenced code blocks leak into requirement text (#312, residual).** The parser added a code-fence mask for section detection, but the requirement-body loop in `parseRequirements` still breaks on any line starting with `#` ([markdown-parser.ts:213](../../../src/core/parsers/markdown-parser.ts)) without consulting the mask. A `#`-comment inside a fenced code block in a requirement body truncates the captured text. +``` +### Requirement: Realtime quest updates +Quest-related operations (creation, claiming, completion, approval, denial) +SHALL propagate to all family members' dashboards in real-time. +``` -These are deterministic, reproducible, and currently unaddressed by any open PR. They undermine confidence in `validate`/`archive` as a gate. +- `openspec validate --strict` → `✗ [ERROR] ... ADDED "Realtime quest updates" must contain SHALL or MUST` (delta path: `Validator.extractRequirementText` returns the first substantial line). +- `openspec validate --strict` → `✗ [ERROR] requirements.0.text: Requirement must contain SHALL or MUST keyword` (main-spec path: `MarkdownParser.parseRequirements` sets `text` to `firstLine`, then `RequirementSchema` refines it). + +Both paths fail on the same valid input. Users are forced to reformat correct prose to satisfy the tool. + +### 2. `validate` and `archive` disagree on what a requirement is (#498) — confirmed live + +`openspec validate` and `openspec archive` recognize requirements by **different rules**: + +- `openspec validate ` runs only `validateChangeDeltaSpecs`, which splits sections on the canonical `### Requirement:` header. A stray divider such as `### Documentation Requirements` is simply not a requirement, so **validate passes**. +- `openspec archive` additionally runs `validateChange(proposal.md)`, whose `parseRequirements` treats **every** level-3 header under a Requirements/ADDED section as a requirement. The stray divider becomes a phantom requirement with no `SHALL` and no scenario. + +Reproduced: a change whose delta spec contains a stray `### Documentation Requirements` divider before a valid requirement validates cleanly, but at archive time prints: + +``` +Proposal warnings in proposal.md (non-blocking): + ⚠ Requirement must contain SHALL or MUST keyword + ⚠ Requirement must have at least one scenario +``` + +These warnings name a "requirement" the author never wrote and that `validate` never reported. (In this path the warnings are non-blocking and the archive still completes — `specs-apply` independently filters to `### Requirement:` blocks, so the rebuilt main spec is clean. The defect is the **inconsistent, confusing signal**, not a hard archive failure. The same phantom appears as a blocking error from `openspec validate ` on a main spec that contains a stray level-3 header.) + +### 3. Fenced code blocks are a regression hazard for the multi-line fix (#312) + +The original #312 (a `#`-comment inside a fenced code block parsed as a header, corrupting requirement counts) is **already fixed** at the section level by the `codeFenceLineMask` added since v0.15.0 — verified: a spec whose requirement body contains a ` ```bash ` block with `#` comments validates and reports the correct requirement count today. However, the body-extraction loop in `parseRequirements` still breaks on any line starting with `#` **without consulting the fence mask** (`src/core/parsers/markdown-parser.ts`, the `line.trim().startsWith('#')` guard). This is harmless today only because the loop's result is reduced to the first line. The moment fix #1 captures the **full** body, that fence-unaware guard would truncate any requirement body containing a fenced `#` line — silently reintroducing #312. The multi-line extractor must therefore be fence-aware from the start. ## What Changes -- Make requirement-body text extraction **multi-line and fence-aware** in both the validator and the markdown parser, sharing one implementation so the two paths cannot drift again. The captured requirement text spans all body lines from the header down to the first `#### Scenario:` header, skips fenced code blocks, and skips `**metadata**:` lines — then `SHALL`/`MUST` detection runs over the whole body. -- Make `MarkdownParser.parseRequirements` recognize a requirement **only** when its level-3 header matches `### Requirement:`. Non-matching level-3 headers inside the Requirements section are no longer treated as phantom requirements, eliminating the `validate`/`archive` divergence in #498. -- Guarantee **`validate`/`archive` parity**: the rebuilt-spec validation performed during `archive` applies the same requirement-recognition rules as `openspec validate`, so a change that passes `validate --strict` cannot newly fail validation at `archive`. -- Add regression tests covering each reproduction (#361 wrapped keyword, #498 stray header, #312 fenced `#`), plus a parity test asserting `validate` and `archive` agree on the same fixtures. +- **One shared, multi-line, fence-aware requirement-body extractor**, used by both `MarkdownParser.parseRequirements` and `Validator.extractRequirementText`, so the two paths cannot drift again. It captures every body line from after the `### Requirement:` header down to the first `#### Scenario:` header, skipping fence-masked lines and `**metadata**:` lines. Normative-keyword detection runs over the full captured body. +- **Unify requirement recognition on the canonical rule.** `parseRequirements` recognizes a level-3 header as a requirement only when it matches the same `REQUIREMENT_HEADER_REGEX` (`/^###\s*Requirement:\s*(.+)$/i`) already used by the delta parser and `specs-apply`. Other level-3 headers under Requirements are not requirements. This closes the #498 divergence at the source rather than papering over it with a second validation surface. +- **One normative-keyword predicate.** The shared extractor's `SHALL`/`MUST` check uses a single predicate everywhere (today the Zod schema uses substring `text.includes('SHALL')` while the delta path uses word-boundary `\b(SHALL|MUST)\b` — a latent third inconsistency). +- **Regression + parity tests** for every reproduction above, plus a guard that valid single-line requirements and the repo's own specs are unaffected. -Out of scope (investigated, deferred): #559 (folder-name vs. title confusion). Its reproduction transcript shows an agent dereferencing an unqualified `changes/...` path (missing the `openspec/` prefix) rather than a pure name/title mismatch; the root cause is ambiguous and warrants its own change once clarified. See `design.md`. +Out of scope (investigated, deferred): #559 (folder-name vs. title confusion). Its reproduction transcript shows an agent dereferencing an unqualified `changes/...` path (missing the `openspec/` prefix), not a demonstrated folder-vs-title mismatch; the root cause is ambiguous and warrants its own change once clarified. See `design.md`. ## Capabilities @@ -27,13 +55,16 @@ _None._ ### Modified Capabilities -- `cli-validate`: requirement-text extraction becomes multi-line and fence-aware; `SHALL`/`MUST` detection runs over the full requirement body. -- `cli-archive`: rebuilt-spec validation recognizes requirements using the same rules as `openspec validate` (parity guarantee). -- `openspec-conventions`: only `### Requirement:`-prefixed level-3 headers identify requirements; other level-3 headers under Requirements are not requirements. +- `cli-validate`: requirement-text extraction becomes multi-line and fence-aware; `SHALL`/`MUST` detection runs over the full requirement body using a single predicate. +- `cli-archive`: archive's requirement-recognition matches `openspec validate` — it no longer reports phantom-requirement warnings for non-`Requirement:` headers. +- `openspec-conventions`: only `### Requirement:`-prefixed level-3 headers identify requirements; recognition uses the canonical, case-insensitive header rule consistently across all parsers. ## Impact -- `src/core/parsers/markdown-parser.ts` — multi-line, fence-aware requirement-body extraction; recognize only `### Requirement:` headers. -- `src/core/validation/validator.ts` — `extractRequirementText` captures the full body; share extraction logic with the parser. +- `src/core/parsers/markdown-parser.ts` — multi-line, fence-aware requirement-body extraction; recognize only canonical `### Requirement:` headers. +- `src/core/validation/validator.ts` — `extractRequirementText` captures the full body via the shared helper; single normative-keyword predicate. +- `src/core/parsers/requirement-blocks.ts` — export/reuse `REQUIREMENT_HEADER_REGEX` as the shared recognition predicate. +- `src/core/schemas/base.schema.ts` — align the `SHALL`/`MUST` refine with the shared predicate. - `test/core/parsers/*`, `test/core/validation/*` — regression + parity tests. -- Fixes #361, #498, #312. Related: #559 (deferred), and the archive data-integrity work in #1112/#1246/#1277 (this change hardens the *reader* those rely on). +- Affects all consumers of `parseRequirements`/`parseSpec` (`validate`, `view`, `show`, `archive`) consistently; verified zero non-`Requirement:` level-3 headers exist in the repo's specs, so valid specs are unaffected. +- Fixes #361, #498. Hardens #312 against regression. Related: #559 (deferred), and the archive data-integrity work in #1112/#1246/#1277 (this change hardens the *reader* those rely on; it does not touch their merge/drop logic). diff --git a/openspec/changes/fix-spec-parser-fidelity/specs/cli-archive/spec.md b/openspec/changes/fix-spec-parser-fidelity/specs/cli-archive/spec.md index be3264202e..309de02825 100644 --- a/openspec/changes/fix-spec-parser-fidelity/specs/cli-archive/spec.md +++ b/openspec/changes/fix-spec-parser-fidelity/specs/cli-archive/spec.md @@ -1,14 +1,15 @@ ## ADDED Requirements -### Requirement: Archive rebuilt-spec validation SHALL match validate semantics -The rebuilt-spec validation performed during `openspec archive` SHALL recognize requirements using the same rules as `openspec validate`. A change that passes `openspec validate --strict` SHALL NOT newly fail validation at archive time due to requirement-recognition differences between the delta-block parser and the full-spec parser. +### Requirement: Archive requirement-recognition SHALL match validate +The requirement validation performed during `openspec archive` SHALL recognize requirements using the same canonical `### Requirement:` rule as `openspec validate`. Archive SHALL NOT report requirement-recognition issues (for example phantom `must contain SHALL or MUST` or `must have at least one scenario` warnings) for level-3 headers that `openspec validate` does not treat as requirements. -#### Scenario: Stray non-requirement header does not block archive -- **GIVEN** a change whose spec deltas pass `openspec validate --strict` and whose Requirements section contains a stray level-3 header that is not a `### Requirement:` header +#### Scenario: Stray non-requirement header produces no phantom warning at archive +- **GIVEN** a change whose spec deltas pass `openspec validate --strict` and whose Requirements/ADDED section contains a stray level-3 header that is not a `### Requirement:` header - **WHEN** running `openspec archive ` -- **THEN** the rebuilt-spec validation SHALL NOT treat the stray header as a phantom requirement and SHALL NOT report `must contain SHALL or MUST` or `must have at least one scenario` for it +- **THEN** the `Proposal warnings in proposal.md` output SHALL NOT include phantom requirement warnings derived from the stray header +- **AND** the archive SHALL succeed as it does today -#### Scenario: Genuinely invalid spec still fails consistently +#### Scenario: Genuinely invalid requirement fails consistently across commands - **GIVEN** a change whose spec contains a real `### Requirement:` block with no `SHALL`/`MUST` and no scenario -- **WHEN** running both `openspec validate --strict` and `openspec archive ` -- **THEN** both commands SHALL report the same requirement as invalid +- **WHEN** running `openspec validate --strict` and `openspec archive ` +- **THEN** both commands SHALL report the same requirement as invalid using consistent messaging diff --git a/openspec/changes/fix-spec-parser-fidelity/specs/cli-validate/spec.md b/openspec/changes/fix-spec-parser-fidelity/specs/cli-validate/spec.md index 8802bffead..da9aa38d20 100644 --- a/openspec/changes/fix-spec-parser-fidelity/specs/cli-validate/spec.md +++ b/openspec/changes/fix-spec-parser-fidelity/specs/cli-validate/spec.md @@ -1,22 +1,35 @@ ## ADDED Requirements ### Requirement: Multi-line requirement bodies SHALL be parsed for normative keywords -The validator SHALL detect `SHALL`/`MUST` across the entire requirement body, not only the first body line. Requirement-text extraction SHALL capture every body line from after the `### Requirement:` header up to the first `#### Scenario:` header (skipping blank lines and `**metadata**:` lines), and normative-keyword detection SHALL run over the full captured body. +The validator SHALL detect `SHALL`/`MUST` across the entire requirement body, not only the first body line. Requirement-text extraction SHALL capture every body line from after the `### Requirement:` header up to the first `#### Scenario:` header (skipping blank lines and `**metadata**:` lines), and normative-keyword detection SHALL run over the full captured body. Both the change-delta validation path and the main-spec validation path SHALL use the same extraction logic. -#### Scenario: Normative keyword on the second wrapped line -- **GIVEN** a requirement whose descriptive text wraps across two lines and whose `SHALL` keyword is on the second line +#### Scenario: Normative keyword on the second wrapped line of a change delta +- **GIVEN** a delta requirement whose descriptive text wraps across two lines and whose `SHALL` keyword is on the second line - **WHEN** running `openspec validate --strict` -- **THEN** validation SHALL recognize the requirement as containing `SHALL` and NOT report `must contain SHALL or MUST` +- **THEN** validation SHALL recognize the requirement as containing `SHALL` and SHALL NOT report `must contain SHALL or MUST` -#### Scenario: Single-line requirement unaffected +#### Scenario: Normative keyword on the second wrapped line of a main spec +- **GIVEN** a main spec requirement whose `SHALL` statement wraps onto the second body line +- **WHEN** running `openspec validate --strict` +- **THEN** validation SHALL recognize the keyword and SHALL NOT report `Requirement must contain SHALL or MUST keyword` + +#### Scenario: Single-line requirement is unaffected - **GIVEN** a requirement whose `SHALL` statement is on a single body line -- **WHEN** running `openspec validate --strict` -- **THEN** validation behavior and messages SHALL be unchanged from before this change +- **WHEN** running `openspec validate --strict` +- **THEN** validation behavior, messages, and the displayed requirement text SHALL be unchanged from before this change ### Requirement: Fenced code blocks SHALL be ignored during requirement-text extraction -The validator and markdown parser SHALL ignore lines inside fenced code blocks (` ``` ` or `~~~`) when extracting requirement body text. A `#`-prefixed line inside a fenced code block in a requirement body SHALL NOT truncate the captured text or be mistaken for a section header. +The validator and markdown parser SHALL ignore lines inside fenced code blocks (` ``` ` or `~~~`) when extracting requirement body text and when locating the first `#### Scenario:` boundary. A `#`-prefixed line inside a fenced code block in a requirement body SHALL NOT truncate the captured body or be mistaken for a header or scenario boundary. #### Scenario: Hash comment inside a fenced block in the requirement body -- **GIVEN** a requirement body that contains a fenced code block with lines beginning with `#` (for example a shell comment) +- **GIVEN** a requirement whose body spans multiple lines and contains a fenced code block with lines beginning with `#` (for example a shell comment) - **WHEN** the spec is parsed for validation - **THEN** the full requirement body SHALL be captured, the requirement count SHALL be correct, and scenarios SHALL parse normally + +### Requirement: A single normative-keyword predicate SHALL be used across validation paths +All `SHALL`/`MUST` detection SHALL use one predicate so that the delta-spec validation path and the schema-based validation path accept and reject identical text. The predicate SHALL match `SHALL` or `MUST` as whole words. + +#### Scenario: Keyword detection agrees across paths +- **GIVEN** identical requirement body text validated once as a change delta and once as a main spec +- **WHEN** running `openspec validate` on each +- **THEN** both SHALL reach the same conclusion about whether the body contains a normative keyword diff --git a/openspec/changes/fix-spec-parser-fidelity/specs/openspec-conventions/spec.md b/openspec/changes/fix-spec-parser-fidelity/specs/openspec-conventions/spec.md index ea0016d701..7c2c54a773 100644 --- a/openspec/changes/fix-spec-parser-fidelity/specs/openspec-conventions/spec.md +++ b/openspec/changes/fix-spec-parser-fidelity/specs/openspec-conventions/spec.md @@ -1,7 +1,7 @@ ## ADDED Requirements ### Requirement: Only Requirement-prefixed headers SHALL identify requirements -When parsing a spec's Requirements section, the parser SHALL treat a level-3 header as a requirement only when its title begins with `Requirement:` (case-insensitive, after normalization). Other level-3 headers within the Requirements section SHALL NOT be treated as requirements. +When parsing a spec's Requirements section or a change's ADDED/MODIFIED sections, the parser SHALL treat a level-3 header as a requirement only when it matches the canonical, case-insensitive header rule `### Requirement: `. Other level-3 headers within those sections SHALL NOT be treated as requirements. All parsers (delta-block validation, main-spec validation, and the archive spec rebuild) SHALL use this same recognition rule. #### Scenario: Stray level-3 divider is not a requirement - **GIVEN** a Requirements section containing `### Documentation Requirements` followed by a valid `### Requirement: AI Application Documentation` block @@ -10,5 +10,10 @@ When parsing a spec's Requirements section, the parser SHALL treat a level-3 hea - **AND** `### Documentation Requirements` SHALL NOT produce a phantom requirement that fails `SHALL`/scenario validation #### Scenario: Recognition is consistent across commands -- **WHEN** the same spec content is processed by `openspec validate` and by the rebuilt-spec validation in `openspec archive` -- **THEN** both SHALL identify the same set of requirements +- **WHEN** the same spec content is processed by `openspec validate `, `openspec validate `, and the archive spec rebuild +- **THEN** all SHALL identify the same set of requirements + +#### Scenario: REMOVED and RENAMED sections are unaffected +- **GIVEN** a change with `## REMOVED Requirements` or `## RENAMED Requirements` sections using their bullet-list or `FROM:`/`TO:` syntax +- **WHEN** the change is parsed +- **THEN** those requirements SHALL continue to be recognized by their existing dedicated parsing, independent of the level-3 header rule diff --git a/openspec/changes/fix-spec-parser-fidelity/tasks.md b/openspec/changes/fix-spec-parser-fidelity/tasks.md index b1dd2ae422..a3cc255695 100644 --- a/openspec/changes/fix-spec-parser-fidelity/tasks.md +++ b/openspec/changes/fix-spec-parser-fidelity/tasks.md @@ -1,24 +1,30 @@ -## 1. Shared requirement-body extraction (#361, #312) +## 1. Shared requirement-body extraction (#361, #312 hazard) -- [ ] 1.1 Add a shared `extractRequirementBody(lines, fenceMask, startIndex)` helper (in `src/core/parsers/`) that returns the full requirement body: all lines after the header up to the first `#### Scenario:` header, skipping fence-masked lines and `**metadata**:` lines. -- [ ] 1.2 Rewrite `MarkdownParser.parseRequirements` to use the helper for body capture (replacing the first-line-only `firstLine` logic at `markdown-parser.ts`), and consult `codeFenceLineMask` so `#` inside fenced blocks no longer truncates the body. -- [ ] 1.3 Rewrite `Validator.extractRequirementText` to use the same helper (or delegate to it), returning the full body rather than the first substantial line. +- [ ] 1.1 Add a shared `extractRequirementBody(lines, fenceMask, startIndex)` helper in `src/core/parsers/` that returns the full requirement body: all lines after the header up to the first `#### Scenario:` header detected on a **non-fence-masked** line, skipping fence-masked lines and `**metadata**:` lines. +- [ ] 1.2 Rewrite `MarkdownParser.parseRequirements` to use the helper (replacing the first-line-only `firstLine` logic) and to consult `codeFenceLineMask` so a `#` inside a fenced block no longer truncates the body. +- [ ] 1.3 Rewrite `Validator.extractRequirementText` to delegate to the same helper, returning the full body rather than the first substantial line. - [ ] 1.4 Run `SHALL`/`MUST` detection over the full captured body in both paths. -## 2. Requirement recognition + parity (#498) +## 2. Canonical requirement recognition + parity (#498) -- [ ] 2.1 In `MarkdownParser.parseRequirements`, treat a level-3 child as a requirement only when its title matches `### Requirement:` (case-insensitive after normalization); ignore other level-3 headers. -- [ ] 2.2 Confirm `archive`'s rebuilt-spec validation (`validateSpecContent` → `parseSpec`) now recognizes requirements identically to `openspec validate`. +- [ ] 2.1 Export `REQUIREMENT_HEADER_REGEX` from `requirement-blocks.ts` (or a shared `isRequirementHeader` predicate). +- [ ] 2.2 In `MarkdownParser.parseRequirements`, recognize a level-3 child as a requirement only when its header matches that canonical predicate; ignore other level-3 headers. Confirm `## REMOVED`/`## RENAMED` parsing (separate functions) is unaffected. +- [ ] 2.3 Confirm `openspec validate `, `openspec validate `, and `openspec archive` now recognize the same set of requirements (no phantom-requirement warnings for non-`Requirement:` headers). -## 3. Tests +## 3. Single normative-keyword predicate -- [ ] 3.1 Regression (#361): a requirement with `SHALL` wrapped onto the second body line passes `validate --strict`. -- [ ] 3.2 Regression (#312): a requirement body containing a fenced code block with `#`-comment lines captures the full text and parses scenarios/counts correctly. -- [ ] 3.3 Regression (#498): a spec with a stray `### Documentation Requirements` divider passes `validate` AND `archive` (no phantom requirement). -- [ ] 3.4 Parity test: `validate` and `archive` agree (pass/fail and messages) over the #361/#498/#312 fixtures. -- [ ] 3.5 Guard test: legitimate single-line requirements are unaffected (display text and counts unchanged). -- [ ] 3.6 Cross-platform: fixtures and assertions use `path.join()`; multi-line capture works for LF and CRLF inputs. +- [ ] 3.1 Replace the substring check in `src/core/schemas/base.schema.ts` (`text.includes('SHALL')`) with the shared `containsShallOrMust` (`/\b(SHALL|MUST)\b/`), so the Zod refine and the delta path agree. -## 4. Release +## 4. Tests -- [ ] 4.1 Add a changeset describing the parser-fidelity fixes (Fixes #361, #498, #312). +- [ ] 4.1 Regression (#361, delta path): a `SHALL` wrapped onto body line 2 passes `validate --strict`. +- [ ] 4.2 Regression (#361, spec path): the same wrapped `SHALL` passes `validate --strict`. +- [ ] 4.3 Regression (#312 hazard): a requirement body containing a fenced code block with `#`-comment lines captures the full body, parses scenarios, and keeps the correct requirement count. +- [ ] 4.4 Regression (#498): a spec/change with a stray `### Documentation Requirements` divider produces no phantom-requirement issue from `validate `, `validate `, or `archive`. +- [ ] 4.5 Parity test: `validate `, `validate `, and `archive` agree (same recognized requirements, same pass/fail) over the #361/#498/#312 fixtures. +- [ ] 4.6 Guard: legitimate single-line requirements are byte-for-byte unchanged in display text and counts; predicate change does not alter results for existing valid specs. +- [ ] 4.7 Cross-platform: fixtures and assertions use `path.join()`; multi-line capture verified for LF and CRLF inputs. + +## 5. Release + +- [ ] 5.1 Add a changeset describing the parser-fidelity fixes (Fixes #361, #498; hardens #312) and the `view`/`show` count note. From 550671103e78fab4612c3b43b95cf77957be8d43 Mon Sep 17 00:00:00 2001 From: Clay Good Date: Mon, 29 Jun 2026 18:54:25 -0500 Subject: [PATCH 3/8] =?UTF-8?q?docs(openspec):=20deepen=20parser-fidelity?= =?UTF-8?q?=20proposal=20=E2=80=94=20add=20#418,=20upgrade=20#312,=20tier?= =?UTF-8?q?=20the=20risk?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Second adversarial bulletproofing pass (reproduced everything against main): - Add #418 (metadata-before-description): live on the spec path (req.text = "**ID**: ...") but ALREADY fixed on the delta path. The asymmetry is direct evidence for unifying the two extractors. - Upgrade #312 from "regression hazard" to LIVE bug: a fenced code block before the prose line makes req.text = "```bash" on both paths today (distinct from the already-fixed section-count manifestation). - Tier the fixes by risk after auditing the existing test contract (markdown-parser.test.ts, 15 tests green on main): Tier 1 (false-negative fixes #361/#418/#312): only widens what is read; updates one test (:331, which asserts the first-line bug). Fence tests (:106/:139) preserved because skip-and-join keeps SHALL-first bodies. Tier 2 (recognition tightening #498): canonical ### Requirement: only; a deliberate behavior change that updates bare-header tests (:258/:310) and needs a migration note. Flagged for maintainer decision, with a conservative opt-in-lint alternative documented. - Surface the four-column extractor divergence table (capture / metadata / recognition / predicate) and an explicit "Behavior changes and test impact" section with exact test line refs. Proposal passes `openspec validate --strict`. Does not claim #1156 (PR #1280). Co-Authored-By: Claude Opus 4.8 (1M context) --- .../fix-spec-parser-fidelity/design.md | 100 +++++++++++------- .../fix-spec-parser-fidelity/proposal.md | 77 +++++++------- .../specs/cli-validate/spec.md | 34 +++--- .../specs/openspec-conventions/spec.md | 12 ++- .../changes/fix-spec-parser-fidelity/tasks.md | 48 +++++---- 5 files changed, 155 insertions(+), 116 deletions(-) diff --git a/openspec/changes/fix-spec-parser-fidelity/design.md b/openspec/changes/fix-spec-parser-fidelity/design.md index 3edbdf8ae2..3f6c47b5bd 100644 --- a/openspec/changes/fix-spec-parser-fidelity/design.md +++ b/openspec/changes/fix-spec-parser-fidelity/design.md @@ -2,67 +2,93 @@ ## Verified call graph (against `main`) -Two requirement extractors exist, reached by different commands: +| Extractor | Recognition rule | Body capture | Metadata skip | `SHALL`/`MUST` check | Reached by | +|-----------|------------------|--------------|---------------|----------------------|------------| +| `Validator.extractRequirementText` (+ `countScenarios`) | canonical `REQUIREMENT_HEADER_REGEX` `/^###\s*Requirement:\s*(.+)$/i` | first substantial line only | **yes** (`/^\*\*[^*]+\*\*:/`) | `containsShallOrMust` → `/\b(SHALL\|MUST)\b/` | `validate ` | +| `MarkdownParser.parseRequirements` → `req.text` | **every** level-3 child | first non-empty line only | **no** | `RequirementSchema.refine` → `text.includes('SHALL')` | `validate `; `archive` (proposal + rebuilt-spec checks) | -| Extractor | Recognition rule | `SHALL`/`MUST` check | Reached by | -|-----------|------------------|----------------------|------------| -| `Validator.extractRequirementText` (+ `countScenarios`) over delta blocks | canonical `REQUIREMENT_HEADER_REGEX` `/^###\s*Requirement:\s*(.+)$/i` (via `parseRequirementBlocksFromSection`) | `containsShallOrMust` → `/\b(SHALL\|MUST)\b/` | `openspec validate ` (`validateChangeDeltaSpecs`) | -| `MarkdownParser.parseRequirements` → `req.text` | **every** level-3 child of the Requirements/ADDED section | `RequirementSchema.refine` → `text.includes('SHALL')` | `openspec validate ` (`validateSpec`), `openspec archive` (`validateChange` on proposal.md **and** `validateSpecContent` on the rebuilt spec) | +`ChangeParser extends MarkdownParser` and calls `this.parseRequirements`, so it is the same extractor — there is no third implementation. `specs-apply` (the archive rebuild) parses with `parseRequirementBlocksFromSection`, i.e. the canonical rule, so rebuilt main specs are already clean. -`ChangeParser extends MarkdownParser` and calls `this.parseRequirements(...)`, so it is the *same* extractor — there is no third implementation. `specs-apply` (the archive rebuild) parses with `parseRequirementBlocksFromSection`, i.e. the *canonical* rule, so rebuilt main specs are already clean. +The table is the whole story: the two extractors differ in **four** columns (capture, metadata, recognition, predicate). Each difference is a reproduced bug. -Both extractors share two defects: **(a)** they capture only the first body line, and **(b)** they recognize requirements by different rules. That combination produces the bugs. - -## Root causes (each reproduced; outputs in proposal.md) +## Root causes (each reproduced) ### 1. Single-line body capture (#361) -`extractRequirementText` returns the first substantial line and stops (`return trimmed`). `parseRequirements` selects `directContent.split('\n').find(l => l.trim())` — also the first non-empty line. A normative keyword on body line 2 is never seen. Confirmed false-negative on **both** the delta path and the main-spec path. +Both extractors return the first line and stop. A keyword on body line 2 is never seen. + +``` +### Requirement: Realtime quest updates +Quest-related operations (creation, claiming, completion, approval, denial) +SHALL propagate to all family members' dashboards in real-time. +``` +- `validate --strict` → `✗ ADDED "Realtime quest updates" must contain SHALL or MUST` +- `validate --strict` → `✗ requirements.0.text: Requirement must contain SHALL or MUST keyword` + +### 2. Metadata before description, spec path only (#418) -### 2. Divergent requirement recognition (#498) +``` +### Requirement: File Serving +**ID**: REQ-FILE-001 +**Priority**: P1 -`parseRequirements` treats every level-3 child as a requirement, including dividers like `### Documentation Requirements`. The delta-block parser only ever sees canonical `### Requirement:` blocks. So: +The system MUST serve static files from the root directory. +``` +- `validate ` → **valid** (delta extractor skips metadata) +- `validate ` → `✗ requirements.0.text: ...`; captured `req.text` = `**ID**: REQ-FILE-001` -- `openspec validate ` → passes (divider is not a requirement). -- `openspec archive` → `validateChange(proposal.md)` counts the divider as a phantom requirement → non-blocking `Proposal warnings in proposal.md` for a "requirement" the author never wrote. -- `openspec validate ` (main spec with the same divider) → the phantom surfaces as a **blocking** error. +The delta extractor was already taught to skip metadata; the spec extractor was not. Unifying them fixes #418 and prevents the next such drift. -The archive does **not** hard-fail here, because `specs-apply` filters to canonical headers when rebuilding, so `validateSpecContent(rebuilt)` passes and the write succeeds. The bug is the inconsistent, confusing signal across commands, not data loss. +### 3. Fenced block before the prose corrupts text (#312) -### 3. Fence-mask not consulted in the body loop (#312, regression hazard) +``` +### Requirement: Config example then rule +```bash +# example config +export TOKEN=abc +``` +The system SHALL load the token from the environment. +``` +- `validate ` and `validate ` both → `✗ ... must contain SHALL or MUST`; captured `req.text` = `` ```bash `` -The original #312 is fixed: `codeFenceLineMask` makes `parseSections` skip fenced lines, so requirement counts are correct today (verified). But the body loop in `parseRequirements` breaks on `line.trim().startsWith('#')` without consulting the mask. Harmless now (only the first line is used), but fix #1 captures the full body, at which point a fenced `#` line would truncate it — reintroducing #312. The new extractor must be fence-aware. +The body loop breaks on `line.trim().startsWith('#')` (the `#` comment inside the fence) without consulting `codeFenceLineMask`. The original #312 (requirement counts) is already fixed at the section level; this is a distinct, still-live manifestation in the body extractor. + +### 4. Divergent requirement recognition (#498) + +`parseRequirements` treats every level-3 header as a requirement; the delta parser only canonical `### Requirement:`. A stray `### Documentation Requirements` divider → passes `validate `, but `archive` emits non-blocking phantom `Proposal warnings in proposal.md`, and `validate ` emits a blocking error. Archive does not hard-fail because `specs-apply` filters when rebuilding; the bug is the inconsistent signal. ## Approach -**One shared extractor.** A single helper takes the requirement block's lines plus the fence mask and returns the full body: all lines from after the header to the first `#### Scenario:` header (the scenario boundary is detected only on **non-fenced** lines), skipping fence-masked lines and `**metadata**:` lines, joined preserving readable text. Both `Validator.extractRequirementText` and `MarkdownParser.parseRequirements` delegate to it, so they cannot drift again. +**One shared extractor.** A single helper takes the requirement block's lines plus the fence mask and returns the full body: all lines from after the header to the first `#### Scenario:` header detected on a **non-fenced** line, skipping fence-masked lines and `**metadata**:` lines. Both `extractRequirementText` and `parseRequirements` delegate to it. `SHALL`/`MUST` detection runs over the full body via one predicate (`containsShallOrMust`), replacing the substring/word-boundary split. + +This is fence-aware **skip-and-join**, which is why the existing fence tests still pass: in `markdown-parser.test.ts:106`/`:139` the `SHALL` line comes first and the fenced markdown block after it, so skipping fenced lines leaves `text` exactly equal to the `SHALL` line — the asserted value. The case that breaks today (#312) is the inverse: fence *before* the prose, which no test covers. -**Canonical recognition.** `parseRequirements` filters level-3 children through the exported `REQUIREMENT_HEADER_REGEX` (case-insensitive), identical to the delta parser and `specs-apply`. `## REMOVED`/`## RENAMED` requirements are parsed by separate functions (`parseRemovedNames`, `parseRenamedPairs`) and are unaffected. +**Canonical recognition (Tier 2).** `parseRequirements` filters level-3 children through the exported `REQUIREMENT_HEADER_REGEX`. `## REMOVED`/`## RENAMED` are parsed by separate functions (`parseRemovedNames`, `parseRenamedPairs`) and are unaffected. -**One keyword predicate.** The shared extractor exposes one `containsShallOrMust` used by the Zod refine and the delta path, replacing the substring/word-boundary split. +## Existing test contract this change touches -**Parity guarantee.** Because every command recognizes requirements by the same rule and detects keywords over the same full body, `validate` (change and spec) and `archive` agree on which requirements exist and whether each is well-formed. A parity test asserts this over the bug fixtures. +`test/core/parsers/markdown-parser.test.ts` (15 tests, all green on `main`) encodes the current — buggy — behavior in three places: -## Edge cases the implementation and tests must cover +- `:331` *extract requirement text from first non-empty content line* — asserts `req.text` is only the first of two body lines. **Tier 1** changes `req.text` to the full body; this test is updated to assert the joined body. (Its premise is the #361 bug.) +- `:258` *handle nested sections correctly* — fixtures use bare `### The system SHALL …` headers and assert two requirements. **Tier 2** recognition makes bare headers non-requirements; updated to use `### Requirement: …`. +- `:310` *use requirement heading as fallback when no content is provided* — bare header. **Tier 2**; updated similarly. -- **Display vs. detection.** `req.text` feeds display and the `MAX_REQUIREMENT_TEXT_LENGTH` INFO check. Capturing the full body must not spuriously trip length INFO for legitimately multi-line requirements (the INFO is non-blocking, but tests assert single-line output/counts are byte-for-byte unchanged). -- **Metadata-only body.** A requirement with only `**ID**:`-style lines before its scenarios still yields no normative keyword → still correctly flagged. -- **Fenced scenario-looking lines.** A `#### Scenario:` or `#`-comment line *inside* a fenced block in the body must not end body capture or fabricate a scenario boundary. -- **Line endings.** Capture operates on `normalizeContent`-normalized text (LF/CRLF/CR), consistent with the existing "Parser SHALL handle cross-platform line endings" requirement. -- **Fence variants.** ` ``` ` and `~~~`, fences of length ≥3, and leading-whitespace fences are all masked (existing `buildCodeFenceMask` behavior); the body extractor relies on that mask rather than re-detecting fences. -- **No reclassification of valid specs.** Verified: zero non-`Requirement:` level-3 headers exist under Requirements in the repo's own specs, so the recognition change is behavior-preserving for all valid specs and CI fixtures. +Tier 1 alone updates only `:331`. Tier 2 additionally updates `:258` and `:310`. No other tests are affected; the fence tests (`:106`, `:139`) are preserved. ## Alternatives considered -- *Patch each extractor separately.* Rejected — duplicated logic is exactly how the paths drifted; a shared extractor is the durable fix. -- *Make `archive`/`validate ` warn-only on phantom headers.* Rejected — it adds a second validation surface and keeps semantics divergent instead of removing the divergence. -- *Treat a stray level-3 header as an error everywhere.* Rejected for this change — it would newly fail specs that today pass `validate `; aligning on the canonical rule (divider is not a requirement) is the least-surprising, backward-compatible choice. A separate lint for stray headers could be proposed later. +- **Patch each extractor separately.** Rejected — duplicated logic is exactly how they drifted (metadata skip in one, not the other). +- **Tier 2 as a separate opt-in lint instead of tightening recognition.** Keep `parseRequirements` permissive but add a warning when a level-3 header under Requirements is not `### Requirement:`. This avoids the behavior change and keeps bare-header support, but leaves `validate ` and `validate ` recognizing different requirement *sets*, so it does not fully close #498. Offered as the conservative option; the proposal recommends tightening because all in-repo specs and the convention already require `### Requirement:`. +- **Treat a stray level-3 header as a hard error everywhere.** Rejected — newly fails specs that pass `validate ` today; tightening-to-convention is the least-surprising consistent rule. -## Out of scope: #559 (folder name vs. title) +## Edge cases for tests -Investigated and deferred. The reproduction transcript shows the agent reading `changes//proposal.md` (unqualified) and getting `ENOENT`, then succeeding at `openspec/changes//proposal.md` — a missing-`openspec/`-prefix path resolution, not a demonstrated folder-vs-title divergence. Folding a speculative fix into a parser-fidelity change would blur its scope. Recommend a separate change once the intended behavior (warn on mismatch? canonicalize unqualified paths?) is confirmed with the reporter. +- Display vs. detection: `req.text` becomes the full body; assert single-line requirements are unchanged and the `MAX_REQUIREMENT_TEXT_LENGTH` INFO (non-blocking) is not spuriously tripped for legitimate multi-line bodies. +- Metadata-only body (no prose) still correctly flags missing `SHALL`/`MUST`. +- Fenced `#### Scenario:`-looking or `#`-comment lines in the body do not end capture or fabricate a scenario. +- LF/CRLF/CR via `normalizeContent`; `~~~` and length-≥3 / leading-whitespace fences via existing `buildCodeFenceMask`. +- Guard: zero non-conventional level-3 headers under Requirements in `openspec/specs/` — Tier 2 is behavior-preserving for all in-repo specs. -## Risks +## Out of scope: #559 -- **Behavioral change to `view`/`show` counts.** Filtering non-`Requirement:` headers changes counts only for specs that violate the convention; none exist in-repo. Mitigation: guard test over repo specs; changelog note. -- **Full-body text in downstream display.** Mitigation: keep display text concise (first line) while running detection over the full body, or document the widened `text`; tests pin single-line behavior. +Deferred — transcript shows an unqualified `changes//...` path (missing `openspec/` prefix), not a demonstrated folder-vs-title mismatch. Recommend a separate change once intended behavior is confirmed. diff --git a/openspec/changes/fix-spec-parser-fidelity/proposal.md b/openspec/changes/fix-spec-parser-fidelity/proposal.md index d740e3c158..7506335b1a 100644 --- a/openspec/changes/fix-spec-parser-fidelity/proposal.md +++ b/openspec/changes/fix-spec-parser-fidelity/proposal.md @@ -1,51 +1,52 @@ ## Why -OpenSpec's promise is that the spec is the source of truth, and `validate`/`archive` are the gate that protects it. That gate is undermined by a fragmented requirement-parsing layer: there are **two** requirement extractors that disagree with each other and with the canonical delta parser. Each defect below was reproduced against `main` with the bundled CLI; outputs are quoted verbatim. +OpenSpec's promise is that the spec is the source of truth, and `validate`/`archive` are the gate that protects it. That gate is undermined by a fragmented requirement-parsing layer: there are **two** requirement extractors that disagree with each other and with the canonical delta parser. Each defect below was reproduced against `main` with the bundled CLI; outputs are quoted verbatim in `design.md`. -### 1. Wrapped `SHALL`/`MUST` is invisible (#361) — confirmed live, both paths +The two extractors are `MarkdownParser.parseRequirements` (used by `validate `, and by `archive` via the proposal/rebuilt-spec checks) and `Validator.extractRequirementText` (used by `validate `). `ChangeParser extends MarkdownParser`, so it reuses `parseRequirements` — there is no third implementation. The two have already drifted: the delta extractor skips `**metadata**:` lines; the spec extractor does not. That drift is the bug surface. -Requirement-text extraction captures only the **first** non-blank body line, then checks that line for a normative keyword. When an author wraps a requirement across two lines and the keyword lands on line 2, validation falsely rejects it. +### 1. Wrapped `SHALL`/`MUST` is invisible (#361) — live, both paths -``` -### Requirement: Realtime quest updates -Quest-related operations (creation, claiming, completion, approval, denial) -SHALL propagate to all family members' dashboards in real-time. -``` +Extraction captures only the **first** non-blank body line, then checks that line. When a requirement wraps and the keyword lands on line 2, both `validate --strict` and `validate --strict` falsely report `must contain SHALL or MUST`. -- `openspec validate --strict` → `✗ [ERROR] ... ADDED "Realtime quest updates" must contain SHALL or MUST` (delta path: `Validator.extractRequirementText` returns the first substantial line). -- `openspec validate --strict` → `✗ [ERROR] requirements.0.text: Requirement must contain SHALL or MUST keyword` (main-spec path: `MarkdownParser.parseRequirements` sets `text` to `firstLine`, then `RequirementSchema` refines it). +### 2. Metadata before the description breaks the spec path (#418) — live, asymmetric -Both paths fail on the same valid input. Users are forced to reformat correct prose to satisfy the tool. +A requirement that places `**ID**:`/`**Priority**:` metadata lines before its prose validates fine as a **change** (the delta extractor skips metadata) but fails as a **spec**: `validate ` returns `req.text` = `**ID**: REQ-FILE-001` and reports `must contain SHALL or MUST`. This asymmetry is direct evidence for unifying the two extractors. -### 2. `validate` and `archive` disagree on what a requirement is (#498) — confirmed live +### 3. A fenced block before the prose corrupts requirement text (#312) — live -`openspec validate` and `openspec archive` recognize requirements by **different rules**: +The original #312 (code-fence `#` lines counted as section headers, corrupting requirement counts) is **already fixed** by the `codeFenceLineMask` added since v0.15.0 — verified. But the body-extraction loop is still fence-unaware: it breaks on any line starting with `#`. When a requirement body opens with a fenced code block (e.g. a config example) before the `SHALL` line, the `#`-comment inside the fence ends extraction early and `req.text` becomes `` ```bash ``. Reproduced today on **both** paths. The same fence-unawareness would also truncate multi-line bodies once fix #1 lands, so the new extractor must be fence-aware from the start. -- `openspec validate ` runs only `validateChangeDeltaSpecs`, which splits sections on the canonical `### Requirement:` header. A stray divider such as `### Documentation Requirements` is simply not a requirement, so **validate passes**. -- `openspec archive` additionally runs `validateChange(proposal.md)`, whose `parseRequirements` treats **every** level-3 header under a Requirements/ADDED section as a requirement. The stray divider becomes a phantom requirement with no `SHALL` and no scenario. +### 4. `validate` and `archive` disagree on what a requirement is (#498) — live -Reproduced: a change whose delta spec contains a stray `### Documentation Requirements` divider before a valid requirement validates cleanly, but at archive time prints: +`validate ` recognizes requirements only by the canonical `### Requirement:` header; `parseRequirements` (used by `archive` and `validate `) treats **every** level-3 header as a requirement. A stray divider such as `### Documentation Requirements` is ignored by `validate ` but becomes a phantom requirement: `archive` prints non-blocking `Proposal warnings in proposal.md` for a "requirement" the author never wrote, and `validate ` reports it as a blocking error. (Archive still completes — `specs-apply` independently filters to `### Requirement:`, so the rebuilt spec is clean. The defect is the inconsistent, confusing signal.) -``` -Proposal warnings in proposal.md (non-blocking): - ⚠ Requirement must contain SHALL or MUST keyword - ⚠ Requirement must have at least one scenario -``` +## What Changes -These warnings name a "requirement" the author never wrote and that `validate` never reported. (In this path the warnings are non-blocking and the archive still completes — `specs-apply` independently filters to `### Requirement:` blocks, so the rebuilt main spec is clean. The defect is the **inconsistent, confusing signal**, not a hard archive failure. The same phantom appears as a blocking error from `openspec validate ` on a main spec that contains a stray level-3 header.) +The fixes fall into two tiers with different risk profiles. They are described separately so they can be reviewed — and if desired, merged — independently. -### 3. Fenced code blocks are a regression hazard for the multi-line fix (#312) +### Tier 1 — false-negative fixes (low risk): #361, #418, #312 -The original #312 (a `#`-comment inside a fenced code block parsed as a header, corrupting requirement counts) is **already fixed** at the section level by the `codeFenceLineMask` added since v0.15.0 — verified: a spec whose requirement body contains a ` ```bash ` block with `#` comments validates and reports the correct requirement count today. However, the body-extraction loop in `parseRequirements` still breaks on any line starting with `#` **without consulting the fence mask** (`src/core/parsers/markdown-parser.ts`, the `line.trim().startsWith('#')` guard). This is harmless today only because the loop's result is reduced to the first line. The moment fix #1 captures the **full** body, that fence-unaware guard would truncate any requirement body containing a fenced `#` line — silently reintroducing #312. The multi-line extractor must therefore be fence-aware from the start. +- **One shared, multi-line, fence-aware, metadata-aware requirement-body extractor**, used by both `parseRequirements` and `extractRequirementText`, so they cannot drift again. It captures every body line from after the `### Requirement:` header to the first `#### Scenario:` header detected on a non-fenced line, skipping fence-masked lines and `**metadata**:` lines, and `SHALL`/`MUST` detection runs over the full captured body. +- **One normative-keyword predicate** everywhere (today the Zod schema uses substring `text.includes('SHALL')` while the delta path uses word-boundary `\b(SHALL|MUST)\b`). -## What Changes +Tier 1 only widens what is *read*; it does not change which headers count as requirements. It fixes false negatives without rejecting anything that passes today. + +### Tier 2 — recognition consistency (behavior change, flagged for decision): #498 + +- **Unify requirement recognition on the canonical rule.** `parseRequirements` recognizes a level-3 header as a requirement only when it matches the same `REQUIREMENT_HEADER_REGEX` (`/^###\s*Requirement:\s*(.+)$/i`) used by the delta parser and `specs-apply`. This removes the phantom-requirement divergence in #498. + +Tier 2 is a deliberate **tightening to the documented convention**. The parser is currently permissive — it accepts bare `### ` headers as requirements — and that permissiveness is what lets stray dividers become phantoms. Tightening aligns all commands but changes behavior for specs that use non-conventional headers (see "Behavior changes" below). The alternative — a separate opt-in lint that flags stray level-3 headers without changing recognition — is described in `design.md`; we recommend the tightening but defer the call to maintainers. + +Out of scope (investigated, deferred): #559 (folder-name vs. title) — its transcript shows an unqualified `changes/...` path, not a proven name/title mismatch. See `design.md`. + +## Behavior changes and existing-test impact + +All 15 tests in `test/core/parsers/markdown-parser.test.ts` pass on `main`; this change updates three of them, each encoding behavior that is itself part of the bug: -- **One shared, multi-line, fence-aware requirement-body extractor**, used by both `MarkdownParser.parseRequirements` and `Validator.extractRequirementText`, so the two paths cannot drift again. It captures every body line from after the `### Requirement:` header down to the first `#### Scenario:` header, skipping fence-masked lines and `**metadata**:` lines. Normative-keyword detection runs over the full captured body. -- **Unify requirement recognition on the canonical rule.** `parseRequirements` recognizes a level-3 header as a requirement only when it matches the same `REQUIREMENT_HEADER_REGEX` (`/^###\s*Requirement:\s*(.+)$/i`) already used by the delta parser and `specs-apply`. Other level-3 headers under Requirements are not requirements. This closes the #498 divergence at the source rather than papering over it with a second validation surface. -- **One normative-keyword predicate.** The shared extractor's `SHALL`/`MUST` check uses a single predicate everywhere (today the Zod schema uses substring `text.includes('SHALL')` while the delta path uses word-boundary `\b(SHALL|MUST)\b` — a latent third inconsistency). -- **Regression + parity tests** for every reproduction above, plus a guard that valid single-line requirements and the repo's own specs are unaffected. +- **Tier 1** updates `should extract requirement text from first non-empty content line` (`:331`) — it asserts `req.text` equals only the first body line. After the fix, `req.text` is the full (metadata-/fence-skipped) body. The existing fence tests (`:106`, `:139`), which put `SHALL` first and the fence after, are **preserved** because fenced lines are skipped during capture. +- **Tier 2** updates `should handle nested sections correctly` (`:258`) and `should use requirement heading as fallback when no content is provided` (`:310`) — both rely on bare `### …` headers being treated as requirements. After the tightening, requirements must use `### Requirement:`. -Out of scope (investigated, deferred): #559 (folder-name vs. title confusion). Its reproduction transcript shows an agent dereferencing an unqualified `changes/...` path (missing the `openspec/` prefix), not a demonstrated folder-vs-title mismatch; the root cause is ambiguous and warrants its own change once clarified. See `design.md`. +Migration for Tier 2: a changelog note that non-conventional `### ` requirement headers are no longer recognized; authors must use `### Requirement: ` (which the convention already mandates and all in-repo specs already follow — verified zero non-conventional level-3 headers exist under Requirements in `openspec/specs/`). ## Capabilities @@ -55,16 +56,16 @@ _None._ ### Modified Capabilities -- `cli-validate`: requirement-text extraction becomes multi-line and fence-aware; `SHALL`/`MUST` detection runs over the full requirement body using a single predicate. -- `cli-archive`: archive's requirement-recognition matches `openspec validate` — it no longer reports phantom-requirement warnings for non-`Requirement:` headers. -- `openspec-conventions`: only `### Requirement:`-prefixed level-3 headers identify requirements; recognition uses the canonical, case-insensitive header rule consistently across all parsers. +- `cli-validate`: requirement-text extraction becomes multi-line, fence-aware, and metadata-aware; `SHALL`/`MUST` detection runs over the full body using a single predicate. +- `cli-archive`: archive's requirement-recognition matches `openspec validate` — no phantom-requirement warnings for non-`Requirement:` headers (Tier 2). +- `openspec-conventions`: only `### Requirement:`-prefixed level-3 headers identify requirements, applied consistently across all parsers (Tier 2). ## Impact -- `src/core/parsers/markdown-parser.ts` — multi-line, fence-aware requirement-body extraction; recognize only canonical `### Requirement:` headers. -- `src/core/validation/validator.ts` — `extractRequirementText` captures the full body via the shared helper; single normative-keyword predicate. +- `src/core/parsers/markdown-parser.ts` — shared multi-line/fence/metadata-aware body extraction; canonical recognition (Tier 2). +- `src/core/validation/validator.ts` — `extractRequirementText` delegates to the shared helper; single keyword predicate. - `src/core/parsers/requirement-blocks.ts` — export/reuse `REQUIREMENT_HEADER_REGEX` as the shared recognition predicate. - `src/core/schemas/base.schema.ts` — align the `SHALL`/`MUST` refine with the shared predicate. -- `test/core/parsers/*`, `test/core/validation/*` — regression + parity tests. -- Affects all consumers of `parseRequirements`/`parseSpec` (`validate`, `view`, `show`, `archive`) consistently; verified zero non-`Requirement:` level-3 headers exist in the repo's specs, so valid specs are unaffected. -- Fixes #361, #498. Hardens #312 against regression. Related: #559 (deferred), and the archive data-integrity work in #1112/#1246/#1277 (this change hardens the *reader* those rely on; it does not touch their merge/drop logic). +- `test/core/parsers/markdown-parser.test.ts`, `test/core/validation/*` — update the three tests above; add regression + parity tests. +- Affects all consumers of `parseRequirements`/`parseSpec` (`validate`, `view`, `show`, `archive`) consistently. +- Fixes #361, #418, #312. Tier 2 fixes #498. Related: #559 (deferred); hardens the *reader* the archive data-integrity work (#1112/#1246/#1277) relies on, without touching their merge/drop logic. Does not claim #1156 (covered by PR #1280). diff --git a/openspec/changes/fix-spec-parser-fidelity/specs/cli-validate/spec.md b/openspec/changes/fix-spec-parser-fidelity/specs/cli-validate/spec.md index da9aa38d20..b765774083 100644 --- a/openspec/changes/fix-spec-parser-fidelity/specs/cli-validate/spec.md +++ b/openspec/changes/fix-spec-parser-fidelity/specs/cli-validate/spec.md @@ -1,33 +1,33 @@ ## ADDED Requirements -### Requirement: Multi-line requirement bodies SHALL be parsed for normative keywords -The validator SHALL detect `SHALL`/`MUST` across the entire requirement body, not only the first body line. Requirement-text extraction SHALL capture every body line from after the `### Requirement:` header up to the first `#### Scenario:` header (skipping blank lines and `**metadata**:` lines), and normative-keyword detection SHALL run over the full captured body. Both the change-delta validation path and the main-spec validation path SHALL use the same extraction logic. +### Requirement: Requirement bodies SHALL be parsed in full for normative keywords +The validator SHALL detect `SHALL`/`MUST` across the entire requirement body, not only the first body line. Requirement-text extraction SHALL capture every body line from after the `### Requirement:` header up to the first `#### Scenario:` header, skipping blank lines, `**metadata**:` lines, and lines inside fenced code blocks. Normative-keyword detection SHALL run over the full captured body. The change-delta path and the main-spec path SHALL use the same extraction logic so they cannot diverge. -#### Scenario: Normative keyword on the second wrapped line of a change delta -- **GIVEN** a delta requirement whose descriptive text wraps across two lines and whose `SHALL` keyword is on the second line -- **WHEN** running `openspec validate --strict` -- **THEN** validation SHALL recognize the requirement as containing `SHALL` and SHALL NOT report `must contain SHALL or MUST` +#### Scenario: Normative keyword on the second wrapped line (change and spec) +- **GIVEN** a requirement whose descriptive text wraps across two lines with `SHALL` on the second line +- **WHEN** running `openspec validate --strict` for both a change delta and a main spec +- **THEN** both SHALL recognize the keyword and SHALL NOT report a missing-`SHALL`/`MUST` error -#### Scenario: Normative keyword on the second wrapped line of a main spec -- **GIVEN** a main spec requirement whose `SHALL` statement wraps onto the second body line +#### Scenario: Metadata fields precede the description +- **GIVEN** a requirement whose body begins with `**ID**:`/`**Priority**:` metadata lines before a `MUST` description - **WHEN** running `openspec validate --strict` -- **THEN** validation SHALL recognize the keyword and SHALL NOT report `Requirement must contain SHALL or MUST keyword` +- **THEN** validation SHALL skip the metadata lines, detect `MUST` in the description, and pass — matching the existing behavior of `openspec validate ` #### Scenario: Single-line requirement is unaffected - **GIVEN** a requirement whose `SHALL` statement is on a single body line - **WHEN** running `openspec validate --strict` -- **THEN** validation behavior, messages, and the displayed requirement text SHALL be unchanged from before this change +- **THEN** validation behavior and messages SHALL be unchanged from before this change -### Requirement: Fenced code blocks SHALL be ignored during requirement-text extraction -The validator and markdown parser SHALL ignore lines inside fenced code blocks (` ``` ` or `~~~`) when extracting requirement body text and when locating the first `#### Scenario:` boundary. A `#`-prefixed line inside a fenced code block in a requirement body SHALL NOT truncate the captured body or be mistaken for a header or scenario boundary. +### Requirement: Fenced code blocks SHALL NOT corrupt requirement-text extraction +The validator and markdown parser SHALL ignore lines inside fenced code blocks (` ``` ` or `~~~`) when extracting requirement body text and when locating the first `#### Scenario:` boundary. A fenced code block appearing before the prose line of a requirement SHALL NOT cause the fence marker or a `#`-comment inside it to be taken as the requirement text. -#### Scenario: Hash comment inside a fenced block in the requirement body -- **GIVEN** a requirement whose body spans multiple lines and contains a fenced code block with lines beginning with `#` (for example a shell comment) -- **WHEN** the spec is parsed for validation -- **THEN** the full requirement body SHALL be captured, the requirement count SHALL be correct, and scenarios SHALL parse normally +#### Scenario: Fenced block before the prose line +- **GIVEN** a requirement whose body opens with a fenced code block containing `#`-comment lines, followed by the `SHALL` prose line +- **WHEN** the spec or change is validated +- **THEN** the captured requirement text SHALL be the prose line (not the fence marker), and validation SHALL pass ### Requirement: A single normative-keyword predicate SHALL be used across validation paths -All `SHALL`/`MUST` detection SHALL use one predicate so that the delta-spec validation path and the schema-based validation path accept and reject identical text. The predicate SHALL match `SHALL` or `MUST` as whole words. +All `SHALL`/`MUST` detection SHALL use one predicate that matches `SHALL` or `MUST` as whole words, so the delta-spec validation path and the schema-based validation path accept and reject identical text. #### Scenario: Keyword detection agrees across paths - **GIVEN** identical requirement body text validated once as a change delta and once as a main spec diff --git a/openspec/changes/fix-spec-parser-fidelity/specs/openspec-conventions/spec.md b/openspec/changes/fix-spec-parser-fidelity/specs/openspec-conventions/spec.md index 7c2c54a773..308c27d0f8 100644 --- a/openspec/changes/fix-spec-parser-fidelity/specs/openspec-conventions/spec.md +++ b/openspec/changes/fix-spec-parser-fidelity/specs/openspec-conventions/spec.md @@ -1,11 +1,11 @@ ## ADDED Requirements -### Requirement: Only Requirement-prefixed headers SHALL identify requirements -When parsing a spec's Requirements section or a change's ADDED/MODIFIED sections, the parser SHALL treat a level-3 header as a requirement only when it matches the canonical, case-insensitive header rule `### Requirement: `. Other level-3 headers within those sections SHALL NOT be treated as requirements. All parsers (delta-block validation, main-spec validation, and the archive spec rebuild) SHALL use this same recognition rule. +### Requirement: Requirement recognition SHALL use the canonical header rule consistently +All parsers — change-delta validation, main-spec validation, and the archive spec rebuild — SHALL recognize a level-3 header as a requirement only when it matches the canonical, case-insensitive rule `### Requirement: `. Other level-3 headers within a Requirements/ADDED/MODIFIED section SHALL NOT be treated as requirements. This tightens the previously permissive main-spec parser (which accepted any `### ` header) to match the rule already enforced by the delta parser, the convention, and `specs-apply`. #### Scenario: Stray level-3 divider is not a requirement - **GIVEN** a Requirements section containing `### Documentation Requirements` followed by a valid `### Requirement: AI Application Documentation` block -- **WHEN** the spec is parsed +- **WHEN** the spec is parsed by any command - **THEN** only `### Requirement: AI Application Documentation` SHALL be counted as a requirement - **AND** `### Documentation Requirements` SHALL NOT produce a phantom requirement that fails `SHALL`/scenario validation @@ -13,6 +13,12 @@ When parsing a spec's Requirements section or a change's ADDED/MODIFIED sections - **WHEN** the same spec content is processed by `openspec validate `, `openspec validate `, and the archive spec rebuild - **THEN** all SHALL identify the same set of requirements +#### Scenario: Non-conventional bare headers require migration +- **GIVEN** a legacy spec that used a bare `### ` header (without the `Requirement:` prefix) to declare a requirement +- **WHEN** the spec is parsed after this change +- **THEN** that header SHALL no longer be recognized as a requirement +- **AND** the change SHALL ship a changelog note instructing authors to use `### Requirement: ` as the convention already requires + #### Scenario: REMOVED and RENAMED sections are unaffected - **GIVEN** a change with `## REMOVED Requirements` or `## RENAMED Requirements` sections using their bullet-list or `FROM:`/`TO:` syntax - **WHEN** the change is parsed diff --git a/openspec/changes/fix-spec-parser-fidelity/tasks.md b/openspec/changes/fix-spec-parser-fidelity/tasks.md index a3cc255695..36c7a6a35b 100644 --- a/openspec/changes/fix-spec-parser-fidelity/tasks.md +++ b/openspec/changes/fix-spec-parser-fidelity/tasks.md @@ -1,30 +1,36 @@ -## 1. Shared requirement-body extraction (#361, #312 hazard) +## 1. Tier 1 — shared body extraction (#361, #418, #312) -- [ ] 1.1 Add a shared `extractRequirementBody(lines, fenceMask, startIndex)` helper in `src/core/parsers/` that returns the full requirement body: all lines after the header up to the first `#### Scenario:` header detected on a **non-fence-masked** line, skipping fence-masked lines and `**metadata**:` lines. -- [ ] 1.2 Rewrite `MarkdownParser.parseRequirements` to use the helper (replacing the first-line-only `firstLine` logic) and to consult `codeFenceLineMask` so a `#` inside a fenced block no longer truncates the body. -- [ ] 1.3 Rewrite `Validator.extractRequirementText` to delegate to the same helper, returning the full body rather than the first substantial line. -- [ ] 1.4 Run `SHALL`/`MUST` detection over the full captured body in both paths. +- [ ] 1.1 Add a shared `extractRequirementBody(lines, fenceMask, startIndex)` helper in `src/core/parsers/` that returns the full body: all lines after the header up to the first `#### Scenario:` header on a non-fence-masked line, skipping fence-masked lines and `**metadata**:` lines. +- [ ] 1.2 Rewrite `MarkdownParser.parseRequirements` to use the helper (replacing first-line-only logic), consulting `codeFenceLineMask` so a `#` inside a fence no longer truncates the body, and skipping metadata lines (parity with the delta path). +- [ ] 1.3 Rewrite `Validator.extractRequirementText` to delegate to the same helper, returning the full body. +- [ ] 1.4 Run `SHALL`/`MUST` detection over the full body in both paths. -## 2. Canonical requirement recognition + parity (#498) +## 2. Tier 1 — single normative-keyword predicate -- [ ] 2.1 Export `REQUIREMENT_HEADER_REGEX` from `requirement-blocks.ts` (or a shared `isRequirementHeader` predicate). -- [ ] 2.2 In `MarkdownParser.parseRequirements`, recognize a level-3 child as a requirement only when its header matches that canonical predicate; ignore other level-3 headers. Confirm `## REMOVED`/`## RENAMED` parsing (separate functions) is unaffected. -- [ ] 2.3 Confirm `openspec validate `, `openspec validate `, and `openspec archive` now recognize the same set of requirements (no phantom-requirement warnings for non-`Requirement:` headers). +- [ ] 2.1 Replace the substring check in `src/core/schemas/base.schema.ts` (`text.includes('SHALL')`) with the shared `containsShallOrMust` (`/\b(SHALL|MUST)\b/`) so the Zod refine and the delta path agree. -## 3. Single normative-keyword predicate +## 3. Tier 2 — canonical requirement recognition (#498) -- [ ] 3.1 Replace the substring check in `src/core/schemas/base.schema.ts` (`text.includes('SHALL')`) with the shared `containsShallOrMust` (`/\b(SHALL|MUST)\b/`), so the Zod refine and the delta path agree. +- [ ] 3.1 Export `REQUIREMENT_HEADER_REGEX` from `requirement-blocks.ts` (or a shared `isRequirementHeader` predicate). +- [ ] 3.2 In `MarkdownParser.parseRequirements`, recognize a level-3 child as a requirement only when it matches that canonical predicate; confirm `## REMOVED`/`## RENAMED` parsing is unaffected. +- [ ] 3.3 Confirm `validate `, `validate `, and `archive` recognize the same requirement set (no phantom-requirement warnings). -## 4. Tests +## 4. Update existing tests (encode the corrected behavior) -- [ ] 4.1 Regression (#361, delta path): a `SHALL` wrapped onto body line 2 passes `validate --strict`. -- [ ] 4.2 Regression (#361, spec path): the same wrapped `SHALL` passes `validate --strict`. -- [ ] 4.3 Regression (#312 hazard): a requirement body containing a fenced code block with `#`-comment lines captures the full body, parses scenarios, and keeps the correct requirement count. -- [ ] 4.4 Regression (#498): a spec/change with a stray `### Documentation Requirements` divider produces no phantom-requirement issue from `validate `, `validate `, or `archive`. -- [ ] 4.5 Parity test: `validate `, `validate `, and `archive` agree (same recognized requirements, same pass/fail) over the #361/#498/#312 fixtures. -- [ ] 4.6 Guard: legitimate single-line requirements are byte-for-byte unchanged in display text and counts; predicate change does not alter results for existing valid specs. -- [ ] 4.7 Cross-platform: fixtures and assertions use `path.join()`; multi-line capture verified for LF and CRLF inputs. +- [ ] 4.1 `markdown-parser.test.ts:331` (*first non-empty content line*) → assert `req.text` is the full joined body (Tier 1). +- [ ] 4.2 `markdown-parser.test.ts:258` (*nested sections*) → use `### Requirement: …` headers (Tier 2). +- [ ] 4.3 `markdown-parser.test.ts:310` (*heading fallback*) → use `### Requirement: …` header (Tier 2). +- [ ] 4.4 Confirm the fence tests (`:106`, `:139`) still pass unchanged. -## 5. Release +## 5. Regression + parity tests -- [ ] 5.1 Add a changeset describing the parser-fidelity fixes (Fixes #361, #498; hardens #312) and the `view`/`show` count note. +- [ ] 5.1 (#361, both paths) `SHALL` wrapped onto body line 2 passes `validate ` and `validate `. +- [ ] 5.2 (#418, spec path) metadata lines before the prose pass `validate `; delta path stays green. +- [ ] 5.3 (#312) fenced code block before the prose line captures the real body; requirement count and scenarios are correct. +- [ ] 5.4 (#498) a stray `### Documentation Requirements` divider yields no phantom-requirement issue from `validate `, `validate `, or `archive`. +- [ ] 5.5 Parity: the three commands agree (same recognized requirements, same pass/fail) over the fixtures above. +- [ ] 5.6 Guard: legitimate single-line requirements unchanged; existing in-repo specs still validate; LF/CRLF covered. + +## 6. Release + +- [ ] 6.1 Add a changeset: Fixes #361, #418, #312; Tier 2 fixes #498. Include the Tier 2 migration note (non-conventional `### ` requirement headers are no longer recognized; use `### Requirement: `). From c63913b932c7f843d34d00f6eef41a1a6bcebb1a Mon Sep 17 00:00:00 2001 From: Clay Good Date: Mon, 29 Jun 2026 20:10:16 -0500 Subject: [PATCH 4/8] =?UTF-8?q?docs(openspec):=20third=20pass=20=E2=80=94?= =?UTF-8?q?=20reject=20recognition=20tightening,=20add=20fenced-scenario?= =?UTF-8?q?=20bug,=20#498=E2=86=92safe=20INFO?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Third deep pass found the prior Tier 2 (recognition tightening to `### Requirement:`) was the WRONG fix and over-scoped: - Bare `### ` headers are a SUPPORTED, tested requirement format: test/core/validation.test.ts asserts a bare-header spec is valid, and bare headers appear across json-converter/archive/spec tests and tmp-init fixtures. Tightening would break a large test surface and silently drop requirements from real specs. REJECTED, with evidence documented. - Replace the #498 fix with a SAFE INFO note in validate that surfaces non-`### Requirement:` headers in delta sections. INFO never fails validation (strict: valid = no errors && no warnings), so nothing newly fails. - New bug found and folded in: countScenarios is fence-unaware, so a `#### Scenario:` inside a fenced block is counted as real — a malformed delta passes validate while validate correctly fails. Same fence family. - Proved the archive WRITE path is independent of the reader: specs-apply rebuilds from raw `### Requirement:` blocks (extractRequirementsSection + RequirementBlock.raw), never parseSpec/req.text → Part A cannot change archived content. Net effect: recognition is unchanged, so the proposal now updates exactly ONE existing test (:331, the first-line assertion) instead of breaking bare-header tests. Consolidated to a single cli-validate delta (dropped cli-archive and openspec-conventions deltas). Dropped the no-space-header hypothesis (no divergence). Passes `openspec validate --strict`. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../fix-spec-parser-fidelity/design.md | 108 +++++++----------- .../fix-spec-parser-fidelity/proposal.md | 76 ++++++------ .../specs/cli-archive/spec.md | 15 --- .../specs/cli-validate/spec.md | 43 +++++-- .../specs/openspec-conventions/spec.md | 25 ---- .../changes/fix-spec-parser-fidelity/tasks.md | 46 ++++---- 6 files changed, 132 insertions(+), 181 deletions(-) delete mode 100644 openspec/changes/fix-spec-parser-fidelity/specs/cli-archive/spec.md delete mode 100644 openspec/changes/fix-spec-parser-fidelity/specs/openspec-conventions/spec.md diff --git a/openspec/changes/fix-spec-parser-fidelity/design.md b/openspec/changes/fix-spec-parser-fidelity/design.md index 3f6c47b5bd..d332d7a274 100644 --- a/openspec/changes/fix-spec-parser-fidelity/design.md +++ b/openspec/changes/fix-spec-parser-fidelity/design.md @@ -1,94 +1,68 @@ # Design: Spec parser reading fidelity -## Verified call graph (against `main`) +## The requirement reader is implemented twice -| Extractor | Recognition rule | Body capture | Metadata skip | `SHALL`/`MUST` check | Reached by | -|-----------|------------------|--------------|---------------|----------------------|------------| -| `Validator.extractRequirementText` (+ `countScenarios`) | canonical `REQUIREMENT_HEADER_REGEX` `/^###\s*Requirement:\s*(.+)$/i` | first substantial line only | **yes** (`/^\*\*[^*]+\*\*:/`) | `containsShallOrMust` → `/\b(SHALL\|MUST)\b/` | `validate ` | -| `MarkdownParser.parseRequirements` → `req.text` | **every** level-3 child | first non-empty line only | **no** | `RequirementSchema.refine` → `text.includes('SHALL')` | `validate `; `archive` (proposal + rebuilt-spec checks) | +| | spec reader: `MarkdownParser.parseRequirements` → `req.text` | delta reader: `Validator.extractRequirementText` / `countScenarios` | +|---|---|---| +| Recognition | every level-3 child of the section | canonical `REQUIREMENT_HEADER_REGEX` `/^###\s*Requirement:\s*(.+)$/i` | +| Body capture | first non-empty line | first substantial line | +| Skip `**metadata**:` | no | yes | +| Fenced code in body | not skipped | not skipped | +| Fenced `#### Scenario:` | not counted (parseSections fence-masks it) | **counted** (`/^####\s+/gm` is fence-unaware) | +| `SHALL`/`MUST` | `text.includes('SHALL')` (substring) | `/\b(SHALL\|MUST)\b/` (word boundary) | +| Reached by | `validate `, `archive` | `validate ` | -`ChangeParser extends MarkdownParser` and calls `this.parseRequirements`, so it is the same extractor — there is no third implementation. `specs-apply` (the archive rebuild) parses with `parseRequirementBlocksFromSection`, i.e. the canonical rule, so rebuilt main specs are already clean. +`ChangeParser extends MarkdownParser` and reuses `parseRequirements`, so there is no third reader. Every row where the two columns differ is a reproduced defect. -The table is the whole story: the two extractors differ in **four** columns (capture, metadata, recognition, predicate). Each difference is a reproduced bug. +## Reproductions (against `main`) -## Root causes (each reproduced) +- **#361** — `### Requirement: …` with `SHALL` on body line 2 → `validate ` `✗ must contain SHALL or MUST`; `validate ` `✗ requirements.0.text: …`. +- **#418** — metadata lines before a `MUST` description → `validate ` **valid**; `validate ` `✗`, `req.text` = `**ID**: REQ-FILE-001`. +- **#312** — fenced block (with `#` comments) before the prose line → both paths `✗`; `req.text` = `` ```bash ``. (Distinct from the already-fixed section-count manifestation.) +- **Fenced scenario** — requirement whose only `#### Scenario:` is inside a ` ```markdown ` block → `validate ` **valid** (counts the fenced scenario); `validate ` `✗ requirements.0.scenarios: must have at least one scenario`. The delta reader passes a malformed requirement. +- **#498** — stray `### Documentation Requirements` divider → `validate ` **valid**; `archive` prints non-blocking phantom `Proposal warnings in proposal.md`; `validate ` blocking `✗`. (Also: `show`/`view` count the divider as a requirement — `count=2` with `text='Documentation Notes'`.) -### 1. Single-line body capture (#361) - -Both extractors return the first line and stop. A keyword on body line 2 is never seen. - -``` -### Requirement: Realtime quest updates -Quest-related operations (creation, claiming, completion, approval, denial) -SHALL propagate to all family members' dashboards in real-time. -``` -- `validate --strict` → `✗ ADDED "Realtime quest updates" must contain SHALL or MUST` -- `validate --strict` → `✗ requirements.0.text: Requirement must contain SHALL or MUST keyword` - -### 2. Metadata before description, spec path only (#418) - -``` -### Requirement: File Serving -**ID**: REQ-FILE-001 -**Priority**: P1 - -The system MUST serve static files from the root directory. -``` -- `validate ` → **valid** (delta extractor skips metadata) -- `validate ` → `✗ requirements.0.text: ...`; captured `req.text` = `**ID**: REQ-FILE-001` - -The delta extractor was already taught to skip metadata; the spec extractor was not. Unifying them fixes #418 and prevents the next such drift. +## Approach -### 3. Fenced block before the prose corrupts text (#312) +### Part A — one shared, fence-aware extraction -``` -### Requirement: Config example then rule -```bash -# example config -export TOKEN=abc -``` -The system SHALL load the token from the environment. -``` -- `validate ` and `validate ` both → `✗ ... must contain SHALL or MUST`; captured `req.text` = `` ```bash `` +A single helper takes the requirement block's lines plus the fence mask and returns the full body: lines from after the header to the first `#### Scenario:` header found on a **non-fence-masked** line, skipping fence-masked lines and `**metadata**:` lines. A companion fence-aware scenario counter counts only non-fence-masked `####` headers. Both readers delegate to these. `SHALL`/`MUST` detection uses one predicate. -The body loop breaks on `line.trim().startsWith('#')` (the `#` comment inside the fence) without consulting `codeFenceLineMask`. The original #312 (requirement counts) is already fixed at the section level; this is a distinct, still-live manifestation in the body extractor. +Why the existing fence tests still pass: in `markdown-parser.test.ts:106`/`:139` the `SHALL` line is first and the fenced block follows, so skipping fenced lines leaves `text` exactly equal to the `SHALL` line — the asserted value. The breaking case (#312) is the inverse — fence *before* prose — which no test covers. -### 4. Divergent requirement recognition (#498) +### Part B — surface the #498 divergence (INFO, no recognition change) -`parseRequirements` treats every level-3 header as a requirement; the delta parser only canonical `### Requirement:`. A stray `### Documentation Requirements` divider → passes `validate `, but `archive` emits non-blocking phantom `Proposal warnings in proposal.md`, and `validate ` emits a blocking error. Archive does not hard-fail because `specs-apply` filters when rebuilding; the bug is the inconsistent signal. +`validateChangeDeltaSpecs` emits an INFO issue when an `## ADDED`/`## MODIFIED Requirements` section contains a level-3 header that does not match `REQUIREMENT_HEADER_REGEX` (so the delta reader will skip it). Under `--strict`, `valid = errors === 0 && warnings === 0` — **INFO is excluded**, so this never changes pass/fail; it only informs. This is the minimal change that makes `validate ` stop *silently* passing the #498 input. -## Approach +## Why recognition tightening is rejected -**One shared extractor.** A single helper takes the requirement block's lines plus the fence mask and returns the full body: all lines from after the header to the first `#### Scenario:` header detected on a **non-fenced** line, skipping fence-masked lines and `**metadata**:` lines. Both `extractRequirementText` and `parseRequirements` delegate to it. `SHALL`/`MUST` detection runs over the full body via one predicate (`containsShallOrMust`), replacing the substring/word-boundary split. +The obvious #498 fix is to make `parseRequirements` recognize only `### Requirement:` headers. It is rejected because **bare `### ` headers are a supported, tested requirement format**, not a convention violation: -This is fence-aware **skip-and-join**, which is why the existing fence tests still pass: in `markdown-parser.test.ts:106`/`:139` the `SHALL` line comes first and the fenced markdown block after it, so skipping fenced lines leaves `text` exactly equal to the `SHALL` line — the asserted value. The case that breaks today (#312) is the inverse: fence *before* the prose, which no test covers. +- `test/core/validation.test.ts` builds a spec whose requirements are `### The system SHALL provide secure user authentication` (no `Requirement:` prefix) and asserts `report.valid === true`. +- Bare headers also appear as valid requirements in `test/core/converters/json-converter.test.ts`, `test/core/archive.test.ts`, `test/commands/spec.test.ts`, and `test/core/parsers/markdown-parser.test.ts` (`:258`, `:310`, and the fixtures at `:14`/`:22`/`:55`/`:85`). -**Canonical recognition (Tier 2).** `parseRequirements` filters level-3 children through the exported `REQUIREMENT_HEADER_REGEX`. `## REMOVED`/`## RENAMED` are parsed by separate functions (`parseRemovedNames`, `parseRenamedPairs`) and are unaffected. +Tightening would reclassify all of these as non-requirements, breaking those tests and silently dropping requirements from any real spec that uses the bare style. The cost is not justified by #498, whose harm is a *confusing signal*, not data loss (the archive rebuild already filters to `### Requirement:` blocks, so rebuilt specs are correct regardless). Part B fixes the signal safely. If maintainers later decide to make `### Requirement:` mandatory, that belongs in its own change with a deprecation cycle and fixture migration. -## Existing test contract this change touches +## Safety: write path is independent of the reader -`test/core/parsers/markdown-parser.test.ts` (15 tests, all green on `main`) encodes the current — buggy — behavior in three places: +`src/core/specs-apply.ts` rebuilds specs during archive from `extractRequirementsSection` + `RequirementBlock.raw` (raw text split on the canonical header). It does not import or call `parseSpec`/`parseRequirements` and never reads `req.text`. Consequently Part A changes only what is *read/validated/displayed*; archived spec bytes are unchanged. (Note: this means `specs-apply` already uses the canonical `### Requirement:` rule — another reason recognition divergence is a reader-only concern.) -- `:331` *extract requirement text from first non-empty content line* — asserts `req.text` is only the first of two body lines. **Tier 1** changes `req.text` to the full body; this test is updated to assert the joined body. (Its premise is the #361 bug.) -- `:258` *handle nested sections correctly* — fixtures use bare `### The system SHALL …` headers and assert two requirements. **Tier 2** recognition makes bare headers non-requirements; updated to use `### Requirement: …`. -- `:310` *use requirement heading as fallback when no content is provided* — bare header. **Tier 2**; updated similarly. +## Read-only blast radius (no write path) -Tier 1 alone updates only `:331`. Tier 2 additionally updates `:258` and `:310`. No other tests are affected; the fence tests (`:106`, `:139`) are preserved. +Consumers of `parseSpec`/`req.text`: `view.ts`/`list.ts` (requirement **counts** — unchanged, since recognition is unchanged), `json-converter.ts` (JSON `text` — now the full body), `spec.ts` (display), `change-parser.ts:96` (delta descriptions `Add requirement: ${req.text}` — may span lines), and the `MAX_REQUIREMENT_TEXT_LENGTH` INFO (non-blocking). None affect archived content or pass/fail of valid specs. -## Alternatives considered +## Edge cases for tests -- **Patch each extractor separately.** Rejected — duplicated logic is exactly how they drifted (metadata skip in one, not the other). -- **Tier 2 as a separate opt-in lint instead of tightening recognition.** Keep `parseRequirements` permissive but add a warning when a level-3 header under Requirements is not `### Requirement:`. This avoids the behavior change and keeps bare-header support, but leaves `validate ` and `validate ` recognizing different requirement *sets*, so it does not fully close #498. Offered as the conservative option; the proposal recommends tightening because all in-repo specs and the convention already require `### Requirement:`. -- **Treat a stray level-3 header as a hard error everywhere.** Rejected — newly fails specs that pass `validate ` today; tightening-to-convention is the least-surprising consistent rule. +- Single-line requirement unchanged (text and count byte-for-byte). +- Metadata-only body still flags missing `SHALL`/`MUST`. +- Fenced `#### Scenario:` / `#`-comment lines do not corrupt text or inflate scenario count. +- LF/CRLF/CR via `normalizeContent`; `~~~`/length-≥3/leading-whitespace fences via existing `buildCodeFenceMask`. +- INFO note appears for a stray delta header but does not change `valid` (including `--strict`). -## Edge cases for tests +## Prior art -- Display vs. detection: `req.text` becomes the full body; assert single-line requirements are unchanged and the `MAX_REQUIREMENT_TEXT_LENGTH` INFO (non-blocking) is not spuriously tripped for legitimate multi-line bodies. -- Metadata-only body (no prose) still correctly flags missing `SHALL`/`MUST`. -- Fenced `#### Scenario:`-looking or `#`-comment lines in the body do not end capture or fabricate a scenario. -- LF/CRLF/CR via `normalizeContent`; `~~~` and length-≥3 / leading-whitespace fences via existing `buildCodeFenceMask`. -- Guard: zero non-conventional level-3 headers under Requirements in `openspec/specs/` — Tier 2 is behavior-preserving for all in-repo specs. +`findMainSpecStructureIssues` (`spec-structure.ts`) already flags a `### Requirement:` header *outside* the `## Requirements` section and delta headers inside a main spec. The Part B INFO note is complementary: it flags non-`Requirement:` headers *inside* a delta Requirements section, which that function does not cover. ## Out of scope: #559 -Deferred — transcript shows an unqualified `changes//...` path (missing `openspec/` prefix), not a demonstrated folder-vs-title mismatch. Recommend a separate change once intended behavior is confirmed. +Deferred — transcript shows an unqualified `changes//...` path (missing `openspec/` prefix), not a demonstrated folder-vs-title mismatch. diff --git a/openspec/changes/fix-spec-parser-fidelity/proposal.md b/openspec/changes/fix-spec-parser-fidelity/proposal.md index 7506335b1a..e203833e8d 100644 --- a/openspec/changes/fix-spec-parser-fidelity/proposal.md +++ b/openspec/changes/fix-spec-parser-fidelity/proposal.md @@ -1,52 +1,54 @@ ## Why -OpenSpec's promise is that the spec is the source of truth, and `validate`/`archive` are the gate that protects it. That gate is undermined by a fragmented requirement-parsing layer: there are **two** requirement extractors that disagree with each other and with the canonical delta parser. Each defect below was reproduced against `main` with the bundled CLI; outputs are quoted verbatim in `design.md`. +OpenSpec's promise is that the spec is the source of truth, and `validate`/`archive` are the gate that protects it. That gate is undermined by a fragmented requirement-parsing layer: the requirement **reader** is implemented twice — `MarkdownParser.parseRequirements` (used by `validate ` and `archive`) and `Validator.extractRequirementText` + `countScenarios` (used by `validate `) — and the two have drifted apart. Every defect below was reproduced against `main` with the bundled CLI; outputs are quoted in `design.md`. -The two extractors are `MarkdownParser.parseRequirements` (used by `validate `, and by `archive` via the proposal/rebuilt-spec checks) and `Validator.extractRequirementText` (used by `validate `). `ChangeParser extends MarkdownParser`, so it reuses `parseRequirements` — there is no third implementation. The two have already drifted: the delta extractor skips `**metadata**:` lines; the spec extractor does not. That drift is the bug surface. +The two readers differ in ways that are each a reproduced bug: -### 1. Wrapped `SHALL`/`MUST` is invisible (#361) — live, both paths +| | spec reader (`parseRequirements`) | delta reader (`extractRequirementText`/`countScenarios`) | +|---|---|---| +| Body capture | first line only | first line only | +| Skips `**metadata**:` lines | **no** | yes | +| Ignores fenced code in body | **no** | **no** | +| Counts fenced `#### Scenario:` | no (fence-masked) | **yes** | +| `SHALL`/`MUST` predicate | substring `includes('SHALL')` | word-boundary `\b(SHALL\|MUST)\b` | -Extraction captures only the **first** non-blank body line, then checks that line. When a requirement wraps and the keyword lands on line 2, both `validate --strict` and `validate --strict` falsely report `must contain SHALL or MUST`. +### Reproduced bugs -### 2. Metadata before the description breaks the spec path (#418) — live, asymmetric - -A requirement that places `**ID**:`/`**Priority**:` metadata lines before its prose validates fine as a **change** (the delta extractor skips metadata) but fails as a **spec**: `validate ` returns `req.text` = `**ID**: REQ-FILE-001` and reports `must contain SHALL or MUST`. This asymmetry is direct evidence for unifying the two extractors. - -### 3. A fenced block before the prose corrupts requirement text (#312) — live - -The original #312 (code-fence `#` lines counted as section headers, corrupting requirement counts) is **already fixed** by the `codeFenceLineMask` added since v0.15.0 — verified. But the body-extraction loop is still fence-unaware: it breaks on any line starting with `#`. When a requirement body opens with a fenced code block (e.g. a config example) before the `SHALL` line, the `#`-comment inside the fence ends extraction early and `req.text` becomes `` ```bash ``. Reproduced today on **both** paths. The same fence-unawareness would also truncate multi-line bodies once fix #1 lands, so the new extractor must be fence-aware from the start. - -### 4. `validate` and `archive` disagree on what a requirement is (#498) — live - -`validate ` recognizes requirements only by the canonical `### Requirement:` header; `parseRequirements` (used by `archive` and `validate `) treats **every** level-3 header as a requirement. A stray divider such as `### Documentation Requirements` is ignored by `validate ` but becomes a phantom requirement: `archive` prints non-blocking `Proposal warnings in proposal.md` for a "requirement" the author never wrote, and `validate ` reports it as a blocking error. (Archive still completes — `specs-apply` independently filters to `### Requirement:`, so the rebuilt spec is clean. The defect is the inconsistent, confusing signal.) +- **#361 — wrapped keyword invisible.** Both readers capture only the first body line, so a `SHALL`/`MUST` on line 2 fails both `validate ` and `validate `. +- **#418 — metadata before description, spec path only.** A requirement that opens with `**ID**:`/`**Priority**:` lines passes `validate ` (delta reader skips metadata) but fails `validate ` (`req.text` = `**ID**: REQ-FILE-001`). +- **#312 — fenced block before prose corrupts text.** The original count-corruption is already fixed by `codeFenceLineMask`, but the body loop is still fence-unaware: a fenced code block before the `SHALL` line makes `req.text` = `` ```bash `` on both paths today. +- **Fenced scenario counted as real (discovered during hardening, no open issue).** `countScenarios` matches `^####` with a fence-unaware regex, so a requirement whose only `#### Scenario:` lives inside a fenced example passes `validate ` — while the same content correctly fails `validate `. A malformed delta slips through the gate. +- **#498 — validate and archive disagree.** `validate ` recognizes requirements only by the canonical `### Requirement:` header; `parseRequirements` treats every level-3 header as a requirement. A stray divider like `### Documentation Requirements` is silently ignored by `validate ` but flagged by `archive` (non-blocking phantom warning) and `validate ` (blocking error). The author gets no signal at validate time. ## What Changes -The fixes fall into two tiers with different risk profiles. They are described separately so they can be reviewed — and if desired, merged — independently. +### Part A — unify the reader (fixes #361, #418, #312, fenced-scenario counting) + +One shared, fence-/metadata-/multi-line-aware extraction used by **both** readers, so they cannot drift again: -### Tier 1 — false-negative fixes (low risk): #361, #418, #312 +- Requirement-body capture spans every line from after the `### Requirement:` header to the first `#### Scenario:` header found on a **non-fenced** line, skipping fence-masked lines and `**metadata**:` lines; `SHALL`/`MUST` detection runs over the full body. +- Scenario counting ignores fence-masked `####` lines, so fenced examples never count as real scenarios. +- One normative-keyword predicate (`\b(SHALL|MUST)\b`) replaces the substring/word-boundary split. -- **One shared, multi-line, fence-aware, metadata-aware requirement-body extractor**, used by both `parseRequirements` and `extractRequirementText`, so they cannot drift again. It captures every body line from after the `### Requirement:` header to the first `#### Scenario:` header detected on a non-fenced line, skipping fence-masked lines and `**metadata**:` lines, and `SHALL`/`MUST` detection runs over the full captured body. -- **One normative-keyword predicate** everywhere (today the Zod schema uses substring `text.includes('SHALL')` while the delta path uses word-boundary `\b(SHALL|MUST)\b`). +Part A only corrects what is *detected*. It fixes false negatives (#361/#418/#312) and one false positive (fenced scenario), and does **not** change which headers count as requirements. -Tier 1 only widens what is *read*; it does not change which headers count as requirements. It fixes false negatives without rejecting anything that passes today. +### Part B — make the #498 divergence visible (safe, no recognition change) -### Tier 2 — recognition consistency (behavior change, flagged for decision): #498 +`validate ` emits an **INFO**-level note when an `## ADDED`/`## MODIFIED Requirements` section contains a level-3 header that is not a canonical `### Requirement:` header — i.e. one the delta reader will silently skip. This surfaces the stray-header problem at validate time instead of letting it appear only at archive, **without** changing recognition. INFO never fails validation (not even `--strict`), so no currently-passing change newly fails. -- **Unify requirement recognition on the canonical rule.** `parseRequirements` recognizes a level-3 header as a requirement only when it matches the same `REQUIREMENT_HEADER_REGEX` (`/^###\s*Requirement:\s*(.+)$/i`) used by the delta parser and `specs-apply`. This removes the phantom-requirement divergence in #498. +### Rejected: tightening recognition to `### Requirement:` only -Tier 2 is a deliberate **tightening to the documented convention**. The parser is currently permissive — it accepts bare `### ` headers as requirements — and that permissiveness is what lets stray dividers become phantoms. Tightening aligns all commands but changes behavior for specs that use non-conventional headers (see "Behavior changes" below). The alternative — a separate opt-in lint that flags stray level-3 headers without changing recognition — is described in `design.md`; we recommend the tightening but defer the call to maintainers. +The tempting #498 fix — make `parseRequirements` recognize only `### Requirement:` headers — is **rejected**. Bare `### ` headers (e.g. `### The system SHALL …`) are a **supported, widely-tested requirement format**: `test/core/validation.test.ts` asserts a bare-header spec is `valid`, and bare headers appear across `json-converter`, `archive`, and `spec` tests plus the `tmp-init` fixtures. Tightening would reclassify those as non-requirements and break a large swath of the suite (and likely real user specs). Surfacing the divergence (Part B) achieves consistency of *signal* without a breaking change to recognition. See `design.md` for the full analysis. -Out of scope (investigated, deferred): #559 (folder-name vs. title) — its transcript shows an unqualified `changes/...` path, not a proven name/title mismatch. See `design.md`. +Out of scope (investigated, deferred): #559 — its transcript shows an unqualified `changes/...` path, not a proven folder-vs-title mismatch. -## Behavior changes and existing-test impact +## Safety: the archive write path is unaffected -All 15 tests in `test/core/parsers/markdown-parser.test.ts` pass on `main`; this change updates three of them, each encoding behavior that is itself part of the bug: +`specs-apply` (the archive rebuild) reconstructs specs from raw `### Requirement:` blocks via `extractRequirementsSection` + `RequirementBlock.raw` — it never calls `parseSpec`/`parseRequirements` and never reads `req.text`. Therefore changing the reader (Part A) **cannot alter archived spec content**; it only changes what `validate`/`view`/`show` report. Verified by inspection of `src/core/specs-apply.ts`. -- **Tier 1** updates `should extract requirement text from first non-empty content line` (`:331`) — it asserts `req.text` equals only the first body line. After the fix, `req.text` is the full (metadata-/fence-skipped) body. The existing fence tests (`:106`, `:139`), which put `SHALL` first and the fence after, are **preserved** because fenced lines are skipped during capture. -- **Tier 2** updates `should handle nested sections correctly` (`:258`) and `should use requirement heading as fallback when no content is provided` (`:310`) — both rely on bare `### …` headers being treated as requirements. After the tightening, requirements must use `### Requirement:`. +## Existing-test impact -Migration for Tier 2: a changelog note that non-conventional `### ` requirement headers are no longer recognized; authors must use `### Requirement: ` (which the convention already mandates and all in-repo specs already follow — verified zero non-conventional level-3 headers exist under Requirements in `openspec/specs/`). +All 15 tests in `test/core/parsers/markdown-parser.test.ts` pass on `main`. Because recognition is unchanged, this proposal updates **one** test: `should extract requirement text from first non-empty content line` (`:331`), which asserts `req.text` is only the first body line — the #361 bug itself; it is updated to expect the full body. The fence tests (`:106`, `:139`) are preserved (skip-and-join keeps `SHALL`-first bodies intact). Bare-header tests (`:258`, `:310`) and `validation.test.ts`/`json-converter.test.ts` are **not** affected, because recognition does not change. ## Capabilities @@ -56,16 +58,14 @@ _None._ ### Modified Capabilities -- `cli-validate`: requirement-text extraction becomes multi-line, fence-aware, and metadata-aware; `SHALL`/`MUST` detection runs over the full body using a single predicate. -- `cli-archive`: archive's requirement-recognition matches `openspec validate` — no phantom-requirement warnings for non-`Requirement:` headers (Tier 2). -- `openspec-conventions`: only `### Requirement:`-prefixed level-3 headers identify requirements, applied consistently across all parsers (Tier 2). +- `cli-validate`: requirement-text extraction becomes multi-line, fence-aware, and metadata-aware; scenario counting becomes fence-aware; one normative-keyword predicate; an INFO note surfaces non-`Requirement:` headers in delta sections. ## Impact -- `src/core/parsers/markdown-parser.ts` — shared multi-line/fence/metadata-aware body extraction; canonical recognition (Tier 2). -- `src/core/validation/validator.ts` — `extractRequirementText` delegates to the shared helper; single keyword predicate. -- `src/core/parsers/requirement-blocks.ts` — export/reuse `REQUIREMENT_HEADER_REGEX` as the shared recognition predicate. +- `src/core/parsers/markdown-parser.ts` — shared multi-line/fence/metadata-aware body extraction. +- `src/core/validation/validator.ts` — `extractRequirementText` and `countScenarios` delegate to the shared, fence-aware helpers; INFO note for stray delta headers. +- `src/core/parsers/requirement-blocks.ts` — export the canonical `REQUIREMENT_HEADER_REGEX` for the INFO check. - `src/core/schemas/base.schema.ts` — align the `SHALL`/`MUST` refine with the shared predicate. -- `test/core/parsers/markdown-parser.test.ts`, `test/core/validation/*` — update the three tests above; add regression + parity tests. -- Affects all consumers of `parseRequirements`/`parseSpec` (`validate`, `view`, `show`, `archive`) consistently. -- Fixes #361, #418, #312. Tier 2 fixes #498. Related: #559 (deferred); hardens the *reader* the archive data-integrity work (#1112/#1246/#1277) relies on, without touching their merge/drop logic. Does not claim #1156 (covered by PR #1280). +- `test/core/parsers/markdown-parser.test.ts:331` updated; regression tests added. +- Read-only blast radius (display only, no write path): `view`/`list` requirement counts and `json-converter`/`spec` JSON `text` reflect the fuller body; `change-parser` delta descriptions built from `req.text` may span multiple lines; the `MAX_REQUIREMENT_TEXT_LENGTH` check is INFO (non-blocking). Requirement **counts** are unchanged (recognition unchanged). +- Fixes #361, #418, #312; surfaces #498. Related: #559 (deferred). Does not claim #1156 (PR #1280). Hardens the reader that #1112/#1246/#1277 rely on. diff --git a/openspec/changes/fix-spec-parser-fidelity/specs/cli-archive/spec.md b/openspec/changes/fix-spec-parser-fidelity/specs/cli-archive/spec.md deleted file mode 100644 index 309de02825..0000000000 --- a/openspec/changes/fix-spec-parser-fidelity/specs/cli-archive/spec.md +++ /dev/null @@ -1,15 +0,0 @@ -## ADDED Requirements - -### Requirement: Archive requirement-recognition SHALL match validate -The requirement validation performed during `openspec archive` SHALL recognize requirements using the same canonical `### Requirement:` rule as `openspec validate`. Archive SHALL NOT report requirement-recognition issues (for example phantom `must contain SHALL or MUST` or `must have at least one scenario` warnings) for level-3 headers that `openspec validate` does not treat as requirements. - -#### Scenario: Stray non-requirement header produces no phantom warning at archive -- **GIVEN** a change whose spec deltas pass `openspec validate --strict` and whose Requirements/ADDED section contains a stray level-3 header that is not a `### Requirement:` header -- **WHEN** running `openspec archive ` -- **THEN** the `Proposal warnings in proposal.md` output SHALL NOT include phantom requirement warnings derived from the stray header -- **AND** the archive SHALL succeed as it does today - -#### Scenario: Genuinely invalid requirement fails consistently across commands -- **GIVEN** a change whose spec contains a real `### Requirement:` block with no `SHALL`/`MUST` and no scenario -- **WHEN** running `openspec validate --strict` and `openspec archive ` -- **THEN** both commands SHALL report the same requirement as invalid using consistent messaging diff --git a/openspec/changes/fix-spec-parser-fidelity/specs/cli-validate/spec.md b/openspec/changes/fix-spec-parser-fidelity/specs/cli-validate/spec.md index b765774083..26eb4cfde9 100644 --- a/openspec/changes/fix-spec-parser-fidelity/specs/cli-validate/spec.md +++ b/openspec/changes/fix-spec-parser-fidelity/specs/cli-validate/spec.md @@ -1,35 +1,54 @@ ## ADDED Requirements ### Requirement: Requirement bodies SHALL be parsed in full for normative keywords -The validator SHALL detect `SHALL`/`MUST` across the entire requirement body, not only the first body line. Requirement-text extraction SHALL capture every body line from after the `### Requirement:` header up to the first `#### Scenario:` header, skipping blank lines, `**metadata**:` lines, and lines inside fenced code blocks. Normative-keyword detection SHALL run over the full captured body. The change-delta path and the main-spec path SHALL use the same extraction logic so they cannot diverge. +The validator SHALL detect `SHALL`/`MUST` across the entire requirement body, not only the first body line. Requirement-text extraction SHALL capture every body line from after the `### Requirement:` header up to the first `#### Scenario:` header, skipping blank lines, `**metadata**:` lines, and lines inside fenced code blocks. Detection SHALL run over the full captured body. The change-delta reader and the main-spec reader SHALL share this extraction so they cannot diverge. #### Scenario: Normative keyword on the second wrapped line (change and spec) -- **GIVEN** a requirement whose descriptive text wraps across two lines with `SHALL` on the second line +- **GIVEN** a requirement whose text wraps across two lines with `SHALL` on the second line - **WHEN** running `openspec validate --strict` for both a change delta and a main spec -- **THEN** both SHALL recognize the keyword and SHALL NOT report a missing-`SHALL`/`MUST` error +- **THEN** both SHALL detect the keyword and SHALL NOT report a missing-`SHALL`/`MUST` error #### Scenario: Metadata fields precede the description -- **GIVEN** a requirement whose body begins with `**ID**:`/`**Priority**:` metadata lines before a `MUST` description +- **GIVEN** a requirement whose body begins with `**ID**:`/`**Priority**:` lines before a `MUST` description - **WHEN** running `openspec validate --strict` -- **THEN** validation SHALL skip the metadata lines, detect `MUST` in the description, and pass — matching the existing behavior of `openspec validate ` +- **THEN** validation SHALL skip the metadata lines, detect `MUST`, and pass — matching `openspec validate ` #### Scenario: Single-line requirement is unaffected - **GIVEN** a requirement whose `SHALL` statement is on a single body line - **WHEN** running `openspec validate --strict` -- **THEN** validation behavior and messages SHALL be unchanged from before this change +- **THEN** validation behavior, messages, and displayed text SHALL be unchanged from before this change -### Requirement: Fenced code blocks SHALL NOT corrupt requirement-text extraction -The validator and markdown parser SHALL ignore lines inside fenced code blocks (` ``` ` or `~~~`) when extracting requirement body text and when locating the first `#### Scenario:` boundary. A fenced code block appearing before the prose line of a requirement SHALL NOT cause the fence marker or a `#`-comment inside it to be taken as the requirement text. +### Requirement: Fenced code blocks SHALL NOT corrupt extraction or scenario counting +The validator and markdown parser SHALL ignore lines inside fenced code blocks (` ``` ` or `~~~`) when extracting requirement body text, when locating the first `#### Scenario:` boundary, and when counting scenarios. A fenced block before the prose line SHALL NOT make the fence marker the requirement text, and a `#### Scenario:` inside a fenced block SHALL NOT count as a real scenario. #### Scenario: Fenced block before the prose line - **GIVEN** a requirement whose body opens with a fenced code block containing `#`-comment lines, followed by the `SHALL` prose line - **WHEN** the spec or change is validated -- **THEN** the captured requirement text SHALL be the prose line (not the fence marker), and validation SHALL pass +- **THEN** the captured requirement text SHALL be the prose line (not the fence marker) and validation SHALL pass -### Requirement: A single normative-keyword predicate SHALL be used across validation paths -All `SHALL`/`MUST` detection SHALL use one predicate that matches `SHALL` or `MUST` as whole words, so the delta-spec validation path and the schema-based validation path accept and reject identical text. +#### Scenario: Fenced scenario is not a real scenario +- **GIVEN** a requirement whose only `#### Scenario:` appears inside a fenced code example, with no real scenario +- **WHEN** running `openspec validate --strict` +- **THEN** validation SHALL report the requirement as missing a scenario — the same result as `openspec validate ` -#### Scenario: Keyword detection agrees across paths +### Requirement: A single normative-keyword predicate SHALL be used across readers +All `SHALL`/`MUST` detection SHALL use one predicate that matches `SHALL` or `MUST` as whole words, so the change-delta reader and the schema-based reader accept and reject identical text. + +#### Scenario: Keyword detection agrees across readers - **GIVEN** identical requirement body text validated once as a change delta and once as a main spec - **WHEN** running `openspec validate` on each - **THEN** both SHALL reach the same conclusion about whether the body contains a normative keyword + +### Requirement: Non-canonical headers in delta sections SHALL be surfaced without changing recognition +When an `## ADDED`/`## MODIFIED Requirements` section in a change delta contains a level-3 header that is not a canonical `### Requirement:` header, `openspec validate ` SHALL emit an INFO-level note identifying it, because the delta reader will otherwise skip it silently. This note SHALL NOT change which headers are recognized as requirements, and SHALL NOT change the `valid` result — including under `--strict`. + +#### Scenario: Stray divider header is reported, not silently skipped +- **GIVEN** a delta whose `## ADDED Requirements` section contains `### Documentation Requirements` followed by a valid `### Requirement: …` block +- **WHEN** running `openspec validate --strict` +- **THEN** validation SHALL emit an INFO note naming the stray `### Documentation Requirements` header +- **AND** the `valid` result SHALL be unchanged from current behavior (the INFO does not cause failure) + +#### Scenario: Bare requirement headers in main specs remain supported +- **GIVEN** a main spec whose requirements use bare `### ` headers without the `Requirement:` prefix +- **WHEN** running `openspec validate --strict` +- **THEN** those headers SHALL continue to be recognized as requirements exactly as before this change diff --git a/openspec/changes/fix-spec-parser-fidelity/specs/openspec-conventions/spec.md b/openspec/changes/fix-spec-parser-fidelity/specs/openspec-conventions/spec.md deleted file mode 100644 index 308c27d0f8..0000000000 --- a/openspec/changes/fix-spec-parser-fidelity/specs/openspec-conventions/spec.md +++ /dev/null @@ -1,25 +0,0 @@ -## ADDED Requirements - -### Requirement: Requirement recognition SHALL use the canonical header rule consistently -All parsers — change-delta validation, main-spec validation, and the archive spec rebuild — SHALL recognize a level-3 header as a requirement only when it matches the canonical, case-insensitive rule `### Requirement: `. Other level-3 headers within a Requirements/ADDED/MODIFIED section SHALL NOT be treated as requirements. This tightens the previously permissive main-spec parser (which accepted any `### ` header) to match the rule already enforced by the delta parser, the convention, and `specs-apply`. - -#### Scenario: Stray level-3 divider is not a requirement -- **GIVEN** a Requirements section containing `### Documentation Requirements` followed by a valid `### Requirement: AI Application Documentation` block -- **WHEN** the spec is parsed by any command -- **THEN** only `### Requirement: AI Application Documentation` SHALL be counted as a requirement -- **AND** `### Documentation Requirements` SHALL NOT produce a phantom requirement that fails `SHALL`/scenario validation - -#### Scenario: Recognition is consistent across commands -- **WHEN** the same spec content is processed by `openspec validate `, `openspec validate `, and the archive spec rebuild -- **THEN** all SHALL identify the same set of requirements - -#### Scenario: Non-conventional bare headers require migration -- **GIVEN** a legacy spec that used a bare `### ` header (without the `Requirement:` prefix) to declare a requirement -- **WHEN** the spec is parsed after this change -- **THEN** that header SHALL no longer be recognized as a requirement -- **AND** the change SHALL ship a changelog note instructing authors to use `### Requirement: ` as the convention already requires - -#### Scenario: REMOVED and RENAMED sections are unaffected -- **GIVEN** a change with `## REMOVED Requirements` or `## RENAMED Requirements` sections using their bullet-list or `FROM:`/`TO:` syntax -- **WHEN** the change is parsed -- **THEN** those requirements SHALL continue to be recognized by their existing dedicated parsing, independent of the level-3 header rule diff --git a/openspec/changes/fix-spec-parser-fidelity/tasks.md b/openspec/changes/fix-spec-parser-fidelity/tasks.md index 36c7a6a35b..300f8d01d6 100644 --- a/openspec/changes/fix-spec-parser-fidelity/tasks.md +++ b/openspec/changes/fix-spec-parser-fidelity/tasks.md @@ -1,36 +1,34 @@ -## 1. Tier 1 — shared body extraction (#361, #418, #312) +## 1. Part A — shared, fence-aware extraction (#361, #418, #312, fenced-scenario) -- [ ] 1.1 Add a shared `extractRequirementBody(lines, fenceMask, startIndex)` helper in `src/core/parsers/` that returns the full body: all lines after the header up to the first `#### Scenario:` header on a non-fence-masked line, skipping fence-masked lines and `**metadata**:` lines. -- [ ] 1.2 Rewrite `MarkdownParser.parseRequirements` to use the helper (replacing first-line-only logic), consulting `codeFenceLineMask` so a `#` inside a fence no longer truncates the body, and skipping metadata lines (parity with the delta path). -- [ ] 1.3 Rewrite `Validator.extractRequirementText` to delegate to the same helper, returning the full body. -- [ ] 1.4 Run `SHALL`/`MUST` detection over the full body in both paths. +- [ ] 1.1 Add a shared `extractRequirementBody(lines, fenceMask, startIndex)` helper in `src/core/parsers/` returning the full body: lines after the header up to the first `#### Scenario:` on a non-fence-masked line, skipping fence-masked and `**metadata**:` lines. +- [ ] 1.2 Add a fence-aware scenario counter (count only non-fence-masked `####` headers). +- [ ] 1.3 Rewrite `MarkdownParser.parseRequirements` to use the body helper (replacing first-line logic) and consult `codeFenceLineMask`. +- [ ] 1.4 Rewrite `Validator.extractRequirementText` to delegate to the body helper, and `countScenarios` to the fence-aware counter. +- [ ] 1.5 Run `SHALL`/`MUST` detection over the full body in both paths. -## 2. Tier 1 — single normative-keyword predicate +## 2. Part A — single normative-keyword predicate -- [ ] 2.1 Replace the substring check in `src/core/schemas/base.schema.ts` (`text.includes('SHALL')`) with the shared `containsShallOrMust` (`/\b(SHALL|MUST)\b/`) so the Zod refine and the delta path agree. +- [ ] 2.1 Replace the substring check in `src/core/schemas/base.schema.ts` (`text.includes('SHALL')`) with the shared `containsShallOrMust` (`/\b(SHALL|MUST)\b/`). -## 3. Tier 2 — canonical requirement recognition (#498) +## 3. Part B — surface the #498 divergence (INFO, no recognition change) -- [ ] 3.1 Export `REQUIREMENT_HEADER_REGEX` from `requirement-blocks.ts` (or a shared `isRequirementHeader` predicate). -- [ ] 3.2 In `MarkdownParser.parseRequirements`, recognize a level-3 child as a requirement only when it matches that canonical predicate; confirm `## REMOVED`/`## RENAMED` parsing is unaffected. -- [ ] 3.3 Confirm `validate `, `validate `, and `archive` recognize the same requirement set (no phantom-requirement warnings). +- [ ] 3.1 Export `REQUIREMENT_HEADER_REGEX` from `requirement-blocks.ts`. +- [ ] 3.2 In `validateChangeDeltaSpecs`, emit an INFO issue when an `## ADDED`/`## MODIFIED Requirements` section contains a level-3 header that does not match the canonical regex. Do **not** change recognition. +- [ ] 3.3 Confirm INFO does not affect `valid` under `--strict` (`valid = errors === 0 && warnings === 0`). -## 4. Update existing tests (encode the corrected behavior) +## 4. Update the one affected existing test -- [ ] 4.1 `markdown-parser.test.ts:331` (*first non-empty content line*) → assert `req.text` is the full joined body (Tier 1). -- [ ] 4.2 `markdown-parser.test.ts:258` (*nested sections*) → use `### Requirement: …` headers (Tier 2). -- [ ] 4.3 `markdown-parser.test.ts:310` (*heading fallback*) → use `### Requirement: …` header (Tier 2). -- [ ] 4.4 Confirm the fence tests (`:106`, `:139`) still pass unchanged. +- [ ] 4.1 `markdown-parser.test.ts:331` (*first non-empty content line*) → assert `req.text` is the full joined body. Confirm `:106`/`:139` (fence) and `:258`/`:310` (bare-header) tests still pass unchanged. -## 5. Regression + parity tests +## 5. Regression tests -- [ ] 5.1 (#361, both paths) `SHALL` wrapped onto body line 2 passes `validate ` and `validate `. -- [ ] 5.2 (#418, spec path) metadata lines before the prose pass `validate `; delta path stays green. -- [ ] 5.3 (#312) fenced code block before the prose line captures the real body; requirement count and scenarios are correct. -- [ ] 5.4 (#498) a stray `### Documentation Requirements` divider yields no phantom-requirement issue from `validate `, `validate `, or `archive`. -- [ ] 5.5 Parity: the three commands agree (same recognized requirements, same pass/fail) over the fixtures above. -- [ ] 5.6 Guard: legitimate single-line requirements unchanged; existing in-repo specs still validate; LF/CRLF covered. +- [ ] 5.1 (#361) `SHALL` wrapped onto body line 2 passes `validate ` and `validate `. +- [ ] 5.2 (#418) metadata lines before the prose pass `validate `; delta path stays green. +- [ ] 5.3 (#312) fenced block before the prose line captures the real body and passes. +- [ ] 5.4 (fenced scenario) a requirement whose only `#### Scenario:` is inside a fence FAILS `validate ` (parity with `validate `). +- [ ] 5.5 (#498) a stray `### Documentation Requirements` divider in a delta yields an INFO note from `validate ` and does not change `valid` (including `--strict`). +- [ ] 5.6 Guard: single-line requirements unchanged; bare-header specs still valid; LF/CRLF covered. ## 6. Release -- [ ] 6.1 Add a changeset: Fixes #361, #418, #312; Tier 2 fixes #498. Include the Tier 2 migration note (non-conventional `### ` requirement headers are no longer recognized; use `### Requirement: `). +- [ ] 6.1 Add a changeset: Fixes #361, #418, #312; surfaces #498. Note the read-only display changes (fuller `req.text` in JSON/descriptions); no archived-content change. From 4fb865853f38242818881075b5ddb4bbefa7ae4f Mon Sep 17 00:00:00 2001 From: Clay Good Date: Tue, 30 Jun 2026 16:10:44 -0500 Subject: [PATCH 5/8] fix(parser): unify the requirement reader, fence/metadata/multi-line aware (#361, #418, #312); surface #498 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The requirement reader was implemented twice — MarkdownParser.parseRequirements (validate /archive) and Validator.extractRequirementText/countScenarios (validate ) — and the two had drifted. Both now delegate to one shared, fence-/metadata-/multi-line-aware extraction in parsers/requirement-text.ts so they cannot diverge again. Part A — unify the reader: - Capture the full requirement body up to the first non-fenced `#### Scenario:`, skipping blank, `**metadata**:`, and fenced-code lines; run SHALL/MUST detection over the whole body. Fixes a wrapped keyword being dropped (#361), metadata before the description failing validate (#418), and a fenced block before the prose line becoming the requirement text (#312). - Count only non-fenced `#### ` headers, so a `#### Scenario:` inside a fenced example no longer counts as a real scenario in validate (parity with validate ). - One whole-word `\b(SHALL|MUST)\b` predicate (containsShallOrMust) shared by the validator and base.schema, replacing the substring/word-boundary split. - Extract buildCodeFenceMask into the shared module; MarkdownParser and ChangeParser import it (single fence implementation). Part B — surface #498 safely: - validate emits an INFO note when an ADDED/MODIFIED Requirements section contains a non-`### Requirement:` level-3 header (one the delta reader silently skips). INFO never changes the valid result, including under --strict, so nothing newly fails. Recognition is unchanged: bare `### ` headers remain a supported requirement format. Write path is unaffected: specs-apply rebuilds from raw `### Requirement:` blocks, never req.text, so archived content cannot change. Displayed text in JSON output and delta descriptions now reflects the full body. Tests: markdown-parser.test.ts:331 updated to expect the full body; regression tests added for #361/#418/#312, the fenced scenario, the #498 INFO note, a single-line guard, and CRLF. Changeset added (patch). tasks.md completed. Co-Authored-By: Claude Opus 4.8 (1M context) --- .changeset/spec-parser-reading-fidelity.md | 16 ++ .../changes/fix-spec-parser-fidelity/tasks.md | 34 ++-- src/core/parsers/change-parser.ts | 3 +- src/core/parsers/markdown-parser.ts | 89 +-------- src/core/parsers/requirement-blocks.ts | 47 ++++- src/core/parsers/requirement-text.ts | 123 ++++++++++++ src/core/schemas/base.schema.ts | 3 +- src/core/validation/validator.ts | 59 +++--- test/core/parsers/markdown-parser.test.ts | 140 +++++++++++++- test/core/validation.test.ts | 177 ++++++++++++++++++ 10 files changed, 560 insertions(+), 131 deletions(-) create mode 100644 .changeset/spec-parser-reading-fidelity.md create mode 100644 src/core/parsers/requirement-text.ts diff --git a/.changeset/spec-parser-reading-fidelity.md b/.changeset/spec-parser-reading-fidelity.md new file mode 100644 index 0000000000..aaa0a347e2 --- /dev/null +++ b/.changeset/spec-parser-reading-fidelity.md @@ -0,0 +1,16 @@ +--- +"@fission-ai/openspec": patch +--- + +### Bug Fixes + +- **Requirement reading fidelity** — The requirement reader used by `validate `, `validate `, and `archive` is now unified into one fence-, metadata-, and multi-line-aware extraction, so the change-delta path and the main-spec path can no longer disagree: + - A `SHALL`/`MUST` keyword that wraps onto a later body line is detected instead of dropped (#361). + - Metadata lines (`**ID**:`, `**Priority**:`) before the description are skipped on the spec path, matching the change path (#418). + - A fenced code block before the prose line no longer becomes the requirement text (#312). + - A `#### Scenario:` inside a fenced example no longer counts as a real scenario in `validate `, matching `validate `. + - `SHALL`/`MUST` detection uses one whole-word predicate across all readers. + + Displayed requirement text (e.g. in JSON output and delta descriptions) now reflects the full requirement body rather than only its first line. Archived spec content is unchanged — the archive rebuild reads raw `### Requirement:` blocks, not the parsed text. + +- **Surface non-canonical delta headers** — `validate ` now emits an INFO note when an `## ADDED`/`## MODIFIED Requirements` section contains a level-3 header that is not a canonical `### Requirement:` header (one the delta reader silently skips, such as a stray `### Documentation Requirements` divider). The note never changes the `valid` result, including under `--strict` (#498). diff --git a/openspec/changes/fix-spec-parser-fidelity/tasks.md b/openspec/changes/fix-spec-parser-fidelity/tasks.md index 300f8d01d6..fe1323c83f 100644 --- a/openspec/changes/fix-spec-parser-fidelity/tasks.md +++ b/openspec/changes/fix-spec-parser-fidelity/tasks.md @@ -1,34 +1,34 @@ ## 1. Part A — shared, fence-aware extraction (#361, #418, #312, fenced-scenario) -- [ ] 1.1 Add a shared `extractRequirementBody(lines, fenceMask, startIndex)` helper in `src/core/parsers/` returning the full body: lines after the header up to the first `#### Scenario:` on a non-fence-masked line, skipping fence-masked and `**metadata**:` lines. -- [ ] 1.2 Add a fence-aware scenario counter (count only non-fence-masked `####` headers). -- [ ] 1.3 Rewrite `MarkdownParser.parseRequirements` to use the body helper (replacing first-line logic) and consult `codeFenceLineMask`. -- [ ] 1.4 Rewrite `Validator.extractRequirementText` to delegate to the body helper, and `countScenarios` to the fence-aware counter. -- [ ] 1.5 Run `SHALL`/`MUST` detection over the full body in both paths. +- [x] 1.1 Add a shared `extractRequirementBody(lines, fenceMask, startIndex)` helper in `src/core/parsers/` returning the full body: lines after the header up to the first `#### Scenario:` on a non-fence-masked line, skipping fence-masked and `**metadata**:` lines. +- [x] 1.2 Add a fence-aware scenario counter (count only non-fence-masked `####` headers). +- [x] 1.3 Rewrite `MarkdownParser.parseRequirements` to use the body helper (replacing first-line logic) and consult `codeFenceLineMask`. +- [x] 1.4 Rewrite `Validator.extractRequirementText` to delegate to the body helper, and `countScenarios` to the fence-aware counter. +- [x] 1.5 Run `SHALL`/`MUST` detection over the full body in both paths. ## 2. Part A — single normative-keyword predicate -- [ ] 2.1 Replace the substring check in `src/core/schemas/base.schema.ts` (`text.includes('SHALL')`) with the shared `containsShallOrMust` (`/\b(SHALL|MUST)\b/`). +- [x] 2.1 Replace the substring check in `src/core/schemas/base.schema.ts` (`text.includes('SHALL')`) with the shared `containsShallOrMust` (`/\b(SHALL|MUST)\b/`). ## 3. Part B — surface the #498 divergence (INFO, no recognition change) -- [ ] 3.1 Export `REQUIREMENT_HEADER_REGEX` from `requirement-blocks.ts`. -- [ ] 3.2 In `validateChangeDeltaSpecs`, emit an INFO issue when an `## ADDED`/`## MODIFIED Requirements` section contains a level-3 header that does not match the canonical regex. Do **not** change recognition. -- [ ] 3.3 Confirm INFO does not affect `valid` under `--strict` (`valid = errors === 0 && warnings === 0`). +- [x] 3.1 Export `REQUIREMENT_HEADER_REGEX` from `requirement-blocks.ts`. +- [x] 3.2 In `validateChangeDeltaSpecs`, emit an INFO issue when an `## ADDED`/`## MODIFIED Requirements` section contains a level-3 header that does not match the canonical regex. Do **not** change recognition. +- [x] 3.3 Confirm INFO does not affect `valid` under `--strict` (`valid = errors === 0 && warnings === 0`). ## 4. Update the one affected existing test -- [ ] 4.1 `markdown-parser.test.ts:331` (*first non-empty content line*) → assert `req.text` is the full joined body. Confirm `:106`/`:139` (fence) and `:258`/`:310` (bare-header) tests still pass unchanged. +- [x] 4.1 `markdown-parser.test.ts:331` (*first non-empty content line*) → assert `req.text` is the full joined body. Confirm `:106`/`:139` (fence) and `:258`/`:310` (bare-header) tests still pass unchanged. ## 5. Regression tests -- [ ] 5.1 (#361) `SHALL` wrapped onto body line 2 passes `validate ` and `validate `. -- [ ] 5.2 (#418) metadata lines before the prose pass `validate `; delta path stays green. -- [ ] 5.3 (#312) fenced block before the prose line captures the real body and passes. -- [ ] 5.4 (fenced scenario) a requirement whose only `#### Scenario:` is inside a fence FAILS `validate ` (parity with `validate `). -- [ ] 5.5 (#498) a stray `### Documentation Requirements` divider in a delta yields an INFO note from `validate ` and does not change `valid` (including `--strict`). -- [ ] 5.6 Guard: single-line requirements unchanged; bare-header specs still valid; LF/CRLF covered. +- [x] 5.1 (#361) `SHALL` wrapped onto body line 2 passes `validate ` and `validate `. +- [x] 5.2 (#418) metadata lines before the prose pass `validate `; delta path stays green. +- [x] 5.3 (#312) fenced block before the prose line captures the real body and passes. +- [x] 5.4 (fenced scenario) a requirement whose only `#### Scenario:` is inside a fence FAILS `validate ` (parity with `validate `). +- [x] 5.5 (#498) a stray `### Documentation Requirements` divider in a delta yields an INFO note from `validate ` and does not change `valid` (including `--strict`). +- [x] 5.6 Guard: single-line requirements unchanged; bare-header specs still valid; LF/CRLF covered. ## 6. Release -- [ ] 6.1 Add a changeset: Fixes #361, #418, #312; surfaces #498. Note the read-only display changes (fuller `req.text` in JSON/descriptions); no archived-content change. +- [x] 6.1 Add a changeset: Fixes #361, #418, #312; surfaces #498. Note the read-only display changes (fuller `req.text` in JSON/descriptions); no archived-content change. diff --git a/src/core/parsers/change-parser.ts b/src/core/parsers/change-parser.ts index a2c364b70c..2473d16ace 100644 --- a/src/core/parsers/change-parser.ts +++ b/src/core/parsers/change-parser.ts @@ -1,4 +1,5 @@ import { MarkdownParser, Section } from './markdown-parser.js'; +import { buildCodeFenceMask } from './requirement-text.js'; import { Change, Delta, DeltaOperation, Requirement } from '../schemas/index.js'; import path from 'path'; import { promises as fs } from 'fs'; @@ -179,7 +180,7 @@ export class ChangeParser extends MarkdownParser { private parseSectionsFromContent(content: string): Section[] { const normalizedContent = ChangeParser.normalizeContent(content); const lines = normalizedContent.split('\n'); - const codeFenceLineMask = ChangeParser.buildCodeFenceMask(lines); + const codeFenceLineMask = buildCodeFenceMask(lines); const sections: Section[] = []; const stack: Section[] = []; diff --git a/src/core/parsers/markdown-parser.ts b/src/core/parsers/markdown-parser.ts index abad78df22..2355368ae8 100644 --- a/src/core/parsers/markdown-parser.ts +++ b/src/core/parsers/markdown-parser.ts @@ -1,4 +1,5 @@ import { Spec, Change, Requirement, Scenario, Delta, DeltaOperation } from '../schemas/index.js'; +import { buildCodeFenceMask, extractRequirementBody } from './requirement-text.js'; export interface Section { level: number; @@ -15,7 +16,7 @@ export class MarkdownParser { constructor(content: string) { const normalized = MarkdownParser.normalizeContent(content); this.lines = normalized.split('\n'); - this.codeFenceLineMask = MarkdownParser.buildCodeFenceMask(this.lines); + this.codeFenceLineMask = buildCodeFenceMask(this.lines); this.currentLine = 0; } @@ -23,54 +24,6 @@ export class MarkdownParser { return content.replace(/\r\n?/g, '\n'); } - protected static buildCodeFenceMask(lines: string[]): boolean[] { - const mask = new Array(lines.length).fill(false); - let activeFence: { marker: '`' | '~'; length: number } | null = null; - - for (let i = 0; i < lines.length; i++) { - const fence = MarkdownParser.getFenceMarker(lines[i]); - - if (!activeFence) { - if (fence) { - activeFence = fence; - mask[i] = true; - } - continue; - } - - mask[i] = true; - if (MarkdownParser.isClosingFence(lines[i], activeFence)) { - activeFence = null; - } - } - - return mask; - } - - private static getFenceMarker(line: string): { marker: '`' | '~'; length: number } | null { - const fenceMatch = line.match(/^\s*(`{3,}|~{3,})/); - if (!fenceMatch) { - return null; - } - - return { - marker: fenceMatch[1][0] as '`' | '~', - length: fenceMatch[1].length, - }; - } - - private static isClosingFence( - line: string, - activeFence: { marker: '`' | '~'; length: number } - ): boolean { - const fenceMatch = line.match(/^\s*(`{3,}|~{3,})\s*$/); - return Boolean( - fenceMatch && - fenceMatch[1][0] === activeFence.marker && - fenceMatch[1].length >= activeFence.length - ); - } - parseSpec(name: string): Spec { const sections = this.parseSections(); const purpose = this.findSection(sections, 'Purpose')?.content || ''; @@ -197,43 +150,21 @@ export class MarkdownParser { protected parseRequirements(section: Section): Requirement[] { const requirements: Requirement[] = []; - + for (const child of section.children) { - // Extract requirement text from first non-empty content line, fall back to heading - let text = child.title; - - // Get content before any child sections (scenarios) - if (child.content.trim()) { - // Split content into lines and find content before any child headers - const lines = child.content.split('\n'); - const contentBeforeChildren: string[] = []; - - for (const line of lines) { - // Stop at child headers (scenarios start with ####) - if (line.trim().startsWith('#')) { - break; - } - contentBeforeChildren.push(line); - } - - // Find first non-empty line - const directContent = contentBeforeChildren.join('\n').trim(); - if (directContent) { - const firstLine = directContent.split('\n').find(l => l.trim()); - if (firstLine) { - text = firstLine.trim(); - } - } - } - + // Capture the full requirement body (multi-line, fence- and metadata-aware) + // via the shared reader, falling back to the heading when there is no body. + const body = extractRequirementBody(child.content.split('\n')); + const text = body || child.title; + const scenarios = this.parseScenarios(child); - + requirements.push({ text, scenarios, }); } - + return requirements; } diff --git a/src/core/parsers/requirement-blocks.ts b/src/core/parsers/requirement-blocks.ts index afc55f8914..73a64aca5d 100644 --- a/src/core/parsers/requirement-blocks.ts +++ b/src/core/parsers/requirement-blocks.ts @@ -12,11 +12,14 @@ export interface RequirementsSectionParts { after: string; } +import { buildCodeFenceMask } from './requirement-text.js'; + export function normalizeRequirementName(name: string): string { return name.trim(); } -const REQUIREMENT_HEADER_REGEX = /^###\s*Requirement:\s*(.+)\s*$/i; +/** The canonical requirement header the delta reader recognizes. */ +export const REQUIREMENT_HEADER_REGEX = /^###\s*Requirement:\s*(.+)\s*$/i; /** * Extracts the Requirements section from a spec file and parses requirement blocks. @@ -212,6 +215,48 @@ function parseRemovedNames(sectionBody: string): string[] { return names; } +/** + * Find level-3 headers inside `## ADDED`/`## MODIFIED Requirements` sections + * that are NOT canonical `### Requirement:` headers — the ones the delta reader + * silently skips. Fence-aware: headers inside fenced examples are ignored. Used + * to surface (as INFO, not a recognition change) headers like a stray + * `### Documentation Requirements` divider that `validate ` would + * otherwise pass without comment. + */ +export function findNonRequirementLevel3Headers( + content: string +): Array<{ header: string; section: string; line: number }> { + const normalized = normalizeLineEndings(content); + const lines = normalized.split('\n'); + const mask = buildCodeFenceMask(lines); + const results: Array<{ header: string; section: string; line: number }> = []; + + let currentSection: string | null = null; + for (let i = 0; i < lines.length; i++) { + if (mask[i]) continue; // inside a fenced code block + const line = lines[i]; + + const h2 = line.match(/^##\s+(.+?)\s*$/); + if (h2) { + const title = h2[1].trim().toLowerCase(); + currentSection = + title === 'added requirements' || title === 'modified requirements' + ? h2[1].trim() + : null; + continue; + } + + if (!currentSection) continue; + + const h3 = line.match(/^###\s+(.+?)\s*$/); + if (h3 && !REQUIREMENT_HEADER_REGEX.test(line)) { + results.push({ header: h3[1].trim(), section: currentSection, line: i + 1 }); + } + } + + return results; +} + function parseRenamedPairs(sectionBody: string): Array<{ from: string; to: string }> { if (!sectionBody) return []; const pairs: Array<{ from: string; to: string }> = []; diff --git a/src/core/parsers/requirement-text.ts b/src/core/parsers/requirement-text.ts new file mode 100644 index 0000000000..4bfeb65e23 --- /dev/null +++ b/src/core/parsers/requirement-text.ts @@ -0,0 +1,123 @@ +/** + * Shared, fence-aware requirement-reading helpers. + * + * The requirement reader used to be implemented twice — once for main specs + * (`MarkdownParser.parseRequirements`) and once for change deltas + * (`Validator.extractRequirementText` / `countScenarios`) — and the two drifted + * apart. These helpers are the single source of truth both readers delegate to, + * so requirement-body extraction, scenario counting, and `SHALL`/`MUST` + * detection behave identically for `validate `, `validate `, and + * `archive`. + */ + +/** + * Build a per-line mask marking lines that fall inside a fenced code block + * (``` ``` ``` or ``` ~~~ ```), including the fence lines themselves. Mirrors the + * fence rules markdown uses: a fence opens on the first ```` ```/~~~ ```` of + * length >= 3 and closes on a line of the same marker whose length is >= the + * opening length, with nothing but whitespace after it. + */ +export function buildCodeFenceMask(lines: string[]): boolean[] { + const mask = new Array(lines.length).fill(false); + let activeFence: { marker: '`' | '~'; length: number } | null = null; + + for (let i = 0; i < lines.length; i++) { + const fence = getFenceMarker(lines[i]); + + if (!activeFence) { + if (fence) { + activeFence = fence; + mask[i] = true; + } + continue; + } + + mask[i] = true; + if (isClosingFence(lines[i], activeFence)) { + activeFence = null; + } + } + + return mask; +} + +function getFenceMarker(line: string): { marker: '`' | '~'; length: number } | null { + const fenceMatch = line.match(/^\s*(`{3,}|~{3,})/); + if (!fenceMatch) { + return null; + } + + return { + marker: fenceMatch[1][0] as '`' | '~', + length: fenceMatch[1].length, + }; +} + +function isClosingFence( + line: string, + activeFence: { marker: '`' | '~'; length: number } +): boolean { + const fenceMatch = line.match(/^\s*(`{3,}|~{3,})\s*$/); + return Boolean( + fenceMatch && + fenceMatch[1][0] === activeFence.marker && + fenceMatch[1].length >= activeFence.length + ); +} + +/** Lines that look like `**ID**: ...` / `**Priority**: ...` metadata. */ +const METADATA_LINE = /^\*\*[^*]+\*\*:/; + +/** A level-4 scenario header (`#### Scenario: ...`). */ +const SCENARIO_HEADER = /^####\s+/; + +/** + * 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 + * and reject identical text. + */ +export function containsShallOrMust(text: string): boolean { + return /\b(SHALL|MUST)\b/.test(text); +} + +/** + * Extract the full requirement body from the lines that follow a + * `### Requirement:` header (the lines may include scenarios and fenced code). + * + * Captures every body line from the start up to the first `#### Scenario:` + * header found on a non-fenced line, skipping blank lines, `**metadata**:` + * lines, and any line inside a fenced code block. Captured lines are trimmed and + * joined with newlines so a requirement whose text wraps across lines — or whose + * `SHALL`/`MUST` lands on a later line — is read in full. + */ +export function extractRequirementBody(bodyLines: string[]): string { + const mask = buildCodeFenceMask(bodyLines); + const captured: string[] = []; + + for (let i = 0; i < bodyLines.length; i++) { + if (mask[i]) continue; // inside a fenced code block + const line = bodyLines[i]; + if (SCENARIO_HEADER.test(line)) break; // reached the first real scenario + const trimmed = line.trim(); + if (trimmed.length === 0) continue; // blank + if (METADATA_LINE.test(trimmed)) continue; // **ID**: / **Priority**: ... + captured.push(trimmed); + } + + return captured.join('\n'); +} + +/** + * 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. + */ +export function countScenarios(bodyLines: string[]): number { + const mask = buildCodeFenceMask(bodyLines); + let count = 0; + for (let i = 0; i < bodyLines.length; i++) { + if (mask[i]) continue; + if (SCENARIO_HEADER.test(bodyLines[i])) count++; + } + return count; +} diff --git a/src/core/schemas/base.schema.ts b/src/core/schemas/base.schema.ts index 548ef35e56..95d54affe7 100644 --- a/src/core/schemas/base.schema.ts +++ b/src/core/schemas/base.schema.ts @@ -1,5 +1,6 @@ import { z } from 'zod'; import { VALIDATION_MESSAGES } from '../validation/constants.js'; +import { containsShallOrMust } from '../parsers/requirement-text.js'; export const ScenarioSchema = z.object({ rawText: z.string().min(1, VALIDATION_MESSAGES.SCENARIO_EMPTY), @@ -9,7 +10,7 @@ export const RequirementSchema = z.object({ text: z.string() .min(1, VALIDATION_MESSAGES.REQUIREMENT_EMPTY) .refine( - (text) => text.includes('SHALL') || text.includes('MUST'), + (text) => containsShallOrMust(text), VALIDATION_MESSAGES.REQUIREMENT_NO_SHALL ), scenarios: z.array(ScenarioSchema) diff --git a/src/core/validation/validator.ts b/src/core/validation/validator.ts index 47071ed477..59a1e0cdc4 100644 --- a/src/core/validation/validator.ts +++ b/src/core/validation/validator.ts @@ -10,7 +10,12 @@ import { MAX_REQUIREMENT_TEXT_LENGTH, VALIDATION_MESSAGES } from './constants.js'; -import { parseDeltaSpec, normalizeRequirementName } from '../parsers/requirement-blocks.js'; +import { parseDeltaSpec, normalizeRequirementName, findNonRequirementLevel3Headers } from '../parsers/requirement-blocks.js'; +import { + extractRequirementBody, + containsShallOrMust as containsShallOrMustShared, + countScenarios as countScenariosShared, +} from '../parsers/requirement-text.js'; import { findMainSpecStructureIssues } from '../parsers/spec-structure.js'; import { FileSystemUtils } from '../../utils/file-system.js'; @@ -134,6 +139,21 @@ export class Validator { const plan = parseDeltaSpec(content); const entryPath = `${specName}/spec.md`; + + // Surface (as INFO, never a failure) any level-3 header inside an + // ADDED/MODIFIED section that is not a canonical "### Requirement:" + // header — the delta reader silently skips these, so without this note + // a stray divider like "### Documentation Requirements" would pass + // validate while failing archive/validate . + for (const stray of findNonRequirementLevel3Headers(content)) { + issues.push({ + level: 'INFO', + path: entryPath, + line: stray.line, + message: `Header "### ${stray.header}" in ${stray.section} is not a "### Requirement:" header and is ignored by validation. Use "### Requirement: ${stray.header}" if it should be validated as a requirement.`, + }); + } + const sectionNames: string[] = []; if (plan.sectionPresence.added) sectionNames.push('## ADDED Requirements'); if (plan.sectionPresence.modified) sectionNames.push('## MODIFIED Requirements'); @@ -413,35 +433,15 @@ export class Validator { } private extractRequirementText(blockRaw: string): string | undefined { - const lines = blockRaw.split('\n'); - // Skip header line (index 0) - let i = 1; - - // Find the first substantial text line, skipping metadata and blank lines - for (; i < lines.length; i++) { - const line = lines[i]; - - // Stop at scenario headers - if (/^####\s+/.test(line)) break; - - const trimmed = line.trim(); - - // Skip blank lines - if (trimmed.length === 0) continue; - - // Skip metadata lines (lines starting with ** like **ID**, **Priority**, etc.) - if (/^\*\*[^*]+\*\*:/.test(trimmed)) continue; - - // Found first non-metadata, non-blank line - this is the requirement text - return trimmed; - } - - // No requirement text found - return undefined; + // Delegate to the shared, fence-/metadata-/multi-line-aware reader so the + // change-delta path and the main-spec path cannot diverge. Drop the header + // line (index 0) and read the body that follows. + const body = extractRequirementBody(blockRaw.split('\n').slice(1)); + return body || undefined; } private containsShallOrMust(text: string): boolean { - return /\b(SHALL|MUST)\b/.test(text); + return containsShallOrMustShared(text); } /** @@ -463,8 +463,9 @@ export class Validator { } private countScenarios(blockRaw: string): number { - const matches = blockRaw.match(/^####\s+/gm); - return matches ? matches.length : 0; + // Fence-aware count via the shared reader: a `#### Scenario:` inside a fenced + // example is not a real scenario. Drop the header line (index 0). + return countScenariosShared(blockRaw.split('\n').slice(1)); } private formatSectionList(sections: string[]): string { diff --git a/test/core/parsers/markdown-parser.test.ts b/test/core/parsers/markdown-parser.test.ts index 751ab98db0..1b93b764a7 100644 --- a/test/core/parsers/markdown-parser.test.ts +++ b/test/core/parsers/markdown-parser.test.ts @@ -328,7 +328,7 @@ Then result`; expect(spec.requirements[0].text).toBe('The system SHALL use heading text when no content'); }); - it('should extract requirement text from first non-empty content line', () => { + it('should extract the full requirement body, not only the first content line', () => { const content = `# Test Spec ## Purpose @@ -348,8 +348,142 @@ Then result`; const parser = new MarkdownParser(content); const spec = parser.parseSpec('test'); - - expect(spec.requirements[0].text).toBe('This is the actual requirement text.'); + + // Body spans both lines up to the first scenario (the #361 fix); the + // reader no longer drops everything after line one. + expect(spec.requirements[0].text).toBe( + 'This is the actual requirement text.\nThis is additional description.' + ); + }); + }); + + describe('requirement body reading fidelity', () => { + it('captures a normative keyword that wraps onto a later body line (#361)', () => { + const content = `# Test Spec + +## Purpose +Test overview for wrapped keyword handling. + +## Requirements + +### Requirement: Wrapped keyword +The system performs the described behavior and it +continues onto a second line where SHALL appears. + +#### Scenario: Test +Given test +When action +Then result`; + + const parser = new MarkdownParser(content); + const spec = parser.parseSpec('test'); + + expect(spec.requirements[0].text).toContain('SHALL appears'); + expect(spec.requirements[0].text).toContain('The system performs the described behavior'); + }); + + it('skips **metadata**: lines before the description (#418)', () => { + const content = `# Test Spec + +## Purpose +Test overview for metadata-first requirements. + +## Requirements + +### Requirement: Metadata first +**ID**: REQ-FILE-001 +**Priority**: P1 (High) +The system MUST persist the uploaded file. + +#### Scenario: Test +Given test +When action +Then result`; + + const parser = new MarkdownParser(content); + const spec = parser.parseSpec('test'); + + expect(spec.requirements[0].text).toBe('The system MUST persist the uploaded file.'); + }); + + it('ignores a fenced code block that precedes the prose line (#312)', () => { + const content = `# Test Spec + +## Purpose +Test overview for fence-before-prose handling. + +## Requirements + +### Requirement: Fence first +\`\`\`bash +# this is a shell comment, not the requirement text +echo hello +\`\`\` +The system SHALL handle fenced examples before the prose line. + +#### Scenario: Test +Given test +When action +Then result`; + + const parser = new MarkdownParser(content); + const spec = parser.parseSpec('test'); + + expect(spec.requirements[0].text).toBe( + 'The system SHALL handle fenced examples before the prose line.' + ); + expect(spec.requirements[0].scenarios).toHaveLength(1); + }); + + it('does not count a #### Scenario inside a fenced example as a real scenario', () => { + const content = `# Test Spec + +## Purpose +Test overview for fenced scenario handling. + +## Requirements + +### Requirement: Fenced scenario only +The system SHALL do something real. + +\`\`\`markdown +#### Scenario: not a real scenario +- **WHEN** a reader studies the example +- **THEN** it stays inside the fence +\`\`\``; + + const parser = new MarkdownParser(content); + const spec = parser.parseSpec('test'); + + expect(spec.requirements[0].text).toBe('The system SHALL do something real.'); + expect(spec.requirements[0].scenarios).toHaveLength(0); + }); + + it('reads a wrapped body the same way under CRLF line endings', () => { + const content = [ + '# Test Spec', + '', + '## Purpose', + 'Test overview for CRLF body extraction.', + '', + '## Requirements', + '', + '### Requirement: Wrapped keyword', + 'The system performs the described behavior and it', + 'continues onto a second line where SHALL appears.', + '', + '#### Scenario: Test', + 'Given test', + 'When action', + 'Then result', + ].join('\r\n'); + + const parser = new MarkdownParser(content); + const spec = parser.parseSpec('test'); + + expect(spec.requirements[0].text).toBe( + 'The system performs the described behavior and it\ncontinues onto a second line where SHALL appears.' + ); }); }); }); diff --git a/test/core/validation.test.ts b/test/core/validation.test.ts index 72ebc2aba6..d295bf06b3 100644 --- a/test/core/validation.test.ts +++ b/test/core/validation.test.ts @@ -677,4 +677,181 @@ The system MUST support mixed case delta headers. expect(report.summary.info).toBe(0); }); }); + + describe('parser reading fidelity (#361, #418, #312, fenced scenario, #498)', () => { + async function writeChangeDelta(name: string, deltaSpec: string): Promise { + const changeDir = path.join(testDir, name); + const specsDir = path.join(changeDir, 'specs', 'test-spec'); + await fs.mkdir(specsDir, { recursive: true }); + await fs.writeFile(path.join(specsDir, 'spec.md'), deltaSpec); + return changeDir; + } + + async function writeSpec(name: string, specContent: string): Promise { + const specPath = path.join(testDir, `${name}.md`); + await fs.writeFile(specPath, specContent); + return specPath; + } + + it('#361: a normative keyword on a wrapped body line passes both change and spec', async () => { + const delta = `# Test Spec + +## ADDED Requirements + +### Requirement: Wrapped keyword +The system performs the described behavior and it +continues onto a second line where SHALL appears in full. + +#### Scenario: Wrapped +**Given** a request +**When** it is handled +**Then** the behavior occurs`; + + const changeDir = await writeChangeDelta('fidelity-361', delta); + const changeReport = await new Validator(true).validateChangeDeltaSpecs(changeDir); + expect(changeReport.valid).toBe(true); + expect(changeReport.summary.errors).toBe(0); + + const spec = `# Test Spec + +## Purpose +This spec exercises a normative keyword wrapped onto a second line. + +## Requirements + +### Requirement: Wrapped keyword +The system performs the described behavior and it +continues onto a second line where SHALL appears in full. + +#### Scenario: Wrapped +**Given** a request +**When** it is handled +**Then** the behavior occurs`; + + const specPath = await writeSpec('fidelity-361-spec', spec); + const specReport = await new Validator(true).validateSpec(specPath); + expect(specReport.valid).toBe(true); + expect(specReport.summary.errors).toBe(0); + }); + + it('#418: metadata before the description passes validate (matching )', async () => { + const spec = `# Test Spec + +## Purpose +This spec exercises metadata fields preceding the requirement description. + +## Requirements + +### Requirement: Metadata first +**ID**: REQ-FILE-001 +**Priority**: P1 (High) +The system MUST persist the uploaded file. + +#### Scenario: Persisted +**Given** an uploaded file +**When** the request completes +**Then** the file is stored`; + + const specPath = await writeSpec('fidelity-418-spec', spec); + const specReport = await new Validator(true).validateSpec(specPath); + expect(specReport.valid).toBe(true); + expect(specReport.summary.errors).toBe(0); + }); + + it('#312: a fenced block before the prose line passes both change and spec', async () => { + const delta = `# Test Spec + +## ADDED Requirements + +### Requirement: Fence first +\`\`\`bash +# this is a shell comment, not the requirement text +echo hello +\`\`\` +The system SHALL handle fenced examples before the prose line. + +#### Scenario: Handled +**Given** a fenced example +**When** the requirement is read +**Then** the prose line is the requirement text`; + + const changeDir = await writeChangeDelta('fidelity-312', delta); + const changeReport = await new Validator(true).validateChangeDeltaSpecs(changeDir); + expect(changeReport.valid).toBe(true); + expect(changeReport.summary.errors).toBe(0); + }); + + it('fenced scenario: a #### Scenario inside a fence does not count (change matches spec)', async () => { + const delta = `# Test Spec + +## ADDED Requirements + +### Requirement: Fenced scenario only +The system SHALL do something real. + +\`\`\`markdown +#### Scenario: not a real scenario +- **WHEN** a reader studies the example +- **THEN** it stays inside the fence +\`\`\``; + + const changeDir = await writeChangeDelta('fidelity-fenced-scenario', delta); + const changeReport = await new Validator(true).validateChangeDeltaSpecs(changeDir); + + // The only scenario is fenced, so the requirement has zero real scenarios + // and must fail — the same verdict validate already gives. + expect(changeReport.valid).toBe(false); + expect( + changeReport.issues.some(i => i.message.includes('must include at least one scenario')) + ).toBe(true); + }); + + it('#498: a stray ### divider yields an INFO note and does not change valid (even strict)', async () => { + const delta = `# Test Spec + +## ADDED Requirements + +### Documentation Requirements + +### Requirement: Real requirement +The system SHALL do the real thing. + +#### Scenario: Works +**Given** a request +**When** it is handled +**Then** the behavior occurs`; + + const changeDir = await writeChangeDelta('fidelity-498', delta); + const report = await new Validator(true).validateChangeDeltaSpecs(changeDir); + + // INFO surfaces the stray header but never fails validation. + expect(report.valid).toBe(true); + expect(report.summary.errors).toBe(0); + const info = report.issues.find( + i => i.level === 'INFO' && i.message.includes('Documentation Requirements') + ); + expect(info).toBeDefined(); + expect(report.summary.info).toBeGreaterThan(0); + }); + + it('guard: a single-line requirement is read byte-for-byte as before', async () => { + const delta = `# Test Spec + +## ADDED Requirements + +### Requirement: Single line +The system SHALL remain unchanged for single-line bodies. + +#### Scenario: Unchanged +**Given** a single-line requirement +**When** it is validated +**Then** nothing changes`; + + const changeDir = await writeChangeDelta('fidelity-single-line', delta); + const report = await new Validator(true).validateChangeDeltaSpecs(changeDir); + expect(report.valid).toBe(true); + expect(report.summary.errors).toBe(0); + expect(report.summary.info).toBe(0); + }); + }); }); From 3fd2da54b45eebef70dd518cfa0d6753d5f1978f Mon Sep 17 00:00:00 2001 From: Clay Good Date: Tue, 30 Jun 2026 16:17:53 -0500 Subject: [PATCH 6/8] test(parser): add cross-reader predicate + metadata-only guards (design edge cases) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Exhaustive verification of the unified reader surfaced two design "edge cases for tests" not yet covered by committed unit tests: - Cross-reader predicate agreement: a SHALL substring inside a word ("MARSHALL") is rejected identically by validate and validate — proving the one shared whole-word predicate, and guarding against a regression to the old substring check. - Metadata-only body still fails validation (no requirement text) on the delta path. Behavior unchanged; tests only. Full end-to-end parity across all four spec requirements confirmed against the real Validator; no spurious INFO note fires on any existing repo change. Co-Authored-By: Claude Opus 4.8 (1M context) --- test/core/validation.test.ts | 48 ++++++++++++++++++++++++++++++++++++ 1 file changed, 48 insertions(+) diff --git a/test/core/validation.test.ts b/test/core/validation.test.ts index d295bf06b3..ea74d60736 100644 --- a/test/core/validation.test.ts +++ b/test/core/validation.test.ts @@ -853,5 +853,53 @@ The system SHALL remain unchanged for single-line bodies. expect(report.summary.errors).toBe(0); expect(report.summary.info).toBe(0); }); + + it('predicate agrees across readers: a SHALL substring inside a word is not a keyword', async () => { + // "MARSHALL" contains the substring SHALL but is not a whole-word normative + // keyword. Both readers must reject it identically (the shared predicate). + const body = `### Requirement: Marshalling +The MARSHALL coordinates parade logistics. + +#### Scenario: Coordinated +**Given** a parade +**When** it begins +**Then** logistics are coordinated`; + + const changeDir = await writeChangeDelta('fidelity-predicate', `# Test Spec\n\n## ADDED Requirements\n\n${body}`); + const changeReport = await new Validator(true).validateChangeDeltaSpecs(changeDir); + expect(changeReport.valid).toBe(false); + + const spec = `# Test Spec + +## Purpose +This spec checks that a SHALL substring inside a word is not treated as a keyword. + +## Requirements + +${body}`; + const specPath = await writeSpec('fidelity-predicate-spec', spec); + const specReport = await new Validator(true).validateSpec(specPath); + expect(specReport.valid).toBe(false); + }); + + it('guard: a metadata-only body still fails validation (no requirement text)', async () => { + const delta = `# Test Spec + +## ADDED Requirements + +### Requirement: Metadata only +**ID**: REQ-META-001 +**Priority**: P1 (High) + +#### Scenario: Present +**Given** a metadata-only body +**When** it is validated +**Then** validation fails`; + + const changeDir = await writeChangeDelta('fidelity-metadata-only', delta); + const report = await new Validator(true).validateChangeDeltaSpecs(changeDir); + expect(report.valid).toBe(false); + expect(report.summary.errors).toBeGreaterThan(0); + }); }); }); From 44fb1d74a749c9d432a9e2a67031e8c75f8b6c0c Mon Sep 17 00:00:00 2001 From: Clay Good Date: Thu, 2 Jul 2026 17:25:12 -0500 Subject: [PATCH 7/8] =?UTF-8?q?fix(parser):=20address=20review=20=E2=80=94?= =?UTF-8?q?=20metadata-only=20bodies,=20header-bounded=20extraction,=20rea?= =?UTF-8?q?der-derived=20INFO?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Skip **metadata**: lines only when other body text remains; a body written entirely as metadata (e.g. `**Constraint**: The system MUST ...`) is kept as the requirement text instead of being emptied (was a regression vs main). - Move the empty-body rule into the shared reader: both paths fall back to the header title, so the same block cannot pass one path and fail the other. - End body extraction at any non-fenced markdown header, restoring old-reader parity: a stray `### Background` divider's notes no longer satisfy the SHALL/MUST check. - Replace the standalone fence-aware INFO scanner with skipped-header collection inside parseDeltaSpec, so the note reflects exactly what the reader skipped (same section boundaries, no whole-file fence mask). - Special-case the nameless `### Requirement:` INFO message; document that the any-#### scenario match is deliberate spec-path parity; un-export REQUIREMENT_HEADER_REGEX; move the import up top. - Soften the changeset claim and list the known remaining divergences in design.md. Co-Authored-By: Claude Fable 5 --- .changeset/spec-parser-reading-fidelity.md | 6 +- .../fix-spec-parser-fidelity/design.md | 12 +- .../specs/cli-validate/spec.md | 23 ++- .../changes/fix-spec-parser-fidelity/tasks.md | 13 +- src/core/parsers/markdown-parser.ts | 9 +- src/core/parsers/requirement-blocks.ts | 120 +++++++------ src/core/parsers/requirement-text.ts | 46 ++++- src/core/validation/validator.ts | 34 ++-- test/core/parsers/markdown-parser.test.ts | 26 +++ test/core/validation.test.ts | 164 +++++++++++++++++- 10 files changed, 356 insertions(+), 97 deletions(-) diff --git a/.changeset/spec-parser-reading-fidelity.md b/.changeset/spec-parser-reading-fidelity.md index aaa0a347e2..a5d82f1f61 100644 --- a/.changeset/spec-parser-reading-fidelity.md +++ b/.changeset/spec-parser-reading-fidelity.md @@ -4,12 +4,12 @@ ### Bug Fixes -- **Requirement reading fidelity** — The requirement reader used by `validate `, `validate `, and `archive` is now unified into one fence-, metadata-, and multi-line-aware extraction, so the change-delta path and the main-spec path can no longer disagree: +- **Requirement reading fidelity** — The requirement reader used by `validate `, `validate `, and `archive` is now unified into one fence-, metadata-, and multi-line-aware extraction, closing the known divergences between the change-delta path and the main-spec path (the remaining ones are documented in the change's design doc): - A `SHALL`/`MUST` keyword that wraps onto a later body line is detected instead of dropped (#361). - - Metadata lines (`**ID**:`, `**Priority**:`) before the description are skipped on the spec path, matching the change path (#418). + - Metadata lines (`**ID**:`, `**Priority**:`) before the description are skipped on the spec path, matching the change path (#418). A requirement written entirely as metadata (e.g. `**Constraint**: The system MUST ...`) keeps that line as its text instead of being emptied. - A fenced code block before the prose line no longer becomes the requirement text (#312). - A `#### Scenario:` inside a fenced example no longer counts as a real scenario in `validate `, matching `validate `. - - `SHALL`/`MUST` detection uses one whole-word predicate across all readers. + - `SHALL`/`MUST` detection uses one whole-word predicate across all readers, and a requirement with no body text falls back to its header title on both paths. Displayed requirement text (e.g. in JSON output and delta descriptions) now reflects the full requirement body rather than only its first line. Archived spec content is unchanged — the archive rebuild reads raw `### Requirement:` blocks, not the parsed text. diff --git a/openspec/changes/fix-spec-parser-fidelity/design.md b/openspec/changes/fix-spec-parser-fidelity/design.md index d332d7a274..9725c88d6b 100644 --- a/openspec/changes/fix-spec-parser-fidelity/design.md +++ b/openspec/changes/fix-spec-parser-fidelity/design.md @@ -26,13 +26,13 @@ ### Part A — one shared, fence-aware extraction -A single helper takes the requirement block's lines plus the fence mask and returns the full body: lines from after the header to the first `#### Scenario:` header found on a **non-fence-masked** line, skipping fence-masked lines and `**metadata**:` lines. A companion fence-aware scenario counter counts only non-fence-masked `####` headers. Both readers delegate to these. `SHALL`/`MUST` detection uses one predicate. +A single helper takes the requirement block's lines plus the fence mask and returns the full body: lines from after the header to the first markdown header found on a **non-fence-masked** line (usually `#### Scenario:`, but also a stray `###` divider the delta reader absorbed into the block — its notes must not feed the keyword check), skipping fence-masked lines and blank lines. `**metadata**:` lines are skipped only when other body text remains; a requirement written entirely as `**Constraint**: The system MUST ...` keeps that line as its body. When the body comes back empty, one shared rule falls back to the header title — this is what lets bare `### The system SHALL ...` headers validate on the spec path, and it keeps the two paths from reaching different verdicts for the same block. A companion fence-aware scenario counter counts only non-fence-masked `####` headers (deliberately *any* `####`, since the spec path treats every level-4 child as a scenario). Both readers delegate to these. `SHALL`/`MUST` detection uses one predicate. Why the existing fence tests still pass: in `markdown-parser.test.ts:106`/`:139` the `SHALL` line is first and the fenced block follows, so skipping fenced lines leaves `text` exactly equal to the `SHALL` line — the asserted value. The breaking case (#312) is the inverse — fence *before* prose — which no test covers. ### Part B — surface the #498 divergence (INFO, no recognition change) -`validateChangeDeltaSpecs` emits an INFO issue when an `## ADDED`/`## MODIFIED Requirements` section contains a level-3 header that does not match `REQUIREMENT_HEADER_REGEX` (so the delta reader will skip it). Under `--strict`, `valid = errors === 0 && warnings === 0` — **INFO is excluded**, so this never changes pass/fail; it only informs. This is the minimal change that makes `validate ` stop *silently* passing the #498 input. +`parseDeltaSpec` records the non-canonical level-3 headers it skips *while parsing* the `## ADDED`/`## MODIFIED Requirements` sections, and `validateChangeDeltaSpecs` emits each as an INFO issue. Collecting during the parse (rather than with a separate scanner) guarantees the note describes the reader's real boundaries — a header the reader never saw (e.g. after a fenced `##` line ended the section early) gets no note, and a fenced `###` example line, which the body reader treats as content, is not reported. Under `--strict`, `valid = errors === 0 && warnings === 0` — **INFO is excluded**, so this never changes pass/fail; it only informs. This is the minimal change that makes `validate ` stop *silently* passing the #498 input. ## Why recognition tightening is rejected @@ -59,6 +59,14 @@ Consumers of `parseSpec`/`req.text`: `view.ts`/`list.ts` (requirement **counts** - LF/CRLF/CR via `normalizeContent`; `~~~`/length-≥3/leading-whitespace fences via existing `buildCodeFenceMask`. - INFO note appears for a stray delta header but does not change `valid` (including `--strict`). +## Known remaining divergences + +Unification closes the reproduced defects; these divergences remain and are accepted: + +- **Empty scenarios** — a `#### Scenario:` header with no body counts on the delta path (`countScenarios` counts headers) but not on the spec path (`parseScenarios` keeps only scenarios with content), so `validate ` passes what `validate `/`archive` rejects. +- **Recognition** — bare `### ` headers are requirements on the spec path but skipped on the delta path. Deliberate (see "Why recognition tightening is rejected"); the Part B INFO note surfaces it instead of unifying it. +- **Delta section/block splitting is not fence-aware** — `splitTopLevelSections` and `parseRequirementBlocksFromSection` treat a fenced `## ...` line as a section boundary and a fenced `### Requirement:` line as a new block, while the spec path fence-masks its sectioning. The skipped-header INFO is collected during the actual parse precisely so it reflects these boundaries instead of describing different ones. + ## Prior art `findMainSpecStructureIssues` (`spec-structure.ts`) already flags a `### Requirement:` header *outside* the `## Requirements` section and delta headers inside a main spec. The Part B INFO note is complementary: it flags non-`Requirement:` headers *inside* a delta Requirements section, which that function does not cover. diff --git a/openspec/changes/fix-spec-parser-fidelity/specs/cli-validate/spec.md b/openspec/changes/fix-spec-parser-fidelity/specs/cli-validate/spec.md index 26eb4cfde9..0a15518f74 100644 --- a/openspec/changes/fix-spec-parser-fidelity/specs/cli-validate/spec.md +++ b/openspec/changes/fix-spec-parser-fidelity/specs/cli-validate/spec.md @@ -1,7 +1,7 @@ ## ADDED Requirements ### Requirement: Requirement bodies SHALL be parsed in full for normative keywords -The validator SHALL detect `SHALL`/`MUST` across the entire requirement body, not only the first body line. Requirement-text extraction SHALL capture every body line from after the `### Requirement:` header up to the first `#### Scenario:` header, skipping blank lines, `**metadata**:` lines, and lines inside fenced code blocks. Detection SHALL run over the full captured body. The change-delta reader and the main-spec reader SHALL share this extraction so they cannot diverge. +The validator SHALL detect `SHALL`/`MUST` across the entire requirement body, not only the first body line. Requirement-text extraction SHALL capture every body line from after the `### Requirement:` header up to the first Markdown header on a non-fenced line (a `#### Scenario:` header, or a stray `###` divider absorbed into a delta block), skipping blank lines and lines inside fenced code blocks. `**metadata**:` lines SHALL be skipped only when other body text remains; a body consisting solely of metadata lines SHALL be kept as the requirement text. When a requirement block has no body text at all, both readers SHALL fall back to the header title as the requirement text. Detection SHALL run over the full captured body. The change-delta reader and the main-spec reader SHALL share this extraction so they cannot diverge. #### Scenario: Normative keyword on the second wrapped line (change and spec) - **GIVEN** a requirement whose text wraps across two lines with `SHALL` on the second line @@ -13,13 +13,23 @@ The validator SHALL detect `SHALL`/`MUST` across the entire requirement body, no - **WHEN** running `openspec validate --strict` - **THEN** validation SHALL skip the metadata lines, detect `MUST`, and pass — matching `openspec validate ` +#### Scenario: Requirement written entirely as a metadata line +- **GIVEN** a requirement whose whole body is `**Constraint**: The system MUST ...` +- **WHEN** running `openspec validate --strict` for both a change delta and a main spec +- **THEN** both SHALL keep that line as the requirement text and detect the `MUST` + +#### Scenario: Stray divider bounds the requirement body +- **GIVEN** a delta requirement followed by a stray `### Background` divider whose notes contain `MUST` +- **WHEN** running `openspec validate --strict` +- **THEN** the requirement body SHALL end at the divider and the `MUST` in the notes SHALL NOT satisfy the keyword check + #### Scenario: Single-line requirement is unaffected - **GIVEN** a requirement whose `SHALL` statement is on a single body line - **WHEN** running `openspec validate --strict` - **THEN** validation behavior, messages, and displayed text SHALL be unchanged from before this change ### Requirement: Fenced code blocks SHALL NOT corrupt extraction or scenario counting -The validator and markdown parser SHALL ignore lines inside fenced code blocks (` ``` ` or `~~~`) when extracting requirement body text, when locating the first `#### Scenario:` boundary, and when counting scenarios. A fenced block before the prose line SHALL NOT make the fence marker the requirement text, and a `#### Scenario:` inside a fenced block SHALL NOT count as a real scenario. +The validator and Markdown parser SHALL ignore lines inside fenced code blocks (` ``` ` or `~~~`) when extracting requirement body text, when locating the body-ending header boundary, and when counting scenarios. A fenced block before the prose line SHALL NOT make the fence marker the requirement text, and a `#### Scenario:` inside a fenced block SHALL NOT count as a real scenario. #### Scenario: Fenced block before the prose line - **GIVEN** a requirement whose body opens with a fenced code block containing `#`-comment lines, followed by the `SHALL` prose line @@ -32,7 +42,7 @@ The validator and markdown parser SHALL ignore lines inside fenced code blocks ( - **THEN** validation SHALL report the requirement as missing a scenario — the same result as `openspec validate ` ### Requirement: A single normative-keyword predicate SHALL be used across readers -All `SHALL`/`MUST` detection SHALL use one predicate that matches `SHALL` or `MUST` as whole words, so the change-delta reader and the schema-based reader accept and reject identical text. +All `SHALL`/`MUST` detection SHALL use one predicate that matches `SHALL` or `MUST` as whole words (delimited by word boundaries, so a substring inside a longer word such as `MARSHALL` does not match), so the change-delta reader and the schema-based reader accept and reject identical text. #### Scenario: Keyword detection agrees across readers - **GIVEN** identical requirement body text validated once as a change delta and once as a main spec @@ -40,7 +50,7 @@ All `SHALL`/`MUST` detection SHALL use one predicate that matches `SHALL` or `MU - **THEN** both SHALL reach the same conclusion about whether the body contains a normative keyword ### Requirement: Non-canonical headers in delta sections SHALL be surfaced without changing recognition -When an `## ADDED`/`## MODIFIED Requirements` section in a change delta contains a level-3 header that is not a canonical `### Requirement:` header, `openspec validate ` SHALL emit an INFO-level note identifying it, because the delta reader will otherwise skip it silently. This note SHALL NOT change which headers are recognized as requirements, and SHALL NOT change the `valid` result — including under `--strict`. +When an `## ADDED`/`## MODIFIED Requirements` section in a change delta contains a level-3 header that is not a canonical `### Requirement:` header, `openspec validate ` SHALL emit an INFO-level note identifying it, because the delta reader will otherwise skip it silently. The note SHALL be derived from the headers the delta reader actually skips while parsing, so it describes the reader's real section and fence boundaries. This note SHALL NOT change which headers are recognized as requirements, and SHALL NOT change the `valid` result — including under `--strict`. This behavior applies only to change deltas: bare `### ` headers in main specs are recognized requirements (see the scenario below) and SHALL NOT trigger such notes. #### Scenario: Stray divider header is reported, not silently skipped - **GIVEN** a delta whose `## ADDED Requirements` section contains `### Documentation Requirements` followed by a valid `### Requirement: …` block @@ -48,6 +58,11 @@ When an `## ADDED`/`## MODIFIED Requirements` section in a change delta contains - **THEN** validation SHALL emit an INFO note naming the stray `### Documentation Requirements` header - **AND** the `valid` result SHALL be unchanged from current behavior (the INFO does not cause failure) +#### Scenario: Nameless requirement header gets a dedicated hint +- **GIVEN** a delta whose `## ADDED Requirements` section contains a bare `### Requirement:` header with no name +- **WHEN** running `openspec validate ` +- **THEN** the INFO note SHALL say the header is missing a requirement name (not suggest `### Requirement: Requirement:`) + #### Scenario: Bare requirement headers in main specs remain supported - **GIVEN** a main spec whose requirements use bare `### ` headers without the `Requirement:` prefix - **WHEN** running `openspec validate --strict` diff --git a/openspec/changes/fix-spec-parser-fidelity/tasks.md b/openspec/changes/fix-spec-parser-fidelity/tasks.md index fe1323c83f..754e6792b8 100644 --- a/openspec/changes/fix-spec-parser-fidelity/tasks.md +++ b/openspec/changes/fix-spec-parser-fidelity/tasks.md @@ -12,8 +12,8 @@ ## 3. Part B — surface the #498 divergence (INFO, no recognition change) -- [x] 3.1 Export `REQUIREMENT_HEADER_REGEX` from `requirement-blocks.ts`. -- [x] 3.2 In `validateChangeDeltaSpecs`, emit an INFO issue when an `## ADDED`/`## MODIFIED Requirements` section contains a level-3 header that does not match the canonical regex. Do **not** change recognition. +- [x] 3.1 Record the non-canonical level-3 headers `parseDeltaSpec` skips while parsing ADDED/MODIFIED sections (`DeltaPlan.skippedHeaders`), so the note reflects the reader's real boundaries. +- [x] 3.2 In `validateChangeDeltaSpecs`, emit an INFO issue for each skipped header. Do **not** change recognition. Special-case a nameless `### Requirement:` header. - [x] 3.3 Confirm INFO does not affect `valid` under `--strict` (`valid = errors === 0 && warnings === 0`). ## 4. Update the one affected existing test @@ -32,3 +32,12 @@ ## 6. Release - [x] 6.1 Add a changeset: Fixes #361, #418, #312; surfaces #498. Note the read-only display changes (fuller `req.text` in JSON/descriptions); no archived-content change. + +## 7. Review fixes (PR #1281) + +- [x] 7.1 Skip `**metadata**:` lines only when other body text remains; a metadata-only body (e.g. `**Constraint**: The system MUST ...`) is kept as the requirement text. +- [x] 7.2 Move the empty-body rule into the shared reader: both paths fall back to the header title. +- [x] 7.3 End the body at any non-fenced Markdown header, so a stray `###` divider's notes cannot satisfy the keyword check (old-reader parity). +- [x] 7.4 Replace the standalone INFO scanner with skipped-header collection inside `parseDeltaSpec` (notes match the reader's real boundaries). +- [x] 7.5 Special-case the nameless `### Requirement:` INFO message; document that the any-`####` scenario match is deliberate; un-export `REQUIREMENT_HEADER_REGEX`. +- [x] 7.6 Soften the changeset wording and document the known remaining divergences in `design.md`. diff --git a/src/core/parsers/markdown-parser.ts b/src/core/parsers/markdown-parser.ts index 2355368ae8..8dca1ef64f 100644 --- a/src/core/parsers/markdown-parser.ts +++ b/src/core/parsers/markdown-parser.ts @@ -1,5 +1,5 @@ import { Spec, Change, Requirement, Scenario, Delta, DeltaOperation } from '../schemas/index.js'; -import { buildCodeFenceMask, extractRequirementBody } from './requirement-text.js'; +import { buildCodeFenceMask, extractRequirementText } from './requirement-text.js'; export interface Section { level: number; @@ -152,10 +152,9 @@ export class MarkdownParser { const requirements: Requirement[] = []; for (const child of section.children) { - // Capture the full requirement body (multi-line, fence- and metadata-aware) - // via the shared reader, falling back to the heading when there is no body. - const body = extractRequirementBody(child.content.split('\n')); - const text = body || child.title; + // Read the requirement text via the shared reader (multi-line, fence- and + // metadata-aware, with the shared header-title fallback for empty bodies). + const text = extractRequirementText(child.title, child.content.split('\n')); const scenarios = this.parseScenarios(child); diff --git a/src/core/parsers/requirement-blocks.ts b/src/core/parsers/requirement-blocks.ts index 73a64aca5d..adb8138aea 100644 --- a/src/core/parsers/requirement-blocks.ts +++ b/src/core/parsers/requirement-blocks.ts @@ -1,3 +1,5 @@ +import { buildCodeFenceMask } from './requirement-text.js'; + export interface RequirementBlock { headerLine: string; // e.g., '### Requirement: Something' name: string; // e.g., 'Something' @@ -12,14 +14,12 @@ export interface RequirementsSectionParts { after: string; } -import { buildCodeFenceMask } from './requirement-text.js'; - export function normalizeRequirementName(name: string): string { return name.trim(); } /** The canonical requirement header the delta reader recognizes. */ -export const REQUIREMENT_HEADER_REGEX = /^###\s*Requirement:\s*(.+)\s*$/i; +const REQUIREMENT_HEADER_REGEX = /^###\s*Requirement:\s*(.+)\s*$/i; /** * Extracts the Requirements section from a spec file and parses requirement blocks. @@ -99,11 +99,23 @@ export function extractRequirementsSection(content: string): RequirementsSection }; } +/** + * A level-3 header inside `## ADDED`/`## MODIFIED Requirements` that is not a + * canonical `### Requirement:` header, recorded at the moment the delta reader + * skips over it. Surfaced as an INFO note by `validate ` (#498). + */ +export interface SkippedHeader { + header: string; // header text without the leading ### + section: string; // the ## section title as written + line: number; // 1-based line number in the delta file +} + export interface DeltaPlan { added: RequirementBlock[]; modified: RequirementBlock[]; removed: string[]; // requirement names renamed: Array<{ from: string; to: string }>; + skippedHeaders: SkippedHeader[]; // non-canonical ### headers the reader skipped sectionPresence: { added: boolean; modified: boolean; @@ -126,15 +138,26 @@ export function parseDeltaSpec(content: string): DeltaPlan { const modifiedLookup = getSectionCaseInsensitive(sections, 'MODIFIED Requirements'); const removedLookup = getSectionCaseInsensitive(sections, 'REMOVED Requirements'); const renamedLookup = getSectionCaseInsensitive(sections, 'RENAMED Requirements'); - const added = parseRequirementBlocksFromSection(addedLookup.body); - const modified = parseRequirementBlocksFromSection(modifiedLookup.body); + const skippedHeaders: SkippedHeader[] = []; + const added = parseRequirementBlocksFromSection(addedLookup.body, { + section: addedLookup.title, + bodyStartLine: addedLookup.bodyStartLine, + sink: skippedHeaders, + }); + const modified = parseRequirementBlocksFromSection(modifiedLookup.body, { + section: modifiedLookup.title, + bodyStartLine: modifiedLookup.bodyStartLine, + sink: skippedHeaders, + }); const removedNames = parseRemovedNames(removedLookup.body); const renamedPairs = parseRenamedPairs(renamedLookup.body); + skippedHeaders.sort((a, b) => a.line - b.line); return { added, modified, removed: removedNames, renamed: renamedPairs, + skippedHeaders, sectionPresence: { added: addedLookup.found, modified: modifiedLookup.found, @@ -144,9 +167,9 @@ export function parseDeltaSpec(content: string): DeltaPlan { }; } -function splitTopLevelSections(content: string): Record { +function splitTopLevelSections(content: string): Record { const lines = content.split('\n'); - const result: Record = {}; + const result: Record = {}; const indices: Array<{ title: string; index: number; level: number }> = []; for (let i = 0; i < lines.length; i++) { const m = lines[i].match(/^(##)\s+(.+)$/); @@ -159,27 +182,53 @@ function splitTopLevelSections(content: string): Record { const current = indices[i]; const next = indices[i + 1]; const body = lines.slice(current.index + 1, next ? next.index : lines.length).join('\n'); - result[current.title] = body; + // First body line, 1-based: the header is at 0-based current.index. + result[current.title] = { body, bodyStartLine: current.index + 2 }; } return result; } -function getSectionCaseInsensitive(sections: Record, desired: string): { body: string; found: boolean } { +function getSectionCaseInsensitive( + sections: Record, + desired: string +): { title: string; body: string; bodyStartLine: number; found: boolean } { const target = desired.toLowerCase(); - for (const [title, body] of Object.entries(sections)) { - if (title.toLowerCase() === target) return { body, found: true }; + for (const [title, { body, bodyStartLine }] of Object.entries(sections)) { + if (title.toLowerCase() === target) return { title, body, bodyStartLine, found: true }; } - return { body: '', found: false }; + return { title: desired, body: '', bodyStartLine: 0, found: false }; } -function parseRequirementBlocksFromSection(sectionBody: string): RequirementBlock[] { +function parseRequirementBlocksFromSection( + sectionBody: string, + skipped?: { section: string; bodyStartLine: number; sink: SkippedHeader[] } +): RequirementBlock[] { if (!sectionBody) return []; const lines = normalizeLineEndings(sectionBody).split('\n'); + // Record the non-canonical level-3 headers this reader skips, at the moment + // it skips them, so the INFO note describes the reader's real boundaries. + // Fence-masked lines are excluded: the body reader treats them as fenced + // content, not as headers. + const fenceMask = skipped ? buildCodeFenceMask(lines) : undefined; + const recordIfSkippedHeader = (index: number) => { + if (!skipped || fenceMask![index]) return; + const h3 = lines[index].match(/^###\s+(.+?)\s*$/); + if (h3 && !REQUIREMENT_HEADER_REGEX.test(lines[index])) { + skipped.sink.push({ + header: h3[1].trim(), + section: skipped.section, + line: skipped.bodyStartLine + index, + }); + } + }; const blocks: RequirementBlock[] = []; let i = 0; while (i < lines.length) { // Seek next requirement header - while (i < lines.length && !REQUIREMENT_HEADER_REGEX.test(lines[i])) i++; + while (i < lines.length && !REQUIREMENT_HEADER_REGEX.test(lines[i])) { + recordIfSkippedHeader(i); + i++; + } if (i >= lines.length) break; const headerLine = lines[i]; const m = headerLine.match(REQUIREMENT_HEADER_REGEX); @@ -188,6 +237,7 @@ function parseRequirementBlocksFromSection(sectionBody: string): RequirementBloc const buf: string[] = [headerLine]; i++; while (i < lines.length && !REQUIREMENT_HEADER_REGEX.test(lines[i]) && !/^##\s+/.test(lines[i])) { + recordIfSkippedHeader(i); buf.push(lines[i]); i++; } @@ -215,48 +265,6 @@ function parseRemovedNames(sectionBody: string): string[] { return names; } -/** - * Find level-3 headers inside `## ADDED`/`## MODIFIED Requirements` sections - * that are NOT canonical `### Requirement:` headers — the ones the delta reader - * silently skips. Fence-aware: headers inside fenced examples are ignored. Used - * to surface (as INFO, not a recognition change) headers like a stray - * `### Documentation Requirements` divider that `validate ` would - * otherwise pass without comment. - */ -export function findNonRequirementLevel3Headers( - content: string -): Array<{ header: string; section: string; line: number }> { - const normalized = normalizeLineEndings(content); - const lines = normalized.split('\n'); - const mask = buildCodeFenceMask(lines); - const results: Array<{ header: string; section: string; line: number }> = []; - - let currentSection: string | null = null; - for (let i = 0; i < lines.length; i++) { - if (mask[i]) continue; // inside a fenced code block - const line = lines[i]; - - const h2 = line.match(/^##\s+(.+?)\s*$/); - if (h2) { - const title = h2[1].trim().toLowerCase(); - currentSection = - title === 'added requirements' || title === 'modified requirements' - ? h2[1].trim() - : null; - continue; - } - - if (!currentSection) continue; - - const h3 = line.match(/^###\s+(.+?)\s*$/); - if (h3 && !REQUIREMENT_HEADER_REGEX.test(line)) { - results.push({ header: h3[1].trim(), section: currentSection, line: i + 1 }); - } - } - - return results; -} - function parseRenamedPairs(sectionBody: string): Array<{ from: string; to: string }> { if (!sectionBody) return []; const pairs: Array<{ from: string; to: string }> = []; diff --git a/src/core/parsers/requirement-text.ts b/src/core/parsers/requirement-text.ts index 4bfeb65e23..2140860d12 100644 --- a/src/core/parsers/requirement-text.ts +++ b/src/core/parsers/requirement-text.ts @@ -68,7 +68,15 @@ function isClosingFence( /** Lines that look like `**ID**: ...` / `**Priority**: ...` metadata. */ const METADATA_LINE = /^\*\*[^*]+\*\*:/; -/** A level-4 scenario header (`#### Scenario: ...`). */ +/** Any markdown header line — the boundary where a requirement body ends. */ +const HEADER_LINE = /^#{1,6}\s/; + +/** + * A level-4 header. Deliberately matches ANY `####` header, not only + * `#### Scenario:` — the spec path treats every level-4 child of a requirement + * as a scenario, so the delta counter must too (parity). Don't tighten this to + * `Scenario:` without changing both paths together. + */ const SCENARIO_HEADER = /^####\s+/; /** @@ -84,27 +92,47 @@ export function containsShallOrMust(text: string): boolean { * Extract the full requirement body from the lines that follow a * `### Requirement:` header (the lines may include scenarios and fenced code). * - * Captures every body line from the start up to the first `#### Scenario:` - * header found on a non-fenced line, skipping blank lines, `**metadata**:` - * lines, and any line inside a fenced code block. Captured lines are trimmed and - * joined with newlines so a requirement whose text wraps across lines — or whose - * `SHALL`/`MUST` lands on a later line — is read in full. + * Captures every body line from the start up to the first header found on a + * non-fenced line — usually the first `#### Scenario:`, but also a stray `###` + * divider the delta reader absorbed into the block — skipping blank lines and + * any line inside a fenced code block. `**metadata**:` lines are skipped only + * when other body text remains: a requirement written entirely as + * `**Constraint**: The system MUST ...` keeps that line as its body. Captured + * lines are trimmed and joined with newlines so a requirement whose text wraps + * across lines — or whose `SHALL`/`MUST` lands on a later line — is read in + * full. */ export function extractRequirementBody(bodyLines: string[]): string { const mask = buildCodeFenceMask(bodyLines); const captured: string[] = []; + const metadata: string[] = []; for (let i = 0; i < bodyLines.length; i++) { if (mask[i]) continue; // inside a fenced code block const line = bodyLines[i]; - if (SCENARIO_HEADER.test(line)) break; // reached the first real scenario + if (HEADER_LINE.test(line)) break; // first scenario or stray divider const trimmed = line.trim(); if (trimmed.length === 0) continue; // blank - if (METADATA_LINE.test(trimmed)) continue; // **ID**: / **Priority**: ... + if (METADATA_LINE.test(trimmed)) { + metadata.push(trimmed); // **ID**: / **Priority**: ... + continue; + } captured.push(trimmed); } - return captured.join('\n'); + if (captured.length > 0) return captured.join('\n'); + return metadata.join('\n'); // metadata-only body: the metadata IS the body +} + +/** + * The one empty-body rule both readers share: a requirement block with no body + * text falls back to its header title. This is what lets a bare + * `### The system SHALL ...` header validate on the spec path (the title is the + * requirement), and it keeps the delta path from reaching a different verdict + * than the spec path for the same block. + */ +export function extractRequirementText(headerTitle: string, bodyLines: string[]): string { + return extractRequirementBody(bodyLines) || headerTitle.trim(); } /** diff --git a/src/core/validation/validator.ts b/src/core/validation/validator.ts index 59a1e0cdc4..35755d73dc 100644 --- a/src/core/validation/validator.ts +++ b/src/core/validation/validator.ts @@ -10,9 +10,9 @@ import { MAX_REQUIREMENT_TEXT_LENGTH, VALIDATION_MESSAGES } from './constants.js'; -import { parseDeltaSpec, normalizeRequirementName, findNonRequirementLevel3Headers } from '../parsers/requirement-blocks.js'; +import { parseDeltaSpec, normalizeRequirementName } from '../parsers/requirement-blocks.js'; import { - extractRequirementBody, + extractRequirementText as extractRequirementTextShared, containsShallOrMust as containsShallOrMustShared, countScenarios as countScenariosShared, } from '../parsers/requirement-text.js'; @@ -140,17 +140,21 @@ export class Validator { const plan = parseDeltaSpec(content); const entryPath = `${specName}/spec.md`; - // Surface (as INFO, never a failure) any level-3 header inside an - // ADDED/MODIFIED section that is not a canonical "### Requirement:" - // header — the delta reader silently skips these, so without this note - // a stray divider like "### Documentation Requirements" would pass - // validate while failing archive/validate . - for (const stray of findNonRequirementLevel3Headers(content)) { + // Surface (as INFO, never a failure) the non-canonical level-3 headers + // the delta reader skipped while parsing ADDED/MODIFIED sections — + // without this note a stray divider like "### Documentation + // Requirements" would pass validate while failing + // archive/validate . The list comes from the parse itself, so it + // reflects exactly what the reader skipped. + for (const stray of plan.skippedHeaders) { + const nameless = /^requirement:?$/i.test(stray.header); issues.push({ level: 'INFO', path: entryPath, line: stray.line, - message: `Header "### ${stray.header}" in ${stray.section} is not a "### Requirement:" header and is ignored by validation. Use "### Requirement: ${stray.header}" if it should be validated as a requirement.`, + message: nameless + ? `Header "### ${stray.header}" in ${stray.section} is missing a requirement name and is ignored by validation. Add a name, e.g. "### Requirement: ".` + : `Header "### ${stray.header}" in ${stray.section} is not a "### Requirement:" header and is ignored by validation. Use "### Requirement: ${stray.header}" if it should be validated as a requirement.`, }); } @@ -433,11 +437,13 @@ export class Validator { } private extractRequirementText(blockRaw: string): string | undefined { - // Delegate to the shared, fence-/metadata-/multi-line-aware reader so the - // change-delta path and the main-spec path cannot diverge. Drop the header - // line (index 0) and read the body that follows. - const body = extractRequirementBody(blockRaw.split('\n').slice(1)); - return body || undefined; + // Delegate to the shared, fence-/metadata-/multi-line-aware reader (with + // the shared header-title fallback for empty bodies) so the change-delta + // path and the main-spec path cannot diverge. Line 0 is the + // "### Requirement: ..." header. + const [headerLine, ...bodyLines] = blockRaw.split('\n'); + const headerTitle = headerLine.replace(/^#{1,6}\s*/, ''); + return extractRequirementTextShared(headerTitle, bodyLines) || undefined; } private containsShallOrMust(text: string): boolean { diff --git a/test/core/parsers/markdown-parser.test.ts b/test/core/parsers/markdown-parser.test.ts index 1b93b764a7..7083fd95f2 100644 --- a/test/core/parsers/markdown-parser.test.ts +++ b/test/core/parsers/markdown-parser.test.ts @@ -406,6 +406,32 @@ Then result`; expect(spec.requirements[0].text).toBe('The system MUST persist the uploaded file.'); }); + it('keeps a metadata-only body as the requirement text', () => { + const content = `# Test Spec + +## Purpose +Test overview for metadata-only requirement bodies. + +## Requirements + +### Requirement: Constraint style +**Constraint**: The system MUST respond within the configured deadline. + +#### Scenario: Test +Given test +When action +Then result`; + + const parser = new MarkdownParser(content); + const spec = parser.parseSpec('test'); + + // Metadata lines are skipped only when other body text remains; when the + // whole body is metadata, the metadata IS the body. + expect(spec.requirements[0].text).toBe( + '**Constraint**: The system MUST respond within the configured deadline.' + ); + }); + it('ignores a fenced code block that precedes the prose line (#312)', () => { const content = `# Test Spec diff --git a/test/core/validation.test.ts b/test/core/validation.test.ts index ea74d60736..7505cbee0e 100644 --- a/test/core/validation.test.ts +++ b/test/core/validation.test.ts @@ -882,7 +882,7 @@ ${body}`; expect(specReport.valid).toBe(false); }); - it('guard: a metadata-only body still fails validation (no requirement text)', async () => { + it('guard: a metadata-only body without a keyword still fails validation', async () => { const delta = `# Test Spec ## ADDED Requirements @@ -899,7 +899,167 @@ ${body}`; const changeDir = await writeChangeDelta('fidelity-metadata-only', delta); const report = await new Validator(true).validateChangeDeltaSpecs(changeDir); expect(report.valid).toBe(false); - expect(report.summary.errors).toBeGreaterThan(0); + // The metadata IS the body when nothing else remains, so the failure is + // the missing keyword, not missing text. + expect( + report.issues.some(i => i.message.includes('must contain SHALL or MUST')) + ).toBe(true); + }); + + it('a requirement written entirely as **Constraint**: metadata keeps its MUST (change and spec)', async () => { + const body = `### Requirement: Constraint style +**Constraint**: The system MUST respond within the configured deadline. + +#### Scenario: Deadline honored +**Given** a configured deadline +**When** a request is handled +**Then** the response arrives in time`; + + const changeDir = await writeChangeDelta('fidelity-constraint-only', `# Test Spec\n\n## ADDED Requirements\n\n${body}`); + const changeReport = await new Validator(true).validateChangeDeltaSpecs(changeDir); + expect(changeReport.valid).toBe(true); + expect(changeReport.summary.errors).toBe(0); + + const spec = `# Test Spec + +## Purpose +This spec exercises a requirement whose whole body is a metadata-style line. + +## Requirements + +${body}`; + const specPath = await writeSpec('fidelity-constraint-only-spec', spec); + const specReport = await new Validator(true).validateSpec(specPath); + expect(specReport.valid).toBe(true); + expect(specReport.summary.errors).toBe(0); + }); + + it('empty body falls back to the header title identically on both paths', async () => { + const body = `### Requirement: The tool MUST support header-only requirements + +#### Scenario: Header only +**Given** a requirement with no body text +**When** it is validated +**Then** both paths reach the same verdict`; + + const changeDir = await writeChangeDelta('fidelity-empty-body', `# Test Spec\n\n## ADDED Requirements\n\n${body}`); + const changeReport = await new Validator(true).validateChangeDeltaSpecs(changeDir); + expect(changeReport.valid).toBe(true); + expect(changeReport.summary.errors).toBe(0); + + const spec = `# Test Spec + +## Purpose +This spec exercises the shared empty-body fallback to the header title. + +## Requirements + +${body}`; + const specPath = await writeSpec('fidelity-empty-body-spec', spec); + const specReport = await new Validator(true).validateSpec(specPath); + expect(specReport.valid).toBe(true); + expect(specReport.summary.errors).toBe(0); + }); + + it('a stray ### divider ends the requirement body: a MUST in its notes does not count', async () => { + const delta = `# Test Spec + +## ADDED Requirements + +### Requirement: Divider absorbed +The system performs the described behavior without a keyword. + +### Background +These notes explain that the system MUST NOT be read as requirement text. + +#### Scenario: Bounded +**Given** a stray divider +**When** the requirement is read +**Then** the body stops at the divider`; + + const changeDir = await writeChangeDelta('fidelity-divider-body', delta); + const report = await new Validator(true).validateChangeDeltaSpecs(changeDir); + + // The body ends at "### Background", so the MUST in the notes is not + // seen and the requirement fails the keyword check (as it did on main) — + // and the skipped divider is surfaced as INFO. + expect(report.valid).toBe(false); + expect( + report.issues.some(i => i.level === 'ERROR' && i.message.includes('must contain SHALL or MUST')) + ).toBe(true); + expect( + report.issues.some(i => i.level === 'INFO' && i.message.includes('"### Background"')) + ).toBe(true); + }); + + it('a nameless "### Requirement:" header gets a dedicated INFO message', async () => { + const delta = `# Test Spec + +## ADDED Requirements + +### Requirement: + +### Requirement: Real requirement +The system SHALL do the real thing. + +#### Scenario: Works +**Given** a request +**When** it is handled +**Then** the behavior occurs`; + + const changeDir = await writeChangeDelta('fidelity-nameless', delta); + const report = await new Validator(true).validateChangeDeltaSpecs(changeDir); + + expect(report.valid).toBe(true); + const info = report.issues.find( + i => i.level === 'INFO' && i.message.includes('missing a requirement name') + ); + expect(info).toBeDefined(); + expect(info!.message).not.toContain('Requirement: Requirement:'); + }); + + it('the skipped-header INFO reflects the reader: a fenced divider is not reported', async () => { + const delta = `# Test Spec + +## ADDED Requirements + +### Requirement: Fence with divider example +The system SHALL treat fenced headers as content. + +\`\`\`markdown +### Not A Real Divider +\`\`\` + +#### Scenario: Fenced +**Given** a fenced example containing a level-3 header +**When** the delta is validated +**Then** no INFO note is emitted for it`; + + const changeDir = await writeChangeDelta('fidelity-fenced-divider', delta); + const report = await new Validator(true).validateChangeDeltaSpecs(changeDir); + + expect(report.valid).toBe(true); + expect(report.summary.info).toBe(0); + }); + + it('any #### header counts as a scenario on the delta path (deliberate spec-path parity)', async () => { + const delta = `# Test Spec + +## ADDED Requirements + +### Requirement: Notes as scenario +The system SHALL accept any level-4 child, matching the spec path. + +#### Notes +The spec path treats every level-4 child of a requirement as a scenario.`; + + const changeDir = await writeChangeDelta('fidelity-h4-parity', delta); + const report = await new Validator(true).validateChangeDeltaSpecs(changeDir); + + // The spec path (parseScenarios) counts every level-4 child with content + // as a scenario, so the delta counter deliberately does the same. + expect(report.valid).toBe(true); + expect(report.summary.errors).toBe(0); }); }); }); From a7cc64c726416b6b8b802e8c102e4df33a51e698 Mon Sep 17 00:00:00 2001 From: Clay Good Date: Fri, 3 Jul 2026 10:03:27 -0500 Subject: [PATCH 8/8] docs(openspec): record the no-space ###Requirement: divergence as a known leftover MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Jun's edge (reproduced): the delta/write reader's REQUIREMENT_HEADER_REGEX accepts `###Requirement:` with no space, but MarkdownParser.parseSections requires whitespace (per GFM) — so a no-space requirement validates as a change with zero INFO, syncs as-is, then fails validate . Pre-existing on main and out of scope here (tightening the shared regex would change write-path recognition); documented under known remaining divergences with the follow-up options, folded together with the bullet from the merge resolution. Corrects c63913b's 'no divergence' note. Co-Authored-By: Claude Fable 5 --- openspec/changes/fix-spec-parser-fidelity/design.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/openspec/changes/fix-spec-parser-fidelity/design.md b/openspec/changes/fix-spec-parser-fidelity/design.md index 4caba76d8d..f9909941aa 100644 --- a/openspec/changes/fix-spec-parser-fidelity/design.md +++ b/openspec/changes/fix-spec-parser-fidelity/design.md @@ -65,7 +65,7 @@ Unification closes the reproduced defects; these divergences remain and are acce - **Empty scenarios** — a `#### Scenario:` header with no body counts on the delta path (`countScenarios` counts headers) but not on the spec path (`parseScenarios` keeps only scenarios with content), so `validate ` passes what `validate `/`archive` rejects. - **Recognition** — bare `### ` headers are requirements on the spec path but skipped on the delta path. Deliberate (see "Why recognition tightening is rejected"); the Part B INFO note surfaces it instead of unifying it. -- **No-space canonical headers** — `###Requirement:` is accepted by the delta/write requirement-block reader but is not a Markdown heading on the spec path, so it can validate before sync and fail after sync. Closing this should be a separate compatibility change: either deprecate no-space headers with INFO/WARN first, or broaden the skipped-header scanner before tightening recognition. +- **No-space `###Requirement:` headers** — `REQUIREMENT_HEADER_REGEX` (`\s*` after `###`) accepts them on the delta and write paths, but `MarkdownParser.parseSections` requires whitespace (matching GFM, which does not treat `###Requirement:` as a heading). So a no-space requirement validates as a change with zero INFO (the reader accepts it, so the skip note never fires), syncs into the main spec as-is, and the synced spec then fails `validate ` — the same shape as #498. Pre-existing (both regexes unchanged from `main`) and accepted here: the no-space form is a tested normalization case (`requirement-blocks.test.ts`), and tightening the shared regex would change write-path recognition. Closing it should be a separate compatibility change — deprecate no-space headers with an INFO/WARN first, or broaden the skipped-header collection to any `^###` line before tightening recognition. - **Delta section/block splitting is not fence-aware** — `splitTopLevelSections` and `parseRequirementBlocksFromSection` treat a fenced `## ...` line as a section boundary and a fenced `### Requirement:` line as a new block, while the spec path fence-masks its sectioning. The skipped-header INFO is collected during the actual parse precisely so it reflects these boundaries instead of describing different ones. ## Prior art