fix(parser): stop delta section dividers from becoming phantom requirements#1411
fix(parser): stop delta section dividers from becoming phantom requirements#1411clay-good wants to merge 3 commits into
Conversation
`openspec validate --strict` reported a change as valid while `openspec archive` printed "Proposal warnings in proposal.md" for the same change, blaming requirements that do not exist. Archive validates the proposal with `validateChange`, which parses the change together with its delta specs. Requirement-level issues from those deltas were printed in the proposal block even though they are not proposal issues. Two problems followed: - The change parser records every requirement under both `requirement` and `requirements`, so each defect was printed twice, then a third time by the delta report. - A heading inside a delta section that is not a `### Requirement:` heading was parsed as a requirement, producing a scenario warning against a requirement that does not exist. The delta reader already handles this correctly and reports it as an informational note. Proposal warnings now report proposal-level issues only. Delta spec issues keep being reported once, by the delta report, with the capability file path and requirement name. Exit codes are unchanged: this block was already non-blocking, and blocking delta validation is untouched. Refs #498 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughArchive validation now filters delta requirement issues from proposal warning output, ignores non-requirement delta headers, preserves genuine proposal warnings, reports blocking delta errors once, and adds specification and regression coverage. ChangesArchive warning filtering
Estimated code review effort: 3 (Moderate) | ~20 minutes 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 |
Review follow-ups, no behavior change: - The delta report prints only the issue message, never `issue.path`, so it does not name the capability file. Drop that claim from the spec scenario and the code comment; two capabilities with the same defect print two identical lines. - Only the missing-scenario class was reported three times. Say that precisely instead of generalizing to every delta error. - Widen the spec scenario: the filter applies to every archive, not only to changes carrying a stray heading. - Add a test pinning that applyChangeRules bracket paths (`deltas[<n>].description`) survive the dot-anchored filter, so a future path normalization cannot silently widen it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/core/archive.test.ts (1)
1614-1639: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winVerify that warning-only archives complete successfully.
Both tests assert warning output but not archive state. A regression that turns proposal warnings into blocking validation failures could therefore pass because
archiveCommand.executereturns without throwing.
test/core/archive.test.ts#L1614-L1639: assert theshort-whychange is moved toarchive/.test/core/archive.test.ts#L1641-L1661: assertchangeDiris removed and its archive target exists.🤖 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/archive.test.ts` around lines 1614 - 1639, Verify warning-only archives complete by asserting the short-why change is moved to archive/ in the test at test/core/archive.test.ts lines 1614-1639. In the test at test/core/archive.test.ts lines 1641-1661, assert that changeDir is removed and its archive target exists; retain the existing warning-output 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/archive.test.ts`:
- Around line 1614-1639: Verify warning-only archives complete by asserting the
short-why change is moved to archive/ in the test at test/core/archive.test.ts
lines 1614-1639. In the test at test/core/archive.test.ts lines 1641-1661,
assert that changeDir is removed and its archive target exists; retain the
existing warning-output assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: af99edfe-5c0f-4e35-8480-cf773462b907
📒 Files selected for processing (3)
openspec/specs/cli-archive/spec.mdsrc/core/archive.tstest/core/archive.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/core/archive.ts
Fixes the cause of #498 rather than one of its symptoms. A header inside a delta section that is not a `### Requirement:` header — a divider such as `### Documentation Requirements` — was read as a requirement with no scenario. That invented a delta that does not exist: `openspec archive` warned about a missing scenario, and `openspec show <change> --json` and `openspec change list` counted it. ChangeParser now filters those headers before reading requirements, matching REQUIREMENT_HEADER_REGEX, which the delta reader already uses. The override lives in ChangeParser, so main spec parsing — view, list, spec --json, spec validation — is untouched. The archive filter stays: it covers the half the parser cannot. The change parser records every requirement under both `requirement` and `requirements`, so each delta defect was printed twice, and REMOVED requirements are names-only by design yet were reported as missing a scenario on every correct removal. Also from review: - Soften the spec scenario; delta spec validation does not always run (the hasDeltaSpecs gate is case-sensitive), so it cannot be promised as the reporter. - Assert VALIDATION_MESSAGES constants instead of message literals. - Add parser-level and REMOVED-only regression tests. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
alfred-openspec
left a comment
There was a problem hiding this comment.
Verified at 8f25d8d: the delta-only parser filter matches the canonical requirement reader, proposal-warning filtering preserves genuine proposal issues, and real delta defects still block archive. Build, 123 focused parser/archive/validation tests, a clean CLI reproduction, and exact-head CI pass.
Status: Ready for review. Non-breaking. No exit code changes.
What was wrong
openspec validate --stricttold you a change was fine, thenopenspec archivewarned about it anyway — naming a requirement that doesn't exist.The cause: a header inside a delta section that isn't a
### Requirement:header — a divider like### Documentation Requirements— was read as a requirement with no scenario.On
main, with such a divider above a well-formed requirement:The requirement it complained about did have a scenario. The archive succeeded anyway, so the warnings were pure noise contradicting
validate.And the phantom wasn't only cosmetic — it was in the JSON agents read:
How it was fixed
1. The cause —
ChangeParsernow ignores non-### Requirement:headers in delta sections. This matchesREQUIREMENT_HEADER_REGEX, which the delta reader already uses, so the two readers finally agree. The override lives inChangeParser, notMarkdownParser, so main spec parsing —view,list,spec --json, spec validation — is untouched.2. Archive's proposal warnings no longer repeat requirement-level issues from the delta specs. This covers the half the parser can't:
requirementandrequirements, so each delta defect was printed twice.## REMOVED Requirementsentries are names-only by design — delta spec validation exempts them, the proposal report did not. Every correct removal warned "Requirement must have at least one scenario". This is probably the most common real-world trigger.Exit codes are untouched (this block was already non-blocking), blocking delta validation is untouched, and genuine proposal-level warnings still print.
Deliberately not attempted: making
validatereport proposal issues instead.convertZodErrorsstamps every Zod issueERROR, so that direction would flip changes from exit 0 to exit 1 — including 4 of this repo's own 16 active changes — breaking CI for existing users.Proof it works
Same input, on this branch:
The real requirement still merges into the main spec with its scenario. Real delta defects still block, exit 1:
No real change loses a delta. Comparing
openspec change list --jsonacross this repo's own 16 active changes,mainand this branch produce byte-identical delta counts — the parser only drops phantoms.Seven regression tests (five in
test/core/archive.test.ts, two intest/core/parsers/change-parser.test.ts). Each was mutation-tested: disabling the parser override, removing the filter, narrowingrequirements?, inverting it, dropping the length gate, or widening it to bracket paths are all caught.Full suite: 2034 passed. The only failures are the 17 known environment-only
zsh-installerfailures from a local Oh My Zsh install, documented inCLAUDE.mdand unrelated. Verified on Windows/CRLF/CJK/nested-spec/store/non-TTY layouts, and at 120 deltas.Notes / nits
mainand this branch), but it deserves a follow-up:validateChangeDeltaSpecs, which archive does block on, could report non-### Requirement:headers inside delta sections. It already notes them at INFO level; archive prints only ERROR/WARNING. This disproportionately affects users whose IME produces a full-width colon (### 需求:…).Consider splitting changes with more than 10 deltasis emitted at pathdeltasand still surfaces only at archive time, so it remains an instance of the same "archive says something validate doesn't" shape. Left alone deliberately: it shares its path withChange must have at least one delta, which is a genuine proposal-level signal, and suppressing it is a separate judgment call.hasDeltaSpecsgate is case-sensitive while the merge path is not, so## Added Requirementsskips delta validation. Default archives still abort at the rebuilt-spec check; only--skip-specsslips through. Fixing it would newly block archives that succeed today, so it belongs in its own PR.Closes #498
🤖 Generated with Claude Code
Summary by CodeRabbit
openspec archiveso non-blocking “proposal warnings” remain proposal-level and no longer duplicate requirement issues coming from delta specs.