fix(parser): ignore fenced code blocks when parsing delta specs#1151
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)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughFence-aware parsing is centralized in a shared mask utility. Requirement, delta-spec, and fenced-content parsing now ignore Markdown structures inside code fences, with regression tests covering requirements, directives, renamed or removed entries, and validation scenarios. ChangesFence-aware Delta-spec Parsing
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 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 |
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
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:
|
alfred-openspec
left a comment
There was a problem hiding this comment.
Re-reviewed after the latest commits and this is still ready to merge.
The new fenced-only scenario regression covers the previous CodeRabbit nit directly: fenced #### Scenario: examples no longer count toward the required scenario count, so real missing-scenario errors still fire. I re-ran the focused parser/validator tests and build locally on head 0816db3, all green.
|
Hi, is there anything I can do on my side to move forward with this PR? |
|
Thanks for checking in. Nothing needed from your side right now, this is already approved and mergeable. I’m leaving the actual merge to Tabish, but the PR itself looks ready. |
Deterministic, CLI-enforced prevention of silent spec drop in the spec-driven workflow, addressing one root fault (correctness decisions in agent prompts) across four layers: schema-aware delta gate shared by validate+archive (Fission-AI#997), the non-transitive apply-gate loop fix (incorporates Fission-AI#1250), the keystone (all archive templates call `openspec archive` — Fission-AI#656/Fission-AI#863), and a specced archived-drift audit. Hardened across three review rounds (alfred-openspec, CodeRabbit), an internal adversarial pass, and an open-PR coordination scan: - reconciles Fission-AI#977 (allow-specless) via the schema-aware gate + --skip-specs, deliberately not allowing silent specless archives under spec-driven; - coordinates with Fission-AI#902 (propose/ff spec discovery), the unify-template-generation-pipeline manifest, add-change-stacking-awareness (provides/touches markers + overlap warnings), and add-artifact- regeneration-support (complementary staleness); - rebases onto approved Fission-AI#1186/Fission-AI#1151/Fission-AI#1153/Fission-AI#1252; Fission-AI#1252 is a prerequisite. Closes Fission-AI#1212 Fission-AI#1260 Fission-AI#1222 Fission-AI#1264 Fission-AI#799 Fission-AI#656 Fission-AI#863 Fission-AI#913. Supersedes PRs Fission-AI#1250 Fission-AI#1271 Fission-AI#1241 Fission-AI#1233. Addresses Fission-AI#997 Fission-AI#977 Fission-AI#902 Fission-AI#164 Fission-AI#426 Fission-AI#911. Out of scope Fission-AI#1246 Fission-AI#1112 Fission-AI#1252(prereq) Fission-AI#1120 Fission-AI#827 Fission-AI#1265. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
alfred-openspec
left a comment
There was a problem hiding this comment.
Revalidated the shared fence mask through parser, validator, scenario-count, and archive-facing paths; the only changes since the prior approval are main merges. Full current CI is green.
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