fix(parser): ignore fenced code blocks when parsing delta specs#1151
fix(parser): ignore fenced code blocks when parsing delta specs#1151javigomez wants to merge 4 commits into
Conversation
Requirement headers, delta section headers, scenarios and REMOVED/RENAMED entries written inside fenced code blocks were parsed as real content by the delta-spec parser. A fenced `### Requirement:` example became a phantom requirement, producing spurious `validate` errors and risking incorrect `archive` output. Fence detection was duplicated across MarkdownParser and spec-structure but missing entirely from requirement-blocks (which powers both validate and archive). Extract a single shared `buildCodeFenceMask` helper and make the delta-spec parser and validator block helpers honor it, so all parsers treat fenced code consistently. Co-authored-by: Cursor <cursoragent@cursor.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a shared fenced-code-block mask and updates Markdown parsing, requirement/delta splitting, and validation to ignore structural markers found inside fenced code blocks; includes tests and a changeset documenting the fix. ChangesFence-aware Markdown and Delta-Spec Parsing
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly Related Issues
Possibly Related PRs
Suggested Reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/core/validation.test.ts (1)
651-686: ⚡ Quick winAdd a fenced-only scenario regression here.
This test still passes if
countScenarios()starts counting#### Scenario:lines inside fences again, because Line 663 already provides a real scenario. Add a sibling case with no unfenced scenario and assert the validator still reports the missing-scenario error.Example follow-up case
+ it('does not count scenario headers inside fenced code blocks toward the required scenario count', async () => { + const changeDir = path.join(testDir, 'test-change-fenced-scenario-only'); + const specsDir = path.join(changeDir, 'specs', 'test-spec'); + await fs.mkdir(specsDir, { recursive: true }); + + const deltaSpec = `# Test Spec + +## ADDED Requirements + +### Requirement: Documentation Generator +The system SHALL render a delta example in its output. + +\`\`\`markdown +#### Scenario: Example scenario +\`\`\` +`; + + const specPath = path.join(specsDir, 'spec.md'); + await fs.writeFile(specPath, deltaSpec); + + const report = await new Validator(true).validateChangeDeltaSpecs(changeDir); + + expect(report.valid).toBe(false); + expect( + report.issues.some((i) => i.message.includes('must include at least one scenario')) + ).toBe(true); + });🤖 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 `@test/core/validation.test.ts` around lines 651 - 686, The test currently only covers a case where a fenced code block contains a faux requirement+scenario but an unfenced scenario exists so a regression in countScenarios() could be missed; add a sibling test case in test/core/validation.test.ts that writes a spec where the only "#### Scenario:" is inside a fenced block (no unfenced scenario), call Validator.validateChangeDeltaSpecs on that changeDir, and assert the validator reports the missing-scenario error (e.g., report.valid is false, report.summary.errors > 0 and report.issues includes a message about missing scenario). This ensures countScenarios() and the Validator class logic correctly ignore fenced code blocks and surface the missing-scenario error when appropriate.
🤖 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 `@test/core/validation.test.ts`:
- Around line 651-686: The test currently only covers a case where a fenced code
block contains a faux requirement+scenario but an unfenced scenario exists so a
regression in countScenarios() could be missed; add a sibling test case in
test/core/validation.test.ts that writes a spec where the only "#### Scenario:"
is inside a fenced block (no unfenced scenario), call
Validator.validateChangeDeltaSpecs on that changeDir, and assert the validator
reports the missing-scenario error (e.g., report.valid is false,
report.summary.errors > 0 and report.issues includes a message about missing
scenario). This ensures countScenarios() and the Validator class logic correctly
ignore fenced code blocks and surface the missing-scenario error when
appropriate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 725c321b-78bf-4210-a01d-d12e1da4475b
📒 Files selected for processing (8)
.changeset/fence-aware-delta-parsing.mdsrc/core/parsers/code-fence.tssrc/core/parsers/markdown-parser.tssrc/core/parsers/requirement-blocks.tssrc/core/parsers/spec-structure.tssrc/core/validation/validator.tstest/core/parsers/requirement-blocks.test.tstest/core/validation.test.ts
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
Reviewed the parser/validator paths and this looks good to merge.
The shared buildCodeFenceMask keeps the existing MarkdownParser/spec-structure behavior while wiring the missing fence awareness into requirement-blocks.ts, which is the important archive/validate path. I also checked the CodeRabbit scenario-count nit: the current implementation does ignore fenced-only #### Scenario: lines and still reports the missing-scenario error, so I do not think that extra test should block merge. It would still be a nice regression to add if you want the belt-and-suspenders coverage.
Local verification:
pnpm exec vitest run test/core/parsers/requirement-blocks.test.ts test/core/validation.test.tspnpm exec vitest run test/specs/source-specs-normalization.test.ts test/core/parsers/markdown-parser.test.ts test/core/parsers/change-parser.test.ts- fenced-only scenario throwaway fixture against built validator
pnpm run buildpnpm run lintpnpm test→ 1659 passed
Add a regression test asserting that a `#### Scenario:` appearing only inside a fenced code block does not count toward the required scenario count, so the validator still reports the missing-scenario error. This guards the fence awareness of `countScenarios()`: the existing fenced-example test always includes a real (unfenced) scenario, so it would not catch a regression that began counting fenced scenario headers. Addresses the review suggestion on PR Fission-AI#1151. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Addressed the scenario-count nit from the review. Pushed Test-only change: no source/behavior changes, and no new changeset (the existing Verification:
|
2 participants
Summary
Markdown structure written inside fenced code blocks was being parsed as
real content by the delta-spec parser (
src/core/parsers/requirement-blocks.ts).For example, documenting the delta format inside a scenario:
…caused
### Requirement: Example onlyto be parsed as a second, phantomrequirement. Because that phantom block has no body text,
openspec validatereported a spurious
ADDED "Example only" is missing requirement texterror,and the same mis-parse feeds
archive(specs-apply.ts), risking incorrectspec output.
Root cause
Fenced-code detection already existed in
MarkdownParserandspec-structure.ts(and
ChangeParseruses it), but it was duplicated in each place andmissing entirely from
requirement-blocks.ts, which powers bothvalidateand
archive.Changes
buildCodeFenceMaskhelper (src/core/parsers/code-fence.ts).parseDeltaSpecandextractRequirementsSectionhonor fenced code whendetecting section/requirement/removed/renamed headers.
countScenarios/extractRequirementTextfence-aware too.MarkdownParserandspec-structure.tsto reuse the shared helperinstead of their own copies (no behavior change there; removes the drift that
caused this bug). Public/protected APIs are unchanged.
No new features, no config or output format changes.
Test plan
test/core/parsers/requirement-blocks.test.ts(fenced requirement headers, REMOVED bullets and RENAMED pairs are ignored;
main-spec requirements section too).
test/core/validation.test.ts(fenced example no longer triggers a spurious error).
node build.js(tsc) passes.eslint src/passes.patchchangeset.Made with Cursor
Summary by CodeRabbit