fix(validator): hint when SHALL/MUST appears only in requirement header#1135
fix(validator): hint when SHALL/MUST appears only in requirement header#1135Pluviobyte wants to merge 1 commit into
Conversation
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 Fission-AI#356, running `openspec validate <change>` 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 Fission-AI#356 Co-authored-by: Cursor <cursoragent@cursor.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe PR refines delta-spec validation error messages to detect when SHALL/MUST keywords appear only in requirement headers and provide helpful guidance to users. A new helper function conditionally selects either a generic error or a header-specific message, replacing fixed templates in ADDED and MODIFIED requirement validation paths. Three test cases validate the improved messaging behavior. ChangesSHALL/MUST validation error messaging improvement
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Summary
Fixes #356.
When a change delta contains a requirement whose body is missing
SHALL/MUSTbut whose header (the text after### Requirement:) already contains the keyword, the validator currently emits the generic error:Authors then re-read the spec, see
SHALLright there in the header, and have no idea what the validator wants. The reporter of #356 hit exactly this case with an LLM-generated spec where the keyword lived only in the heading.Per the OpenSpec conventions (and
openspec/specs/openspec-conventions/spec.md, Structured Format for Behavioral Specs), the SHALL/MUST statement has to live on the requirement body line, i.e. the line immediately after the header. When the keyword is present in the header only, this PR appends a one-sentence hint that points the author at that exact fix:The general "keyword missing everywhere" case keeps the original short message — no behaviour change other than the diagnostic for the header-only situation.
Scope
Two-line touch in
src/core/validation/validator.ts(thevalidateChangeDeltaSpecsADDED + MODIFIED branches) plus a small helper that picks the message based on whetherblock.nameitself containsSHALL/MUST.RequirementSchema(used byvalidateSpec) is intentionally untouched — that path already promotes body text to the requirement string and reports its own error; this PR is limited to the change-delta diagnostic the bug report actually triggers.src/core/validation/validator.ts+22 -2— new privatebuildMissingShallOrMustMessage('ADDED' | 'MODIFIED', name)helper; both error sites delegate to ittest/core/validation.test.ts+86 -2— three new vitest cases insideValidator > validateChangeDeltaSpecs with metadataDiff (key sites)
src/core/validation/validator.ts:Behaviour matrix
Reproduction & verification
Reproduced #356 directly with the spec from the bug report against the rebuilt CLI:
Author follows the hint and rewrites the body:
$ openspec validate test-356 Change 'test-356' is validGeneric case (no SHALL/MUST anywhere) keeps the short message:
$ openspec validate test-356 ✗ [ERROR] agent-management/spec.md: ADDED "Logging Feature" must contain SHALL or MUSTRegression evidence
Tests genuinely cover the new branch —
git stashthevalidator.tschange, rerun:git stash popand both pass again.Local CI
(The previously flaky
change-initiative-link.test.ts > sets and surfaces initiative links ...10s timeout happened on the very first baseline run before any of my changes and did not reproduce on subsequent runs.)Notes
claude-4.6-opus-max-thinking). All reasoning, diffs, tests, repro runs, and the PR body were reviewed by me before submission.README.mdContributing.Made with Cursor
Summary by CodeRabbit
Bug Fixes
Tests