Skip to content

proposal: fix metadata field validation bug #419

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
harikrishnan83 wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

proposal: fix metadata field validation bug #419

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 33 additions & 0 deletions openspec/changes/fix-metadata-field-validation-bug/proposal.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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
29 changes: 29 additions & 0 deletions openspec/changes/fix-metadata-field-validation-bug/specs/cli-validate/spec.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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
38 changes: 38 additions & 0 deletions openspec/changes/fix-metadata-field-validation-bug/tasks.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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 `/^\*\*[^*]+\*\*:/`
Copy link
Contributor

@TabishB TabishB Dec 29, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@harikrishnan83 harikrishnan83 Dec 30, 2025

Choose a reason for hiding this comment

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

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.

- 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
34 changes: 34 additions & 0 deletions test/core/validation.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
});
Comment on lines +489 to +521
Copy link
Contributor

@coderabbitai coderabbitai bot Dec 29, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

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.

});
});
Loading