fix(validate): report scenarios a MODIFIED requirement would drop - #1482
Conversation
`openspec validate <change>` accepted a MODIFIED requirement that omits a scenario the main spec still has, even with --strict. Archive refuses to apply that block (a MODIFIED replaces the whole requirement, so the omitted scenario would be lost), so the change could pass validation, be implemented and reviewed, and fail only days later at archive time (#1477). Validate now runs the same non-mutating check against the main specs and reports each omitted scenario, naming the delta file. The comparison itself moved to the parser module so archive and validate share one implementation and cannot drift. The check is silent when the main spec file or the requirement header is absent — a MODIFIED written against a sister change still in flight is a separate condition archive gates — so validate can only report what archive already refuses. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Keep the moved scenario parser module-private, derive change validate's main specs root from the changes root it already resolved, replace the rename re-keying with a lookup fallback, and say at archive's call site why it does not opt in. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A delta that renames A to B and then B to C leaves C holding A's block at archive time. Walk the rename map instead of looking it up once, so the chained case reports the same loss archive refuses. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthrough
ChangesScenario-loss validation
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
🚥 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.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@test/core/validation.scenario-loss.test.ts`:
- Line 196: Update the nested-path assertion in the validation scenario test to
normalize issue?.path before comparison and construct the expected
platform/session/spec.md path using the platform-native path utility. Preserve
the existing assertion semantics while avoiding hard-coded path separators.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 595ab1d3-a9e3-4261-ae47-d292e07065e3
📒 Files selected for processing (9)
.changeset/validate-scenario-loss.mdopenspec/specs/cli-validate/spec.mdsrc/commands/change.tssrc/commands/validate.tssrc/core/archive.tssrc/core/parsers/requirement-blocks.tssrc/core/specs-apply.tssrc/core/validation/validator.tstest/core/validation.scenario-loss.test.ts
|
|
||
| expect(report.valid).toBe(false); | ||
| const issue = report.issues.find((i) => i.message.includes('omits scenario(s)')); | ||
| expect(issue?.path).toBe('platform/session/spec.md'); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Make the nested-path assertion platform-neutral.
Line 196 hard-codes /, so this can fail when issue.path uses Windows separators. Normalize the actual value and build the expected path natively.
Proposed fix
- expect(issue?.path).toBe('platform/session/spec.md');
+ expect(path.normalize(issue?.path ?? '')).toBe(path.join('platform', 'session', 'spec.md'));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| expect(issue?.path).toBe('platform/session/spec.md'); | |
| expect(path.normalize(issue?.path ?? '')).toBe(path.join('platform', 'session', 'spec.md')); |
🤖 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.scenario-loss.test.ts` at line 196, Update the
nested-path assertion in the validation scenario test to normalize issue?.path
before comparison and construct the expected platform/session/spec.md path using
the platform-native path utility. Preserve the existing assertion semantics
while avoiding hard-coded path separators.
Source: Coding guidelines
Code: - An unreadable main spec was swallowed, so a change archive aborts on validated clean. Only ENOENT/ENOTDIR mean "no main spec" now; anything else is reported. - A MODIFIED naming a header the same delta renames away no longer names scenarios from the block it would not land on. That contradiction is already reported on its own, and the scenario list pointed at the wrong requirement. Guidance: the sync-specs skill told agents a MODIFIED block may carry only the changed scenario, and its format reference showed one. Both validate and archive reject that shape, so the template, the generated skill, and the golden hashes are updated to match the schema's own rule. Tests: the CLI wiring had no coverage at all — removing the argument that turns the check on broke nothing. Adds end-to-end coverage of every entry point and exit code, plus the non-strict default, a fenced scenario in the delta, an unreadable main spec, the rename-away case, and a rename cycle. Loose assertions now pin the scenario-loss issue itself. Docs: a troubleshooting entry for the new message, and the changeset says that a stale change will newly fail. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Deploying openspec-docs with
|
| Latest commit: |
65c0f14
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://f4fe2c4e.openspec-docs.pages.dev |
| Branch Preview URL: | https://fix-validate-archive-parity.openspec-docs.pages.dev |
|
Ran five adversarial reviews over this branch (differential fuzzing, API/compat, robustness, spec-and-docs consistency, and a mutation-testing pass on the tests). Pushed 9b49084 with the fixes. The property held. ~5,200 randomized main-spec/delta pairs (~7,900 comparisons) plus 26 hand-built edge cases and 100 CLI runs found no case where validate reports a change archive would apply cleanly. Positive controls confirm the harness bites: validate fired 548 times in one 1,200-case run and agreed with the archive oracle 537/537. Fixed
Not changed, with reasons
Verification after the fixes: 3407 tests across 117 files green, lint and typecheck clean, both template parity tests green after regeneration, |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
ast-grep outline src/utils/file-system.ts --items all --match 'toPosixPath|FileSystemUtils'
printf '\n--- referenced implementation ---\n'
nl -ba src/utils/file-system.ts | sed -n '70,96p'
printf '\n--- issue.path normalization call sites ---\n'
rg -n -C 2 'toPosixPath\s*\(|issue\.path' src testLength of output: 1026 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- FileSystemUtils.toPosixPath implementation ---'
sed -n '78,102p' src/utils/file-system.ts
printf '%s\n' '--- production usages involving normalized issue paths ---'
grep -RIn -E 'toPosixPath[[:space:]]*\(|issue\.path' src 2>/dev/null | head -n 160
printf '%s\n' '--- relevant test assertions ---'
grep -RIn -E 'issue\.path|platform/session/spec\.md' test 2>/dev/null | head -n 120Length of output: 2985
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@docs/troubleshooting.md`:
- Around line 99-106: Add the text language identifier to the fenced code
example in the troubleshooting documentation, changing the bare fence around the
MODIFIED requirement message to a text-labeled fence while preserving the
example content.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 1ced4b7f-2aac-42ef-b461-f96daa30adaf
📒 Files selected for processing (10)
.changeset/validate-scenario-loss.mddocs/cli.mddocs/troubleshooting.mdopenspec/specs/cli-validate/spec.mdskills/openspec-sync-specs/SKILL.mdsrc/core/templates/workflows/sync-specs.tssrc/core/validation/validator.tstest/cli-e2e/validate-scenario-loss.test.tstest/core/templates/skill-templates-parity.test.tstest/core/validation.scenario-loss.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- .changeset/validate-scenario-loss.md
- openspec/specs/cli-validate/spec.md
- src/core/validation/validator.ts
The unreadable-main-spec report added in the last commit fired for any errno that was not ENOENT/ENOTDIR, which includes resource errors like EMFILE that say nothing about the file. `validate --all` reads six changes at once, so a busy process could have failed a change that is fine. Reported now only for the codes that mean the file itself is unusable and will be just as unusable when archive reads it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
Validation now catches the same scenario loss archive refuses, across every CLI entry point, while preserving rename, fenced-content, missing-base, and transient read-error behavior. The exact head passed the hosted CI/security matrix, CodeRabbit, an isolated build, and 22 focused core and real-CLI tests.
Every other fence in the file names its language. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Fixed in 89bd416: added the Still declining the earlier suggestion on |
|
Tip For best results, initiate chat on the files or code changes.
Agreed on the test assertion: since 🐇 ✅ ✏️ Learnings added
If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
The only change since the previously verified implementation is the MD040 text fence label. Exact head 89bd416 remains clean, with the full hosted CI/security matrix green and the scenario-loss behavior already validated by the isolated focused suite.
2 participants
Closes #1477.
Status: ready. Full suite green (3397 tests), lint and typecheck clean, and verified against this repo's own 54 changes and specs.
What was wrong
A change could pass
openspec validate— even with--strict— and still be impossible to archive.A
MODIFIEDrequirement replaces the whole requirement block, so archive refuses to apply one that omits a scenario the main spec still has (it would silently lose that scenario). Validate never ran that check, so the mistake — usually a renamed or forgotten scenario — survived authoring, implementation and review, and only surfaced days later when someone ranopenspec archive.Reproduced on 1.7.0 and on current
main:How it was fixed
openspec validate <change>now runs the same non-mutating check against the main specs and reports each omitted scenario, naming the delta file:The comparison itself moved into the parser module, so archive and validate share one implementation and cannot drift. It applies on every validate path:
validate <change>,validate --changes/--all, and the deprecatedchange validate.The check stays silent when the main spec file or the requirement header is absent — a
MODIFIEDwritten against a sister change still in flight is a separate condition that archive gates (#1112) — so validate can only report what archive already refuses.openspec archiveis unchanged; it still enforces this itself, with the same message and JSON error code as before.Proof it works
test/core/validation.scenario-loss.test.ts. Each one asserts validate's verdict and what archive does with the same fixture, so the parity claim is tested, not assumed.--all) found no case where validate reports a change archive would accept.Notes
MODIFIEDblock will see it fail validate where it passed before. Those changes were already unarchivable — this reports the failure at authoring time instead of at archive time, with the fix in the message.🤖 Generated with Claude Code
Summary by CodeRabbit
openspec validatenow errors when a change’sMODIFIEDrequirement omits scenarios that still exist in the replaced main spec (including repeated scenarios and rename scenarios), matching archive application rejection.MODIFIED.