Repository navigation
fix(archive): refuse requirement names differing only in case - #1864
Conversation
ADDED and the RENAMED target compared requirement names exactly, while REMOVED and the RENAMED source already treated a name that differs only in case or interior whitespace as a mistyped header. An ADDED `late fees` beside an existing `Late Fees`, or a rename to `LATE FEES`, therefore archived cleanly and left two contradicting copies of one requirement in the main spec, which validate then accepted. Both now refuse with an error naming the existing requirement. The source of a rename is exempt from the target check, so a case-only rename of a requirement to its own name still applies, and ADDED is still checked against the spec as it stands after the earlier operations, so a variant of a requirement the same delta removes or renames away is allowed.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (5)
Included review availability: Your plan provides up to 10 included reviews per hour; 5 remain after this review. 📝 WalkthroughWalkthroughThe archive flow now rejects ADDED and RENAMED requirement names that differ from existing names only by case or whitespace. Core and end-to-end tests cover rejection, operation ordering, unchanged behavior, and valid case-only self-renames. ChangesRequirement name validation
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix · Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to The archive validation change has no identified merge-blocking risk. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
The near-match guard handles ADDED and RENAMED targets, preserves exact-duplicate behavior and self case-only renames, and respects operation order. Unit and end-to-end tests pass.
Closes #1863.
Why
Requirement names are matched exactly, but two spellings that differ only in
case or interior whitespace are one requirement written twice — the comment on
foldRequirementName(src/core/parsers/requirement-blocks.ts:25-26) calls them"a mistake, never two requirements". REMOVED (
src/core/specs-apply.ts:447) andthe RENAMED source (
:400) already refuse such a near-miss. The two operationsthat introduce a name did not:
nameToBlock.get(key);specs-apply.ts:413) used an exactnameToBlock.has(to).So an ADDED
late feesbeside an existingLate Fees, or a rename toLATE FEES, archived with exit 0 and left two contradicting requirements in themain spec (
Late Fees"30 days" andlate fees"15 days"), whichopenspec validatethen accepted.Verified against a project built entirely by
openspec init+openspec new change, with the main spec written by archiving a first change.What Changes
case and whitespace,
buildUpdatedSpecthrowsbilling ADDED failed for header "### Requirement: late fees" - "### Requirement: Late Fees" already exists and differs only in case or spacing; use MODIFIED with that exact header to change it, or choose a distinct name.rename's own source, with
...; choose a distinct name.foldRequirementNamedoc comment now lists ADDED and RENAMED among itsuses.
Unchanged, and pinned by tests:
target already existserror;Late Fees→LATE FEES) still applies — the source is exempt from the target check;allowed. Operations apply RENAMED → REMOVED → MODIFIED → ADDED, and ADDED is
checked against the spec as it stands at that point, exactly as the existing
exact-name check is, so the result holds one requirement, not two;
- not found) and still does.Testing
Written first and watched fail, then passing:
test/core/specs-apply.requirement-name-near-miss.test.ts— 14 tests(ADDED ×8, RENAMED ×5, MODIFIED ×1)
test/cli-e2e/archive-requirement-name-near-miss.test.ts— 4 end-to-endtests through the built CLI: exact duplicate refused (control), ADDED case
variant refused with spec and change left untouched, RENAMED case variant
refused, case-only self-rename still archives
Before the fix: 9 failed / 9 passed (the 9 passing are controls and the
ordering cases that were already right). After: 18 passed.
Edge cases covered: case variant, interior-whitespace variant, a case variant
whose body is identical to the existing requirement (not treated as an
early-sync no-op), two ADDED entries in one delta that differ only in case, a
variant of a requirement removed or renamed away in the same delta, a RENAMED
target colliding with a requirement the same delta removes (refused, as an
exact target already was, since RENAMED runs first).
The round-2 report's own reproduction for this bug (4 tests through the built
CLI) failed on
mainand passes on this branch.CI parity, run on Node 20.19.0 (the CI version) with the commands from
.github/workflows/ci.yml:pnpm run build— passpnpm exec tsc --noEmit— passpnpm lint— passchangeset status --since=origin/main— pass (patch → 1.13.1), run onNode 22 because
@changesets/cli3 needsmodule.enableCompileCache; theCI job that runs it uses Node 24
VITEST_MAX_WORKERS=4 pnpm test— 4562 passed, 19 failed of 4581. All 19are timing failures on a heavily shared host (load average 8–19 on 6 cores):
18 × "Test timed out in 10000ms" and 1 ×
spawnSync npm ETIMEDOUT, instore, workset, package-install and CLI e2e files that do not touch spec
merging. None is an assertion failure.
VITEST_MAX_WORKERS=2): 196 of 197passed. The one left is the
npm pack+tsctest inpackage-install-scripts.test.ts, which timed out at 11.6s.npm packtest run alone, twice each onmain, this branch andfix-closed-requirement-heading: 8 of 8 passed every time.VITEST_MAX_WORKERS=2 pnpm test): 4580of 4581 passed. The one failure was that same
npm packtest timing out(12.1s against the 10s limit). Rerun on its own immediately afterwards, it
passed 8 of 8 on this branch and on clean
main. The test builds athrowaway fixture in a temp directory from
build.jsand thepackage.jsonscripts, neither of which this branch changes.Changeset
.changeset/archive-refuses-requirement-name-near-miss.md(patch). Worth notingfor release notes: a change that used to archive by adding a case variant of an
existing requirement is now rejected until the name is fixed.
Notes for reviewers
validatestill does not compare ADDED names against the main spec — not forexact duplicates either — so this stays an archive-time check, consistent with
the existing exact-duplicate behavior.
fix-closed-requirement-headingalso editssrc/core/parsers/requirement-blocks.ts(thenormalizeRequirementNamebody,a few lines above the comment changed here). The hunks do not overlap.
Summary by CodeRabbit