diff --git a/.changeset/allow-non-english-requirements.md b/.changeset/allow-non-english-requirements.md new file mode 100644 index 0000000000..065f15a6a8 --- /dev/null +++ b/.changeset/allow-non-english-requirements.md @@ -0,0 +1,5 @@ +--- +"@fission-ai/openspec": patch +--- + +`openspec validate` now treats the English `SHALL`/`MUST` convention as guidance in normal mode, so requirements written in other languages can validate. Strict mode continues to enforce the convention. diff --git a/openspec/specs/cli-validate/spec.md b/openspec/specs/cli-validate/spec.md index 5f213978c4..4c904bb0ab 100644 --- a/openspec/specs/cli-validate/spec.md +++ b/openspec/specs/cli-validate/spec.md @@ -43,6 +43,34 @@ The validator SHALL recognize bulleted lines that look like scenarios (e.g., lin - **AND** ... ``` +### Requirement: Normative keyword guidance SHALL not require English + +The validation report SHALL include a warning for a non-empty requirement body without the literal English keywords `SHALL` or `MUST`. Normal validation SHALL remain valid when that warning is the only issue, while strict validation SHALL remain invalid because strict mode treats warnings as failures. + +A requirement with no body content before its scenarios SHALL remain an error. + +#### Scenario: Non-English main spec + +- **WHEN** a main spec has a non-empty requirement body written without the English keywords `SHALL` or `MUST` +- **THEN** the validation report includes an RFC 2119 guidance warning +- **AND** normal validation succeeds + +#### Scenario: Non-English change delta + +- **WHEN** an ADDED or MODIFIED requirement has a non-empty body written without the English keywords `SHALL` or `MUST` +- **THEN** the validation report includes an RFC 2119 guidance warning +- **AND** normal validation succeeds + +#### Scenario: Strict validation preserves keyword enforcement + +- **WHEN** the same main spec or change is validated in strict mode +- **THEN** the warning causes validation to fail + +#### Scenario: Requirement body is missing + +- **WHEN** a requirement has no body content before its scenarios +- **THEN** validation reports an error + ### Requirement: All issues SHALL include file paths and structured locations Error, warning, and info messages SHALL include: - Source file path (`openspec/changes/{id}/proposal.md`, `.../specs/{cap}/spec.md`) @@ -245,4 +273,3 @@ The markdown parser SHALL correctly identify sections regardless of line ending - **AND** the document contains `## Why` and `## What Changes` - **WHEN** running `openspec validate ` - **THEN** validation SHALL recognize the sections and NOT raise parsing errors - diff --git a/src/core/validation/validator.ts b/src/core/validation/validator.ts index 67ca299848..6beb944338 100644 --- a/src/core/validation/validator.ts +++ b/src/core/validation/validator.ts @@ -136,7 +136,8 @@ export class Validator { * Validate delta-formatted spec files under a change directory. * Enforces: * - At least one delta across all files - * - ADDED/MODIFIED: each requirement has SHALL/MUST and at least one scenario + * - ADDED/MODIFIED: each requirement has at least one scenario; missing + * English SHALL/MUST keywords are guidance unless strict mode is enabled * - REMOVED: names only; no scenario/description required * - RENAMED: pairs well-formed * - No duplicates within sections; no cross-section conflicts per spec @@ -247,7 +248,15 @@ export class Validator { : `ADDED "${block.name}" is missing requirement text`, }); } else if (!this.containsShallOrMust(requirementText)) { - issues.push({ level: 'ERROR', path: entryPath, message: this.buildMissingShallOrMustMessage(`ADDED "${block.name}"`, block.name) }); + issues.push({ + level: 'WARNING', + path: entryPath, + message: this.buildMissingShallOrMustMessage( + `ADDED "${block.name}"`, + block.name, + true + ), + }); } const scenarioCount = this.countScenarios(block.raw); if (scenarioCount < 1) { @@ -274,7 +283,15 @@ export class Validator { : `MODIFIED "${block.name}" is missing requirement text`, }); } else if (!this.containsShallOrMust(requirementText)) { - issues.push({ level: 'ERROR', path: entryPath, message: this.buildMissingShallOrMustMessage(`MODIFIED "${block.name}"`, block.name) }); + issues.push({ + level: 'WARNING', + path: entryPath, + message: this.buildMissingShallOrMustMessage( + `MODIFIED "${block.name}"`, + block.name, + true + ), + }); } const scenarioCount = this.countScenarios(block.raw); if (scenarioCount < 1) { @@ -589,21 +606,30 @@ export class Validator { } }); - // SHALL/MUST body-keyword enforcement for main specs (#1156). The main-spec + // SHALL/MUST body-keyword guidance for main specs (#1156, #243). The main-spec // parser collapses the requirement header into `text`, so we recover the // header+body pairs here (the same source the delta path trusts) and reuse - // the delta detection: a body that omits the keyword errors, with the - // targeted "move it to the body line" hint when the keyword is in the header - // only and the generic message otherwise. Emitted exactly once per + // the delta detection. A non-empty body that omits the English keyword gets + // guidance, while a missing body remains an error. Emitted exactly once per // requirement (the Zod refine that used to emit a generic error is removed). extractRequirementsSection(content).bodyBlocks.forEach((block, index) => { const requirementText = this.extractRequirementText(block.raw); - if (!requirementText || !this.containsShallOrMust(requirementText)) { + if (!requirementText) { issues.push({ level: 'ERROR', path: `requirements[${index}]`, message: this.buildMissingShallOrMustMessage(`Requirement "${block.name}"`, block.name), }); + } else if (!this.containsShallOrMust(requirementText)) { + issues.push({ + level: 'WARNING', + path: `requirements[${index}]`, + message: this.buildMissingShallOrMustMessage( + `Requirement "${block.name}"`, + block.name, + true + ), + }); } }); @@ -709,7 +735,7 @@ export class Validator { } /** - * Build an error message for a requirement block whose body lacks SHALL/MUST. + * Build a 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 @@ -718,12 +744,17 @@ export class Validator { * 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(prefix: string, blockName: string): string { - const base = `${prefix} must contain SHALL or MUST`; + private buildMissingShallOrMustMessage( + prefix: string, + blockName: string, + guidanceOnly = false + ): string { + const base = `${prefix} ${guidanceOnly ? 'should' : 'must'} contain SHALL or MUST`; + const suffix = guidanceOnly ? ' (RFC 2119 best practice for English specs)' : ''; 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} in the requirement body, not only in the header. Move the SHALL/MUST statement to the line immediately after the "### Requirement: ..." header.${suffix}`; } - return base; + return `${base}${suffix}`; } private countScenarios(blockRaw: string): number { diff --git a/test/cli-e2e/validate-international.test.ts b/test/cli-e2e/validate-international.test.ts new file mode 100644 index 0000000000..da0ceb040f --- /dev/null +++ b/test/cli-e2e/validate-international.test.ts @@ -0,0 +1,143 @@ +import { afterAll, describe, expect, it } from 'vitest'; +import { promises as fs } from 'fs'; +import path from 'path'; +import { tmpdir } from 'os'; +import { runCLI } from '../helpers/run-cli.js'; + +const tempRoots: string[] = []; + +/** Create a temporary project containing a non-English main spec. */ +async function prepareNonEnglishSpec(): Promise { + const projectDir = await fs.mkdtemp(path.join(tmpdir(), 'openspec-i18n-validation-')); + tempRoots.push(projectDir); + const specDir = path.join(projectDir, 'openspec', 'specs', '日志记录'); + await fs.mkdir(specDir, { recursive: true }); + await fs.writeFile( + path.join(specDir, 'spec.md'), + `# 日志记录 + +## Purpose +记录应用程序中的重要事件，以便团队能够诊断问题、调查故障、审计活动并了解长期的系统行为。 + +## Requirements + +### Requirement: 事件记录 +系统必须记录应用程序中的重要事件。 + +#### Scenario: 事件发生 +- **WHEN** 应用程序生成重要事件 +- **THEN** 系统保存该事件 +` + ); + return projectDir; +} + +/** Create a temporary project containing a non-English change delta. */ +async function prepareNonEnglishChange(): Promise { + const projectDir = await fs.mkdtemp(path.join(tmpdir(), 'openspec-i18n-change-validation-')); + tempRoots.push(projectDir); + const specDir = path.join( + projectDir, + 'openspec', + 'changes', + '添加日志', + 'specs', + '日志记录' + ); + await fs.mkdir(specDir, { recursive: true }); + await fs.writeFile( + path.join(specDir, 'spec.md'), + `# 日志记录变更 + +## ADDED Requirements + +### Requirement: 事件记录 +系统必须记录应用程序中的重要事件。 + +#### Scenario: 事件发生 +- **WHEN** 应用程序生成重要事件 +- **THEN** 系统保存该事件 +` + ); + return projectDir; +} + +afterAll(async () => { + await Promise.all(tempRoots.map((dir) => fs.rm(dir, { recursive: true, force: true }))); +}); + +describe('non-English validation (#243)', () => { + it('passes normally with guidance but still fails in strict mode', async () => { + const projectDir = await prepareNonEnglishSpec(); + + const normal = await runCLI( + ['validate', '日志记录', '--type', 'spec', '--no-interactive'], + { cwd: projectDir } + ); + expect(normal.exitCode).toBe(0); + expect(normal.stdout).toContain('Specification'); + expect(normal.stdout).toContain('is valid'); + + const normalJson = await runCLI( + ['validate', '日志记录', '--type', 'spec', '--json', '--no-interactive'], + { cwd: projectDir } + ); + const report = JSON.parse(normalJson.stdout); + expect(normalJson.exitCode).toBe(0); + expect(report.summary.totals).toMatchObject({ passed: 1, failed: 0 }); + expect(report.items[0].valid).toBe(true); + expect(report.items[0].issues).toContainEqual( + expect.objectContaining({ + level: 'WARNING', + message: expect.stringContaining('should contain SHALL or MUST'), + }) + ); + + const strict = await runCLI( + ['validate', '日志记录', '--type', 'spec', '--strict', '--no-interactive'], + { cwd: projectDir } + ); + expect(strict.exitCode).toBe(1); + const strictOutput = `${strict.stdout}${strict.stderr}`; + expect(strictOutput).toContain('should contain SHALL or MUST'); + expect(strictOutput).toContain('has issues'); + }); + + it('validates a non-English change delta normally but not in strict mode', async () => { + const projectDir = await prepareNonEnglishChange(); + + const normalJson = await runCLI( + ['validate', '添加日志', '--type', 'change', '--json', '--no-interactive'], + { cwd: projectDir } + ); + const normalReport = JSON.parse(normalJson.stdout); + expect(normalJson.exitCode).toBe(0); + expect(normalReport.summary.totals).toMatchObject({ passed: 1, failed: 0 }); + expect(normalReport.items[0]).toMatchObject({ + id: '添加日志', + type: 'change', + valid: true, + }); + expect(normalReport.items[0].issues).toContainEqual( + expect.objectContaining({ + level: 'WARNING', + message: expect.stringContaining('should contain SHALL or MUST'), + }) + ); + + const strictJson = await runCLI( + ['validate', '添加日志', '--type', 'change', '--strict', '--json', '--no-interactive'], + { cwd: projectDir } + ); + const strictReport = JSON.parse(strictJson.stdout); + expect(strictJson.exitCode).toBe(1); + expect(strictReport.summary.totals).toMatchObject({ passed: 0, failed: 1 }); + expect(strictReport.items[0].valid).toBe(false); + expect(strictReport.items[0].issues).toContainEqual( + expect.objectContaining({ + level: 'WARNING', + message: expect.stringContaining('should contain SHALL or MUST'), + }) + ); + }); +}); diff --git a/test/core/archive.test.ts b/test/core/archive.test.ts index 516b79cf4a..c32e017e14 100644 --- a/test/core/archive.test.ts +++ b/test/core/archive.test.ts @@ -3321,15 +3321,13 @@ The system SHALL log all events.`; const changeSpecDir = path.join(changeDir, 'specs', 'bad-capability'); await fs.mkdir(changeSpecDir, { recursive: true }); - // Delta spec missing required SHALL/MUST keyword -> validation error + // Delta spec missing requirement text -> validation error const specContent = `# Bad Capability - Changes ## ADDED Requirements ### Requirement: Logging Feature -The system will log all events. - #### Scenario: Event recorded - **WHEN** an event occurs - **THEN** it is captured`; diff --git a/test/core/validation.test.ts b/test/core/validation.test.ts index 04c63d943e..e00d3c851a 100644 --- a/test/core/validation.test.ts +++ b/test/core/validation.test.ts @@ -641,7 +641,7 @@ The system SHALL record request metrics. expect(report.summary.errors).toBe(0); }); - it('should fail when requirement text lacks SHALL/MUST', async () => { + it('should fail strict validation 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 }); @@ -663,14 +663,54 @@ The system will log all events. const specPath = path.join(specsDir, 'spec.md'); await fs.writeFile(specPath, deltaSpec); - const validator = new Validator(true); - const report = await validator.validateChangeDeltaSpecs(changeDir); + const normalReport = await new Validator().validateChangeDeltaSpecs(changeDir); + expect(normalReport.valid).toBe(true); + expect(normalReport.summary.errors).toBe(0); + expect(normalReport.summary.warnings).toBe(1); + const report = await new Validator(true).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); + expect(report.summary.errors).toBe(0); + expect(report.summary.warnings).toBe(1); + expect( + report.issues.some( + i => i.level === 'WARNING' && i.message.includes('should contain SHALL or MUST') + ) + ).toBe(true); }); + it.each(['ADDED', 'MODIFIED'] as const)( + 'should keep missing requirement text as an error for %s requirements', + async operation => { + const changeDir = path.join(testDir, `test-change-missing-${operation.toLowerCase()}-text`); + const specsDir = path.join(changeDir, 'specs', 'test-spec'); + await fs.mkdir(specsDir, { recursive: true }); + await fs.writeFile( + path.join(specsDir, 'spec.md'), + `# Test Spec + +## ${operation} Requirements + +### Requirement: Logging Feature + +#### Scenario: Event occurs +- **WHEN** an event occurs +- **THEN** it is logged` + ); + + const report = await new Validator().validateChangeDeltaSpecs(changeDir); + expect(report.valid).toBe(false); + expect(report.summary.errors).toBe(1); + expect(report.summary.warnings).toBe(0); + expect(report.issues).toContainEqual( + expect.objectContaining({ + level: 'ERROR', + message: expect.stringContaining('missing requirement text'), + }) + ); + } + ); + 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'); @@ -695,7 +735,8 @@ Error handling logic goes here. 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')); + const shallMessage = report.issues.find(i => i.message.includes('should contain SHALL or MUST')); + expect(shallMessage?.level).toBe('WARNING'); expect(shallMessage?.message).toContain('not only in the header'); expect(shallMessage?.message).toContain('### Requirement:'); }); @@ -724,12 +765,13 @@ Please describe how validation should work here. 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')); + const shallMessage = report.issues.find(i => i.message.includes('should contain SHALL or MUST')); + expect(shallMessage?.level).toBe('WARNING'); 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 () => { + it('should keep generic SHALL/MUST guidance 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 }); @@ -753,7 +795,8 @@ The system will log all events. 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')); + const shallMessage = report.issues.find(i => i.message.includes('should contain SHALL or MUST')); + expect(shallMessage?.level).toBe('WARNING'); expect(shallMessage?.message).not.toContain('not only in the header'); }); @@ -918,7 +961,7 @@ The system MUST support mixed case delta headers. // actionable sentence byte-identical to the change-delta path, emitted once. describe('main-spec SHALL/MUST body-keyword hint (#1156)', () => { const ACTIONABLE_SENTENCE = - 'must contain SHALL or MUST in the requirement body, not only in the header. Move the SHALL/MUST statement to the line immediately after the "### Requirement: ..." header.'; + 'should contain SHALL or MUST in the requirement body, not only in the header. Move the SHALL/MUST statement to the line immediately after the "### Requirement: ..." header. (RFC 2119 best practice for English specs)'; const buildSpec = (requirementBlock: string): string => [ @@ -967,7 +1010,7 @@ The system MUST support mixed case delta headers. expect(deltaMsg.startsWith('ADDED "The system SHALL log"')).toBe(true); }); - it('keeps a generic missing-keyword error when neither header nor body has the keyword', async () => { + it('keeps generic missing-keyword guidance when neither header nor body has the keyword', async () => { const content = buildSpec( '### Requirement: Logging\nThe system will log all events.\n\n#### Scenario: S\n- **WHEN** x\n- **THEN** y' ); @@ -977,6 +1020,20 @@ The system MUST support mixed case delta headers. expect(issues[0].message).not.toContain('not only in the header'); }); + it('allows non-English requirement text in normal mode and warns about English keywords', async () => { + const content = buildSpec( + '### Requirement: 事件记录\n系统必须记录应用程序中的重要事件。\n\n#### Scenario: 事件发生\n- **WHEN** 应用程序生成重要事件\n- **THEN** 系统保存该事件' + ); + const report = await new Validator().validateSpecContent('demo', content); + const issues = report.issues.filter(i => i.message.includes('SHALL or MUST')); + + expect(report.valid).toBe(true); + expect(report.summary.errors).toBe(0); + expect(issues).toHaveLength(1); + expect(issues[0].level).toBe('WARNING'); + expect(issues[0].message).toContain('best practice for English specs'); + }); + it('does not flag a requirement whose body line contains the keyword', async () => { const content = buildSpec( '### Requirement: Logging\nThe system SHALL log all events.\n\n#### Scenario: S\n- **WHEN** x\n- **THEN** y' @@ -999,7 +1056,11 @@ The system MUST support mixed case delta headers. ); const report = await new Validator().validateSpecContent('demo', content); const issues = shallIssues(report.issues); + expect(report.valid).toBe(false); + expect(report.summary.errors).toBe(1); + expect(report.summary.warnings).toBe(0); expect(issues).toHaveLength(1); + expect(issues[0].level).toBe('ERROR'); expect(issues[0].message).toContain('not only in the header'); }); @@ -1240,7 +1301,9 @@ ${body}`; // The metadata IS the body when nothing else remains, so the failure is // the missing keyword, not missing text. expect( - report.issues.some(i => i.message.includes('must contain SHALL or MUST')) + report.issues.some( + i => i.level === 'WARNING' && i.message.includes('should contain SHALL or MUST') + ) ).toBe(true); }); @@ -1327,7 +1390,9 @@ These notes explain that the system MUST NOT be read as requirement text. // and the skipped divider is surfaced as INFO. expect(report.valid).toBe(false); expect( - report.issues.some(i => i.level === 'ERROR' && i.message.includes('must contain SHALL or MUST')) + report.issues.some( + i => i.level === 'WARNING' && i.message.includes('should contain SHALL or MUST') + ) ).toBe(true); expect( report.issues.some(i => i.level === 'INFO' && i.message.includes('"### Background"'))