fix(specs): warn before archiving deletes a note next to a requirement - #1490
Open
clay-good wants to merge 4 commits into
Open
fix(specs): warn before archiving deletes a note next to a requirement#1490clay-good wants to merge 4 commits into
clay-good wants to merge 4 commits into
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughOpenSpec updates requirement rebuilding to preserve absorbed markdown notes during removal, modification, and rename operations. Boundary detection excludes fenced code and ChangesRequirement recomposition
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant buildUpdatedSpec
participant MarkdownParser
participant RebuiltSpec
buildUpdatedSpec->>MarkdownParser: detect requirement-owned lines and foreign tails
MarkdownParser-->>buildUpdatedSpec: return boundary and tail positions
buildUpdatedSpec->>RebuiltSpec: recompose requirements with positional tails
Possibly related PRs
Suggested reviewers: 🚥 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/core/parsers/requirement-blocks.test.ts (1)
174-213: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the remaining changed parser entry points.
These fixtures only exercise
extractRequirementsSection; add regressions forMarkdownParsersection parsing plus indented requirement/scenario handling throughparseDeltaSpec, so all changed recognizers are protected.As per coding guidelines, run
pnpm exec vitest run test/core/parsers/requirement-blocks.test.tsfor this focused file.🤖 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/parsers/requirement-blocks.test.ts` around lines 174 - 213, Expand the tests in the requirement-blocks suite to cover the changed MarkdownParser section-parsing entry points and indented requirement/scenario handling through parseDeltaSpec, in addition to extractRequirementsSection. Add regression fixtures and assertions for the same indentation and section-boundary behaviors, then run pnpm exec vitest run test/core/parsers/requirement-blocks.test.ts.Source: Coding guidelines
🤖 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 @.changeset/indented-atx-headings.md:
- Line 5: Update the Markdown changeset wording to avoid an inline code span
containing literal leading spaces. Replace that example with “a `###
Requirement: X` line preceded by three spaces,” or show it in a fenced code
block while preserving the explanation of three-space headings and four-space
code blocks.
---
Nitpick comments:
In `@test/core/parsers/requirement-blocks.test.ts`:
- Around line 174-213: Expand the tests in the requirement-blocks suite to cover
the changed MarkdownParser section-parsing entry points and indented
requirement/scenario handling through parseDeltaSpec, in addition to
extractRequirementsSection. Add regression fixtures and assertions for the same
indentation and section-boundary behaviors, then run pnpm exec vitest run
test/core/parsers/requirement-blocks.test.ts.
🪄 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: 059df09a-a66a-4bc7-97dc-8e88ab25d61b
📒 Files selected for processing (4)
.changeset/indented-atx-headings.mdsrc/core/parsers/markdown-parser.tssrc/core/parsers/requirement-blocks.tstest/core/parsers/requirement-blocks.test.ts
| "@fission-ai/openspec": patch | ||
| --- | ||
|
|
||
| Read an indented Markdown heading as a heading. CommonMark lets an ATX heading carry up to three leading spaces, but every heading pattern in the spec parsers was anchored at column 0, so ` ### Requirement: X` was invisible to OpenSpec while a renderer showed it as a requirement. Because a requirement block runs until the next *recognised* header, an indented one was absorbed into the block above it — and archiving a change that removed that block deleted the indented requirement along with it, silently, since nothing counted it and nothing reported it. Four or more leading spaces is still an indented code block, as CommonMark specifies. |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Avoid the MD038-triggering inline code span.
The literal leading spaces inside ` ### Requirement: X` produce the reported markdownlint warning. Reword it as “a ### Requirement: X line preceded by three spaces,” or use a fenced example.
Proposed wording
-Read an indented Markdown heading as a heading. CommonMark lets an ATX heading carry up to three leading spaces, but every heading pattern in the spec parsers was anchored at column 0, so ` ### Requirement: X` was invisible to OpenSpec while a renderer showed it as a requirement.
+Read an indented Markdown heading as a heading. CommonMark lets an ATX heading carry up to three leading spaces, but every heading pattern in the spec parsers was anchored at column 0, so a `### Requirement: X` line preceded by three spaces was invisible to OpenSpec while a renderer showed it as a requirement.📝 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.
Suggested change
| Read an indented Markdown heading as a heading. CommonMark lets an ATX heading carry up to three leading spaces, but every heading pattern in the spec parsers was anchored at column 0, so ` ### Requirement: X` was invisible to OpenSpec while a renderer showed it as a requirement. Because a requirement block runs until the next *recognised* header, an indented one was absorbed into the block above it — and archiving a change that removed that block deleted the indented requirement along with it, silently, since nothing counted it and nothing reported it. Four or more leading spaces is still an indented code block, as CommonMark specifies. | |
| Read an indented Markdown heading as a heading. CommonMark lets an ATX heading carry up to three leading spaces, but every heading pattern in the spec parsers was anchored at column 0, so a `### Requirement: X` line preceded by three spaces was invisible to OpenSpec while a renderer showed it as a requirement. Because a requirement block runs until the next *recognised* header, an indented one was absorbed into the block above it — and archiving a change that removed that block deleted the indented requirement along with it, silently, since nothing counted it and nothing reported it. Four or more leading spaces is still an indented code block, as CommonMark specifies. |
🧰 Tools
🪛 markdownlint-cli2 (0.23.1)
[warning] 5-5: Spaces inside code span elements
(MD038, no-space-in-code)
🤖 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 @.changeset/indented-atx-headings.md at line 5, Update the Markdown changeset
wording to avoid an inline code span containing literal leading spaces. Replace
that example with “a `### Requirement: X` line preceded by three spaces,” or
show it in a fenced code block while preserving the explanation of three-space
headings and four-space code blocks.
Source: Linters/SAST tools
clay-good
added a commit
to clay-good/OpenSpec
that referenced
this pull request
Jul 30, 2026
Replaces the tail-heading veto with a rule that does not read Markdown at all. Six review rounds each found a different way to dress content so a heading scan would miss it: a second `## Requirements` section, a `##` inside an HTML comment ending the section early, a three-space indent, a setext underline. Every fix was another regex approximating a parser, and every round found the next skin. `extractRequirementsSection` has already split the file into the parts this merge understands. So instead of asking "does anything here look like a requirement" - a question a regex and a renderer answer differently - the guard now asks where content ended up: anything non-blank between the `## Requirements` header and the first requirement, or after the section ends, is content the merge carried through without understanding, and a retirement that would delete the file is refused. There is no second opinion to disagree with the first, because there is no second parse. The in-block heading guard stays, and its comment now says why: a `###` heading that is not a requirement header is absorbed into the block above it, so it never reaches the preamble or the tail. Folding that into the rule above needs a parser that ends a block at any `###` heading, which belongs in the parser. This narrows the feature: a spec carrying an authored section beyond Purpose can no longer be retired automatically. That is deliberate. The abort names the lines that stood in the way, and deleting a file whose contents this merge cannot enumerate is exactly the case a person should decide. Depends on Fission-AI#1490 for indented requirement headers, which are swallowed by the block parser before any of this runs. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
alfred-openspec
requested changes
Jul 30, 2026
alfred-openspec
left a comment
alfred-openspec
left a comment
Collaborator
There was a problem hiding this comment.
Two exact-head gaps keep this from closing the silent-loss case: splitTopLevelSections() still requires a column-zero ##, so ## ADDED Requirements produces sectionPresence.added = false and zero parsed requirements, while spec-structure.ts still misses indented delta/requirement headers, allowing the updated main-spec extractor to truncate at ## ADDED Requirements without reporting a structure issue. Please route every structural-heading check through the same 0–3-space CommonMark rule and add regressions for indented delta sections, the main-spec guard, and an indented MODIFIED block reaching specs-apply.ts without its current column-zero header-mismatch failure.
clay-good
changed the title
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/core/parsers/requirement-blocks.ts (1)
248-248: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
ATX_INDENTfor the remaining structural matchers.The shared rule is still duplicated inline here, so a future indentation change can reintroduce parser disagreement.
src/core/parsers/requirement-blocks.ts#L248-L248: replace the literal indentation fragment with a precompiled matcher built fromATX_INDENT.src/core/parsers/requirement-blocks.ts#L378-L387: use one precompiled scenario matcher built fromATX_INDENTfor bothmatchandtest.🤖 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 `@src/core/parsers/requirement-blocks.ts` at line 248, The structural matchers in src/core/parsers/requirement-blocks.ts at lines 248-248 and 378-387 should consistently derive indentation from ATX_INDENT. Update isTopLevelHeader at lines 248-248 to use a precompiled matcher built from ATX_INDENT, and update the scenario matching logic at lines 378-387 to create one precompiled scenario matcher from ATX_INDENT and reuse it for both match and test.
🤖 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 `@src/core/parsers/requirement-blocks.ts`:
- Line 248: The structural matchers in src/core/parsers/requirement-blocks.ts at
lines 248-248 and 378-387 should consistently derive indentation from
ATX_INDENT. Update isTopLevelHeader at lines 248-248 to use a precompiled
matcher built from ATX_INDENT, and update the scenario matching logic at lines
378-387 to create one precompiled scenario matcher from ATX_INDENT and reuse it
for both match and test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: ad855883-564c-4813-93c6-c6a06ff526cf
📒 Files selected for processing (8)
.changeset/indented-atx-headings.mdsrc/core/archive.tssrc/core/parsers/atx.tssrc/core/parsers/requirement-blocks.tssrc/core/parsers/requirement-text.tssrc/core/parsers/spec-structure.tssrc/core/specs-apply.tstest/core/parsers/requirement-blocks.test.ts
alfred-openspec
requested changes
Jul 30, 2026
alfred-openspec
left a comment
alfred-openspec
left a comment
Collaborator
There was a problem hiding this comment.
The three parser gaps are fixed at this head, and the isolated build plus 31 focused tests pass. I also verified an indented MODIFIED section/requirement/scenario applies cleanly with a direct buildUpdatedSpec() repro.
One requested regression is still missing: please commit that indented MODIFIED apply case. The current additions cover delta discovery and the main-spec guard, but no test exercises the new specs-apply.ts header path, so this exact silent-loss boundary can regress independently again.
A requirement block's `raw` runs to the next header the parser RECOGNISES, so a heading it does not - one indented by the 0-3 spaces CommonMark allows, or a plain `### Notes` - is absorbed into the requirement above it. Removing that requirement deleted the absorbed content with it. Silently: nothing counted it, so nothing warned, and the spec left behind still validated. Reproducible on main with no marker and no capability retirement involved. Anything from the first `#`/`##`/`###` heading after a removed block's own header is now kept in place. `####` is excluded deliberately - a requirement's `#### Scenario:` headings are its own and go with it. This replaces an earlier attempt on this branch that widened every heading pattern in both parsers to accept indentation. That was wrong twice over. It reclassified content, so a spec that was valid became invalid - commented-out and indented examples started parsing as real requirements, taking `list` from 1 requirement to 3. And it did not even fix the bug: moving the line out of the block only meant the reconstruction dropped it at a different step, since `rebuilt` is assembled from `before + header + kept blocks + after` and anything skipped is simply gone. So nothing is reclassified now. An indented heading is still not a requirement, exactly as before; it just survives its neighbour's removal, which is all this ever needed to do. The repo's own corpus produces byte-identical `list`, `validate --specs --strict` and `validate --changes --strict` output. Four regressions, each mutation-verified: removing the salvage fails the three absorbed-content cases, and counting `####` as a boundary fails the scenario case. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
clay-good
force-pushed
the
fix/indented-requirement-headers
branch
from
July 30, 2026 16:45
b379fae to
1b3606c
Compare
clay-good
changed the title
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/core/specs-apply.ts (1)
407-431: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winKeep salvaged content at its original position.
Appending
salvagedafter all surviving/new requirements moves a tail such as### Notesfrom betweenDoomedandSurvivorto afterSurvivor, changing document order and potentially section semantics. The test only checks trimmed containment, so it misses this regression.
src/core/specs-apply.ts#L407-L431: add each removed block’s salvaged tail to the ordered output at that block’s original position, rather than collecting it for a final append.test/core/specs-apply.salvage.test.ts#L79-L81: assert the exactforeign.join('\n')sequence occurs before### Requirement: Survivor.🤖 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 `@src/core/specs-apply.ts` around lines 407 - 431, The salvaged tail is currently appended after all requirements instead of remaining at its removed block’s original position. In src/core/specs-apply.ts lines 407-431, update the kept-order construction around salvageForeignTail so each salvaged tail is inserted inline while iterating parts.bodyBlocks, and remove the final salvaged append. In test/core/specs-apply.salvage.test.ts lines 79-81, strengthen the assertion to verify the exact foreign.join('\n') sequence appears before ### Requirement: Survivor.
🤖 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.
Outside diff comments:
In `@src/core/specs-apply.ts`:
- Around line 407-431: The salvaged tail is currently appended after all
requirements instead of remaining at its removed block’s original position. In
src/core/specs-apply.ts lines 407-431, update the kept-order construction around
salvageForeignTail so each salvaged tail is inserted inline while iterating
parts.bodyBlocks, and remove the final salvaged append. In
test/core/specs-apply.salvage.test.ts lines 79-81, strengthen the assertion to
verify the exact foreign.join('\n') sequence appears before ### Requirement:
Survivor.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 5e55f682-3184-480b-b2c1-e8581d6b10eb
📒 Files selected for processing (3)
.changeset/indented-atx-headings.mdsrc/core/specs-apply.tstest/core/specs-apply.salvage.test.ts
A slow audit of the previous commit found the fix covered one of three paths. A requirement block absorbs anything below it that the parser does not read as a new header - a note indented by the 0-3 spaces CommonMark allows, say - so that content rides inside the block. The previous commit salvaged it when the requirement was REMOVED and missed MODIFIED entirely: that path rebuilds the block from the delta, which never carried the note, so it was dropped exactly as before. Verified against the real CLI: main loses it on both paths. RENAMED was the opposite trap. It rewrites the original block's header line in place, so the note is already there - but it also deletes the original key from the block map, which made the requirement look REMOVED to the salvage and produced a duplicate. Tracking which operation applied is therefore not reliable at this point in the merge, so the salvage now asks the assembled result instead: re-insert a note only when nothing else in the rebuilt section already carries it. That is correct for all three paths by construction. Salvaged content also keeps its position now, next to the requirement it was written beside, rather than being appended at the end of the section. Six regressions, three of them mutation-verified against this logic: never re-inserting fails four, always re-inserting duplicates on rename, and appending at the end loses the position. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/core/specs-apply.salvage.test.ts (1)
81-83: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssert the original indentation.
line.trim()allows an implementation to strip the one-to-three leading spaces, thereby reclassifying an indented### Requirement:line while this test still passes. Assertlineunchanged.- expect(rebuilt).toContain(line.trim()); + expect(rebuilt).toContain(line);🤖 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/specs-apply.salvage.test.ts` around lines 81 - 83, Update the assertions in the foreign-line loop to compare rebuilt output against each original line unchanged, replacing the trimmed comparison while preserving the existing rebuilt containment check.
🤖 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 `@src/core/specs-apply.ts`:
- Around line 440-444: Update the pending-tail reinsertion loop in the
specs-apply flow to de-duplicate each tail only against the replacement content
originating from its original block, rather than using orderedBody.some across
all requirements. Preserve reinsertion at the recorded at position when another
requirement or newly added block contains the same tail, and add a regression
covering that case.
---
Outside diff comments:
In `@test/core/specs-apply.salvage.test.ts`:
- Around line 81-83: Update the assertions in the foreign-line loop to compare
rebuilt output against each original line unchanged, replacing the trimmed
comparison while preserving the existing rebuilt containment check.
🪄 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: a7448713-404b-44e8-8355-81e3f644a020
📒 Files selected for processing (3)
.changeset/indented-atx-headings.mdsrc/core/specs-apply.tstest/core/specs-apply.salvage.test.ts
Another audit pass, another defect in my own fix. Deciding whether a note survived by searching the rebuilt section for its text is wrong when two requirements carry the same note: the first copy is found, and the second is dropped. Reproduced - two removed requirements each followed by an identical `### Notes`, one note destroyed. Survival is a question about the block, not about text. An untouched block is the same object the parser produced and still carries its note; a replaced one is a different object and does not. The RENAMED path previously blurred that by copying the whole raw, so it now carries only the requirement's own lines and the salvage puts the note back like every other path. With every replacement uniformly lacking the tail, `replacement !== block` decides it exactly, and no text is compared at all. Four properties, each mutation-verified: matching text instead of identity loses the duplicate note, always re-inserting doubles an untouched block's note, letting RENAMED keep the tail doubles it on rename, and counting `####` as a boundary severs a requirement from its scenarios. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
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 `@src/core/specs-apply.ts`:
- Around line 428-433: The replacement lookup in the recomposition flow loses
renamed requirements’ original ordering, causing the renamed block to move after
Survivor while its tail remains at the old position. Update the
rename/recomposition logic around replacement tracking and the lookup near line
420 to retain the original key for renamed blocks, then extend the rename test
to assert the order Renamed, note, Survivor; validate with pnpm exec vitest run
test/core/specs-apply.salvage.test.ts.
🪄 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: 55bb2d73-17ad-4219-a3e8-824d69c0608c
📒 Files selected for processing (2)
src/core/specs-apply.tstest/core/specs-apply.salvage.test.ts
Comment on lines
+428
to
+433
| // Re-insert the tail unless this block came through untouched, in which | ||
| // case it still carries it. Identity, not text: two requirements can carry | ||
| // the same note, and a containment check would drop the second copy. | ||
| if (foreignTail && replacement !== block) { | ||
| pendingTails.push({ at: orderedBody.length, tail: foreignTail }); | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Keep renamed requirements at their original position.
Line 420 looks up replacements by the original name, but rename removes that key and stores the block under its new name. The renamed block is therefore appended after Survivor, while its salvaged tail is inserted at the old position. Track replacements by original key during rename/recomposition, then extend the rename test to assert Renamed < note < Survivor.
As per coding guidelines, validate with pnpm exec vitest run test/core/specs-apply.salvage.test.ts.
🤖 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 `@src/core/specs-apply.ts` around lines 428 - 433, The replacement lookup in
the recomposition flow loses renamed requirements’ original ordering, causing
the renamed block to move after Survivor while its tail remains at the old
position. Update the rename/recomposition logic around replacement tracking and
the lookup near line 420 to retain the original key for renamed blocks, then
extend the rename test to assert the order Renamed, note, Survivor; validate
with pnpm exec vitest run test/core/specs-apply.salvage.test.ts.
Source: Coding guidelines
An adversarial review found the previous approach was worse than the bug. Salvaging the "foreign tail" out of a requirement block relied on a positional rule: everything after the first heading-shaped line is not the requirement's. That is not true. A `# comment` inside a scenario bullet, or a markdown example, matches the same shape - and on MODIFIED the old text was then spliced back in after the new, so the spec asserted both. The validator called the result valid, and re-applying the same delta grew the file every time. Reproduced end to end. It also turned a working archive into a hard abort: preserving an unindented `### Notes` made the rebuilt spec fail validation as a scenario-less requirement, so changes that archived cleanly on main stopped archiving, with an error that never mentioned the note. Measured before choosing: 3 of 742 requirement blocks in this repo contain a heading-shaped line, and the repro shows those are false positives. Trading a rare silent deletion for silent corruption on the most common operation is a bad trade. So the merge is left exactly as it was - byte-identical output, verified against main - and the loss is reported instead. That fixes the part of the bug that actually hurt: it was silent. A wrong warning costs a line of output; acting on a wrong answer rewrites the spec. Eight tests. Dropping the warning fails three; ignoring the fence mask fails one - the fence case the previous version left unpinned. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
clay-good
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Prerequisite for #1484. Reproducible on
maintoday.The problem
Write a note next to a requirement and archiving can delete it without a word:
A requirement absorbs anything below it that OpenSpec doesn't recognise as a new heading. An indented heading is one of those, so the note becomes part of the requirement — and removing or modifying that requirement takes the note with it.
Nothing warns, because nothing knew the note was there. The spec left behind still validates.
What this changes
openspec archivenow tells you:The merge itself is untouched — the rebuilt spec is byte-identical to what
mainproduces. Only the warning is new.Why it warns instead of rescuing the note
Moving the note automatically was the obvious fix, and it was wrong. A
#line inside a scenario — a comment in an example, a markdown snippet — looks exactly like a note written below the requirement. Nothing in a line-based rule tells them apart.An earlier version of this PR did move it, and on MODIFIED it spliced the superseded text back in after the replacement, so the spec asserted both versions at once. The validator called that valid, and re-running the same change grew the file each time.
In this repo, 3 of 742 requirement blocks contain a heading-shaped line, and all are the false-positive kind. Trading a rare silent deletion for silent corruption on the most common operation isn't worth it. A wrong warning costs one line of output.
Proof
Eight tests covering indented and unindented notes, an absorbed requirement header, a heading inside a fenced example (no warning), an untouched requirement (no warning), and that the rebuilt spec is unchanged. Removing the warning fails three of them; ignoring the fence mask fails another.
Full suite passes (3,467), and this repo's own specs produce byte-identical
list --specs,validate --specs --strict, andvalidate --changes --strictoutput againstmain.