From bb57212c622a28ce0e2e0cae6f0aaa29b3c792cf Mon Sep 17 00:00:00 2001 From: Pluviobyte Date: Thu, 28 May 2026 09:10:50 +0000 Subject: [PATCH] fix(validator): hint when SHALL/MUST appears only in requirement header MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When a change delta has a requirement whose body is missing SHALL/MUST but whose header (the text after `### Requirement:`) already contains the keyword, the validator emitted the generic error "must contain SHALL or MUST". Authors then re-read the spec, see SHALL right there in the header, and have no idea what the validator wants. Per the OpenSpec conventions the keyword has to live on the requirement body line (the line immediately after the header). When the keyword is present in the header only, append guidance explaining exactly where to move it. The fix is scoped to the two `validateChangeDeltaSpecs` call sites (ADDED + MODIFIED) so behaviour for requirements that lack the keyword everywhere stays unchanged. Adds three vitest cases under `test/core/validation.test.ts`: - ADDED block with header-only SHALL → enriched hint - MODIFIED block with header-only MUST → enriched hint - Neither header nor body contain SHALL/MUST → generic message preserved Verified by reproducing the spec from #356, running `openspec validate ` against the rebuilt CLI, and confirming the new diagnostic guides the author to the fix. Reverting `validator.ts` makes the two enriched-hint cases fail, so the tests guard the regression. Fixes #356 Co-authored-by: Cursor --- src/core/validation/validator.ts | 22 +++++++- test/core/validation.test.ts | 86 ++++++++++++++++++++++++++++++++ 2 files changed, 106 insertions(+), 2 deletions(-) diff --git a/src/core/validation/validator.ts b/src/core/validation/validator.ts index 37b43a37d..47071ed47 100644 --- a/src/core/validation/validator.ts +++ b/src/core/validation/validator.ts @@ -165,7 +165,7 @@ export class Validator { if (!requirementText) { issues.push({ level: 'ERROR', path: entryPath, message: `ADDED "${block.name}" is missing requirement text` }); } else if (!this.containsShallOrMust(requirementText)) { - issues.push({ level: 'ERROR', path: entryPath, message: `ADDED "${block.name}" must contain SHALL or MUST` }); + issues.push({ level: 'ERROR', path: entryPath, message: this.buildMissingShallOrMustMessage('ADDED', block.name) }); } const scenarioCount = this.countScenarios(block.raw); if (scenarioCount < 1) { @@ -186,7 +186,7 @@ export class Validator { if (!requirementText) { issues.push({ level: 'ERROR', path: entryPath, message: `MODIFIED "${block.name}" is missing requirement text` }); } else if (!this.containsShallOrMust(requirementText)) { - issues.push({ level: 'ERROR', path: entryPath, message: `MODIFIED "${block.name}" must contain SHALL or MUST` }); + issues.push({ level: 'ERROR', path: entryPath, message: this.buildMissingShallOrMustMessage('MODIFIED', block.name) }); } const scenarioCount = this.countScenarios(block.raw); if (scenarioCount < 1) { @@ -444,6 +444,24 @@ export class Validator { return /\b(SHALL|MUST)\b/.test(text); } + /** + * Build an error message for a requirement block whose body lacks SHALL/MUST. + * + * When the SHALL/MUST keyword already appears in the requirement header (e.g. + * `### Requirement: The system SHALL ...`) the original generic error + * ("must contain SHALL or MUST") is confusing because the keyword is visibly + * present in the spec. Per the OpenSpec conventions the keyword has to live + * on the requirement body line (the line right after the header), so we point + * the author at that exact fix when the keyword is found in the header only. + */ + private buildMissingShallOrMustMessage(action: 'ADDED' | 'MODIFIED', blockName: string): string { + const base = `${action} "${blockName}" must contain SHALL or MUST`; + if (this.containsShallOrMust(blockName)) { + return `${base} in the requirement body, not only in the header. Move the SHALL/MUST statement to the line immediately after the "### Requirement: ..." header.`; + } + return base; + } + private countScenarios(blockRaw: string): number { const matches = blockRaw.match(/^####\s+/gm); return matches ? matches.length : 0; diff --git a/test/core/validation.test.ts b/test/core/validation.test.ts index 972815e51..72ebc2aba 100644 --- a/test/core/validation.test.ts +++ b/test/core/validation.test.ts @@ -535,6 +535,92 @@ The system will log all events. expect(report.issues.some(i => i.message.includes('must contain SHALL or MUST'))).toBe(true); }); + it('should hint the author when ADDED requirement only has SHALL/MUST in the header', async () => { + const changeDir = path.join(testDir, 'test-change-shall-in-header-added'); + const specsDir = path.join(changeDir, 'specs', 'test-spec'); + await fs.mkdir(specsDir, { recursive: true }); + + const deltaSpec = `# Test Spec + +## ADDED Requirements + +### Requirement: The system SHALL log all errors +Error handling logic goes here. + +#### Scenario: Error occurs +**Given** an error +**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); + const shallMessage = report.issues.find(i => i.message.includes('must contain SHALL or MUST')); + expect(shallMessage?.message).toContain('not only in the header'); + expect(shallMessage?.message).toContain('### Requirement:'); + }); + + it('should hint the author when MODIFIED requirement only has SHALL/MUST in the header', async () => { + const changeDir = path.join(testDir, 'test-change-shall-in-header-modified'); + const specsDir = path.join(changeDir, 'specs', 'test-spec'); + await fs.mkdir(specsDir, { recursive: true }); + + const deltaSpec = `# Test Spec + +## MODIFIED Requirements + +### Requirement: The system MUST validate user input +Please describe how validation should work here. + +#### Scenario: Invalid input +**Given** invalid input +**When** validation runs +**Then** an error surfaces`; + + 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); + const shallMessage = report.issues.find(i => i.message.includes('must contain SHALL or MUST')); + expect(shallMessage?.message).toContain('not only in the header'); + expect(shallMessage?.message).toContain('### Requirement:'); + }); + + it('should keep the generic SHALL/MUST error when neither header nor body contain the keyword', async () => { + const changeDir = path.join(testDir, 'test-change-shall-nowhere'); + const specsDir = path.join(changeDir, 'specs', 'test-spec'); + await fs.mkdir(specsDir, { recursive: true }); + + const deltaSpec = `# Test Spec + +## ADDED Requirements + +### Requirement: Logging Feature +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); + const shallMessage = report.issues.find(i => i.message.includes('must contain SHALL or MUST')); + expect(shallMessage?.message).not.toContain('not only in the header'); + }); + it('should handle requirements without metadata fields', async () => { const changeDir = path.join(testDir, 'test-change-4'); const specsDir = path.join(changeDir, 'specs', 'test-spec');