Compare commits

...
Author SHA1 Message Date
Tabish Bidiwale c5c3b33264 Respect --no-validate flag while archiving 2025-10-16 17:13:39 +11:00
Tabish Bidiwale c7824626d3 fix: skip metadata when validating requirement SHALL/MUST keywords
Fixes validation incorrectly checking metadata lines instead of requirement text.

The extractRequirementText() function was returning the first non-empty line
after the requirement header, which was often metadata like **ID**: REQ-001
instead of the actual requirement statement.

Changes:
- Updated extractRequirementText() to skip lines matching **Key**: Value pattern
- Skip blank lines between header and requirement text
- Return first substantive text line for SHALL/MUST validation

Added comprehensive tests for:
- Requirements with metadata before SHALL/MUST text
- Requirements with SHALL in text but not header
- Requirements correctly failing without SHALL/MUST
- Requirements without metadata fields

All 20 validation tests pass.

Fixes #159
2025-10-16 16:27:08 +11:00
5 changed files with 195 additions and 16 deletions
+1 -1
View File
@@ -187,7 +187,7 @@ program
.option('-y, --yes', 'Skip confirmation prompts')
.option('--skip-specs', 'Skip spec update operations (useful for infrastructure, tooling, or doc-only changes)')
.option('--no-validate', 'Skip validation (not recommended, requires confirmation)')
.action(async (changeName?: string, options?: { yes?: boolean; skipSpecs?: boolean; noValidate?: boolean }) => {
.action(async (changeName?: string, options?: { yes?: boolean; skipSpecs?: boolean; noValidate?: boolean; validate?: boolean }) => {
try {
const archiveCommand = new ArchiveCommand();
await archiveCommand.execute(changeName, options);
+9 -4
View File
@@ -19,7 +19,10 @@ interface SpecUpdate {
}
export class ArchiveCommand {
async execute(changeName?: string, options: { yes?: boolean; skipSpecs?: boolean; noValidate?: boolean } = {}): Promise<void> {
async execute(
changeName?: string,
options: { yes?: boolean; skipSpecs?: boolean; noValidate?: boolean; validate?: boolean } = {}
): Promise<void> {
const targetPath = '.';
const changesDir = path.join(targetPath, 'openspec', 'changes');
const archiveDir = path.join(changesDir, 'archive');
@@ -54,8 +57,10 @@ export class ArchiveCommand {
throw new Error(`Change '${changeName}' not found.`);
}
const skipValidation = options.validate === false || options.noValidate === true;
// Validate specs and change before archiving
if (!options.noValidate) {
if (!skipValidation) {
const validator = new Validator();
let hasValidationErrors = false;
@@ -201,7 +206,7 @@ export class ArchiveCommand {
let totals = { added: 0, modified: 0, removed: 0, renamed: 0 };
for (const p of prepared) {
const specName = path.basename(path.dirname(p.update.target));
if (!options.noValidate) {
if (!skipValidation) {
const report = await new Validator().validateSpecContent(specName, p.rebuilt);
if (!report.valid) {
console.log(chalk.red(`\nValidation errors in rebuilt spec for ${specName} (will not write changes):`));
@@ -598,4 +603,4 @@ export class ArchiveCommand {
// Returns date in YYYY-MM-DD format
return new Date().toISOString().split('T')[0];
}
}
}
+20 -6
View File
@@ -375,16 +375,30 @@ export class Validator {
private extractRequirementText(blockRaw: string): string | undefined {
const lines = blockRaw.split('\n');
// Skip header
// Skip header line (index 0)
let i = 1;
const bodyLines: string[] = [];
// Find the first substantial text line, skipping metadata and blank lines
for (; i < lines.length; i++) {
const line = lines[i];
if (/^####\s+/.test(line)) break; // scenarios start
bodyLines.push(line);
// Stop at scenario headers
if (/^####\s+/.test(line)) break;
const trimmed = line.trim();
// Skip blank lines
if (trimmed.length === 0) continue;
// Skip metadata lines (lines starting with ** like **ID**, **Priority**, etc.)
if (/^\*\*[^*]+\*\*:/.test(trimmed)) continue;
// Found first non-metadata, non-blank line - this is the requirement text
return trimmed;
}
const text = bodyLines.join('\n').split('\n').map(l => l.trim()).find(l => l.length > 0);
return text;
// No requirement text found
return undefined;
}
private containsShallOrMust(text: string): boolean {
+42 -1
View File
@@ -1,5 +1,6 @@
import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest';
import { ArchiveCommand } from '../../src/core/archive.js';
import { Validator } from '../../src/core/validation/validator.js';
import { promises as fs } from 'fs';
import path from 'path';
import os from 'os';
@@ -215,6 +216,46 @@ Then expected result happens`;
expect(archives[0]).toMatch(new RegExp(`\\d{4}-\\d{2}-\\d{2}-${changeName}`));
});
it('should skip validation when commander sets validate to false (--no-validate)', async () => {
const changeName = 'skip-validation-flag';
const changeDir = path.join(tempDir, 'openspec', 'changes', changeName);
const changeSpecDir = path.join(changeDir, 'specs', 'unstable-capability');
await fs.mkdir(changeSpecDir, { recursive: true });
const deltaSpec = `# Unstable Capability
## ADDED Requirements
### Requirement: Logging Feature
**ID**: REQ-LOG-001
The system will log all events.
#### Scenario: Event recorded
- **WHEN** an event occurs
- **THEN** it is captured`;
await fs.writeFile(path.join(changeSpecDir, 'spec.md'), deltaSpec);
await fs.writeFile(path.join(changeDir, 'tasks.md'), '- [x] Task 1\n');
const deltaSpy = vi.spyOn(Validator.prototype, 'validateChangeDeltaSpecs');
const specContentSpy = vi.spyOn(Validator.prototype, 'validateSpecContent');
try {
await archiveCommand.execute(changeName, { yes: true, skipSpecs: true, validate: false });
expect(deltaSpy).not.toHaveBeenCalled();
expect(specContentSpy).not.toHaveBeenCalled();
const archiveDir = path.join(tempDir, 'openspec', 'changes', 'archive');
const archives = await fs.readdir(archiveDir);
expect(archives.length).toBe(1);
expect(archives[0]).toMatch(new RegExp(`\\d{4}-\\d{2}-\\d{2}-${changeName}`));
} finally {
deltaSpy.mockRestore();
specContentSpy.mockRestore();
}
});
it('should proceed with archive when user declines spec updates', async () => {
const { confirm } = await import('@inquirer/prompts');
const mockConfirm = confirm as unknown as ReturnType<typeof vi.fn>;
@@ -636,4 +677,4 @@ E1 updated`);
await expect(fs.access(changeDir)).resolves.not.toThrow();
});
});
});
});
+123 -4
View File
@@ -306,10 +306,10 @@ Then result`;
const specPath = path.join(testDir, 'spec.md');
await fs.writeFile(specPath, specContent);
const validator = new Validator(true); // strict mode
const report = await validator.validateSpec(specPath);
expect(report.valid).toBe(false); // Should fail due to brief overview warning
});
@@ -330,12 +330,131 @@ Then result`;
const specPath = path.join(testDir, 'spec.md');
await fs.writeFile(specPath, specContent);
const validator = new Validator(false); // non-strict mode
const report = await validator.validateSpec(specPath);
expect(report.valid).toBe(true); // Should pass despite warnings
expect(report.summary.warnings).toBeGreaterThan(0);
});
});
describe('validateChangeDeltaSpecs with metadata', () => {
it('should validate requirement with metadata before SHALL/MUST text', async () => {
const changeDir = path.join(testDir, 'test-change');
const specsDir = path.join(changeDir, 'specs', 'test-spec');
await fs.mkdir(specsDir, { recursive: true });
const deltaSpec = `# Test Spec
## ADDED Requirements
### Requirement: Circuit Breaker State Management SHALL be implemented
**ID**: REQ-CB-001
**Priority**: P1 (High)
The system MUST implement a circuit breaker with three states.
#### Scenario: Normal operation
**Given** the circuit breaker is in CLOSED state
**When** a request is made
**Then** the request is executed normally`;
const specPath = path.join(specsDir, 'spec.md');
await fs.writeFile(specPath, deltaSpec);
const validator = new Validator(true);
const report = await validator.validateChangeDeltaSpecs(changeDir);
expect(report.valid).toBe(true);
expect(report.summary.errors).toBe(0);
});
it('should validate requirement with SHALL in text but not in header', async () => {
const changeDir = path.join(testDir, 'test-change-2');
const specsDir = path.join(changeDir, 'specs', 'test-spec');
await fs.mkdir(specsDir, { recursive: true });
const deltaSpec = `# Test Spec
## ADDED Requirements
### Requirement: Error Handling
**ID**: REQ-ERR-001
**Priority**: P2
The system SHALL handle all errors gracefully.
#### Scenario: Error occurs
**Given** an error condition
**When** an error occurs
**Then** the error is logged and user is notified`;
const specPath = path.join(specsDir, 'spec.md');
await fs.writeFile(specPath, deltaSpec);
const validator = new Validator(true);
const report = await validator.validateChangeDeltaSpecs(changeDir);
expect(report.valid).toBe(true);
expect(report.summary.errors).toBe(0);
});
it('should fail when requirement text lacks SHALL/MUST', async () => {
const changeDir = path.join(testDir, 'test-change-3');
const specsDir = path.join(changeDir, 'specs', 'test-spec');
await fs.mkdir(specsDir, { recursive: true });
const deltaSpec = `# Test Spec
## ADDED Requirements
### Requirement: Logging Feature
**ID**: REQ-LOG-001
The system will log all events.
#### Scenario: Event occurs
**Given** an event
**When** it occurs
**Then** it is logged`;
const specPath = path.join(specsDir, 'spec.md');
await fs.writeFile(specPath, deltaSpec);
const validator = new Validator(true);
const report = await validator.validateChangeDeltaSpecs(changeDir);
expect(report.valid).toBe(false);
expect(report.summary.errors).toBeGreaterThan(0);
expect(report.issues.some(i => i.message.includes('must contain SHALL or MUST'))).toBe(true);
});
it('should handle requirements without metadata fields', async () => {
const changeDir = path.join(testDir, 'test-change-4');
const specsDir = path.join(changeDir, 'specs', 'test-spec');
await fs.mkdir(specsDir, { recursive: true });
const deltaSpec = `# Test Spec
## ADDED Requirements
### Requirement: Simple Feature
The system SHALL implement this feature.
#### Scenario: Basic usage
**Given** a condition
**When** an action occurs
**Then** a result happens`;
const specPath = path.join(specsDir, 'spec.md');
await fs.writeFile(specPath, deltaSpec);
const validator = new Validator(true);
const report = await validator.validateChangeDeltaSpecs(changeDir);
expect(report.valid).toBe(true);
expect(report.summary.errors).toBe(0);
});
});
});