Skip to content

fix(parser): ignore fenced code blocks when parsing delta specs#1151

Open
javigomez wants to merge 7 commits into
Fission-AI:mainfrom
javigomez:fix/fenced-code-blocks-in-delta-specs
Open

fix(parser): ignore fenced code blocks when parsing delta specs#1151
javigomez wants to merge 7 commits into
Fission-AI:mainfrom
javigomez:fix/fenced-code-blocks-in-delta-specs

Conversation

@javigomez

@javigomez javigomez commented Jun 1, 2026

Copy link
Copy Markdown

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:

## ADDED Requirements

### Requirement: Documentation Generator
The system SHALL render a delta example in its output.

#### Scenario: Renders an example
**Given** a template
**When** documentation is generated
**Then** the following snippet is produced:

```markdown
### Requirement: Example only
#### Scenario: Example scenario
```

…caused ### Requirement: Example only to be parsed as a second, phantom
requirement
. Because that phantom block has no body text, openspec validate
reported a spurious ADDED "Example only" is missing requirement text error,
and the same mis-parse feeds archive (specs-apply.ts), risking incorrect
spec output.

Root cause

Fenced-code detection already existed in MarkdownParser and spec-structure.ts
(and ChangeParser uses it), but it was duplicated in each place and
missing entirely from requirement-blocks.ts, which powers both validate
and archive.

Changes

  • Add a single shared buildCodeFenceMask helper (src/core/parsers/code-fence.ts).
  • Make parseDeltaSpec and extractRequirementsSection honor fenced code when
    detecting section/requirement/removed/renamed headers.
  • Make the validator's countScenarios / extractRequirementText fence-aware too.
  • Refactor MarkdownParser and spec-structure.ts to reuse the shared helper
    instead 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

  • New regression tests in test/core/parsers/requirement-blocks.test.ts
    (fenced requirement headers, REMOVED bullets and RENAMED pairs are ignored;
    main-spec requirements section too).
  • New validator integration test in test/core/validation.test.ts
    (fenced example no longer triggers a spurious error).
  • node build.js (tsc) passes.
  • eslint src/ passes.
  • Full suite green: 1659 passed (was 1655 + 4 new).
  • Added a patch changeset.

Made with Cursor

Summary by CodeRabbit

  • Bug Fixes
    • Improved Markdown parsing to correctly ignore requirement and delta structure inside fenced code blocks.
    • Prevented example content in fenced blocks from creating false requirements, validation errors, or incorrect archive results.
    • Standardized fenced-code handling across specification and change parsing.

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>
@javigomez javigomez requested a review from TabishB as a code owner June 1, 2026 13:54
@coderabbitai

coderabbitai Bot commented Jun 1, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 44ebd699-67f3-4e2d-a576-912bc79f3d05

📥 Commits

Reviewing files that changed from the base of the PR and between b6c7a86 and c248f66.

📒 Files selected for processing (1)
  • src/core/parsers/requirement-blocks.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/core/parsers/requirement-blocks.ts

📝 Walkthrough

Walkthrough

Fence-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.

Changes

Fence-aware Delta-spec Parsing

Layer / File(s) Summary
Shared fence-detection utility
src/core/parsers/code-fence.ts
Adds buildCodeFenceMask for tracking backtick and tilde fences and marking fenced lines.
Parser integration
src/core/parsers/spec-structure.ts
Reuses the shared mask when stripping fenced blocks while preserving line positions.
Requirement and delta parsing
src/core/parsers/requirement-blocks.ts
Makes section, requirement, removed, and renamed entry detection ignore fenced content.
Regression coverage and changeset
test/core/parsers/requirement-blocks.test.ts, test/core/validation.test.ts, .changeset/fence-aware-delta-parsing.md
Adds fenced-content parsing and validation tests and documents the patch release.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related issues

Possibly related PRs

Suggested reviewers: tabishb

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly matches the main change: making delta-spec parsing ignore fenced code blocks.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
test/core/validation.test.ts (1)

651-686: ⚡ Quick win

Add 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

📥 Commits

Reviewing files that changed from the base of the PR and between 055957f and 5d97411.

📒 Files selected for processing (8)
  • .changeset/fence-aware-delta-parsing.md
  • src/core/parsers/code-fence.ts
  • src/core/parsers/markdown-parser.ts
  • src/core/parsers/requirement-blocks.ts
  • src/core/parsers/spec-structure.ts
  • src/core/validation/validator.ts
  • test/core/parsers/requirement-blocks.test.ts
  • test/core/validation.test.ts

alfred-openspec
alfred-openspec previously approved these changes Jun 1, 2026

@alfred-openspec alfred-openspec left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.ts
  • pnpm 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 build
  • pnpm run lint
  • pnpm test → 1659 passed

javigomez and others added 2 commits June 3, 2026 09:52
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>
@javigomez

Copy link
Copy Markdown
Author

Addressed the scenario-count nit from the review.

Pushed b6c7a86, which adds a focused regression test in test/core/validation.test.ts. The existing fenced-example test always includes a real (unfenced) #### Scenario:, so it would still pass even if countScenarios() regressed and began counting scenario headers inside fenced code blocks. The new test covers the case where the only #### Scenario: lives inside a fenced block and asserts the validator still reports the must include at least one scenario error — locking in the fence-aware behavior @alfred-openspec confirmed during review.

Test-only change: no source/behavior changes, and no new changeset (the existing patch changeset already covers the fix).

Verification:

  • pnpm test1660 passed (1659 + 1 new)
  • pnpm exec tsc --noEmit → clean
  • pnpm lint → clean

alfred-openspec
alfred-openspec previously approved these changes Jun 5, 2026

@alfred-openspec alfred-openspec left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@javigomez

Copy link
Copy Markdown
Author

Hi, is there anything I can do on my side to move forward with this PR?

@alfred-openspec

Copy link
Copy Markdown
Collaborator

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.

clay-good added a commit to clay-good/OpenSpec that referenced this pull request Jun 29, 2026
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants