proposal: fix metadata field validation bug #419
Conversation
Add OpenSpec change proposal and test coverage for requirement validation bug where metadata fields (**ID**, **Priority**, etc.) were incorrectly checked for SHALL/MUST keywords instead of the requirement description. The bug caused false validation failures on valid requirements that included metadata fields before the normative description text. Following TDD approach: - Added failing test case for requirements with metadata fields - Test fails with buggy code (pre-c782462) - Test passes with fixed code (commit c782462) - Fix skips metadata field lines when extracting requirement text 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR introduces documentation and a test for fixing a metadata field validation bug in the requirements validator. The change proposes skipping bolded metadata lines (matching Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~4 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 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 |
Review CompleteYour review story is ready! Comment !reviewfast on this PR to re-generate the story. |
|
Looks like this is addressed. Tested on the latest version. No need to merge this PR. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
openspec/changes/fix-metadata-field-validation-bug/proposal.mdopenspec/changes/fix-metadata-field-validation-bug/specs/cli-validate/spec.mdopenspec/changes/fix-metadata-field-validation-bug/tasks.mdtest/core/validation.test.ts
🧰 Additional context used
📓 Path-based instructions (4)
openspec/changes/**/*.md
📄 CodeRabbit inference engine (openspec/AGENTS.md)
Scaffold proposal using
proposal.md,tasks.md, optionaldesign.md, and delta specs underopenspec/changes/<id>/
Files:
openspec/changes/fix-metadata-field-validation-bug/specs/cli-validate/spec.mdopenspec/changes/fix-metadata-field-validation-bug/tasks.mdopenspec/changes/fix-metadata-field-validation-bug/proposal.md
openspec/changes/**/specs/**/spec.md
📄 CodeRabbit inference engine (openspec/AGENTS.md)
openspec/changes/**/specs/**/spec.md: Use## ADDED|MODIFIED|REMOVED|RENAMED Requirementsheaders in spec delta files
Include at least one#### Scenario:per requirement in spec delta files
Use#### Scenario: Nameformat (4 hashtags) for scenario headers, not bullets or bold text
Use## ADDED Requirementsfor new orthogonal capabilities that can stand alone; use## MODIFIED Requirementsfor behavior changes of existing requirements
When using MODIFIED Requirements, paste the full requirement block including header and all scenarios
Files:
openspec/changes/fix-metadata-field-validation-bug/specs/cli-validate/spec.md
openspec/changes/*/tasks.md
📄 CodeRabbit inference engine (openspec/AGENTS.md)
Ensure
tasks.mdcontains implementation checklist with numbered sections and checkbox items
Files:
openspec/changes/fix-metadata-field-validation-bug/tasks.md
openspec/changes/*/proposal.md
📄 CodeRabbit inference engine (openspec/AGENTS.md)
Ensure
proposal.mdincludes sections: Why (1-2 sentences), What Changes (bullet list with breaking change markers), and Impact (affected specs and code)
Files:
openspec/changes/fix-metadata-field-validation-bug/proposal.md
🧠 Learnings (10)
📓 Common learnings
Learnt from: CR
Repo: Fission-AI/OpenSpec PR: 0
File: openspec/AGENTS.md:0-0
Timestamp: 2025-11-25T01:08:19.004Z
Learning: Applies to openspec/changes/*/proposal.md : Ensure `proposal.md` includes sections: Why (1-2 sentences), What Changes (bullet list with breaking change markers), and Impact (affected specs and code)
Learnt from: CR
Repo: Fission-AI/OpenSpec PR: 0
File: openspec/AGENTS.md:0-0
Timestamp: 2025-11-25T01:08:19.004Z
Learning: Applies to openspec/changes/**/specs/**/spec.md : When using MODIFIED Requirements, paste the full requirement block including header and all scenarios
Learnt from: CR
Repo: Fission-AI/OpenSpec PR: 0
File: openspec/AGENTS.md:0-0
Timestamp: 2025-11-25T01:08:19.004Z
Learning: Run `openspec validate [change-id] --strict` before requesting approval
Learnt from: CR
Repo: Fission-AI/OpenSpec PR: 0
File: openspec/AGENTS.md:0-0
Timestamp: 2025-11-25T01:08:19.004Z
Learning: Applies to openspec/changes/**/specs/**/spec.md : Use `## ADDED Requirements` for new orthogonal capabilities that can stand alone; use `## MODIFIED Requirements` for behavior changes of existing requirements
📚 Learning: 2025-11-25T01:08:19.004Z
Learnt from: CR
Repo: Fission-AI/OpenSpec PR: 0
File: openspec/AGENTS.md:0-0
Timestamp: 2025-11-25T01:08:19.004Z
Learning: Applies to openspec/specs/**/spec.md : Use SHALL/MUST for normative requirements in spec files; avoid should/may unless intentionally non-normative
Applied to files:
openspec/changes/fix-metadata-field-validation-bug/specs/cli-validate/spec.mdopenspec/changes/fix-metadata-field-validation-bug/tasks.mdtest/core/validation.test.tsopenspec/changes/fix-metadata-field-validation-bug/proposal.md
📚 Learning: 2025-11-25T01:08:19.004Z
Learnt from: CR
Repo: Fission-AI/OpenSpec PR: 0
File: openspec/AGENTS.md:0-0
Timestamp: 2025-11-25T01:08:19.004Z
Learning: Applies to openspec/changes/**/specs/**/spec.md : Use `## ADDED Requirements` for new orthogonal capabilities that can stand alone; use `## MODIFIED Requirements` for behavior changes of existing requirements
Applied to files:
openspec/changes/fix-metadata-field-validation-bug/specs/cli-validate/spec.mdopenspec/changes/fix-metadata-field-validation-bug/tasks.mdtest/core/validation.test.tsopenspec/changes/fix-metadata-field-validation-bug/proposal.md
📚 Learning: 2025-11-25T01:08:19.004Z
Learnt from: CR
Repo: Fission-AI/OpenSpec PR: 0
File: openspec/AGENTS.md:0-0
Timestamp: 2025-11-25T01:08:19.004Z
Learning: Run `openspec validate [change-id] --strict` before requesting approval
Applied to files:
openspec/changes/fix-metadata-field-validation-bug/specs/cli-validate/spec.mdopenspec/changes/fix-metadata-field-validation-bug/tasks.mdopenspec/changes/fix-metadata-field-validation-bug/proposal.md
📚 Learning: 2025-11-25T01:08:19.004Z
Learnt from: CR
Repo: Fission-AI/OpenSpec PR: 0
File: openspec/AGENTS.md:0-0
Timestamp: 2025-11-25T01:08:19.004Z
Learning: Applies to openspec/changes/**/specs/**/spec.md : Use `## ADDED|MODIFIED|REMOVED|RENAMED Requirements` headers in spec delta files
Applied to files:
openspec/changes/fix-metadata-field-validation-bug/specs/cli-validate/spec.mdopenspec/changes/fix-metadata-field-validation-bug/tasks.mdtest/core/validation.test.tsopenspec/changes/fix-metadata-field-validation-bug/proposal.md
📚 Learning: 2025-11-25T01:08:19.004Z
Learnt from: CR
Repo: Fission-AI/OpenSpec PR: 0
File: openspec/AGENTS.md:0-0
Timestamp: 2025-11-25T01:08:19.004Z
Learning: Applies to openspec/changes/*/proposal.md : Ensure `proposal.md` includes sections: Why (1-2 sentences), What Changes (bullet list with breaking change markers), and Impact (affected specs and code)
Applied to files:
openspec/changes/fix-metadata-field-validation-bug/specs/cli-validate/spec.mdopenspec/changes/fix-metadata-field-validation-bug/tasks.mdopenspec/changes/fix-metadata-field-validation-bug/proposal.md
📚 Learning: 2025-11-25T01:08:19.004Z
Learnt from: CR
Repo: Fission-AI/OpenSpec PR: 0
File: openspec/AGENTS.md:0-0
Timestamp: 2025-11-25T01:08:19.004Z
Learning: Applies to openspec/changes/**/specs/**/spec.md : When using MODIFIED Requirements, paste the full requirement block including header and all scenarios
Applied to files:
openspec/changes/fix-metadata-field-validation-bug/specs/cli-validate/spec.mdopenspec/changes/fix-metadata-field-validation-bug/tasks.mdtest/core/validation.test.tsopenspec/changes/fix-metadata-field-validation-bug/proposal.md
📚 Learning: 2025-11-25T01:08:19.004Z
Learnt from: CR
Repo: Fission-AI/OpenSpec PR: 0
File: openspec/AGENTS.md:0-0
Timestamp: 2025-11-25T01:08:19.004Z
Learning: Applies to openspec/changes/**/specs/**/spec.md : Include at least one `#### Scenario:` per requirement in spec delta files
Applied to files:
openspec/changes/fix-metadata-field-validation-bug/specs/cli-validate/spec.mdopenspec/changes/fix-metadata-field-validation-bug/tasks.mdtest/core/validation.test.ts
📚 Learning: 2025-11-25T01:08:19.004Z
Learnt from: CR
Repo: Fission-AI/OpenSpec PR: 0
File: openspec/AGENTS.md:0-0
Timestamp: 2025-11-25T01:08:19.004Z
Learning: Applies to openspec/changes/*/tasks.md : Ensure `tasks.md` contains implementation checklist with numbered sections and checkbox items
Applied to files:
openspec/changes/fix-metadata-field-validation-bug/tasks.md
📚 Learning: 2025-11-25T01:08:19.004Z
Learnt from: CR
Repo: Fission-AI/OpenSpec PR: 0
File: openspec/AGENTS.md:0-0
Timestamp: 2025-11-25T01:08:19.004Z
Learning: Applies to openspec/changes/**/*.md : Scaffold proposal using `proposal.md`, `tasks.md`, optional `design.md`, and delta specs under `openspec/changes/<id>/`
Applied to files:
openspec/changes/fix-metadata-field-validation-bug/tasks.mdopenspec/changes/fix-metadata-field-validation-bug/proposal.md
🧬 Code graph analysis (1)
test/core/validation.test.ts (1)
src/core/validation/validator.ts (1)
Validator(16-449)
🔇 Additional comments (3)
openspec/changes/fix-metadata-field-validation-bug/specs/cli-validate/spec.md (1)
1-29: LGTM! Well-structured spec delta.The spec delta correctly documents the metadata field extraction behavior using MODIFIED Requirements. All three scenarios are properly formatted with the required
#### Scenario:headers and use normative SHALL language. The regex pattern/^\*\*[^*]+\*\*:/accurately captures metadata field lines.As per coding guidelines, remember to run
openspec validate fix-metadata-field-validation-bug --strictbefore requesting approval.openspec/changes/fix-metadata-field-validation-bug/tasks.md (1)
1-38: LGTM! Clear TDD workflow documentation.The tasks file follows the required structure with numbered sections and checkboxes. The test-driven development approach is well-documented, showing the RED-GREEN cycle and validation steps. The inclusion of the
openspec validatecommand aligns with the coding guidelines.openspec/changes/fix-metadata-field-validation-bug/proposal.md (1)
1-33: LGTM! Comprehensive proposal structure.The proposal includes all required sections (Why, What Changes, Impact) as specified in the coding guidelines. The Why section is more detailed than the suggested "1-2 sentences," but the additional context (bug description, example, root cause, and impact) is valuable for understanding the fix. The What Changes section provides a clear bullet list, and the Impact section properly identifies affected specs and confirms no breaking changes.
Based on coding guidelines, this proposal follows the required structure for
openspec/changes/*/proposal.md.
| it('should pass when requirement has metadata fields before description with SHALL/MUST', async () => { | ||
| const changeDir = path.join(testDir, 'test-change-with-metadata'); | ||
| const specsDir = path.join(changeDir, 'specs', 'test-spec'); | ||
| await fs.mkdir(specsDir, { recursive: true }); | ||
|
|
||
| // This is the exact pattern that was failing before the metadata fix | ||
| // The bug: old code would check **ID** line for SHALL/MUST instead of the description | ||
| const deltaSpec = `# Test Spec | ||
| ## ADDED Requirements | ||
| ### Requirement: File Serving | ||
| **ID**: REQ-FILE-001 | ||
| **Priority**: P1 | ||
| The system MUST serve static files from the root directory. | ||
| #### Scenario: File is requested | ||
| **Given** a static file exists | ||
| **When** the file is requested | ||
| **Then** the system SHALL serve the file successfully`; | ||
|
|
||
| const specPath = path.join(specsDir, 'spec.md'); | ||
| await fs.writeFile(specPath, deltaSpec); | ||
|
|
||
| const validator = new Validator(true); | ||
| const report = await validator.validateChangeDeltaSpecs(changeDir); | ||
|
|
||
| // This should PASS because the description (not metadata) contains MUST | ||
| // Before fix c782462, this would FAIL because it checked the **ID** line | ||
| expect(report.valid).toBe(true); | ||
| expect(report.summary.errors).toBe(0); | ||
| }); |
There was a problem hiding this comment.
Duplicate test coverage detected.
This test case appears to duplicate the existing test at lines 343-371 ("should validate requirement with metadata before SHALL/MUST text"). Both tests verify the same scenario:
- Metadata fields (ID, Priority) before the description
- Description contains MUST/SHALL keywords
- Validation should pass
The only differences are the specific requirement content (Circuit Breaker vs File Serving), which don't provide additional coverage for the metadata extraction logic. Consider removing this test to avoid redundancy, as the existing test at lines 343-371 already covers this scenario.
📊 Comparison of the two tests
Existing test (343-371):
- Title: "Circuit Breaker State Management SHALL be implemented"
- Metadata: ID: REQ-CB-001, Priority: P1 (High)
- Description: "The system MUST implement a circuit breaker..."
New test (489-521):
- Title: "File Serving"
- Metadata: ID: REQ-FILE-001, Priority: P1
- Description: "The system MUST serve static files..."
Both tests validate identical extraction logic for metadata fields.
🤖 Prompt for AI Agents
In test/core/validation.test.ts around lines 489-521 there is a duplicate test
that repeats the existing scenario at lines 343-371 (metadata before description
with SHALL/MUST) — remove this redundant test or replace it with a case that
exercises a different path (for example: metadata present but no SHALL/MUST
anywhere, SHALL/MUST in metadata only, or multiple requirements in one file) so
the suite adds coverage; update the spec files and assertions accordingly and
run tests to ensure no behavioral change.
| - Modified `extractRequirementText()` in src/core/validation/validator.ts | ||
| - Changed from collecting all lines and finding first non-empty | ||
| - To: iterating through lines, skipping blanks and metadata fields | ||
| - Returns first line that is NOT blank and NOT metadata pattern `/^\*\*[^*]+\*\*:/` |
There was a problem hiding this comment.
This address the fix but it's very specific to the case where the metadata is defined in the following format **<tag>**.
I think the option is to either to make it more general or to just incorporate the tags as part of the standard itself.
Could using markdown frontmatter tags here help instead? I see from the example that this is on a per file basis not per requirement.
There was a problem hiding this comment.
Hello @TabishB, I am leaning towards making tags a part of the standard. Let me go over a few examples of specs from real apps and see what make most sense and update this PR. Will keep in draft mode until then. Thanks for your suggestions.
2 participants
Add OpenSpec change proposal and test coverage for requirement validation bug where metadata fields (ID, Priority, etc.) were incorrectly checked for SHALL/MUST keywords instead of the requirement description.
The bug caused false validation failures on valid requirements that included metadata fields before the normative description text.
Following TDD approach:
🤖 Generated with Claude Code
Fixes #418
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.