fix(validate): reject a delta spec at the change's specs/ root#1392
Conversation
📝 WalkthroughWalkthroughValidation now uses shared spec discovery and rejects delta specs at ChangesDelta spec layout enforcement
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 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 |
A `spec.md` written directly under a change's `specs/` directory was accepted by `validate` — including `--strict` — but skipped by the apply/archive merge, which only reads capability folders. The change validated clean, archived successfully, and its requirements never reached `openspec/specs/`. Point the validator at the shared `discoverSpecFiles` helper so it applies exactly the merge path's rules, and report a root-level `specs/spec.md` as an error naming the capability-folder convention. Archive's delta-detection gate now also sees that file, so validation runs and blocks the archive instead of completing with the delta dropped. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
805169b to
61d6a2b
Compare
Follow-up to the same divergence class as the parent commit, found while re-reviewing it. Archive's gate only ran validation when a candidate file carried delta headers, so a root-level `specs/spec.md` written in main-spec shape (`## Requirements`) still archived with exit 0 while `validate` reported an error — the two commands disagreed again. The file is never merged whatever its shape, so existence alone now trips the gate. Also stop reporting a *directory* named `specs/spec.md` as misplaced: that is an ordinary capability folder the merge path reads normally, and `fileExists` matched it. Both sites now require a regular file. Finally, suppress the generic "No deltas found" error when the root-level error already fired: it contradicted the precise message by claiming there were no deltas in the very file just named. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
alfred-openspec
left a comment
There was a problem hiding this comment.
Using the shared discovery helper removes the validator/merge walker divergence, while the explicit root-file check correctly blocks archive regardless of file shape and preserves a directory named spec.md as a valid capability. The focused validation/archive suite passed 97/97 at this exact head, and the full cross-platform CI matrix is green.
Fixes #1385.
Status: LGTM — reproduced on main, fixed, regression-tested.
What was wrong
If you wrote a change's delta spec at
openspec/changes/<change>/specs/spec.md— no capability folder — OpenSpec told you it was fine and then threw it away.openspec validate <change> --strictpassed and counted the deltas. But everything that actually merges specs (show,apply,archive) only readsspecs/<capability>/spec.md, so it saw zero deltas. The change archived successfully and the requirement never reachedopenspec/specs/. The only signal was a non-blocking warning that scrolled by.Two walkers disagreed: the validator had its own recursive scan, while the merge path used the shared
discoverSpecFileshelper.How it's fixed
discoverSpecFiles— the same helper the merge path uses — so it can never accept a layout the merge silently skips.specs/spec.mdis reported as an error that names the fix:Delta spec found at specs/spec.md. Delta specs must live in a capability folder (e.g. specs/<capability>/spec.md) — a file at the specs/ root is ignored when the change is applied or archived.archivetrips its validation gate on that file simply existing, so it blocks instead of archiving a change whose content will be dropped. Existence, not delta headers: a root-level file written in main-spec shape (## Requirements) is dropped just the same.specs/spec.mdis left alone — that is an ordinary capability folder the merge path reads normally.Move the file into a capability folder and everything behaves as before. No other layout changes.
Proof
Before (main), from the issue's repro:
After:
Behavior matrix, each row exercised against the built CLI:
specs/spec.md(delta shape)specs/spec.md(main-spec shape)specs/spec.mdplus a valid capability deltaspecs/<capability>/spec.mdspecs/<area>/<capability>/spec.mdspecs/spec.md/spec.md(folder namedspec.md)Five regression tests added across
validation.test.tsandarchive.test.ts. Full suite: 2,015 passing; the only failures are the 17 known environment-onlyzsh-installercases that also fail on unmodified main.Notes
discoverSpecFilesalso makes the validator skip dot-directories, matching the merge path — a delta hidden in a dot-directory was never merged anyway.spec.md(e.g.specs/billing.md) is invisible to both walkers, so it is dropped the same way without any warning. That predates this change and affects a different code path; happy to file it.--no-validatestill archives without these checks. That is the documented opt-out and already warns that it may archive invalid specs.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
specs/spec.mddirectly under a change’sspecs/folder is now detected during validation.specs/<capability>/spec.md).Tests