Repository navigation
fix(validate): fail --strict on requirements over the length limit - #2020
Conversation
A requirement description over 500 characters was an INFO finding, so `openspec validate --all --strict` still exited 0 and CI could not hold the limit. It is now a WARNING: normal validation and archive still pass, while strict mode fails, the same split the SHALL/MUST keyword warning already uses. The specs instruction and its docs page say so. Closes #1976 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: Fission-AI/OpenSpec/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughOverlong descriptions in main specs and ADDED requirements now produce warnings. Normal validation remains valid, while strict validation fails on these warnings. Guidance describes how to split an existing requirement when requested. ChangesRequirement length handling
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix · Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to Overlong main-spec and ADDED requirement descriptions now warn in normal validation and fail strict validation; MODIFIED requirements remain unchecked as intended. No material merge-blocking risk is established. Security Architecture ReviewSecurity architecture risk: ⚪ Minimal · up to The change strengthens strict validation without expanding filesystem access or privileges. Ordinary validation remains non-blocking for these warnings. Existing projects with overlong requirements may need corrections before strict checks pass. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
No PR-relevant drift confirmed.
|
alfred-openspec
left a comment
There was a problem hiding this comment.
Reviewed the validation behavior, regression coverage, changeset, and canonical docs update. Normal validation remains advisory, strict validation now enforces the documented warning, and CI is green.
With the length finding now failing --strict, a change could still add an overlong requirement and pass `validate <change> --strict`; CI only went red after archive merged it into the main spec. ADDED requirements now get the same warning, using the shared body reader so the limit matches the main spec exactly. MODIFIED is left alone, since its text is the existing requirement the instruction says to keep whole. The specs instruction (and its docs-lab page) now says how to split an existing long requirement in a dedicated change: keep the MODIFIED header and every scenario, cut the description to one behavior, and add each removed behavior as its own ADDED requirement. Verified end to end: the split change validates strict, archives, and the main spec then passes --strict. Closes the rest of #1976. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
alfred-openspec
left a comment
There was a problem hiding this comment.
Re-reviewed the new head. The ADDED-requirement check uses the shared body reader, matches the main-spec 500/501 boundary, preserves normal/archive behavior, and intentionally leaves MODIFIED requirements unchanged. The splitting guidance and regression coverage align with #1976, and the full hosted matrix is green.
Closes #1976
Status: LGTM. Finishes what #1978 started, including the CI gate before archive and how to split an existing long requirement.
What was wrong: #1976 asked for three things. #1978 shipped two of them: agents are now told about the 500-character limit, and the validator message explains how to split. Two gaps stayed open: the finding was
INFO, soopenspec validate --all --strictexited 0 and CI had no way to enforce the limit; and there was no documented way to split an existing long requirement (the instruction only said not to split under MODIFIED).How it was fixed:
INFOWARNINGopenspec validate --allopenspec validate --all --strictopenspec validate <change> --strictwith an overlong ADDED requirementopenspec archiveADDED requirements in a change get the same warning, measured by the same shared body reader, so a change fails
--strictat exactly the length its main spec would after archive. MODIFIED is not checked: its text is the existing requirement, which the instruction says to keep whole.The specs instruction (and its docs-lab page, which quotes it) now says how to split an existing long requirement in a change made for that purpose: keep the MODIFIED header and every scenario, cut the description to one behavior, and add each removed behavior as its own ADDED requirement.
This is the same normal-passes, strict-fails split the SHALL/MUST keyword warning already uses (
cli-validatespec). The specs instruction and its docs-lab page now say "a warning …--strictfails on it" instead of "an informational hint".Replication / proof:
fails strict validation, but not normal validation, on an overlong requirement) and the updated boundary test both fail onmainand pass here.validate --allexits 0;validate --all --strictprinted[WARNING] requirements[0]: Requirement text is very long…and exited 1 (it exited 0 before).dist: a change adding a 501-character requirement exits 1 undervalidate <change> --strictand still archives; the documented split (MODIFIED keeping its scenarios + two ADDED) validates strict, archives, and the main spec then passesvalidate --specs --strict.artifact-workflow"creates skills for Cursor tool",config-profile"confirmed project apply") also fail onmain.tsc --noEmitand eslint are clean.Notes / nits:
--strictusers: projects that already have overlong requirements will seevalidate --strictstart failing after upgrading. That is what the issue asks for, and the message says how to fix it. The changeset calls it out.🤖 Generated with Claude Code
Summary by CodeRabbit