fix(validate): allow non-English requirements - #1502
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughNormal validation accepts non-English requirement text and emits SHALL/MUST guidance warnings. Strict validation treats these warnings as failures. Missing requirement bodies remain errors. Specifications and tests document and verify this behavior. ChangesRequirement keyword validation
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant openspec_validate
participant Validator
participant ValidationReport
User->>openspec_validate: validate non-English specification
openspec_validate->>Validator: inspect requirement body
Validator->>ValidationReport: emit SHALL/MUST warning
openspec_validate->>ValidationReport: return valid result
User->>openspec_validate: validate with strict mode
openspec_validate->>ValidationReport: return failure for warning
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/core/validation.test.ts (1)
701-709: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert that missing-body cases emit no keyword warning.
The test checks one
ERROR, but it does not check the warning count. A regression could emit both the missing-text error and theSHALL/MUSTwarning and still pass. Addexpect(report.summary.warnings).toBe(0).Proposed assertion
expect(report.valid).toBe(false); expect(report.summary.errors).toBe(1); + expect(report.summary.warnings).toBe(0);This preserves the distinction between missing requirement text and missing keyword guidance.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/core/validation.test.ts` around lines 701 - 709, Add an assertion in the missing-body validation test around Validator.validateChangeDeltaSpecs to require report.summary.warnings to equal 0, while preserving the existing invalid status, error count, and missing-text issue assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@test/core/validation.test.ts`:
- Around line 701-709: Add an assertion in the missing-body validation test
around Validator.validateChangeDeltaSpecs to require report.summary.warnings to
equal 0, while preserving the existing invalid status, error count, and
missing-text issue assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 50489259-1f16-46f4-b404-e6c7cff59862
📒 Files selected for processing (3)
openspec/specs/cli-validate/spec.mdtest/cli-e2e/validate-international.test.tstest/core/validation.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- openspec/specs/cli-validate/spec.md
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
Reviewed at exact head 7778fb6. Normal validation now treats English RFC 2119 keywords as language-neutral guidance, strict mode still enforces the warning, and missing requirement bodies remain hard errors. Isolated build plus 163 focused validator, archive, and international E2E tests pass; hosted CI/security is green.
2 participants
Status
LGTM. This is a narrow, backward-compatible validator fix.
What was wrong
Normal validation required every requirement body to contain the literal English keyword
SHALLorMUST. Requirements written in another language therefore failed even when their structure and scenarios were valid. The same input failed in both normal and strict modes.How it was fixed
A non-empty requirement body without those English keywords now produces an RFC 2119 guidance warning. Normal validation succeeds, while
--strictcontinues to fail on the warning. A genuinely missing requirement body remains an error. The behavior is consistent for main specs and ADDED/MODIFIED change deltas.No parser, schema, command shape, public type, dependency, or architecture changed.
Replication / proof
Before the fix, a valid Chinese spec failed with exit code 1 in both modes:
After the fix:
npx, and both pass under the workspace runtimeNotes / nits
This is a clean replacement for closed PR #284, whose approach a maintainer approved but which included unrelated generated
distand packaging changes. This PR contains neither.Fixes #243
Summary by CodeRabbit
New Features
SHALLorMUSTreceive guidance warnings instead of errors.Bug Fixes