refactor: unify requirement reader and surface #498

.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 <change>`, `validate <spec>`, 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 <change>`, matching `validate <spec>`.
+  - `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 <change>` 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).

openspec/changes/fix-spec-parser-fidelity/.openspec.yaml

@@ -0,0 +1,2 @@
+schema: spec-driven
+created: 2026-06-29

openspec/changes/fix-spec-parser-fidelity/design.md

@@ -0,0 +1,68 @@
+# Design: Spec parser reading fidelity
+
+## The requirement reader is implemented twice
+
+| | 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 <spec>`, `archive` | `validate <change>` |
+
+`ChangeParser extends MarkdownParser` and reuses `parseRequirements`, so there is no third reader. Every row where the two columns differ is a reproduced defect.
+
+## Reproductions (against `main`)
+
+- **#361** — `### Requirement: …` with `SHALL` on body line 2 → `validate <change>` `✗ must contain SHALL or MUST`; `validate <spec>` `✗ requirements.0.text: …`.
+- **#418** — metadata lines before a `MUST` description → `validate <change>` **valid**; `validate <spec>` `✗`, `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 <change>` **valid** (counts the fenced scenario); `validate <spec>` `✗ requirements.0.scenarios: must have at least one scenario`. The delta reader passes a malformed requirement.
+- **#498** — stray `### Documentation Requirements` divider → `validate <change>` **valid**; `archive` prints non-blocking phantom `Proposal warnings in proposal.md`; `validate <spec>` blocking `✗`. (Also: `show`/`view` count the divider as a requirement — `count=2` with `text='Documentation Notes'`.)
+
+## Approach
+
+### 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.
+
+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 <change>` stop *silently* passing the #498 input.
+
+## Why recognition tightening is rejected
+
+The obvious #498 fix is to make `parseRequirements` recognize only `### Requirement:` headers. It is rejected because **bare `### <statement>` headers are a supported, tested requirement format**, not a convention violation:
+
+- `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`).
+
+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.
+
+## Safety: write path is independent of the reader
+
+`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.)
+
+## Read-only blast radius (no write path)
+
+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.
+
+## Edge cases for tests
+
+- 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`).
+
+## 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.
+
+## Out of scope: #559
+
+Deferred — transcript shows an unqualified `changes/<id>/...` path (missing `openspec/` prefix), not a demonstrated folder-vs-title mismatch.

openspec/changes/fix-spec-parser-fidelity/proposal.md

@@ -0,0 +1,71 @@
+## 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: the requirement **reader** is implemented twice — `MarkdownParser.parseRequirements` (used by `validate <spec>` and `archive`) and `Validator.extractRequirementText` + `countScenarios` (used by `validate <change>`) — 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 readers differ in ways that are each a reproduced bug:
+
+| | 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` |
+
+### Reproduced bugs
+
+- **#361 — wrapped keyword invisible.** Both readers capture only the first body line, so a `SHALL`/`MUST` on line 2 fails both `validate <change>` and `validate <spec>`.
+- **#418 — metadata before description, spec path only.** A requirement that opens with `**ID**:`/`**Priority**:` lines passes `validate <change>` (delta reader skips metadata) but fails `validate <spec>` (`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 <change>` — while the same content correctly fails `validate <spec>`. A malformed delta slips through the gate.
+- **#498 — validate and archive disagree.** `validate <change>` 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 <change>` but flagged by `archive` (non-blocking phantom warning) and `validate <spec>` (blocking error). The author gets no signal at validate time.
+
+## What Changes
+
+### 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:
+
+- 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.
+
+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.
+
+### Part B — make the #498 divergence visible (safe, no recognition change)
+
+`validate <change>` 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.
+
+### Rejected: tightening recognition to `### Requirement:` only
+
+The tempting #498 fix — make `parseRequirements` recognize only `### Requirement:` headers — is **rejected**. Bare `### <statement>` 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 — its transcript shows an unqualified `changes/...` path, not a proven folder-vs-title mismatch.
+
+## Safety: the archive write path is unaffected
+
+`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`.
+
+## Existing-test impact
+
+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
+
+### New Capabilities
+
+_None._
+
+### Modified Capabilities
+
+- `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.
+- `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: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.