Repository navigation
fix(parser): drop closing hashes from requirement names - #1860
Conversation
CommonMark lets an ATX heading end in a closing run of `#`s, so `### Requirement: Late Fees ###` renders as `Late Fees`. Requirement names kept the run, so a REMOVED written that way missed the requirement and archive exited 0 with a false "treating it as already removed" warning, while a closed MODIFIED or RENAMED heading failed as not found. normalizeRequirementName now strips the closing run with the rule scenario names already use: only a run preceded by a space or tab counts, so `C#` keeps its `#`. Every reader goes through it, and the main-spec duplicate check now does too, so a closed and an open heading of one requirement are reported as duplicates.
|
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; 7 remain after this review. 📝 WalkthroughWalkthroughChangesRequirement heading normalization
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~15 minutes Change: Bug fix · Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to Requirement headings with CommonMark closing hashes are handled consistently across parsing, updates, duplicate detection, and CLI archiving. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 4 functions across 4 files. (1 skipped: 1 unsupported.)
✨ 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.
Closed ATX requirement headings are normalized consistently without altering legitimate hashes in names. Parser and archive end-to-end tests pass.
Closes #1859.
Why
CommonMark lets an ATX heading end in a closing run of
#s, so### Requirement: Late Fees ###renders exactly like### Requirement: Late Fees. Every requirement-name reader kept the run:REQUIREMENT_HEADER_REGEX(src/core/parsers/requirement-blocks.ts:33)captures it, and
normalizeRequirementNameonly trimmed whitespace. Scenarionames already strip it (
requirement-blocks.ts:439,:454).The visible failure is REMOVED. A closed heading names
"Late Fees ###", whichis not in the spec, so
buildUpdatedSpectakes the early-sync branch(
src/core/specs-apply.ts:454), warns that the requirement is "not in thecurrent spec; treating it as already removed", and archive exits 0 — while
Late Feesstays in the spec. The same heading under MODIFIED or as a RENAMEDsource fails as "not found", and a main spec holding both
### Requirement: Late Feesand### Requirement: Late Fees ###was notreported as a duplicate.
What Changes
normalizeRequirementNamestrips a closing sequence — a run of#precededby a space or tab and followed only by spaces or tabs — then trims. This is the
same rule
scenarioNameAtuses, including[ \t]rather than\s, soC#,Foo##andPriority #1keep their#, and an NBSP-separated run is kept, asCommonMark keeps it.
normalizeRequirementName— main-spec blocks, ADDED/MODIFIED/REMOVED headers, the REMOVED bullet form,
RENAMED FROM/TO, the MODIFIED header check in
buildUpdatedSpec, the validatorand
requirement-diff— they all agree without further changes.findMainSpecStructureIssues(src/core/parsers/spec-structure.ts) keyed itsduplicate check on a bare
trim(); it now usesnormalizeRequirementName, soa closed and an open heading of one requirement are reported as duplicates.
A RENAMED target written with a closing run is written back as the rendered name
(
### Requirement: Overdue Fees). Headings without a closing run are unaffected.Testing
Written first and watched fail, then passing:
test/core/parsers/closed-requirement-heading.test.ts— 20 tests:normalizeRequirementName(4 stripped forms, 4 kept forms), the readers(main-spec section, every delta header form, duplicate detection and its
control), and
buildUpdatedSpec(closed REMOVED heading and bullet, closedmain-spec heading, closed MODIFIED either way, closed RENAMED FROM/TO, closed
ADDED of an existing requirement,
C#control)test/cli-e2e/archive-closed-requirement-heading.test.ts— 2 end-to-end teststhrough the built CLI: plain REMOVED (control) and closed REMOVED, asserting
the requirement is removed and the "treating it as already removed" warning is
gone
Before the fix: 14 failed / 8 passed (the 8 passing are controls and the kept
forms). After: 22 passed.
The round-2 report's own reproduction for this bug (2 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— 4528 passed, 57 failed of 4585. All 57are timing failures on a heavily shared host (load average up to 19 on 6
cores; part of the run also shared the machine with a second test run):
56 × "Test timed out in 10000ms" and 1 ×
spawnSync npm ETIMEDOUT. None isan assertion failure.
VITEST_MAX_WORKERS=2): 298 of 299passed. That includes all of
show-diff.test.ts(18) andartifact-workflow.test.ts(78), the two that read requirement names. Theone left is the
npm pack+tsctest inpackage-install-scripts.test.ts,which timed out at 11.2s, as it also did on
fix-requirement-name-near-miss.npm packtest run alone, twice each onmain, this branch andfix-requirement-name-near-miss: 8 of 8 passed every time.VITEST_MAX_WORKERS=2 pnpm test): all4585 tests in 160 files passed.
Changeset
.changeset/requirement-names-drop-closing-hashes.md(patch).Notes for reviewers
src/core/parsers/change-parser.ts(the reader behindshow --json --deltas-only) has its own RENAMED FROM/TO regex that does not gothrough
normalizeRequirementName; it is left alone here becausefix-show-deltas-readerreworks that reader.fix-requirement-name-near-missedits thefoldRequirementNamedoc comment inthe same file, a few lines below the function changed here. The hunks do not
overlap.
Summary by CodeRabbit
#characters are now parsed correctly.C#, remain unchanged.