refactor: unify requirement reader and surface #498#1281
Conversation
…I#361, Fission-AI#498, Fission-AI#312) The requirement-parsing layer silently misreads valid Markdown: - Fission-AI#361: requirement-body extraction returns only the first non-blank line, so a SHALL/MUST that wraps onto line 2 fails `validate --strict`. - Fission-AI#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. - Fission-AI#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. Fission-AI#559 investigated and deferred (ambiguous root cause — see design.md). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a new ChangesParser Fidelity Fix Planning
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…vidence Hardened the proposal after reproducing every claim against main with the bundled CLI and correcting two inaccuracies: - Fission-AI#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. - Fission-AI#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) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
openspec/changes/fix-spec-parser-fidelity/design.md (1)
67-68: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winClarify the display-text mitigation strategy.
The risk mitigation states "keep display text concise (first line) while running detection over the full body, or document the widened
text" — this ambiguity leaves implementers without a single source of truth. Commit to one approach: either preserve first-line display consistently (with full body for validation only), or explicitly widenreq.textand document the breaking change. The "or" creates uncertainty for downstream consumers likeviewandshow.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openspec/changes/fix-spec-parser-fidelity/design.md` around lines 67 - 68, Clarify the mitigation in the spec parser design by choosing one display-text behavior and reflecting it consistently in the relevant spec text: either keep `req.text` as the first line while using the full body only for requirement detection, or explicitly widen `req.text` and document that as a breaking change. Update the section mentioning `view`/`show` counts and downstream display so `design.md` states a single source of truth for `req.text`, removing the current “or” ambiguity.openspec/changes/fix-spec-parser-fidelity/specs/cli-validate/spec.md (1)
29-36: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueClarify the "whole words" specification.
The requirement states the predicate "SHALL match
SHALLorMUSTas whole words" but doesn't define what constitutes a word boundary. For spec completeness and to prevent implementation drift, either:
- Reference the shared predicate by name (e.g.,
containsShallOrMust), or- Include the boundary pattern explicitly (e.g.,
/\b(SHALL|MUST)\b/).This ensures the scenario "Keyword detection agrees across paths" has an unambiguous acceptance criterion.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openspec/changes/fix-spec-parser-fidelity/specs/cli-validate/spec.md` around lines 29 - 36, The normative-keyword predicate description is ambiguous because “whole words” is not defined, which can lead to drift between validation paths. Update the requirement text in cli-validate/spec.md to either reference the shared predicate name used by the validator (such as containsShallOrMust) or state the exact word-boundary rule explicitly so both the delta-spec and schema-based paths use the same acceptance criterion.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@openspec/changes/fix-spec-parser-fidelity/design.md`:
- Around line 67-68: Clarify the mitigation in the spec parser design by
choosing one display-text behavior and reflecting it consistently in the
relevant spec text: either keep `req.text` as the first line while using the
full body only for requirement detection, or explicitly widen `req.text` and
document that as a breaking change. Update the section mentioning `view`/`show`
counts and downstream display so `design.md` states a single source of truth for
`req.text`, removing the current “or” ambiguity.
In `@openspec/changes/fix-spec-parser-fidelity/specs/cli-validate/spec.md`:
- Around line 29-36: The normative-keyword predicate description is ambiguous
because “whole words” is not defined, which can lead to drift between validation
paths. Update the requirement text in cli-validate/spec.md to either reference
the shared predicate name used by the validator (such as containsShallOrMust) or
state the exact word-boundary rule explicitly so both the delta-spec and
schema-based paths use the same acceptance criterion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 693c293e-6ee4-435d-aec0-8110b02cbc6e
📒 Files selected for processing (6)
openspec/changes/fix-spec-parser-fidelity/design.mdopenspec/changes/fix-spec-parser-fidelity/proposal.mdopenspec/changes/fix-spec-parser-fidelity/specs/cli-archive/spec.mdopenspec/changes/fix-spec-parser-fidelity/specs/cli-validate/spec.mdopenspec/changes/fix-spec-parser-fidelity/specs/openspec-conventions/spec.mdopenspec/changes/fix-spec-parser-fidelity/tasks.md
✅ Files skipped from review due to trivial changes (2)
- openspec/changes/fix-spec-parser-fidelity/tasks.md
- openspec/changes/fix-spec-parser-fidelity/specs/cli-archive/spec.md
🚧 Files skipped from review as they are similar to previous changes (1)
- openspec/changes/fix-spec-parser-fidelity/specs/openspec-conventions/spec.md
… upgrade Fission-AI#312, tier the risk Second adversarial bulletproofing pass (reproduced everything against main): - Add Fission-AI#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 Fission-AI#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 Fission-AI#361/Fission-AI#418/Fission-AI#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 Fission-AI#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 Fission-AI#1156 (PR Fission-AI#1280). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
clay-good
changed the title
…d-scenario bug, Fission-AI#498→safe INFO Third deep pass found the prior Tier 2 (recognition tightening to `### Requirement:`) was the WRONG fix and over-scoped: - Bare `### <statement>` 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 Fission-AI#498 fix with a SAFE INFO note in validate <change> 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 <change> while validate <spec> 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) <noreply@anthropic.com>
clay-good
changed the title
There was a problem hiding this comment.
🧹 Nitpick comments (2)
openspec/changes/fix-spec-parser-fidelity/specs/cli-validate/spec.md (2)
42-50: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winClarify scope: does the INFO-note requirement apply only to change deltas?
The requirement specifies "When an
## ADDED/## MODIFIED Requirementssection in a change delta contains..." which correctly scopes it to change deltas. However, the relationship with the subsequent bare-header requirement (lines 51-54) could be clearer. Consider adding an explicit cross-reference note that this INFO-note behavior is specific to change-delta parsing and does not apply to main-spec bare headers, to prevent implementer confusion about whether bare### <statement>headers in main specs should also trigger INFO notes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openspec/changes/fix-spec-parser-fidelity/specs/cli-validate/spec.md` around lines 42 - 50, Clarify that the INFO-note behavior is scoped only to change-delta parsing in the validate spec, not main-spec bare headers. Update the Requirement and Scenario around the `## ADDED`/`## MODIFIED Requirements` section to explicitly reference the separate bare-header behavior so implementers know `openspec validate` should emit INFO only for non-canonical level-3 headers inside change deltas, while the bare `### <statement>` handling in main specs remains unchanged.
22-22: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCapitalize "Markdown" as proper noun.
"Markdown" is a proper noun referring to the specific markup language and should be capitalized.
-The validator and markdown parser SHALL ignore lines inside fenced code blocks +The validator and Markdown parser SHALL ignore lines inside fenced code blocks🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openspec/changes/fix-spec-parser-fidelity/specs/cli-validate/spec.md` at line 22, Update the spec wording in the CLI validate documentation so “markdown” is capitalized as “Markdown” wherever it appears in the affected requirement text. Keep the existing meaning unchanged, and adjust the sentence in the spec entry that describes the validator and Markdown parser behavior so the proper noun is consistently capitalized.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@openspec/changes/fix-spec-parser-fidelity/specs/cli-validate/spec.md`:
- Around line 42-50: Clarify that the INFO-note behavior is scoped only to
change-delta parsing in the validate spec, not main-spec bare headers. Update
the Requirement and Scenario around the `## ADDED`/`## MODIFIED Requirements`
section to explicitly reference the separate bare-header behavior so
implementers know `openspec validate` should emit INFO only for non-canonical
level-3 headers inside change deltas, while the bare `### <statement>` handling
in main specs remains unchanged.
- Line 22: Update the spec wording in the CLI validate documentation so
“markdown” is capitalized as “Markdown” wherever it appears in the affected
requirement text. Keep the existing meaning unchanged, and adjust the sentence
in the spec entry that describes the validator and Markdown parser behavior so
the proper noun is consistently capitalized.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ec15f7df-979b-49a9-a47c-2d854d0ef52e
📒 Files selected for processing (4)
openspec/changes/fix-spec-parser-fidelity/design.mdopenspec/changes/fix-spec-parser-fidelity/proposal.mdopenspec/changes/fix-spec-parser-fidelity/specs/cli-validate/spec.mdopenspec/changes/fix-spec-parser-fidelity/tasks.md
✅ Files skipped from review due to trivial changes (3)
- openspec/changes/fix-spec-parser-fidelity/design.md
- openspec/changes/fix-spec-parser-fidelity/proposal.md
- openspec/changes/fix-spec-parser-fidelity/tasks.md
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
Reviewed the proposal against the parser and validator paths. This is the right fix shape: unify requirement body/scenario reading while preserving the separate specs-apply write path and avoiding a breaking tightening of bare requirement headers. Approved, with focused tests needed for multiline bodies, metadata-before-prose, fenced code, fenced scenario exclusion, and INFO-only skipped-header surfacing.
…aware (Fission-AI#361, Fission-AI#418, Fission-AI#312); surface Fission-AI#498 The requirement reader was implemented twice — MarkdownParser.parseRequirements (validate <spec>/archive) and Validator.extractRequirementText/countScenarios (validate <change>) — 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 (Fission-AI#361), metadata before the description failing validate <spec> (Fission-AI#418), and a fenced block before the prose line becoming the requirement text (Fission-AI#312). - Count only non-fenced `#### ` headers, so a `#### Scenario:` inside a fenced example no longer counts as a real scenario in validate <change> (parity with validate <spec>). - 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 Fission-AI#498 safely: - validate <change> 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 `### <statement>` 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 Fission-AI#361/Fission-AI#418/Fission-AI#312, the fenced scenario, the Fission-AI#498 INFO note, a single-line guard, and CRLF. Changeset added (patch). tasks.md completed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/core/parsers/requirement-text.ts`:
- Around line 71-72: Update the scenario header detection in requirement-text so
countScenarios() only counts real `#### Scenario:` headings, not any level-4
heading. Tighten the `SCENARIO_HEADER` pattern to match the documented scenario
format in `requirement-text.ts`, and keep the logic in `countScenarios()`
unchanged aside from using the stricter matcher. Add a regression test covering
a non-scenario H4 like `#### Notes` to confirm it no longer satisfies the “at
least one scenario” rule.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5af75cae-d271-47ec-a695-212ce1643d7f
📒 Files selected for processing (10)
.changeset/spec-parser-reading-fidelity.mdopenspec/changes/fix-spec-parser-fidelity/tasks.mdsrc/core/parsers/change-parser.tssrc/core/parsers/markdown-parser.tssrc/core/parsers/requirement-blocks.tssrc/core/parsers/requirement-text.tssrc/core/schemas/base.schema.tssrc/core/validation/validator.tstest/core/parsers/markdown-parser.test.tstest/core/validation.test.ts
✅ Files skipped from review due to trivial changes (2)
- .changeset/spec-parser-reading-fidelity.md
- openspec/changes/fix-spec-parser-fidelity/tasks.md
| /** A level-4 scenario header (`#### Scenario: ...`). */ | ||
| const SCENARIO_HEADER = /^####\s+/; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Restrict scenario detection to real #### Scenario: headers.
Line 72 currently matches any level-4 heading, so countScenarios() will accept #### Notes as satisfying the “at least one scenario” rule. This weakens strict delta validation; match the documented scenario format instead and add a regression for a non-scenario H4.
Proposed fix
-const SCENARIO_HEADER = /^####\s+/;
+const SCENARIO_HEADER = /^####\s+Scenario:/i;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** A level-4 scenario header (`#### Scenario: ...`). */ | |
| const SCENARIO_HEADER = /^####\s+/; | |
| /** A level-4 scenario header (`#### Scenario: ...`). */ | |
| const SCENARIO_HEADER = /^####\s+Scenario:/i; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/core/parsers/requirement-text.ts` around lines 71 - 72, Update the
scenario header detection in requirement-text so countScenarios() only counts
real `#### Scenario:` headings, not any level-4 heading. Tighten the
`SCENARIO_HEADER` pattern to match the documented scenario format in
`requirement-text.ts`, and keep the logic in `countScenarios()` unchanged aside
from using the stricter matcher. Add a regression test covering a non-scenario
H4 like `#### Notes` to confirm it no longer satisfies the “at least one
scenario” rule.
There was a problem hiding this comment.
Not taking the tightening: the spec path treats every level-4 child of a requirement as a scenario, so restricting the delta counter to #### Scenario: would reintroduce exactly the cross-reader divergence this PR removes. 44fb1d7 documents the permissive match as deliberate and adds a test locking in that #### Notes satisfies the scenario rule on both paths.
There was a problem hiding this comment.
@clay-good Good catch — I hadn't considered that the spec-path (MarkdownParser) already treats every level-4 child heading as a scenario, so tightening the delta-side matcher would only reintroduce the exact divergence this PR is fixing. Keeping #### ... permissive on both sides for parity makes sense here, and locking it in with a test for #### Notes is a good way to prevent regression. Thanks for the context — withdrawing my suggestion.
✏️ Learnings added
Learnt from: clay-good
Repo: Fission-AI/OpenSpec PR: 1281
File: src/core/parsers/requirement-text.ts:0-0
Timestamp: 2026-07-02T22:26:54.642Z
Learning: In the OpenSpec parser fidelity work (src/core/parsers/requirement-text.ts, requirements-related validators), `countScenarios()`'s `SCENARIO_HEADER` regex deliberately matches any level-4 heading (`/^####\s+/`), not just `#### Scenario:`, because `MarkdownParser` (the main-spec reading path) already treats every level-4 child heading under a requirement as a scenario. Tightening the delta-path matcher to only `#### Scenario:` would reintroduce cross-reader divergence between `validate <change>` and `validate <spec>`. This permissive behavior is intentional and is locked in by a regression test asserting `#### Notes` counts as a scenario on both paths (see commit 44fb1d7).
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
…gn edge cases)
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 <change> and validate <spec> — 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) <noreply@anthropic.com>
clay-good
changed the title
TabishB
left a comment
TabishB
left a comment
There was a problem hiding this comment.
This review is AI-generated.
| } | ||
|
|
||
| /** Lines that look like `**ID**: ...` / `**Priority**: ...` metadata. */ | ||
| const METADATA_LINE = /^\*\*[^*]+\*\*:/; |
There was a problem hiding this comment.
This also swallows requirement text written as **Constraint**: The system MUST ... — that spec is valid on main, fails here, and then blocks archive.
And when the skip leaves the body empty, the spec path falls back to the header title (markdown-parser.ts:158) while the delta path errors with "missing requirement text" — same content, different verdicts. Maybe skip metadata only when other body text remains, and put one empty-body rule in this shared helper?
There was a problem hiding this comment.
Fixed in 44fb1d7: metadata lines are now skipped only when other body text remains, so a **Constraint**: The system MUST ... body is kept as the requirement text (regression tests on both paths). The empty-body rule now lives in the shared extractRequirementText: both readers fall back to the header title, so the spec path and delta path reach the same verdict for the same block.
| 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 |
There was a problem hiding this comment.
This doesn't stop at ### lines. On the delta path, a stray ### Background divider plus its notes gets absorbed into the requirement body, so a MUST in the notes now passes the keyword check (it errored on main) — and the new INFO's "is ignored by validation" becomes untrue. Break on any #-prefixed line like the old reader did?
There was a problem hiding this comment.
Fixed in 44fb1d7: the body now ends at the first non-fenced markdown header (/^#{1,6}\s/), so a stray ### Background divider's notes no longer feed the keyword check — added a regression where the MUST in the notes fails validation again and the divider gets its INFO note.
| const METADATA_LINE = /^\*\*[^*]+\*\*:/; | ||
|
|
||
| /** A level-4 scenario header (`#### Scenario: ...`). */ | ||
| const SCENARIO_HEADER = /^####\s+/; |
There was a problem hiding this comment.
Nit: the comment says #### Scenario: but this matches any #### header. Worth noting that's deliberate (spec-path parity) so nobody "fixes" it later.
There was a problem hiding this comment.
Done in 44fb1d7 — the comment now states the any-#### match is deliberate spec-path parity and warns against tightening it one-sided; also added a test locking in that #### Notes counts on both paths.
| * `### Documentation Requirements` divider that `validate <change>` would | ||
| * otherwise pass without comment. | ||
| */ | ||
| export function findNonRequirementLevel3Headers( |
There was a problem hiding this comment.
This scanner is fence-aware but splitTopLevelSections/parseRequirementBlocksFromSection aren't, so the note can describe boundaries the real reader doesn't use — e.g. a fenced ## ... line ends the section for the reader and a silently dropped requirement gets no INFO, and one unclosed fence suppresses all notes. Simpler for parseDeltaSpec to report the headers it actually skips?
There was a problem hiding this comment.
Reworked in 44fb1d7 as suggested: the standalone scanner is gone; parseDeltaSpec now records the non-canonical ### headers it skips while parsing (DeltaPlan.skippedHeaders), so the note uses the reader's real section boundaries and there's no whole-file fence mask to suppress notes across sections. Fence masking is per-section, matching how extractRequirementBody treats those lines as content.
|
|
||
| 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; |
There was a problem hiding this comment.
Nit: nothing imports this — the INFO check ended up in this same file. Drop the export?
There was a problem hiding this comment.
Dropped the export in 44fb1d7 (and with the scanner folded into parseDeltaSpec, nothing outside the file needs it).
| after: string; | ||
| } | ||
|
|
||
| import { buildCodeFenceMask } from './requirement-text.js'; |
There was a problem hiding this comment.
Nit: move up top with the other imports.
| 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.`, |
There was a problem hiding this comment.
Nit: for a nameless ### Requirement: this reads: it "is not a ### Requirement: header ... Use ### Requirement: Requirement:". Special-case the empty name?
There was a problem hiding this comment.
Special-cased in 44fb1d7: a nameless ### Requirement: now gets "is missing a requirement name ... Add a name, e.g. "### Requirement: "" instead of the Requirement: Requirement: suggestion. Test added.
|
|
||
| ### 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: |
There was a problem hiding this comment.
"can no longer disagree" oversells a bit — e.g. a #### Scenario: with no body still passes validate <change> but fails validate <spec> (and blocks archive). Worth softening, and listing the known leftovers in design.md.
There was a problem hiding this comment.
Softened in 44fb1d7 to "closing the known divergences" with a pointer to the design doc, and design.md now has a "Known remaining divergences" section listing the empty-scenario case you mention, the deliberate recognition split, and the fence-unaware delta section/block splitting.
clay-good
changed the title
…traction, reader-derived INFO - 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 <noreply@anthropic.com>
|
Checked this branch out at 44fb1d7 on top of current main (546224e) and ran the parser/validation suites — 69 passing here, and the #361/#418/#312 and fenced-scenario cases behave as described. One recognition edge slips past the INFO note, found while poking at header variants: ## ADDED Requirements
###Requirement: Export headers
The system SHALL export data with headers.
#### Scenario: export
- **WHEN** exporting
- **THEN** headers are included
Both regexes are unchanged from main, so this is pre-existing and I'd call it non-blocking here — but c63913b's message records the no-space hypothesis as dropped with "no divergence", which doesn't match what I see. Requiring one space in |
|
Jun's no-space edge is real, but I would not tighten Please add this to |
…-conflict # Conflicts: # src/core/schemas/base.schema.ts # src/core/validation/validator.ts
…nown leftover 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 <spec>. 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 <noreply@anthropic.com>
|
@mc856 confirmed — I reproduced your exact trace on this branch: the no-space delta validates clean under Taking @alfred-openspec's route rather than tightening: the no-space form is a tested normalization case ( Re-verified on the merged head (post-#1280): build clean, 1811 tests passing (the 17 |
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
Re-reviewed latest a7cc64c. The docs now clearly call out the remaining no-space divergence as pre-existing/out-of-scope, and the implementation keeps the safer INFO-only behavior for skipped delta headers. CI is green, so this is good to merge from my side.
4 participants
Status
LGTM — ready to review. Planning artifacts (proposal / design / spec delta / tasks) and the implementation + tests are now all in this PR. Passes
openspec validate fix-spec-parser-fidelity --strict; a changeset (patch) is included.What was wrong
The requirement reader was implemented twice and the two copies had drifted.
MarkdownParser.parseRequirements(used byvalidate <spec>/archive) andValidator.extractRequirementText/countScenarios(used byvalidate <change>) disagreed in five ways — each reproduced againstmainwith the bundled CLI:**metadata**:#### Scenario:SHALL/MUSTpredicateincludes\b…\bConcrete bugs this produced:
SHALL/MUSTthat wraps onto body line 2 fails both paths (first-line-only capture drops the keyword).validate <change>but failsvalidate <spec>(req.textbecomes**ID**: …).req.text=```bashon both paths (distinct from the already-fixed count manifestation).#### Scenario:inside a fenced example is counted as real byvalidate <change>, so a requirement with no real scenario passes whilevalidate <spec>correctly fails.### Documentation Requirementsdivider is silently ignored byvalidate <change>but flagged byarchive/validate <spec>.How it was fixed
Part A — unify the reader (fixes #361, #418, #312, fenced-scenario). Both readers now delegate to one shared, fence-/metadata-/multi-line-aware extraction in
src/core/parsers/requirement-text.tsso they cannot diverge again:#### Scenario:, skipping blank,**metadata**:, and fenced-code lines; runSHALL/MUSTdetection over the whole body.####headers, so a fenced#### Scenario:no longer counts (parity withvalidate <spec>).\b(SHALL|MUST)\bpredicate (containsShallOrMust) shared by the validator andbase.schema, replacing the substring/word-boundary split.buildCodeFenceMaskextracted into the shared module;MarkdownParserandChangeParserimport the single implementation.Part B — surface #498 safely.
validate <change>emits an INFO note when anADDED/MODIFIEDRequirements section contains a non-### Requirement:level-3 header (one the delta reader silently skips). INFO never changes thevalidresult, including under--strict(valid = no errors && no warnings), so nothing newly fails.Recognition tightening was rejected (key finding of the review)
The obvious #498 fix — recognize only
### Requirement:headers — is rejected. Bare### <statement>headers are a supported, tested requirement format:test/core/validation.test.tsasserts a bare-header spec isvalid, and bare headers appear acrossjson-converter/archive/spectests and thetmp-initfixtures. Tightening would break a large test surface and silently drop requirements from real specs. Part B gives a consistent signal without that breakage.Proof it works
main— full trace indesign.md(five-column reader table + reproductions).test/core/validation.test.ts,test/core/parsers/markdown-parser.test.ts): Line breaks in headers cause validation issues #361/[BUG] Requirement Validation Fails When Metadata Fields Precede Description #418/Parser incorrectly treats hash symbols inside code blocks as markdown headers #312, the fenced scenario, the BUG: openspec validate --strict succeeds, but openspec archive fails validation #498 INFO note, a single-line guard, CRLF, cross-reader predicate agreement (MARSHALLrejected identically on both paths), and a metadata-only-body guard. End-to-end parity across all four spec requirements confirmed against the realValidator; no spurious INFO fires on any existing repo change.markdown-parser.test.ts:331(the first-line assertion — the Line breaks in headers cause validation issues #361 bug) changes. Fence tests:106/:139and bare-header tests:258/:310are preserved.Safety
specs-applyrebuilds from raw### Requirement:blocks (extractRequirementsSection+RequirementBlock.raw), neverparseSpec/req.text— so Part A cannot change archived spec content (display/validation only). Displayed text in JSON output and delta descriptions now reflects the full body.view/listare unchanged.Notes / scope
design.md). Does not claim 1.4.0 clearer SHALL/MUST hint does not apply to main specs #1156 (that's PR fix(resolution): converge validate, view, and archive onto canonical resolution (#1182, #1202, #1156) #1280).Review surface
src/core/parsers/requirement-text.ts(new shared reader),markdown-parser.ts,change-parser.ts,requirement-blocks.ts,src/core/validation/validator.ts,src/core/schemas/base.schema.ts.test/core/validation.test.ts,test/core/parsers/markdown-parser.test.ts.proposal.md,design.md,tasks.md,specs/cli-validate/spec.md; changeset.changeset/spec-parser-reading-fidelity.md.🤖 Generated with Claude Code