diff --git a/.changeset/spec-parser-reading-fidelity.md b/.changeset/spec-parser-reading-fidelity.md new file mode 100644 index 0000000000..a5d82f1f61 --- /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, 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). 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, 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. + +- **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/.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..f9909941aa --- /dev/null +++ b/openspec/changes/fix-spec-parser-fidelity/design.md @@ -0,0 +1,77 @@ +# 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 `, `archive` | `validate ` | + +`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 ` `✗ 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'`.) + +## 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 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, `MarkdownParser` still falls back to the header title for display and bare-header compatibility; validator body-keyword checks for canonical `### Requirement:` blocks use the body-only extraction so #1280's "keyword only in header" hint remains intact on both validation paths. 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) + +`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 + +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: + +- `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`). + +## 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. +- **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 + +`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. 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..f56cce97f7 --- /dev/null +++ b/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 ` 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 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 ` 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 + +### 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 ` 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 `### ` 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` — schema-level `SHALL`/`MUST` enforcement stays removed after #1280; the imperative validator uses 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. 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..4791804218 --- /dev/null +++ b/openspec/changes/fix-spec-parser-fidelity/specs/cli-validate/spec.md @@ -0,0 +1,69 @@ +## 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 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. Detection SHALL run over the full captured body. Canonical `### Requirement:` blocks with no body text SHALL NOT satisfy body-keyword validation from the header title alone; they SHALL receive the existing body-keyword hint when the keyword appears only in the header. The Markdown parser MAY still use the header title as display text for supported bare-header specs. The change-delta reader and the main-spec validator SHALL share this body 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 +- **WHEN** running `openspec validate --strict` for both a change delta and a main spec +- **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**:` lines before a `MUST` description +- **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 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 +- **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 + +#### 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 ` + +### 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 (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 +- **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. 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 +- **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: 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` +- **THEN** those headers SHALL continue to be recognized as requirements exactly as before this change 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..0c9a3f28ba --- /dev/null +++ b/openspec/changes/fix-spec-parser-fidelity/tasks.md @@ -0,0 +1,43 @@ +## 1. Part A — shared, fence-aware extraction (#361, #418, #312, fenced-scenario) + +- [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 + +- [x] 2.1 Use the shared `containsShallOrMust` (`/\b(SHALL|MUST)\b/`) for validator keyword checks; after the #1280 merge, schema-level keyword enforcement remains removed and owned by the imperative validator. + +## 3. Part B — surface the #498 divergence (INFO, no recognition change) + +- [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 + +- [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 + +- [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 + +- [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 Keep header-title fallback in the Markdown parser for display/bare-header compatibility, while validator checks use body-only extraction so canonical header-only requirements still receive the #1280 body-keyword hint. +- [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/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..8dca1ef64f 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, extractRequirementText } 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,20 @@ 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(); - } - } - } - + // 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); - + requirements.push({ text, scenarios, }); } - + return requirements; } diff --git a/src/core/parsers/requirement-blocks.ts b/src/core/parsers/requirement-blocks.ts index afc55f8914..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' @@ -16,6 +18,7 @@ export function normalizeRequirementName(name: string): string { return name.trim(); } +/** The canonical requirement header the delta reader recognizes. */ const REQUIREMENT_HEADER_REGEX = /^###\s*Requirement:\s*(.+)\s*$/i; /** @@ -96,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; @@ -123,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, @@ -141,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+(.+)$/); @@ -156,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); @@ -185,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++; } diff --git a/src/core/parsers/requirement-text.ts b/src/core/parsers/requirement-text.ts new file mode 100644 index 0000000000..8aa0e89567 --- /dev/null +++ b/src/core/parsers/requirement-text.ts @@ -0,0 +1,151 @@ +/** + * 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 for requirement-body + * extraction, scenario counting, and `SHALL`/`MUST` detection in + * `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 = /^\*\*[^*]+\*\*:/; + +/** 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+/; + +/** + * 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 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 (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)) { + metadata.push(trimmed); // **ID**: / **Priority**: ... + continue; + } + captured.push(trimmed); + } + + if (captured.length > 0) return captured.join('\n'); + return metadata.join('\n'); // metadata-only body: the metadata IS the body +} + +/** + * Parser/display fallback for a requirement block with no body text. This is + * what lets a bare `### The system SHALL ...` header remain readable on the + * spec path (the title is the requirement). Validator body-keyword checks for + * canonical `### Requirement:` blocks use `extractRequirementBody` directly so + * a keyword that appears only in the header still receives the #1156/#1280 + * body-keyword hint. + */ +export function extractRequirementText(headerTitle: string, bodyLines: string[]): string { + return extractRequirementBody(bodyLines) || headerTitle.trim(); +} + +/** + * 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 aa08cea1e9..a6472ddb0a 100644 --- a/src/core/schemas/base.schema.ts +++ b/src/core/schemas/base.schema.ts @@ -19,4 +19,4 @@ export const RequirementSchema = z.object({ }); export type Scenario = z.infer; -export type Requirement = z.infer; \ No newline at end of file +export type Requirement = z.infer; diff --git a/src/core/validation/validator.ts b/src/core/validation/validator.ts index 4f896fb3a4..511662f294 100644 --- a/src/core/validation/validator.ts +++ b/src/core/validation/validator.ts @@ -11,6 +11,11 @@ import { VALIDATION_MESSAGES } from './constants.js'; import { parseDeltaSpec, normalizeRequirementName, extractRequirementsSection } from '../parsers/requirement-blocks.js'; +import { + extractRequirementBody as extractRequirementBodyShared, + 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'; @@ -135,6 +140,25 @@ export class Validator { const plan = parseDeltaSpec(content); const entryPath = FileSystemUtils.toPosixPath(path.relative(specsDir, specFile)); + + // 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: 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.`, + }); + } + const sectionNames: string[] = []; if (plan.sectionPresence.added) sectionNames.push('## ADDED Requirements'); if (plan.sectionPresence.modified) sectionNames.push('## MODIFIED Requirements'); @@ -164,7 +188,13 @@ export class Validator { } const requirementText = this.extractRequirementText(block.raw); if (!requirementText) { - issues.push({ level: 'ERROR', path: entryPath, message: `ADDED "${block.name}" is missing requirement text` }); + issues.push({ + level: 'ERROR', + path: entryPath, + message: this.containsShallOrMust(block.name) + ? this.buildMissingShallOrMustMessage(`ADDED "${block.name}"`, block.name) + : `ADDED "${block.name}" is missing requirement text`, + }); } else if (!this.containsShallOrMust(requirementText)) { issues.push({ level: 'ERROR', path: entryPath, message: this.buildMissingShallOrMustMessage(`ADDED "${block.name}"`, block.name) }); } @@ -185,7 +215,13 @@ export class Validator { } const requirementText = this.extractRequirementText(block.raw); if (!requirementText) { - issues.push({ level: 'ERROR', path: entryPath, message: `MODIFIED "${block.name}" is missing requirement text` }); + issues.push({ + level: 'ERROR', + path: entryPath, + message: this.containsShallOrMust(block.name) + ? this.buildMissingShallOrMustMessage(`MODIFIED "${block.name}"`, block.name) + : `MODIFIED "${block.name}" is missing requirement text`, + }); } else if (!this.containsShallOrMust(requirementText)) { issues.push({ level: 'ERROR', path: entryPath, message: this.buildMissingShallOrMustMessage(`MODIFIED "${block.name}"`, block.name) }); } @@ -460,35 +496,17 @@ 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 body reader. + // Validation intentionally does not use the parser/display header-title + // fallback for canonical `### Requirement:` blocks: #1280 requires a + // SHALL/MUST that appears only in the header to receive the body-keyword + // hint. Line 0 is the `### Requirement: ...` header. + const [, ...bodyLines] = blockRaw.split('\n'); + return extractRequirementBodyShared(bodyLines) || undefined; } private containsShallOrMust(text: string): boolean { - return /\b(SHALL|MUST)\b/.test(text); + return containsShallOrMustShared(text); } /** @@ -510,8 +528,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..7083fd95f2 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,168 @@ 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('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 + +## 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 d7104aa42d..271d162ab4 100644 --- a/test/core/validation.test.ts +++ b/test/core/validation.test.ts @@ -810,4 +810,393 @@ The system MUST support mixed case delta headers. expect(report.issues.some(i => i.message.includes('not only in the header'))).toBe(false); }); }); + + 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); + }); + + 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 without a keyword still fails validation', 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); + // 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('canonical empty bodies keep the body-keyword hint on both paths after #1280', 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 ask for the keyword in the body`; + + 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(false); + expect( + changeReport.issues.some(i => i.message.includes('not only in the header')) + ).toBe(true); + + const spec = `# Test Spec + +## Purpose +This spec exercises the shared body extraction without using the display fallback for validation. + +## Requirements + +${body}`; + const specPath = await writeSpec('fidelity-empty-body-spec', spec); + const specReport = await new Validator(true).validateSpec(specPath); + expect(specReport.valid).toBe(false); + expect( + specReport.issues.some(i => i.message.includes('not only in the header')) + ).toBe(true); + }); + + 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); + }); + }); });