diff --git a/openspec/changes/fix-metadata-field-validation-bug/proposal.md b/openspec/changes/fix-metadata-field-validation-bug/proposal.md new file mode 100644 index 00000000..ff1cf127 --- /dev/null +++ b/openspec/changes/fix-metadata-field-validation-bug/proposal.md @@ -0,0 +1,33 @@ +# Proposal: Fix Metadata Field Validation Bug + +## Why + +The requirement SHALL/MUST validation incorrectly checks metadata fields instead of the requirement description, causing false failures on valid requirements. + +**The Bug**: When a requirement includes metadata fields (like `**ID**: REQ-001` or `**Priority**: P1`) immediately after the title, the validator extracts the first non-empty line as the "requirement text". This returns a metadata field like `**ID**: REQ-001`, which doesn't contain SHALL/MUST, causing validation to fail even when the actual requirement description contains proper normative keywords. + +**Example that fails incorrectly**: +```markdown +### Requirement: File Serving +**ID**: REQ-FILE-001 +**Priority**: P1 + +The system MUST serve static files from the root directory. +``` + +The validator checks `**ID**: REQ-FILE-001` for SHALL/MUST instead of checking `The system MUST serve...`, so it fails. + +**Root Cause**: The `extractRequirementText()` method in validator.ts collected all lines after the requirement header, joined them, and returned the first non-empty line. It didn't skip metadata field lines (matching pattern `/^\*\*[^*]+\*\*:/`). + +**Impact**: Users cannot use metadata fields in requirements without triggering false validation failures, blocking adoption of structured requirement metadata. + +## What Changes + +- **Fix** requirement text extraction to skip metadata field lines before finding the normative description +- **Add** comprehensive test coverage for requirements with metadata fields + +## Impact + +- **Modified specs**: cli-validate +- **Bug fix**: Resolves false positive validation failures +- **No breaking changes**: Existing valid requirements continue to pass; previously failing valid requirements now pass correctly diff --git a/openspec/changes/fix-metadata-field-validation-bug/specs/cli-validate/spec.md b/openspec/changes/fix-metadata-field-validation-bug/specs/cli-validate/spec.md new file mode 100644 index 00000000..3c041739 --- /dev/null +++ b/openspec/changes/fix-metadata-field-validation-bug/specs/cli-validate/spec.md @@ -0,0 +1,29 @@ +# cli-validate Specification + +## MODIFIED Requirements + +### Requirement: Metadata field extraction + +The validator SHALL skip metadata field lines when extracting requirement description text for SHALL/MUST validation. + +#### Scenario: Requirement with metadata fields before description + +- **GIVEN** a requirement with metadata fields (`**ID**:`, `**Priority**:`, etc.) between the title and description +- **WHEN** validating for SHALL/MUST keywords +- **THEN** the validator SHALL skip all lines matching `/^\*\*[^*]+\*\*:/` pattern +- **AND** SHALL extract the first substantial non-metadata line as the requirement description +- **AND** SHALL check that description (not metadata) for SHALL or MUST keywords + +#### Scenario: Requirement without metadata fields + +- **GIVEN** a requirement with no metadata fields +- **WHEN** validating for SHALL/MUST keywords +- **THEN** the validator SHALL extract the first non-empty line after the title +- **AND** SHALL check that line for SHALL or MUST keywords + +#### Scenario: Requirement with blank lines before description + +- **GIVEN** a requirement with blank lines between metadata/title and description +- **WHEN** validating for SHALL/MUST keywords +- **THEN** the validator SHALL skip blank lines +- **AND** SHALL extract the first substantial text line as the requirement description diff --git a/openspec/changes/fix-metadata-field-validation-bug/tasks.md b/openspec/changes/fix-metadata-field-validation-bug/tasks.md new file mode 100644 index 00000000..0ed1a0f6 --- /dev/null +++ b/openspec/changes/fix-metadata-field-validation-bug/tasks.md @@ -0,0 +1,38 @@ +# Tasks + +## Test-Driven Development (Red-Green) + +1. **✅ Write failing test (RED)** + - Added test case: "should pass when requirement has metadata fields before description with SHALL/MUST" + - Test creates requirement with `**ID**` and `**Priority**` metadata before normative description + - Test expects validation to pass (description contains MUST) + - With buggy code (pre-c782462): Test FAILS (validator checks metadata field instead of description) + +2. **✅ Verify test fails for correct reason** + - Confirmed test fails with buggy code + - Error: `expect(report.valid).toBe(true)` but got `false` + - Root cause: `extractRequirementText()` returns `**ID**: REQ-FILE-001` which lacks SHALL/MUST + +3. **✅ Implement fix (GREEN)** + - 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 `/^\*\*[^*]+\*\*:/` + - Fix implemented in commit c782462 + +4. **✅ Verify test passes without modification** + - Test now passes with fixed code + - No changes to test needed - acceptance criteria met + - Validator correctly skips metadata and checks actual description + +## Validation + +5. **Run full test suite** + - Execute `pnpm test` to ensure no regressions + - All existing tests should pass + - New test provides coverage for metadata field bug + +6. **Validate proposal** + - Run `openspec validate fix-metadata-field-validation-bug --strict` + - Ensure proposal structure is correct + - Confirm all sections present and valid diff --git a/test/core/validation.test.ts b/test/core/validation.test.ts index f7323b36..9f4495c9 100644 --- a/test/core/validation.test.ts +++ b/test/core/validation.test.ts @@ -485,5 +485,39 @@ The system MUST support mixed case delta headers. expect(report.summary.warnings).toBe(0); expect(report.summary.info).toBe(0); }); + + 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); + }); }); });