fix(archive): keep the delta spec's Purpose in a new main spec#1431
Conversation
Archiving a change that creates a brand-new capability always overwrote the delta's authored `## Purpose` with the TBD placeholder, so the Purpose had to be re-typed by hand after every archive. buildSpecSkeleton now takes the delta's Purpose when there is one. The placeholder still appears when the delta has no Purpose or an empty one, and an existing main spec's Purpose is never touched. The spec-driven schema now tells agents to open a new capability's delta with a `## Purpose` (and not to add one to a delta for an existing capability), so the default workflow stops producing placeholders. Closes #1413 Closes #369 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
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:
📝 WalkthroughWalkthrough
ChangesArchive Purpose Handling
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant ArchiveCommand
participant buildUpdatedSpec
participant extractPurposeSection
participant buildSpecSkeleton
ArchiveCommand->>buildUpdatedSpec: apply delta content
buildUpdatedSpec->>extractPurposeSection: extract Purpose for missing target
extractPurposeSection-->>buildUpdatedSpec: return Purpose body or empty value
buildUpdatedSpec->>buildSpecSkeleton: create main spec with Purpose override
buildSpecSkeleton-->>ArchiveCommand: return generated main spec content
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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![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 `@src/core/specs-apply.ts`:
- Around line 404-423: Update extractPurposeSection in src/core/specs-apply.ts
(404-423) to filter body lines through mask before trimming, so fenced-only
content returns undefined. In openspec/specs/cli-archive/spec.md (99-103),
document fenced-code-only Purpose as a fallback case; in
test/core/archive.test.ts (521-544), add empty-section and fenced-code-only
cases asserting the TBD placeholder; update
.changeset/archive-carries-delta-purpose.md (5-5) to document this fallback
behavior.
🪄 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: 31c1aeea-8064-4799-9ebe-30f5705de97d
📒 Files selected for processing (5)
.changeset/archive-carries-delta-purpose.mdopenspec/specs/cli-archive/spec.mdschemas/spec-driven/schema.yamlsrc/core/specs-apply.tstest/core/archive.test.ts
Matches the mkdtemp pattern the rest of the suite already uses and clears the CodeQL insecure-temp-file alerts on this file. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
The main carry-over path is sound, but the fenced-only fallback is not implemented yet. At exact head 2c398fa, a real ## Purpose whose body contains only a fenced block is copied verbatim into the new main spec, even though the new contract says only non-fenced Purpose content counts; the 55 focused archive tests still pass because this case is not committed.
Please build the Purpose body from unmasked lines, add empty-section and fenced-only regressions that assert the TBD fallback, and align the changeset/spec wording. This is the same unresolved case CodeRabbit flagged, and a real CLI archive reproduces it.
Review flagged that the spec scenario read as "only non-fenced content counts", which the code does not do. Masking fenced lines out of the Purpose body would truncate a legitimate Purpose that includes an example block, so the code is right and the wording was wrong. - Reword the cli-archive scenarios: the fence check is on the `## Purpose` header, and the section body is copied verbatim. - Add regressions: fenced code inside a real Purpose survives, a Purpose header that only appears inside a fence falls back to TBD, and an empty Purpose section falls back to TBD. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Thanks — you found a real inconsistency, but between the spec wording and the code, not a defect in the code. Pushed Why the suggested fix (mask body lines before trimming) is wrong It would silently truncate a legitimate Purpose that contains an example block. I applied the suggestion locally and archived this delta: ## Purpose
Normalizes config files. The canonical shape is:
```yaml
retries: 3× should keep fenced code inside a real delta Purpose (issue #1413) <- with the suggestion applied |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/core/archive.test.ts (1)
549-551: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the complete fenced-content contract.
These partial-string assertions can pass if fence delimiters are stripped or the illustrative fenced header leaks with different trailing content.
test/core/archive.test.ts#L549-L551: assert the complete fenced YAML block, including```yamland the closing fence.test/core/archive.test.ts#L605-L608: assert that"Illustration only - not this capability's purpose."is absent unconditionally.🤖 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 549 - 551, The archive test at test/core/archive.test.ts lines 549-551 should assert the complete fenced YAML block, including the opening ```yaml delimiter, its content, and the closing fence, rather than only checking retries: 3. At test/core/archive.test.ts lines 605-608, assert unconditionally that “Illustration only - not this capability's purpose.” is absent.
🤖 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 549-551: The archive test at test/core/archive.test.ts lines
549-551 should assert the complete fenced YAML block, including the opening
```yaml delimiter, its content, and the closing fence, rather than only checking
retries: 3. At test/core/archive.test.ts lines 605-608, assert unconditionally
that “Illustration only - not this capability's purpose.” is absent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a8cfeee0-95ca-496e-98b4-cc360d2a8ded
📒 Files selected for processing (3)
.changeset/archive-carries-delta-purpose.mdopenspec/specs/cli-archive/spec.mdtest/core/archive.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- .changeset/archive-carries-delta-purpose.md
- openspec/specs/cli-archive/spec.md
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
Re-reviewed exact head 2ccbf32. The clarified contract is sound: only top-level Purpose headers outside fences are recognized, while the real Purpose body is preserved verbatim so authored code examples are not truncated. The isolated build, all 58 focused archive tests, strict spec validation, Linux/macOS CI, security checks, CodeQL, lint/typecheck, release tracking, and CodeRabbit pass.
Self-review found a regression introduced by the carry-over: a delta whose `## Purpose` body contains a `### Requirement:` header put that header outside `## Requirements` in the new main spec, so the structure guard rejected it and archive exited 1. The same delta archived fine before this branch. Fall back to the placeholder and warn when the carried Purpose would make the new spec structurally invalid, so archive completes as it did before. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Three adversarial reviews of the carry-over found the guard added in 651b42c was too narrow and the guidance half-landed. Addressed: Engine - Replace the two-rule structural guard with a readability check against the parser validate/list/archive actually use. A Purpose body holding a heading or an unterminated code fence used to abort the archive, or write a spec with a duplicated `## Requirements` that its own validator rejects. Both now fall back to the placeholder and warn. - Ignore markdown inside HTML comments when locating the Purpose, so a commented-out draft cannot beat the real section and an unfilled template placeholder counts as empty. - Warn when a carried Purpose is under the strict-mode minimum: the old placeholder always cleared it, so this was the first way archive could leave a spec that `validate --strict` fails. - Warn instead of silently dropping a delta Purpose when the main spec already exists. Guidance, which disagreed with itself and with the agent path - openspec-sync-specs told agents to write TBD, so `/openspec-archive` undid what the CLI now does. It carries the delta Purpose too. - The specs artifact template and the instruction's own example had no `## Purpose` while the prose asked for one. - Document the section in concepts, writing-specs, their website copies, openspec-conventions and specs-sync-skill; state the 50-character threshold and how to change an existing spec's Purpose. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Deploying openspec-docs with
|
| Latest commit: |
04698bc
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://8dbd0bb6.openspec-docs.pages.dev |
| Branch Preview URL: | https://fix-archive-purpose-carryove.openspec-docs.pages.dev |
|
Ran three adversarial reviews over this branch (correctness/hostile input, regression/blast radius, contract/docs). They found four things worth fixing and I've pushed all of them in 1. Blocker: the fix was dead code on the path #369 actually uses
So an agent following the generated skill wrote 2. Two more regressions of the same shape as the one I fixed in
|
| Purpose body contains | Before fff5fb2 |
|---|---|
# Some heading |
truncates the Purpose on read-back → archive exits 1 (main exits 0); with --no-validate, writes a spec validate then rejects |
| unterminated code fence | swallows the skeleton's ## Requirements → duplicated header, every requirement invisible to validate/list/view, change already archived |
Replaced the rule list with a readability check against the same parser validate, list and a later archive use. Both cases now fall back to the placeholder, warn, and complete the archive. The new tests assert the rebuilt spec passes the Validator archive itself runs, so this class can't regress quietly again.
3. The guidance contradicted itself, and the placeholder was hiding a strict-mode failure
- The instruction asked for a
## Purposewhile its ownExample:block and the shippedtemplates/spec.md— which every authoring skill tells the agent to "use as the structure for your output file" — had none. Both now include it. - The placeholder is 72+ characters, so it always cleared the 50-character strict check. A real terse Purpose does not, meaning a team with
openspec validate --strictin CI would have gone red on the first archive of a new capability. Archive now warns at the moment it creates that state, and the guidance states the threshold. - An unfilled template placeholder is an HTML comment, so markdown inside comments is now ignored when locating the Purpose. That also stops a commented-out draft from beating the real section and leaking a bare
-->into the spec.
4. Smaller things
- A delta Purpose aimed at an existing spec was dropped with no signal. Since this PR trains agents to write Purposes, and the delta file looks identical either way, archive now warns and names the file to edit directly.
- Documented the section in
docs/concepts.md(Delta Sections table),docs/writing-specs.md, both website copies,openspec-conventionsandspecs-sync-skill— the delta contract is stated in several places and only some of them knew about it. - Changeset rewritten and switched to the double-quote style its 28 siblings use.
Merge-order note
This adds one conflicting file (openspec/specs/cli-archive/spec.md) to open PR #843, which is already conflicting against main. Worth flagging rather than papering over: #843's PURPOSE_MODIFIED would make a delta Purpose update an existing spec, which is the opposite of the rule this PR writes into the guidance. If #843 lands, this guidance should be revisited rather than merged around. I've corrected the "they do not conflict" line in the PR description.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/core/archive.test.ts (1)
681-757: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the warning, not just the TBD fallback, for both "unreadable Purpose" tests.
Per
openspec/specs/cli-archive/spec.md(lines 107-113), the "would leave the new main spec unreadable" path must both fall back to TBD and warn that the delta Purpose was ignored — the same contract the sibling "widgets" test at line 673 verifies withexpect(console.log).toHaveBeenCalledWith(...). Neither the heading-truncation test (681-717) nor the unterminated-fence test (719-757) checks that a warning was emitted, so a regression that silently drops the warning would pass unnoticed.✅ Suggested additions
expect(updatedContent).not.toContain('# Not a spec title'); + expect(console.log).toHaveBeenCalledWith( + expect.stringContaining('gadgets - delta Purpose ignored') + ); // The rebuilt spec must still satisfy the validator archive itself runs. const report = await new Validator().validateSpecContent('gadgets', updatedContent);// Exactly one Requirements section, and the requirement is still visible. expect(updatedContent.match(/^## Requirements$/gm)).toHaveLength(1); + expect(console.log).toHaveBeenCalledWith( + expect.stringContaining('mesh-config - delta Purpose ignored') + ); const report = await new Validator().validateSpecContent('mesh-config', updatedContent);🤖 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 681 - 757, Add an assertion in both tests for the heading-truncation and unterminated-fence cases to verify console.log was called with the established warning that the delta Purpose was ignored because it would leave the rebuilt main spec unreadable. Match the existing warning assertion from the sibling widgets test while preserving the current TBD fallback and validation checks.
🤖 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 681-757: Add an assertion in both tests for the heading-truncation
and unterminated-fence cases to verify console.log was called with the
established warning that the delta Purpose was ignored because it would leave
the rebuilt main spec unreadable. Match the existing warning assertion from the
sibling widgets test while preserving the current TBD fallback and validation
checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 86ad1ce4-7f29-4474-b293-1a9f035b2f3e
📒 Files selected for processing (13)
.changeset/archive-carries-delta-purpose.mddocs/concepts.mddocs/writing-specs.mdopenspec/specs/cli-archive/spec.mdopenspec/specs/openspec-conventions/spec.mdopenspec/specs/specs-sync-skill/spec.mdschemas/spec-driven/schema.yamlschemas/spec-driven/templates/spec.mdskills/openspec-sync-specs/SKILL.mdsrc/core/specs-apply.tssrc/core/templates/workflows/sync-specs.tstest/core/archive.test.tstest/core/templates/skill-templates-parity.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- schemas/spec-driven/schema.yaml
- .changeset/archive-carries-delta-purpose.md
… the spec Round-two adversarial review found the comment masking added in fff5fb2 was a one-sided defense: it hid markdown from the section scan but handed the raw text to the file, where the spec parsers and markdown renderers have no comment awareness at all. Three ways that broke: - `## Requirements` inside a comment in the Purpose body: the merged requirement landed under the commented-out header, the real section was left empty, and `validate --strict` still passed. - `### Requirement:` inside a comment: archive exited 1 where main exited 0 - the same regression class 651b42c was supposed to have closed. - An unterminated comment: carried verbatim, blanking the whole spec in any markdown renderer while validation stayed green. A carried Purpose containing comment markers is now refused outright, so the spec never reads differently to different readers. This also subsumes the "comment truncates the parsed Purpose" case, where the too-brief warning measured the raw slice and stayed silent while validate failed - the warning now measures the parsed overview, the same string the validator reads. Also from review: - Emptiness now ignores fenced blocks as well as comments, so a Purpose that is only a code sample falls back to the placeholder. This is what CodeRabbit and alfred originally asked for; the earlier reply refuted their mechanism, which truncates a mixed Purpose, but the requirement itself was satisfiable and the shipped spec already claimed it. - The "already has one" warning was false when the target had no Purpose, and noise when the two bodies matched. It now fires only when the spec has a different Purpose of its own, and names the resolved path so it is correct under --store. - sync-specs was silent on the existing-spec case and on `## Purpose` in its delta format reference, and never surfaced a TBD placeholder it wrote. - openspec-conventions said SHALL NOT for a rule nothing enforces and this repo's own deltas break; softened to SHOULD NOT. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Second round of adversarial review (correctness, integration, maintainer). It found that the HTML-comment masking I added in The comment masking was a one-sided defenseIt hid markdown from the section scan but handed the raw text through to the file — and
A carried Purpose containing comment markers is now refused outright, so the spec cannot read differently to different readers. Deleting just that check fails exactly the three new tests. The fenced-only ask — I was wrong to close it outCodeRabbit's original headline was "treat fenced-only Other fixes
Corrected claimsThe maintainer review caught six overstatements, now fixed in the description and changeset:
Not doing, and saying so
Both are listed in the PR description as known gaps. |
CodeQL's "Bad HTML filtering regexp" rule: HTML closes a comment on `--!>` as well as `-->`. The guard already refused anything with a `<!--` in it, so the outcome was safe either way, but the mask now recognizes both spellings and a test pins the case. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The guard rejected any `-->` as well, which threw away a legitimate Purpose over prose like "ingest --> transform --> sink". A bare terminator hides nothing and renders as text; only a `<!--` can conceal markdown. Safety is unchanged: a comment that opens before the section header masks the header itself, so there is no body to carry, and a body can therefore only hide content behind a `<!--` of its own. All three comment hazards still fall back and warn, now pinned alongside an arrow-notation regression. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
At exact head 39add5a, maskHtmlComments() misses an unterminated <!-- that starts before ## Purpose, so the header and body inside that comment are treated as real. I reproduced this with the normal validated CLI archive path: it completed successfully and wrote the commented-out text as the new main spec's Purpose; please mask open comments through EOF and add a regression for an unclosed comment beginning before the Purpose header.
alfred caught that maskHtmlComments only matched closed comments, so an unterminated `<!--` above a `## Purpose` left the commented-out header looking real: archive completed on the normal validated path and wrote the abandoned draft as the new capability's Purpose. An unclosed comment runs to EOF, so everything after it is commented out. Masking it that way restores the invariant readableOverview relies on - a comment opening above the header always masks the header, so a carried body can only hide content behind a `<!--` of its own - and that dependency is now named in the comment rather than left implicit. Regression covers both the closed and unterminated spellings; reverting the EOF masking fails the unterminated one and nothing else. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Good catch, and my reasoning was wrong in a way worth spelling out. Fixed in
The deeper problem was that An unclosed comment runs to end of file, so the mask now blanks from the dangling Regression added for both spellings ( Re-verified end to end after the fix — unterminated comment above the header, unterminated comment inside the body, closed comment above the header, commented-out header followed by a real Purpose, and a plain Purpose — all five now behave correctly and produce strict-valid specs. 2219 tests / 111 files green, |
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
Re-reviewed exact head ecace71. The EOF comment mask closes the reproduced hole: an unterminated <!-- above ## Purpose is now masked through end of file, so normal archive falls back to the placeholder instead of promoting commented-out text. The focused build, 75 archive tests, 12 generation/parity tests, clean skill regeneration, all 36 strict spec validations, the direct validated CLI reproduction, and the full hosted Linux/macOS/Windows, security, CodeQL, lint/typecheck, release-tracking, and CodeRabbit matrix pass.
3 participants
Status
LGTM. 2215 tests / 111 files green,
openspec validate --specs --strict36/36, lint and typecheck clean.What was wrong
Archiving a change that introduced a brand-new capability threw away the
## Purposeyou wrote in the delta and stamped a placeholder over it:## Purpose TBD - created by archiving change my-change. Update Purpose after archive.So every new capability arrived with a to-do in it. Two reports, seven months apart: #1413 (author wrote a Purpose, archive discarded it) and #369 (agents leave the placeholder behind because nothing tells them to fill it).
openspec validate --strictalready accepted a delta carrying a## Purpose, so this was authored content being silently dropped.What it does
A delta that introduces a new capability can open with
## Purpose, and archive uses it as the Purpose of the main spec it creates.buildSpecSkeletontakes the delta's Purpose (specs-apply.ts)specsinstruction, its example, the delta template (schema.yaml, templates/spec.md)openspec-sync-specsskill/openspec-archiveandopenspec archiveagreedocs/concepts.md,docs/writing-specs.md,openspec-conventions,specs-sync-skillArchive keeps the placeholder when the delta has no usable Purpose — no header outside a fence or comment, a body that is only a code sample or only a comment, or a body that would leave a spec its own parser cannot read. In that last case it also warns and still completes rather than aborting. An existing spec's Purpose is never touched.
No new delta operation and no parser change. Archive already invented a Purpose for a new main spec; it now prefers the one you wrote.
Proof it works
Before, on
main— the reproduction from #1413, verbatim:After, same input:
19 Purpose cases in
test/core/archive.test.tscover the carry-over and every fallback. Revertingspecs-apply.tstoorigin/mainfails 12 of them; the rest pin behavior that a plausible-sounding change would break. Each guard is load-bearing — deleting only the HTML-comment check fails exactly the 3 comment tests.The fallback tests assert the rebuilt spec passes the same
Validatorarchive itself runs, so a spec archive writes cannot fail validation it would have passed before.One deliberate trade
Before this PR, "archive succeeded" implied "
validate --specs --strictpasses", because the placeholder is always over the 50-character minimum. A real Purpose can be shorter. Archive now warns at the moment it creates that state rather than falling back — throwing away the author's words to keep a green check would be the wrong trade — but it is a genuine change to a cross-command invariant.Review history
Six rounds of review (two bots, then two rounds of three adversarial reviewers) found four regressions I had introduced and a blocker where the fix never reached the code path the issue was about. Details in the comments below; the short version:
validaterejects — a stray requirement header, a heading, an unterminated fence — all now fall back and warn.## Requirementscaptured the merged requirements while validation stayed green. Comment-bearing Purposes are now refused./openspec-archivenever calls the CLI — it delegates to theopenspec-sync-specsskill, which said "mark as TBD". The fix was dead code on Add "fill purpose after archive" into AGENTS.md #369's actual path until that skill changed.Notes / nits
buildSpecSkeleton's third parameter is optional and the function has no callers outside its module;specs-apply.tsis not re-exported from the package entry.test/core/archive.test.tsbuilds its temp dir withfs.mkdtemp(as 33 other test files do), which clears the CodeQL insecure-temp-file alerts this PR's own new lines raised at6b7f77a.openspec/specs/cli-archive/spec.md) to open PR fix: scenario-level merge for MODIFIED requirements #843, which already conflicts withmain. fix: scenario-level merge for MODIFIED requirements #843'sPURPOSE_MODIFIEDwould make a delta Purpose update an existing spec — the opposite of the rule here. If fix: scenario-level merge for MODIFIED requirements #843 lands, revisit this guidance rather than merging around it.validatedoes not yet warn at review time that a delta Purpose will be ignored for an existing capability (archive warns, but only after merge), and nothing sweeps upTBDplaceholders already sitting in main specs from before this release.Closes #1413
Closes #369
🤖 Generated with Claude Code