From 6b7f77a4ecdd5d2bc94f7ef328ddb6915c6b91e7 Mon Sep 17 00:00:00 2001 From: Clay Good Date: Thu, 23 Jul 2026 08:36:38 -0500 Subject: [PATCH 1/9] fix(archive): keep the delta spec's Purpose in a new main spec 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) --- .changeset/archive-carries-delta-purpose.md | 5 + openspec/specs/cli-archive/spec.md | 12 +++ schemas/spec-driven/schema.yaml | 7 ++ src/core/specs-apply.ts | 36 ++++++- test/core/archive.test.ts | 104 ++++++++++++++++++++ 5 files changed, 160 insertions(+), 4 deletions(-) create mode 100644 .changeset/archive-carries-delta-purpose.md diff --git a/.changeset/archive-carries-delta-purpose.md b/.changeset/archive-carries-delta-purpose.md new file mode 100644 index 0000000000..79136d2061 --- /dev/null +++ b/.changeset/archive-carries-delta-purpose.md @@ -0,0 +1,5 @@ +--- +'@fission-ai/openspec': patch +--- + +`openspec archive` now carries a delta spec's `## Purpose` into the main spec it creates for a brand-new capability, instead of always writing the `TBD - created by archiving change . Update Purpose after archive.` placeholder over it. The placeholder still appears when the delta has no Purpose (or an empty one), and the Purpose of an existing main spec is never touched. diff --git a/openspec/specs/cli-archive/spec.md b/openspec/specs/cli-archive/spec.md index f5f12ccfe4..a42bfb197c 100644 --- a/openspec/specs/cli-archive/spec.md +++ b/openspec/specs/cli-archive/spec.md @@ -90,6 +90,18 @@ Before moving the change to archive, the command SHALL apply delta changes to ma - **THEN** abort with error message showing the conflict - **AND** suggest manual resolution +#### Scenario: New main spec inherits the delta's Purpose + +- **WHEN** a delta creates a main spec that does not exist yet +- **AND** the delta spec has a non-empty `## Purpose` section outside fenced code blocks +- **THEN** write that Purpose into the new main spec + +#### Scenario: New main spec without an authored Purpose + +- **WHEN** a delta creates a main spec that does not exist yet +- **AND** the delta spec has no `## Purpose` section, or an empty one +- **THEN** write the TBD placeholder Purpose naming the change to update after archive + ### Requirement: Confirmation Behavior The spec update confirmation SHALL provide clear visibility into changes before they are applied. diff --git a/schemas/spec-driven/schema.yaml b/schemas/spec-driven/schema.yaml index fd0a2e131f..eaa31471c8 100644 --- a/schemas/spec-driven/schema.yaml +++ b/schemas/spec-driven/schema.yaml @@ -81,6 +81,13 @@ artifacts: - **CRITICAL**: Scenarios MUST use exactly 4 hashtags (`####`). Using 3 hashtags or bullets will fail silently. - Every requirement MUST have at least one scenario. + New capabilities only: start the delta spec with a `## Purpose` section - + one or two sentences describing what the capability is for. Archive copies + it into the main spec it creates; without it the new main spec is left with + a `TBD ... Update Purpose after archive` placeholder to fill in by hand. + Do NOT add `## Purpose` to a delta for an existing capability - that spec + already has one and the delta's is ignored. + MODIFIED requirements workflow: 1. Locate the existing requirement in openspec/specs//spec.md 2. Copy the ENTIRE requirement block (from `### Requirement:` through all scenarios) diff --git a/src/core/specs-apply.ts b/src/core/specs-apply.ts index 7e85fa4314..dbc45437c6 100644 --- a/src/core/specs-apply.ts +++ b/src/core/specs-apply.ts @@ -15,6 +15,7 @@ import { type RequirementBlock, } from './parsers/requirement-blocks.js'; import { findMainSpecStructureIssues } from './parsers/spec-structure.js'; +import { buildCodeFenceMask } from './parsers/code-fence.js'; import { Validator } from './validation/validator.js'; import { discoverSpecFiles } from '../utils/spec-discovery.js'; @@ -221,7 +222,7 @@ export async function buildUpdatedSpec( ); } isNewSpec = true; - targetContent = buildSpecSkeleton(specName, changeName); + targetContent = buildSpecSkeleton(specName, changeName, extractPurposeSection(changeContent)); } const structureIssues = findMainSpecStructureIssues(targetContent); @@ -400,11 +401,38 @@ export async function writeUpdatedSpec( } /** - * Build a skeleton spec for new capabilities. + * Read the body of a `## Purpose` section, ignoring fenced code blocks. + * Returns undefined when the section is absent or empty. */ -export function buildSpecSkeleton(specFolderName: string, changeName: string): string { +function extractPurposeSection(content: string): string | undefined { + const lines = content.replace(/\r\n?/g, '\n').split('\n'); + const mask = buildCodeFenceMask(lines); + const start = lines.findIndex((line, i) => !mask[i] && /^##\s+Purpose\s*$/i.test(line)); + if (start === -1) return undefined; + + let end = lines.length; + for (let i = start + 1; i < lines.length; i++) { + if (!mask[i] && /^##\s+/.test(lines[i])) { + end = i; + break; + } + } + + const body = lines.slice(start + 1, end).join('\n').trim(); + return body || undefined; +} + +/** + * Build a skeleton spec for new capabilities. When the delta spec authored a + * `## Purpose`, carry it over instead of the TBD placeholder (#1413) - archive + * invents the Purpose for a brand-new main spec either way, and the author's + * own wording beats a placeholder they then have to hand-edit. + */ +export function buildSpecSkeleton(specFolderName: string, changeName: string, purpose?: string): string { const titleBase = specFolderName; - return `# ${titleBase} Specification\n\n## Purpose\nTBD - created by archiving change ${changeName}. Update Purpose after archive.\n\n## Requirements\n`; + const purposeBody = + purpose?.trim() || `TBD - created by archiving change ${changeName}. Update Purpose after archive.`; + return `# ${titleBase} Specification\n\n## Purpose\n${purposeBody}\n\n## Requirements\n`; } function findMissingCurrentScenarios(current: RequirementBlock, incoming: RequirementBlock): string[] { diff --git a/test/core/archive.test.ts b/test/core/archive.test.ts index bd6be3dc4d..89a5044112 100644 --- a/test/core/archive.test.ts +++ b/test/core/archive.test.ts @@ -489,6 +489,110 @@ The system SHALL support logo and backgroundColor fields for gift cards. expect(archives.some(a => a.includes(changeName))).toBe(true); }); + it('should carry the delta Purpose into a new main spec (issue #1413)', async () => { + const changeName = 'new-spec-with-purpose'; + const changeSpecDir = path.join(tempDir, 'openspec', 'changes', changeName, 'specs', 'loyalty'); + await fs.mkdir(changeSpecDir, { recursive: true }); + + const specContent = `## Purpose + +Tracks loyalty points earned and redeemed across the storefront. + +## ADDED Requirements + +### Requirement: Earn Points +The system SHALL award loyalty points on each completed order. + +#### Scenario: Order completes +- **WHEN** an order completes +- **THEN** points are credited to the customer +`; + await fs.writeFile(path.join(changeSpecDir, 'spec.md'), specContent); + + await archiveCommand.execute(changeName, { yes: true, noValidate: true }); + + const mainSpecPath = path.join(tempDir, 'openspec', 'specs', 'loyalty', 'spec.md'); + const updatedContent = await fs.readFile(mainSpecPath, 'utf-8'); + expect(updatedContent).toContain('Tracks loyalty points earned and redeemed across the storefront.'); + expect(updatedContent).not.toContain('TBD - created by archiving change'); + expect(updatedContent).toContain('### Requirement: Earn Points'); + }); + + it('should keep the TBD Purpose placeholder when the delta has no Purpose (issue #1413)', async () => { + const changeName = 'new-spec-without-purpose'; + const changeSpecDir = path.join(tempDir, 'openspec', 'changes', changeName, 'specs', 'referrals'); + await fs.mkdir(changeSpecDir, { recursive: true }); + + const specContent = `## ADDED Requirements + +### Requirement: Send Invite +The system SHALL send a referral invite. + +#### Scenario: Invite sent +- **WHEN** a customer refers a friend +- **THEN** an invite email is sent +`; + await fs.writeFile(path.join(changeSpecDir, 'spec.md'), specContent); + + await archiveCommand.execute(changeName, { yes: true, noValidate: true }); + + const mainSpecPath = path.join(tempDir, 'openspec', 'specs', 'referrals', 'spec.md'); + const updatedContent = await fs.readFile(mainSpecPath, 'utf-8'); + expect(updatedContent).toContain( + `TBD - created by archiving change ${changeName}. Update Purpose after archive.` + ); + }); + + it('should not overwrite the Purpose of an existing main spec (issue #1413)', async () => { + const changeName = 'existing-spec-with-purpose'; + const changeSpecDir = path.join(tempDir, 'openspec', 'changes', changeName, 'specs', 'billing'); + await fs.mkdir(changeSpecDir, { recursive: true }); + + const mainSpecDir = path.join(tempDir, 'openspec', 'specs', 'billing'); + await fs.mkdir(mainSpecDir, { recursive: true }); + await fs.writeFile( + path.join(mainSpecDir, 'spec.md'), + `# billing Specification + +## Purpose +The established purpose that must survive archiving. + +## Requirements + +### Requirement: Charge Card +The system SHALL charge the card on file. + +#### Scenario: Card charged +- **WHEN** an invoice is due +- **THEN** the card is charged +` + ); + + await fs.writeFile( + path.join(changeSpecDir, 'spec.md'), + `## Purpose + +A purpose written in the delta that must be ignored for an existing spec. + +## ADDED Requirements + +### Requirement: Refund Card +The system SHALL refund the card on file. + +#### Scenario: Refund issued +- **WHEN** a refund is approved +- **THEN** the card is refunded +` + ); + + await archiveCommand.execute(changeName, { yes: true, noValidate: true }); + + const updatedContent = await fs.readFile(path.join(mainSpecDir, 'spec.md'), 'utf-8'); + expect(updatedContent).toContain('The established purpose that must survive archiving.'); + expect(updatedContent).not.toContain('A purpose written in the delta that must be ignored'); + expect(updatedContent).toContain('### Requirement: Refund Card'); + }); + it('should still error on MODIFIED when creating new spec file', async () => { const changeName = 'new-spec-with-modified'; const changeDir = path.join(tempDir, 'openspec', 'changes', changeName); From 2c398fa74f7cbb71084f2659fd37f32581423453 Mon Sep 17 00:00:00 2001 From: Clay Good Date: Thu, 23 Jul 2026 08:42:37 -0500 Subject: [PATCH 2/9] test(archive): create the temp dir with fs.mkdtemp 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) --- test/core/archive.test.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/test/core/archive.test.ts b/test/core/archive.test.ts index 89a5044112..338d134c08 100644 --- a/test/core/archive.test.ts +++ b/test/core/archive.test.ts @@ -23,8 +23,7 @@ describe('ArchiveCommand', () => { beforeEach(async () => { // Create temp directory - tempDir = path.join(os.tmpdir(), `openspec-archive-test-${Date.now()}`); - await fs.mkdir(tempDir, { recursive: true }); + tempDir = await fs.mkdtemp(path.join(os.tmpdir(), 'openspec-archive-test-')); // Change to temp directory process.chdir(tempDir); From 2ccbf32a8483cd8dafe5e4147aef4faed7bbf073 Mon Sep 17 00:00:00 2001 From: Clay Good Date: Thu, 23 Jul 2026 08:52:01 -0500 Subject: [PATCH 3/9] test(archive): pin fenced-Purpose behavior and align the spec wording 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) --- .changeset/archive-carries-delta-purpose.md | 2 +- openspec/specs/cli-archive/spec.md | 7 +- test/core/archive.test.ts | 93 +++++++++++++++++++++ 3 files changed, 98 insertions(+), 4 deletions(-) diff --git a/.changeset/archive-carries-delta-purpose.md b/.changeset/archive-carries-delta-purpose.md index 79136d2061..c8497a1308 100644 --- a/.changeset/archive-carries-delta-purpose.md +++ b/.changeset/archive-carries-delta-purpose.md @@ -2,4 +2,4 @@ '@fission-ai/openspec': patch --- -`openspec archive` now carries a delta spec's `## Purpose` into the main spec it creates for a brand-new capability, instead of always writing the `TBD - created by archiving change . Update Purpose after archive.` placeholder over it. The placeholder still appears when the delta has no Purpose (or an empty one), and the Purpose of an existing main spec is never touched. +`openspec archive` now carries a delta spec's `## Purpose` into the main spec it creates for a brand-new capability, instead of always writing the `TBD - created by archiving change . Update Purpose after archive.` placeholder over it. The section body is copied verbatim, fenced code blocks included. The placeholder still appears when the delta has no `## Purpose` header outside a code fence, or when the section body is empty, and the Purpose of an existing main spec is never touched. diff --git a/openspec/specs/cli-archive/spec.md b/openspec/specs/cli-archive/spec.md index a42bfb197c..8af2f7f196 100644 --- a/openspec/specs/cli-archive/spec.md +++ b/openspec/specs/cli-archive/spec.md @@ -93,13 +93,14 @@ Before moving the change to archive, the command SHALL apply delta changes to ma #### Scenario: New main spec inherits the delta's Purpose - **WHEN** a delta creates a main spec that does not exist yet -- **AND** the delta spec has a non-empty `## Purpose` section outside fenced code blocks -- **THEN** write that Purpose into the new main spec +- **AND** the delta spec has a `## Purpose` header that is not itself inside a fenced code block +- **AND** that section's body is not empty +- **THEN** write the section body into the new main spec verbatim, fenced code blocks included #### Scenario: New main spec without an authored Purpose - **WHEN** a delta creates a main spec that does not exist yet -- **AND** the delta spec has no `## Purpose` section, or an empty one +- **AND** the delta spec has no `## Purpose` header outside a fenced code block, or the section body is empty - **THEN** write the TBD placeholder Purpose naming the change to update after archive ### Requirement: Confirmation Behavior diff --git a/test/core/archive.test.ts b/test/core/archive.test.ts index 338d134c08..60e6463fb4 100644 --- a/test/core/archive.test.ts +++ b/test/core/archive.test.ts @@ -517,6 +517,41 @@ The system SHALL award loyalty points on each completed order. expect(updatedContent).toContain('### Requirement: Earn Points'); }); + it('should keep fenced code inside a real delta Purpose (issue #1413)', async () => { + const changeName = 'new-spec-with-fenced-purpose'; + const changeSpecDir = path.join(tempDir, 'openspec', 'changes', changeName, 'specs', 'config-format'); + await fs.mkdir(changeSpecDir, { recursive: true }); + + const specContent = `## Purpose + +Normalizes config files. The canonical shape is: + +\`\`\`yaml +retries: 3 +\`\`\` + +## ADDED Requirements + +### Requirement: Normalize Config +The system SHALL normalize config files on load. + +#### Scenario: Config normalized +- **WHEN** a config file is loaded +- **THEN** it is normalized to the canonical shape +`; + await fs.writeFile(path.join(changeSpecDir, 'spec.md'), specContent); + + await archiveCommand.execute(changeName, { yes: true, noValidate: true }); + + const mainSpecPath = path.join(tempDir, 'openspec', 'specs', 'config-format', 'spec.md'); + const updatedContent = await fs.readFile(mainSpecPath, 'utf-8'); + expect(updatedContent).toContain('Normalizes config files. The canonical shape is:'); + // The fenced example is part of the authored Purpose - masking fenced + // lines out of the body would silently truncate it. + expect(updatedContent).toContain('retries: 3'); + expect(updatedContent).not.toContain('TBD - created by archiving change'); + }); + it('should keep the TBD Purpose placeholder when the delta has no Purpose (issue #1413)', async () => { const changeName = 'new-spec-without-purpose'; const changeSpecDir = path.join(tempDir, 'openspec', 'changes', changeName, 'specs', 'referrals'); @@ -542,6 +577,64 @@ The system SHALL send a referral invite. ); }); + it('should keep the TBD placeholder when the only Purpose header is inside a code fence (issue #1413)', async () => { + const changeName = 'new-spec-with-fenced-header'; + const changeSpecDir = path.join(tempDir, 'openspec', 'changes', changeName, 'specs', 'payouts'); + await fs.mkdir(changeSpecDir, { recursive: true }); + + const specContent = `## ADDED Requirements + +### Requirement: Send Payout +The system SHALL send a payout. A main spec looks like: + +\`\`\`markdown +## Purpose +Illustration only - not this capability's purpose. +\`\`\` + +#### Scenario: Payout sent +- **WHEN** a payout is due +- **THEN** it is sent +`; + await fs.writeFile(path.join(changeSpecDir, 'spec.md'), specContent); + + await archiveCommand.execute(changeName, { yes: true, noValidate: true }); + + const mainSpecPath = path.join(tempDir, 'openspec', 'specs', 'payouts', 'spec.md'); + const updatedContent = await fs.readFile(mainSpecPath, 'utf-8'); + expect(updatedContent).toContain( + `TBD - created by archiving change ${changeName}. Update Purpose after archive.` + ); + expect(updatedContent).not.toContain("Illustration only - not this capability's purpose.\n## Requirements"); + }); + + it('should keep the TBD placeholder when the delta Purpose section is empty (issue #1413)', async () => { + const changeName = 'new-spec-with-empty-purpose'; + const changeSpecDir = path.join(tempDir, 'openspec', 'changes', changeName, 'specs', 'notifications'); + await fs.mkdir(changeSpecDir, { recursive: true }); + + const specContent = `## Purpose + +## ADDED Requirements + +### Requirement: Send Notification +The system SHALL send a notification. + +#### Scenario: Notification sent +- **WHEN** an event fires +- **THEN** a notification is sent +`; + await fs.writeFile(path.join(changeSpecDir, 'spec.md'), specContent); + + await archiveCommand.execute(changeName, { yes: true, noValidate: true }); + + const mainSpecPath = path.join(tempDir, 'openspec', 'specs', 'notifications', 'spec.md'); + const updatedContent = await fs.readFile(mainSpecPath, 'utf-8'); + expect(updatedContent).toContain( + `TBD - created by archiving change ${changeName}. Update Purpose after archive.` + ); + }); + it('should not overwrite the Purpose of an existing main spec (issue #1413)', async () => { const changeName = 'existing-spec-with-purpose'; const changeSpecDir = path.join(tempDir, 'openspec', 'changes', changeName, 'specs', 'billing'); From 651b42c6eb6976e9d69b793b5a593c1be505e610 Mon Sep 17 00:00:00 2001 From: Clay Good Date: Thu, 23 Jul 2026 09:07:51 -0500 Subject: [PATCH 4/9] fix(archive): never let a carried Purpose abort the archive 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) --- .changeset/archive-carries-delta-purpose.md | 2 +- openspec/specs/cli-archive/spec.md | 7 ++++ src/core/specs-apply.ts | 13 +++++++ test/core/archive.test.ts | 43 +++++++++++++++++++++ 4 files changed, 64 insertions(+), 1 deletion(-) diff --git a/.changeset/archive-carries-delta-purpose.md b/.changeset/archive-carries-delta-purpose.md index c8497a1308..f752fb5cd3 100644 --- a/.changeset/archive-carries-delta-purpose.md +++ b/.changeset/archive-carries-delta-purpose.md @@ -2,4 +2,4 @@ '@fission-ai/openspec': patch --- -`openspec archive` now carries a delta spec's `## Purpose` into the main spec it creates for a brand-new capability, instead of always writing the `TBD - created by archiving change . Update Purpose after archive.` placeholder over it. The section body is copied verbatim, fenced code blocks included. The placeholder still appears when the delta has no `## Purpose` header outside a code fence, or when the section body is empty, and the Purpose of an existing main spec is never touched. +`openspec archive` now carries a delta spec's `## Purpose` into the main spec it creates for a brand-new capability, instead of always writing the `TBD - created by archiving change . Update Purpose after archive.` placeholder over it. The section body is copied verbatim, fenced code blocks included. The placeholder still appears when the delta has no `## Purpose` header outside a code fence, when the section body is empty, or when carrying the body over would put a requirement header outside `## Requirements` (that last case warns and still completes the archive). The Purpose of an existing main spec is never touched. diff --git a/openspec/specs/cli-archive/spec.md b/openspec/specs/cli-archive/spec.md index 8af2f7f196..30d85e8987 100644 --- a/openspec/specs/cli-archive/spec.md +++ b/openspec/specs/cli-archive/spec.md @@ -103,6 +103,13 @@ Before moving the change to archive, the command SHALL apply delta changes to ma - **AND** the delta spec has no `## Purpose` header outside a fenced code block, or the section body is empty - **THEN** write the TBD placeholder Purpose naming the change to update after archive +#### Scenario: Delta Purpose that would invalidate the new main spec + +- **WHEN** a delta creates a main spec that does not exist yet +- **AND** carrying its `## Purpose` body over would place a requirement header outside the `## Requirements` section +- **THEN** write the TBD placeholder Purpose instead and warn that the delta Purpose was ignored +- **AND** complete the archive rather than aborting it + ### Requirement: Confirmation Behavior The spec update confirmation SHALL provide clear visibility into changes before they are applied. diff --git a/src/core/specs-apply.ts b/src/core/specs-apply.ts index dbc45437c6..9295b9d506 100644 --- a/src/core/specs-apply.ts +++ b/src/core/specs-apply.ts @@ -223,6 +223,19 @@ export async function buildUpdatedSpec( } isNewSpec = true; targetContent = buildSpecSkeleton(specName, changeName, extractPurposeSection(changeContent)); + // A carried Purpose that hides a requirement header would make the new main + // spec structurally invalid and abort an archive that succeeded before + // #1413. Keep the placeholder rather than turning a warning into a failure. + if (findMainSpecStructureIssues(targetContent).length > 0) { + targetContent = buildSpecSkeleton(specName, changeName); + if (!options.silent) { + console.log( + chalk.yellow( + `⚠️ Warning: ${specName} - delta Purpose ignored (it contains a requirement header); wrote the placeholder Purpose instead.` + ) + ); + } + } } const structureIssues = findMainSpecStructureIssues(targetContent); diff --git a/test/core/archive.test.ts b/test/core/archive.test.ts index 60e6463fb4..08a37d8143 100644 --- a/test/core/archive.test.ts +++ b/test/core/archive.test.ts @@ -635,6 +635,49 @@ The system SHALL send a notification. ); }); + it('should fall back to the placeholder when the delta Purpose hides a requirement header (issue #1413)', async () => { + const changeName = 'new-spec-with-stray-header-in-purpose'; + const changeSpecDir = path.join(tempDir, 'openspec', 'changes', changeName, 'specs', 'widgets'); + await fs.mkdir(changeSpecDir, { recursive: true }); + + // A delta an agent can plausibly emit. Carrying this Purpose verbatim + // would put a requirement header outside ## Requirements and abort the + // archive - which succeeded before the Purpose carry-over existed. + const specContent = `## Purpose + +Handles widgets. + +### Requirement: Stray header + +## ADDED Requirements + +### Requirement: Real Requirement +The system SHALL handle widgets. + +#### Scenario: Widget handled +- **WHEN** a widget arrives +- **THEN** it is handled +`; + await fs.writeFile(path.join(changeSpecDir, 'spec.md'), specContent); + + await archiveCommand.execute(changeName, { yes: true, noValidate: true }); + + const mainSpecPath = path.join(tempDir, 'openspec', 'specs', 'widgets', 'spec.md'); + const updatedContent = await fs.readFile(mainSpecPath, 'utf-8'); + expect(updatedContent).toContain( + `TBD - created by archiving change ${changeName}. Update Purpose after archive.` + ); + expect(updatedContent).not.toContain('### Requirement: Stray header'); + expect(updatedContent).toContain('### Requirement: Real Requirement'); + expect(console.log).toHaveBeenCalledWith( + expect.stringContaining(`Warning: widgets - delta Purpose ignored (it contains a requirement header)`) + ); + + // The archive still completed rather than aborting. + const archives = await fs.readdir(path.join(tempDir, 'openspec', 'changes', 'archive')); + expect(archives.some(a => a.includes(changeName))).toBe(true); + }); + it('should not overwrite the Purpose of an existing main spec (issue #1413)', async () => { const changeName = 'existing-spec-with-purpose'; const changeSpecDir = path.join(tempDir, 'openspec', 'changes', changeName, 'specs', 'billing'); From fff5fb2256b5b576d36690817e608785001bf187 Mon Sep 17 00:00:00 2001 From: Clay Good Date: Thu, 23 Jul 2026 09:23:28 -0500 Subject: [PATCH 5/9] fix(archive): make the Purpose carry-over safe and consistent 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) --- .changeset/archive-carries-delta-purpose.md | 6 +- docs/concepts.md | 1 + docs/writing-specs.md | 2 + openspec/specs/cli-archive/spec.md | 24 ++- openspec/specs/openspec-conventions/spec.md | 8 + openspec/specs/specs-sync-skill/spec.md | 2 + schemas/spec-driven/schema.yaml | 19 +- schemas/spec-driven/templates/spec.md | 3 + skills/openspec-sync-specs/SKILL.md | 3 +- src/core/specs-apply.ts | 83 ++++++-- src/core/templates/workflows/sync-specs.ts | 6 +- test/core/archive.test.ts | 186 +++++++++++++++++- .../templates/skill-templates-parity.test.ts | 6 +- 13 files changed, 314 insertions(+), 35 deletions(-) diff --git a/.changeset/archive-carries-delta-purpose.md b/.changeset/archive-carries-delta-purpose.md index f752fb5cd3..04922917c5 100644 --- a/.changeset/archive-carries-delta-purpose.md +++ b/.changeset/archive-carries-delta-purpose.md @@ -1,5 +1,7 @@ --- -'@fission-ai/openspec': patch +"@fission-ai/openspec": patch --- -`openspec archive` now carries a delta spec's `## Purpose` into the main spec it creates for a brand-new capability, instead of always writing the `TBD - created by archiving change . Update Purpose after archive.` placeholder over it. The section body is copied verbatim, fenced code blocks included. The placeholder still appears when the delta has no `## Purpose` header outside a code fence, when the section body is empty, or when carrying the body over would put a requirement header outside `## Requirements` (that last case warns and still completes the archive). The Purpose of an existing main spec is never touched. +A delta spec that introduces a brand-new capability can now open with a `## Purpose`, and `openspec archive` uses it as the Purpose of the main spec it creates instead of always writing the `TBD - created by archiving change . Update Purpose after archive.` placeholder over it. The body is copied trimmed but otherwise verbatim, fenced code blocks included. The `specs` artifact instruction, its example, the delta template and the `openspec-sync-specs` skill all now tell authors and agents to write one, so the CLI and agent-driven sync paths produce the same main spec. + +Archive keeps the placeholder, and says why, when the delta has no `## Purpose` outside a code fence or HTML comment, when the section body is empty, or when carrying the body over would leave a spec the main spec parser cannot read. A carried Purpose shorter than the strict-mode minimum is kept but warned about, since `openspec validate --strict` reports it as too brief. The Purpose of an existing main spec is never touched, and archive now warns instead of dropping a delta's Purpose silently in that case. diff --git a/docs/concepts.md b/docs/concepts.md index cafb78fd0c..caca2bc140 100644 --- a/docs/concepts.md +++ b/docs/concepts.md @@ -393,6 +393,7 @@ The system MUST expire sessions after 15 minutes of inactivity. | `## ADDED Requirements` | New behavior | Appended to main spec | | `## MODIFIED Requirements` | Changed behavior | Replaces existing requirement | | `## REMOVED Requirements` | Deprecated behavior | Deleted from main spec | +| `## Purpose` | What a brand-new capability is for | Seeds the Purpose of the main spec being created; ignored when the spec already exists | ### Why Deltas Instead of Full Specs diff --git a/docs/writing-specs.md b/docs/writing-specs.md index 9e21e6cd80..c894c8f2cb 100644 --- a/docs/writing-specs.md +++ b/docs/writing-specs.md @@ -58,6 +58,8 @@ A change describes its edits to the specs with three section types. Using the ri On archive, ADDED gets appended to the main spec, MODIFIED replaces the old version, and REMOVED is deleted. If you mark a real change as ADDED, you end up with two competing requirements; if you describe new behavior as MODIFIED, there's nothing to replace. When in doubt, open the current spec and see whether the requirement is already there. +One more section is worth knowing about. When your delta creates a capability that doesn't exist yet, open it with `## Purpose` — a sentence or two on what the capability is for. Archive uses it as the Purpose of the main spec it creates; skip it and you get a `TBD` placeholder to fill in by hand. An existing spec already has a Purpose, so a delta's is ignored there — edit `openspec/specs//spec.md` directly to change one. + ## Right-size the change The single most common authoring mistake isn't a badly worded requirement — it's a change that's trying to be three changes. diff --git a/openspec/specs/cli-archive/spec.md b/openspec/specs/cli-archive/spec.md index 30d85e8987..553758c5d1 100644 --- a/openspec/specs/cli-archive/spec.md +++ b/openspec/specs/cli-archive/spec.md @@ -93,23 +93,35 @@ Before moving the change to archive, the command SHALL apply delta changes to ma #### Scenario: New main spec inherits the delta's Purpose - **WHEN** a delta creates a main spec that does not exist yet -- **AND** the delta spec has a `## Purpose` header that is not itself inside a fenced code block -- **AND** that section's body is not empty -- **THEN** write the section body into the new main spec verbatim, fenced code blocks included +- **AND** the delta spec has a line-initial `## Purpose` header that is not inside a fenced code block or an HTML comment +- **AND** the section body, ignoring fenced blocks and HTML comments, is not empty +- **THEN** write the section body into the new main spec, trimmed but otherwise verbatim, fenced code blocks and comments included +- **AND** the section body runs to the next `## ` heading outside a fenced block #### Scenario: New main spec without an authored Purpose - **WHEN** a delta creates a main spec that does not exist yet -- **AND** the delta spec has no `## Purpose` header outside a fenced code block, or the section body is empty +- **AND** the delta spec has no such `## Purpose` header, or that section's body is empty once fenced blocks and HTML comments are ignored - **THEN** write the TBD placeholder Purpose naming the change to update after archive -#### Scenario: Delta Purpose that would invalidate the new main spec +#### Scenario: Delta Purpose that would leave the new main spec unreadable - **WHEN** a delta creates a main spec that does not exist yet -- **AND** carrying its `## Purpose` body over would place a requirement header outside the `## Requirements` section +- **AND** carrying its `## Purpose` body over would leave a spec the main spec parser cannot read - a heading or requirement header that truncates a section, or an unterminated code fence that swallows one - **THEN** write the TBD placeholder Purpose instead and warn that the delta Purpose was ignored - **AND** complete the archive rather than aborting it +#### Scenario: Carried Purpose shorter than the strict-mode minimum + +- **WHEN** a carried Purpose is shorter than the minimum Purpose length strict validation enforces +- **THEN** carry it over unchanged and warn that `openspec validate --strict` reports it as too brief + +#### Scenario: Delta Purpose for a capability that already has a main spec + +- **WHEN** a delta carries a `## Purpose` and the target main spec already exists +- **THEN** leave the existing Purpose untouched +- **AND** warn that the delta Purpose was ignored, naming the main spec to edit directly + ### Requirement: Confirmation Behavior The spec update confirmation SHALL provide clear visibility into changes before they are applied. diff --git a/openspec/specs/openspec-conventions/spec.md b/openspec/specs/openspec-conventions/spec.md index 5a1b8b9619..ee67c4534b 100644 --- a/openspec/specs/openspec-conventions/spec.md +++ b/openspec/specs/openspec-conventions/spec.md @@ -150,10 +150,18 @@ Change proposals SHALL store only the additions, modifications, and removals to The `changes/[name]/specs/` directory SHALL contain: - Delta files showing only what changes - Sections for ADDED, MODIFIED, REMOVED, and RENAMED requirements +- An optional `## Purpose` section on deltas that introduce a new capability - Normalized header matching for requirement identification - Complete requirements using the structured format - Clear indication of change type for each requirement +#### Scenario: Introducing a new capability + +- **WHEN** a delta introduces a capability that has no main spec yet +- **THEN** the delta MAY open with a `## Purpose` section describing the capability +- **AND** that Purpose SHALL seed the main spec created for it +- **AND** a delta for a capability that already has a main spec SHALL NOT carry a `## Purpose`, because the existing Purpose is authoritative + #### Scenario: Using standard output symbols - **WHEN** displaying delta operations in CLI output diff --git a/openspec/specs/specs-sync-skill/spec.md b/openspec/specs/specs-sync-skill/spec.md index 8cc0e081a2..2232637fb0 100644 --- a/openspec/specs/specs-sync-skill/spec.md +++ b/openspec/specs/specs-sync-skill/spec.md @@ -55,6 +55,8 @@ The agent SHALL reconcile main specs with delta specs using the delta operation #### Scenario: New capability spec - **WHEN** delta spec exists for a capability not in main specs - **THEN** create new main spec file at `openspec/specs//spec.md` +- **AND** copy the delta's `## Purpose` body into it when the delta has one, matching what `openspec archive` does +- **AND** write a brief TBD placeholder Purpose only when the delta has none #### Scenario: Merged main spec keeps canonical structure - **WHEN** the agent writes a main spec during sync diff --git a/schemas/spec-driven/schema.yaml b/schemas/spec-driven/schema.yaml index eaa31471c8..3f94206079 100644 --- a/schemas/spec-driven/schema.yaml +++ b/schemas/spec-driven/schema.yaml @@ -82,11 +82,14 @@ artifacts: - Every requirement MUST have at least one scenario. New capabilities only: start the delta spec with a `## Purpose` section - - one or two sentences describing what the capability is for. Archive copies - it into the main spec it creates; without it the new main spec is left with - a `TBD ... Update Purpose after archive` placeholder to fill in by hand. - Do NOT add `## Purpose` to a delta for an existing capability - that spec - already has one and the delta's is ignored. + one or two sentences (50+ characters, or `openspec validate --strict` + reports it as too brief) describing what the capability is for. Archive + copies it into the main spec it creates; without it the new main spec is + left with a `TBD ... Update Purpose after archive` placeholder to fill in + by hand. Do NOT add `## Purpose` to a delta for an existing capability - + that spec already has one and the delta's is ignored. To change an + existing capability's Purpose - including a leftover `TBD` placeholder - + edit `openspec/specs//spec.md` directly. MODIFIED requirements workflow: 1. Locate the existing requirement in openspec/specs//spec.md @@ -97,8 +100,12 @@ artifacts: Common pitfall: Using MODIFIED with partial content loses detail at archive time. If adding new concerns without changing existing behavior, use ADDED instead. - Example: + Example (a new capability, so it opens with `## Purpose`): ``` + ## Purpose + + Lets users take their data out of the product in a portable format. + ## ADDED Requirements ### Requirement: User can export data diff --git a/schemas/spec-driven/templates/spec.md b/schemas/spec-driven/templates/spec.md index 095d711c8f..c12f44d7f5 100644 --- a/schemas/spec-driven/templates/spec.md +++ b/schemas/spec-driven/templates/spec.md @@ -1,3 +1,6 @@ +## Purpose + + ## ADDED Requirements ### Requirement: diff --git a/skills/openspec-sync-specs/SKILL.md b/skills/openspec-sync-specs/SKILL.md index 122a7b6400..b08ed4f80e 100644 --- a/skills/openspec-sync-specs/SKILL.md +++ b/skills/openspec-sync-specs/SKILL.md @@ -78,7 +78,8 @@ This is an **agent-driven** operation - you will read delta specs and directly e d. **Create new main spec** if capability doesn't exist yet: - Create `/openspec/specs//spec.md` - - Add Purpose section (can be brief, mark as TBD) + - Add Purpose section: copy the delta's `## Purpose` body verbatim when it has one + (this is what `openspec archive` does); only write a brief TBD placeholder when it does not - Add Requirements section with the ADDED requirements - Follow the **Main Spec Format Reference** below diff --git a/src/core/specs-apply.ts b/src/core/specs-apply.ts index 9295b9d506..e84ffb28bd 100644 --- a/src/core/specs-apply.ts +++ b/src/core/specs-apply.ts @@ -16,7 +16,9 @@ import { } from './parsers/requirement-blocks.js'; import { findMainSpecStructureIssues } from './parsers/spec-structure.js'; import { buildCodeFenceMask } from './parsers/code-fence.js'; +import { MarkdownParser } from './parsers/markdown-parser.js'; import { Validator } from './validation/validator.js'; +import { MIN_PURPOSE_LENGTH } from './validation/constants.js'; import { discoverSpecFiles } from '../utils/spec-discovery.js'; // ----------------------------------------------------------------------------- @@ -201,10 +203,22 @@ export async function buildUpdatedSpec( } // Load or create base target content + const deltaPurpose = extractPurposeSection(changeContent); let targetContent: string; let isNewSpec = false; try { targetContent = await fs.readFile(update.target, 'utf-8'); + // A delta Purpose only seeds a spec that does not exist yet. Say so rather + // than dropping it silently - the specs instruction tells authors to write + // one for new capabilities, and the delta file looks identical either way. + if (deltaPurpose && !options.silent) { + console.log( + chalk.yellow( + `⚠️ Warning: ${specName} - delta Purpose ignored; ${specName} already has one. ` + + `Edit openspec/specs/${specName}/spec.md directly to change it.` + ) + ); + } } catch { // Target spec does not exist; MODIFIED and RENAMED are not allowed for new specs // REMOVED will be ignored with a warning since there's nothing to remove @@ -222,19 +236,27 @@ export async function buildUpdatedSpec( ); } isNewSpec = true; - targetContent = buildSpecSkeleton(specName, changeName, extractPurposeSection(changeContent)); - // A carried Purpose that hides a requirement header would make the new main - // spec structurally invalid and abort an archive that succeeded before - // #1413. Keep the placeholder rather than turning a warning into a failure. - if (findMainSpecStructureIssues(targetContent).length > 0) { + targetContent = buildSpecSkeleton(specName, changeName, deltaPurpose); + // Keep the placeholder rather than turning this into a failure: these + // deltas archived cleanly before the Purpose carry-over existed. + if (!isSkeletonReadable(targetContent, specName)) { targetContent = buildSpecSkeleton(specName, changeName); if (!options.silent) { console.log( chalk.yellow( - `⚠️ Warning: ${specName} - delta Purpose ignored (it contains a requirement header); wrote the placeholder Purpose instead.` + `⚠️ Warning: ${specName} - delta Purpose ignored (it would leave the new spec unreadable); wrote the placeholder Purpose instead.` ) ); } + } else if (deltaPurpose && deltaPurpose.length < MIN_PURPOSE_LENGTH && !options.silent) { + // The placeholder always cleared this threshold, so a carried Purpose is + // the first way archive can leave a spec that `validate --strict` fails. + console.log( + chalk.yellow( + `⚠️ Warning: ${specName} - carried Purpose is under ${MIN_PURPOSE_LENGTH} characters; ` + + `openspec validate --strict reports it as too brief.` + ) + ); } } @@ -413,28 +435,61 @@ export async function writeUpdatedSpec( if (counts.renamed) console.log(` → ${counts.renamed} renamed`); } +/** Blank out `` spans, preserving line count so indices stay aligned. */ +function maskHtmlComments(content: string): string { + return content.replace(//g, comment => comment.replace(/[^\n]/g, ' ')); +} + /** - * Read the body of a `## Purpose` section, ignoring fenced code blocks. - * Returns undefined when the section is absent or empty. + * Read the body of a `## Purpose` section, ignoring markdown that only appears + * inside fenced code blocks or HTML comments. Returns undefined when the + * section is absent or its body is empty. */ function extractPurposeSection(content: string): string | undefined { - const lines = content.replace(/\r\n?/g, '\n').split('\n'); - const mask = buildCodeFenceMask(lines); - const start = lines.findIndex((line, i) => !mask[i] && /^##\s+Purpose\s*$/i.test(line)); + const normalized = content.replace(/\r\n?/g, '\n'); + const lines = normalized.split('\n'); + // Structure is read from the masked copy so a commented-out or fenced + // `## Purpose` is not mistaken for the real one; the body is returned from + // the original lines so an author's own comments and fences survive intact. + const masked = maskHtmlComments(normalized).split('\n'); + const fenceMask = buildCodeFenceMask(masked); + const isStructural = (i: number) => !fenceMask[i]; + + const start = masked.findIndex((line, i) => isStructural(i) && /^##\s+Purpose\s*$/i.test(line)); if (start === -1) return undefined; - let end = lines.length; - for (let i = start + 1; i < lines.length; i++) { - if (!mask[i] && /^##\s+/.test(lines[i])) { + let end = masked.length; + for (let i = start + 1; i < masked.length; i++) { + if (isStructural(i) && /^##\s+/.test(masked[i])) { end = i; break; } } + // Emptiness is judged on the masked body so a comment-only Purpose (an + // unfilled template placeholder) falls back to the TBD placeholder. + if (!masked.slice(start + 1, end).join('\n').trim()) return undefined; + const body = lines.slice(start + 1, end).join('\n').trim(); return body || undefined; } +/** + * A carried Purpose must leave the new main spec readable by the same parser + * that `validate`, `list` and a later `archive` use. A body containing a + * heading, a stray requirement header, or an unterminated code fence silently + * swallows or truncates the sections around it, so archive would abort or + * write a spec its own validator rejects (#1413). + */ +function isSkeletonReadable(skeleton: string, specName: string): boolean { + if (findMainSpecStructureIssues(skeleton).length > 0) return false; + try { + return new MarkdownParser(skeleton).parseSpec(specName).overview.trim().length > 0; + } catch { + return false; + } +} + /** * Build a skeleton spec for new capabilities. When the delta spec authored a * `## Purpose`, carry it over instead of the TBD placeholder (#1413) - archive diff --git a/src/core/templates/workflows/sync-specs.ts b/src/core/templates/workflows/sync-specs.ts index 9dc3cac26b..714150ac65 100644 --- a/src/core/templates/workflows/sync-specs.ts +++ b/src/core/templates/workflows/sync-specs.ts @@ -80,7 +80,8 @@ ${STORE_SELECTION_GUIDANCE} d. **Create new main spec** if capability doesn't exist yet: - Create \`/openspec/specs//spec.md\` - - Add Purpose section (can be brief, mark as TBD) + - Add Purpose section: copy the delta's \`## Purpose\` body verbatim when it has one + (this is what \`openspec archive\` does); only write a brief TBD placeholder when it does not - Add Requirements section with the ADDED requirements - Follow the **Main Spec Format Reference** below @@ -252,7 +253,8 @@ ${STORE_SELECTION_GUIDANCE} d. **Create new main spec** if capability doesn't exist yet: - Create \`/openspec/specs//spec.md\` - - Add Purpose section (can be brief, mark as TBD) + - Add Purpose section: copy the delta's \`## Purpose\` body verbatim when it has one + (this is what \`openspec archive\` does); only write a brief TBD placeholder when it does not - Add Requirements section with the ADDED requirements - Follow the **Main Spec Format Reference** below diff --git a/test/core/archive.test.ts b/test/core/archive.test.ts index 08a37d8143..c3a84e5df6 100644 --- a/test/core/archive.test.ts +++ b/test/core/archive.test.ts @@ -670,7 +670,7 @@ The system SHALL handle widgets. expect(updatedContent).not.toContain('### Requirement: Stray header'); expect(updatedContent).toContain('### Requirement: Real Requirement'); expect(console.log).toHaveBeenCalledWith( - expect.stringContaining(`Warning: widgets - delta Purpose ignored (it contains a requirement header)`) + expect.stringContaining('Warning: widgets - delta Purpose ignored (it would leave the new spec unreadable)') ); // The archive still completed rather than aborting. @@ -678,6 +678,186 @@ The system SHALL handle widgets. expect(archives.some(a => a.includes(changeName))).toBe(true); }); + it('should fall back to the placeholder when the delta Purpose contains a heading (issue #1413)', async () => { + const changeName = 'new-spec-with-heading-in-purpose'; + const changeSpecDir = path.join(tempDir, 'openspec', 'changes', changeName, 'specs', 'gadgets'); + await fs.mkdir(changeSpecDir, { recursive: true }); + + // An `#` heading truncates the Purpose section when the spec is read back, + // leaving a spec whose own validator rejects it for having no Purpose. + const specContent = `## Purpose + +# Not a spec title +Some body text that is comfortably longer than the strict-mode minimum length. + +## ADDED Requirements + +### Requirement: Handle Gadget +The system SHALL handle gadgets. + +#### Scenario: Gadget handled +- **WHEN** a gadget arrives +- **THEN** it is handled +`; + await fs.writeFile(path.join(changeSpecDir, 'spec.md'), specContent); + + await archiveCommand.execute(changeName, { yes: true, noValidate: true }); + + const updatedContent = await fs.readFile( + path.join(tempDir, 'openspec', 'specs', 'gadgets', 'spec.md'), + 'utf-8' + ); + expect(updatedContent).toContain( + `TBD - created by archiving change ${changeName}. Update Purpose after archive.` + ); + expect(updatedContent).not.toContain('# Not a spec title'); + // The rebuilt spec must still satisfy the validator archive itself runs. + const report = await new Validator().validateSpecContent('gadgets', updatedContent); + expect(report.issues.filter(i => i.level === 'ERROR')).toHaveLength(0); + }); + + it('should fall back to the placeholder when the delta Purpose has an unterminated fence (issue #1413)', async () => { + const changeName = 'new-spec-with-unterminated-fence'; + const changeSpecDir = path.join(tempDir, 'openspec', 'changes', changeName, 'specs', 'mesh-config'); + await fs.mkdir(changeSpecDir, { recursive: true }); + + // The open fence masks everything after it, so the Purpose body would + // swallow the skeleton's own ## Requirements header. + const specContent = `## ADDED Requirements + +### Requirement: Normalize Mesh Config +The system SHALL normalize mesh config. + +#### Scenario: Config normalized +- **WHEN** config is loaded +- **THEN** it is normalized + +## Purpose + +Normalizes configuration for every service in the mesh. Canonical shape: + +\`\`\`yaml +retries: 3 +`; + await fs.writeFile(path.join(changeSpecDir, 'spec.md'), specContent); + + await archiveCommand.execute(changeName, { yes: true, noValidate: true }); + + const updatedContent = await fs.readFile( + path.join(tempDir, 'openspec', 'specs', 'mesh-config', 'spec.md'), + 'utf-8' + ); + expect(updatedContent).toContain( + `TBD - created by archiving change ${changeName}. Update Purpose after archive.` + ); + // Exactly one Requirements section, and the requirement is still visible. + expect(updatedContent.match(/^## Requirements$/gm)).toHaveLength(1); + const report = await new Validator().validateSpecContent('mesh-config', updatedContent); + expect(report.issues.filter(i => i.level === 'ERROR')).toHaveLength(0); + }); + + it('should ignore a commented-out Purpose in favor of the real one (issue #1413)', async () => { + const changeName = 'new-spec-with-commented-purpose'; + const changeSpecDir = path.join(tempDir, 'openspec', 'changes', changeName, 'specs', 'loyalty-v2'); + await fs.mkdir(changeSpecDir, { recursive: true }); + + const specContent = ` + +## Purpose + +Manages the loyalty program end to end across the storefront and admin console. + +## ADDED Requirements + +### Requirement: Earn Points +The system SHALL award loyalty points. + +#### Scenario: Points earned +- **WHEN** an order completes +- **THEN** points are credited +`; + await fs.writeFile(path.join(changeSpecDir, 'spec.md'), specContent); + + await archiveCommand.execute(changeName, { yes: true, noValidate: true }); + + const updatedContent = await fs.readFile( + path.join(tempDir, 'openspec', 'specs', 'loyalty-v2', 'spec.md'), + 'utf-8' + ); + expect(updatedContent).toContain('Manages the loyalty program end to end'); + expect(updatedContent).not.toContain('Draft purpose the author commented out'); + expect(updatedContent).not.toContain('-->'); + }); + + it('should keep the placeholder when the delta Purpose is only an HTML comment (issue #1413)', async () => { + const changeName = 'new-spec-with-unfilled-template'; + const changeSpecDir = path.join(tempDir, 'openspec', 'changes', changeName, 'specs', 'unfilled'); + await fs.mkdir(changeSpecDir, { recursive: true }); + + // This is the shipped delta template left unfilled. + const specContent = `## Purpose + + +## ADDED Requirements + +### Requirement: Do Thing +The system SHALL do the thing. + +#### Scenario: Thing done +- **WHEN** asked +- **THEN** done +`; + await fs.writeFile(path.join(changeSpecDir, 'spec.md'), specContent); + + await archiveCommand.execute(changeName, { yes: true, noValidate: true }); + + const updatedContent = await fs.readFile( + path.join(tempDir, 'openspec', 'specs', 'unfilled', 'spec.md'), + 'utf-8' + ); + expect(updatedContent).toContain( + `TBD - created by archiving change ${changeName}. Update Purpose after archive.` + ); + expect(updatedContent).not.toContain('New capabilities only'); + }); + + it('should warn when a carried Purpose is under the strict-mode minimum (issue #1413)', async () => { + const changeName = 'new-spec-with-brief-purpose'; + const changeSpecDir = path.join(tempDir, 'openspec', 'changes', changeName, 'specs', 'points'); + await fs.mkdir(changeSpecDir, { recursive: true }); + + const specContent = `## Purpose + +Tracks loyalty points. + +## ADDED Requirements + +### Requirement: Track Points +The system SHALL track points. + +#### Scenario: Points tracked +- **WHEN** an order completes +- **THEN** points are tracked +`; + await fs.writeFile(path.join(changeSpecDir, 'spec.md'), specContent); + + await archiveCommand.execute(changeName, { yes: true, noValidate: true }); + + const updatedContent = await fs.readFile( + path.join(tempDir, 'openspec', 'specs', 'points', 'spec.md'), + 'utf-8' + ); + // The author's words are kept - the warning exists so the strict-mode + // failure is not a surprise later. + expect(updatedContent).toContain('Tracks loyalty points.'); + expect(console.log).toHaveBeenCalledWith( + expect.stringContaining('carried Purpose is under 50 characters') + ); + }); + it('should not overwrite the Purpose of an existing main spec (issue #1413)', async () => { const changeName = 'existing-spec-with-purpose'; const changeSpecDir = path.join(tempDir, 'openspec', 'changes', changeName, 'specs', 'billing'); @@ -726,6 +906,10 @@ The system SHALL refund the card on file. expect(updatedContent).toContain('The established purpose that must survive archiving.'); expect(updatedContent).not.toContain('A purpose written in the delta that must be ignored'); expect(updatedContent).toContain('### Requirement: Refund Card'); + // Dropping it silently would be indistinguishable from it having worked. + expect(console.log).toHaveBeenCalledWith( + expect.stringContaining('billing - delta Purpose ignored; billing already has one') + ); }); it('should still error on MODIFIED when creating new spec file', async () => { diff --git a/test/core/templates/skill-templates-parity.test.ts b/test/core/templates/skill-templates-parity.test.ts index 2d41198058..593de285d7 100644 --- a/test/core/templates/skill-templates-parity.test.ts +++ b/test/core/templates/skill-templates-parity.test.ts @@ -42,7 +42,7 @@ const EXPECTED_FUNCTION_HASHES: Record = { getContinueChangeSkillTemplate: '5cc6cf74c055ae67b08373421d934ece65dacbccafbc7452ab5636df3eb9e862', getApplyChangeSkillTemplate: '0f5a15fc7fb9ad6059a5643d0e01365d27642637a4aaebf182f9eabb45348197', getFfChangeSkillTemplate: '097a9ff9533900f227cac0523289eae4e19f06a081e5f355a8374dbecf3ff55d', - getSyncSpecsSkillTemplate: '32c3169e1ee0345a174c0bacb8fd16db73477cc006d8cedbedc6077233c5461b', + getSyncSpecsSkillTemplate: '7479bfc91d28d86d3e791bf5bec905be1dae58dba3c922e8e74f97c92041d0a7', getOnboardSkillTemplate: 'bc2216b72724b01c3a733e63b8bf4aff457f561c0e9ff7288bdacc39780a37a7', getOpsxExploreCommandTemplate: 'eef1f8b4fd90ade6d70be46f0f8c3e6722f221fed175a6f9cf626287ef504a94', getOpsxNewCommandTemplate: '57c600cce318d16b9b4308a18d0d983ea3c0673034e606a7cceec07b4c705e87', @@ -51,7 +51,7 @@ const EXPECTED_FUNCTION_HASHES: Record = { getOpsxFfCommandTemplate: '264b514cc4849f91fb4414f639484c4181f1e5850d0d788ef276c851efa92859', getArchiveChangeSkillTemplate: '206a22b6778e97c30da9145ef51fdad449b8c995538f6fc25752ef551a37b675', getBulkArchiveChangeSkillTemplate: '2b74b1f73380ff32e35f580734780d843c6161a2748c39edb07f1e00453771b4', - getOpsxSyncCommandTemplate: '68dc44c9be2ec1ef719a4ed59830e5a0bc74c3ba6113070650266e1b0d153071', + getOpsxSyncCommandTemplate: '48f2c5171e9b86db1f418723ddfb9215ce7dfb494de34a21ec969eac1fb3de8c', getVerifyChangeSkillTemplate: 'cab4db01b5d2b1243d63d90c53747d8b39e488c60f76eba3fe8b994467f69267', getOpsxArchiveCommandTemplate: '7dea65d0e2e17db366bb666ba6ae5e205ea02707b8c5c7707565200875c78916', getOpsxOnboardCommandTemplate: '9430a0fb6530791ab720e068f4b172bc3dfc4e96a1ae29102bee0b92c2afe7b5', @@ -70,7 +70,7 @@ const EXPECTED_GENERATED_SKILL_CONTENT_HASHES: Record = { 'openspec-continue-change': '02ec4de061ad6277866b877497a1e66142ba364e12b83dd7dedb838579ea88db', 'openspec-apply-change': '09c0e1cdf5ccc82416d0969d6bd715cc70616bdbc3531358a5c36057f78be55a', 'openspec-ff-change': 'ff3bd3eac427a1e50071ad7c70f73b556cffa3db43e90da2726e96849c3fc886', - 'openspec-sync-specs': 'd1bcd420bf8fb55a13f58a2857e6ebde58eb6f9e721a3bf6876bd9f640a63859', + 'openspec-sync-specs': 'e08e40eae3bd55bf825adcda301676b690dcc018c56e3786bd62bbd2bfb81342', 'openspec-archive-change': '64b1611dd7aee04ca268820d1b193e8bf0a39ff3672ec6ba21fb0a1bcb1786c2', 'openspec-bulk-archive-change': '49d410bda408c0411decd584be9c2355335e3b3db760fc6a0adcd82c172a280f', 'openspec-verify-change': '57693d22940f06080c6cf8d590ac2f48240d4a5e9ce7074dacd0f8d3c9945afa', From 7d636e2ea8cfe2c03028f0522cf5ab2fed7b1c2a Mon Sep 17 00:00:00 2001 From: Clay Good Date: Thu, 23 Jul 2026 09:57:37 -0500 Subject: [PATCH 6/9] fix(archive): stop HTML comments in a carried Purpose from corrupting 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) --- .changeset/archive-carries-delta-purpose.md | 10 +- openspec/specs/cli-archive/spec.md | 8 +- openspec/specs/openspec-conventions/spec.md | 2 +- skills/openspec-sync-specs/SKILL.md | 10 + src/core/specs-apply.ts | 67 +++-- src/core/templates/workflows/sync-specs.ts | 20 ++ test/core/archive.test.ts | 233 ++++++++++++++++++ .../templates/skill-templates-parity.test.ts | 6 +- 8 files changed, 324 insertions(+), 32 deletions(-) diff --git a/.changeset/archive-carries-delta-purpose.md b/.changeset/archive-carries-delta-purpose.md index 04922917c5..0bd89ca20a 100644 --- a/.changeset/archive-carries-delta-purpose.md +++ b/.changeset/archive-carries-delta-purpose.md @@ -2,6 +2,12 @@ "@fission-ai/openspec": patch --- -A delta spec that introduces a brand-new capability can now open with a `## Purpose`, and `openspec archive` uses it as the Purpose of the main spec it creates instead of always writing the `TBD - created by archiving change . Update Purpose after archive.` placeholder over it. The body is copied trimmed but otherwise verbatim, fenced code blocks included. The `specs` artifact instruction, its example, the delta template and the `openspec-sync-specs` skill all now tell authors and agents to write one, so the CLI and agent-driven sync paths produce the same main spec. +A delta spec that introduces a brand-new capability can now open with a `## Purpose`, and `openspec archive` uses it as the Purpose of the main spec it creates instead of writing the `TBD - created by archiving change . Update Purpose after archive.` placeholder over it. The `specs` artifact instruction, its example, the delta template and the `openspec-sync-specs` skill all tell authors and agents to write one, so the CLI and agent-driven sync paths produce the same main spec. -Archive keeps the placeholder, and says why, when the delta has no `## Purpose` outside a code fence or HTML comment, when the section body is empty, or when carrying the body over would leave a spec the main spec parser cannot read. A carried Purpose shorter than the strict-mode minimum is kept but warned about, since `openspec validate --strict` reports it as too brief. The Purpose of an existing main spec is never touched, and archive now warns instead of dropping a delta's Purpose silently in that case. +Archive keeps the placeholder when the delta has no usable `## Purpose`: + +- no `## Purpose` header outside a code fence or HTML comment, or a body that is only a code fence or only a comment +- a body that would leave a spec its own parser cannot read — a heading or requirement header that truncates a section, an unterminated fence, or any HTML comment +- in the second case archive also says why, and still completes rather than aborting + +A carried Purpose under 50 characters is kept but warned about, since `openspec validate --strict` reports it as too brief. The Purpose of an existing main spec is never touched; archive warns when it ignores a delta's Purpose there. diff --git a/openspec/specs/cli-archive/spec.md b/openspec/specs/cli-archive/spec.md index 553758c5d1..586075ec24 100644 --- a/openspec/specs/cli-archive/spec.md +++ b/openspec/specs/cli-archive/spec.md @@ -95,7 +95,7 @@ Before moving the change to archive, the command SHALL apply delta changes to ma - **WHEN** a delta creates a main spec that does not exist yet - **AND** the delta spec has a line-initial `## Purpose` header that is not inside a fenced code block or an HTML comment - **AND** the section body, ignoring fenced blocks and HTML comments, is not empty -- **THEN** write the section body into the new main spec, trimmed but otherwise verbatim, fenced code blocks and comments included +- **THEN** write the section body into the new main spec, trimmed but otherwise verbatim, fenced code blocks included - **AND** the section body runs to the next `## ` heading outside a fenced block #### Scenario: New main spec without an authored Purpose @@ -107,20 +107,20 @@ Before moving the change to archive, the command SHALL apply delta changes to ma #### Scenario: Delta Purpose that would leave the new main spec unreadable - **WHEN** a delta creates a main spec that does not exist yet -- **AND** carrying its `## Purpose` body over would leave a spec the main spec parser cannot read - a heading or requirement header that truncates a section, or an unterminated code fence that swallows one +- **AND** carrying its `## Purpose` body over would leave a spec that reads differently to different readers - a heading or requirement header that truncates a section, an unterminated code fence that swallows one, or any HTML comment, which the section scan skips but the file keeps - **THEN** write the TBD placeholder Purpose instead and warn that the delta Purpose was ignored - **AND** complete the archive rather than aborting it #### Scenario: Carried Purpose shorter than the strict-mode minimum -- **WHEN** a carried Purpose is shorter than the minimum Purpose length strict validation enforces +- **WHEN** the Purpose parsed back out of the new main spec is shorter than the minimum Purpose length strict validation enforces - **THEN** carry it over unchanged and warn that `openspec validate --strict` reports it as too brief #### Scenario: Delta Purpose for a capability that already has a main spec - **WHEN** a delta carries a `## Purpose` and the target main spec already exists - **THEN** leave the existing Purpose untouched -- **AND** warn that the delta Purpose was ignored, naming the main spec to edit directly +- **AND** warn that the delta Purpose was ignored, naming the spec file to edit directly, but only when that spec has a Purpose of its own and it differs from the delta's ### Requirement: Confirmation Behavior diff --git a/openspec/specs/openspec-conventions/spec.md b/openspec/specs/openspec-conventions/spec.md index ee67c4534b..b47a98eb3e 100644 --- a/openspec/specs/openspec-conventions/spec.md +++ b/openspec/specs/openspec-conventions/spec.md @@ -160,7 +160,7 @@ The `changes/[name]/specs/` directory SHALL contain: - **WHEN** a delta introduces a capability that has no main spec yet - **THEN** the delta MAY open with a `## Purpose` section describing the capability - **AND** that Purpose SHALL seed the main spec created for it -- **AND** a delta for a capability that already has a main spec SHALL NOT carry a `## Purpose`, because the existing Purpose is authoritative +- **AND** a delta for a capability that already has a main spec SHOULD NOT carry a `## Purpose`, because the existing Purpose is authoritative and the delta's is ignored #### Scenario: Using standard output symbols diff --git a/skills/openspec-sync-specs/SKILL.md b/skills/openspec-sync-specs/SKILL.md index b08ed4f80e..36af5dcd05 100644 --- a/skills/openspec-sync-specs/SKILL.md +++ b/skills/openspec-sync-specs/SKILL.md @@ -76,6 +76,10 @@ This is an **agent-driven** operation - you will read delta specs and directly e **RENAMED Requirements:** - Find the FROM requirement, rename to TO + **`## Purpose` in the delta:** + - The main spec already has one and it is authoritative - leave it alone + (this is what `openspec archive` does; it warns and moves on) + d. **Create new main spec** if capability doesn't exist yet: - Create `/openspec/specs//spec.md` - Add Purpose section: copy the delta's `## Purpose` body verbatim when it has one @@ -88,10 +92,16 @@ This is an **agent-driven** operation - you will read delta specs and directly e After applying all changes, summarize: - Which capabilities were updated - What changes were made (requirements added/modified/removed/renamed) + - Any new main spec left with a TBD Purpose placeholder, so it gets written + now rather than lingering **Delta Spec Format Reference** ```markdown +## Purpose + +Only on a delta that introduces a brand-new capability. Seeds the new main spec. + ## ADDED Requirements ### Requirement: New Feature diff --git a/src/core/specs-apply.ts b/src/core/specs-apply.ts index e84ffb28bd..6ad90b66f2 100644 --- a/src/core/specs-apply.ts +++ b/src/core/specs-apply.ts @@ -211,13 +211,19 @@ export async function buildUpdatedSpec( // A delta Purpose only seeds a spec that does not exist yet. Say so rather // than dropping it silently - the specs instruction tells authors to write // one for new capabilities, and the delta file looks identical either way. + // Only when the spec really does have a different Purpose: claiming it + // "already has one" would be false when it has none, and saying anything at + // all is noise when the two bodies match. if (deltaPurpose && !options.silent) { - console.log( - chalk.yellow( - `⚠️ Warning: ${specName} - delta Purpose ignored; ${specName} already has one. ` + - `Edit openspec/specs/${specName}/spec.md directly to change it.` - ) - ); + const existingPurpose = extractPurposeSection(targetContent); + if (existingPurpose && existingPurpose !== deltaPurpose) { + console.log( + chalk.yellow( + `⚠️ Warning: ${specName} - delta Purpose ignored; ${specName} already has one. ` + + `Edit ${update.target} directly to change it.` + ) + ); + } } } catch { // Target spec does not exist; MODIFIED and RENAMED are not allowed for new specs @@ -237,9 +243,10 @@ export async function buildUpdatedSpec( } isNewSpec = true; targetContent = buildSpecSkeleton(specName, changeName, deltaPurpose); - // Keep the placeholder rather than turning this into a failure: these - // deltas archived cleanly before the Purpose carry-over existed. - if (!isSkeletonReadable(targetContent, specName)) { + const overview = deltaPurpose ? readableOverview(targetContent, specName) : null; + if (deltaPurpose && !overview) { + // Keep the placeholder rather than turning this into a failure: these + // deltas archived cleanly before the Purpose carry-over existed. targetContent = buildSpecSkeleton(specName, changeName); if (!options.silent) { console.log( @@ -248,9 +255,10 @@ export async function buildUpdatedSpec( ) ); } - } else if (deltaPurpose && deltaPurpose.length < MIN_PURPOSE_LENGTH && !options.silent) { + } else if (overview && overview.length < MIN_PURPOSE_LENGTH && !options.silent) { // The placeholder always cleared this threshold, so a carried Purpose is // the first way archive can leave a spec that `validate --strict` fails. + // Measured on the parsed overview, which is what the validator reads. console.log( chalk.yellow( `⚠️ Warning: ${specName} - carried Purpose is under ${MIN_PURPOSE_LENGTH} characters; ` + @@ -466,27 +474,42 @@ function extractPurposeSection(content: string): string | undefined { } } - // Emptiness is judged on the masked body so a comment-only Purpose (an - // unfilled template placeholder) falls back to the TBD placeholder. - if (!masked.slice(start + 1, end).join('\n').trim()) return undefined; + // Emptiness is judged with fenced blocks and HTML comments blanked out, so a + // Purpose that is only a code sample or only an unfilled template comment + // counts as absent and falls back to the TBD placeholder. + const hasProse = masked + .slice(start + 1, end) + .filter((_, offset) => isStructural(start + 1 + offset)) + .join('\n') + .trim(); + if (!hasProse) return undefined; const body = lines.slice(start + 1, end).join('\n').trim(); return body || undefined; } /** - * A carried Purpose must leave the new main spec readable by the same parser - * that `validate`, `list` and a later `archive` use. A body containing a - * heading, a stray requirement header, or an unterminated code fence silently - * swallows or truncates the sections around it, so archive would abort or - * write a spec its own validator rejects (#1413). + * The Purpose a new main spec would end up with, or null when carrying the + * delta's body over would leave a spec the readers downstream cannot handle. + * + * Returns the parsed overview rather than a boolean so callers measure the same + * string `validate` measures, not the raw slice out of the delta. */ -function isSkeletonReadable(skeleton: string, specName: string): boolean { - if (findMainSpecStructureIssues(skeleton).length > 0) return false; +function readableOverview(skeleton: string, specName: string): string | null { + // HTML comments are invisible to the spec parsers but not to the file itself: + // markdown hidden in one is skipped by the boundary scan yet still lands in + // the spec, where it can hide the headers those parsers depend on and blank + // the document out in any markdown renderer. Refuse rather than write a spec + // that reads differently depending on who is reading it (#1413). + if (//.test(skeleton)) return null; + if (findMainSpecStructureIssues(skeleton).length > 0) return null; try { - return new MarkdownParser(skeleton).parseSpec(specName).overview.trim().length > 0; + // A heading or unterminated fence in the body truncates or swallows the + // sections around it, so archive would abort or write a spec its own + // validator rejects. + return new MarkdownParser(skeleton).parseSpec(specName).overview.trim() || null; } catch { - return false; + return null; } } diff --git a/src/core/templates/workflows/sync-specs.ts b/src/core/templates/workflows/sync-specs.ts index 714150ac65..a0844a8b79 100644 --- a/src/core/templates/workflows/sync-specs.ts +++ b/src/core/templates/workflows/sync-specs.ts @@ -78,6 +78,10 @@ ${STORE_SELECTION_GUIDANCE} **RENAMED Requirements:** - Find the FROM requirement, rename to TO + **\`## Purpose\` in the delta:** + - The main spec already has one and it is authoritative - leave it alone + (this is what \`openspec archive\` does; it warns and moves on) + d. **Create new main spec** if capability doesn't exist yet: - Create \`/openspec/specs//spec.md\` - Add Purpose section: copy the delta's \`## Purpose\` body verbatim when it has one @@ -90,10 +94,16 @@ ${STORE_SELECTION_GUIDANCE} After applying all changes, summarize: - Which capabilities were updated - What changes were made (requirements added/modified/removed/renamed) + - Any new main spec left with a TBD Purpose placeholder, so it gets written + now rather than lingering **Delta Spec Format Reference** \`\`\`markdown +## Purpose + +Only on a delta that introduces a brand-new capability. Seeds the new main spec. + ## ADDED Requirements ### Requirement: New Feature @@ -251,6 +261,10 @@ ${STORE_SELECTION_GUIDANCE} **RENAMED Requirements:** - Find the FROM requirement, rename to TO + **\`## Purpose\` in the delta:** + - The main spec already has one and it is authoritative - leave it alone + (this is what \`openspec archive\` does; it warns and moves on) + d. **Create new main spec** if capability doesn't exist yet: - Create \`/openspec/specs//spec.md\` - Add Purpose section: copy the delta's \`## Purpose\` body verbatim when it has one @@ -263,10 +277,16 @@ ${STORE_SELECTION_GUIDANCE} After applying all changes, summarize: - Which capabilities were updated - What changes were made (requirements added/modified/removed/renamed) + - Any new main spec left with a TBD Purpose placeholder, so it gets written + now rather than lingering **Delta Spec Format Reference** \`\`\`markdown +## Purpose + +Only on a delta that introduces a brand-new capability. Seeds the new main spec. + ## ADDED Requirements ### Requirement: New Feature diff --git a/test/core/archive.test.ts b/test/core/archive.test.ts index c3a84e5df6..cc1595d1f8 100644 --- a/test/core/archive.test.ts +++ b/test/core/archive.test.ts @@ -1,6 +1,8 @@ import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; import { ArchiveCommand } from '../../src/core/archive.js'; import { Validator } from '../../src/core/validation/validator.js'; +import { MarkdownParser } from '../../src/core/parsers/markdown-parser.js'; +import { findMainSpecStructureIssues } from '../../src/core/parsers/spec-structure.js'; import { VALIDATION_MESSAGES } from '../../src/core/validation/constants.js'; import { formatLocalDate } from '../../src/utils/date.js'; import { promises as fs } from 'fs'; @@ -711,6 +713,9 @@ The system SHALL handle gadgets. `TBD - created by archiving change ${changeName}. Update Purpose after archive.` ); 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); expect(report.issues.filter(i => i.level === 'ERROR')).toHaveLength(0); @@ -752,6 +757,9 @@ retries: 3 ); // 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); expect(report.issues.filter(i => i.level === 'ERROR')).toHaveLength(0); }); @@ -792,6 +800,161 @@ The system SHALL award loyalty points. expect(updatedContent).not.toContain('-->'); }); + it.each([ + [ + 'a section header hidden in a comment', + 'requirements-hidden-in-comment', + 'hidden-reqs', + `## Purpose +Tracks widgets and keeps their state consistent across restarts. + +Widgets are the core unit of work. +`, + ], + [ + 'a requirement header hidden in a comment', + 'requirement-header-in-comment', + 'hidden-req-header', + `## Purpose +Tracks widgets and keeps their state consistent across restarts. + +`, + ], + [ + 'an unterminated comment', + 'unterminated-comment', + 'dangling-comment', + `## Purpose +Tracks widgets and keeps their state consistent across restarts. + as a comment terminator CodeQL's "Bad HTML filtering regexp" rule: HTML closes a comment on `--!>` as well as `-->`. The guard already refused anything with a `` spans, preserving line count so indices stay aligned. */ function maskHtmlComments(content: string): string { - return content.replace(//g, comment => comment.replace(/[^\n]/g, ' ')); + // `--!>` is a comment terminator as well as `-->`. + return content.replace(//.test(skeleton)) return null; + // `--!>` closes a comment too, so both terminator spellings count. + if (/ terminator', + 'bang-terminated-comment', + 'bang-comment', + `## Purpose +Tracks widgets and keeps their state consistent across restarts. + `, ], ])( From 39add5a7210afab438c02c56aa7aaeddcf7b8fab Mon Sep 17 00:00:00 2001 From: Clay Good Date: Thu, 23 Jul 2026 10:31:40 -0500 Subject: [PATCH 8/9] fix(archive): only an HTML comment opener disqualifies a carried Purpose 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 `` closes a comment too, so both terminator spellings count. - if (/` hides + // nothing and renders as text - rejecting it would throw away a Purpose over + // prose like "ingest --> transform". + if (skeleton.includes('` is not a comment opener; it renders as text and hides nothing, so + // it must not be mistaken for the HTML-comment hazard. + const specContent = `## Purpose + +Routes events through the pipeline: ingest --> transform --> sink, retrying each hop. + +## ADDED Requirements + +### Requirement: Route Events +The system SHALL route events through the pipeline. + +#### Scenario: Event routed +- **WHEN** an event arrives +- **THEN** it is routed +`; + await fs.writeFile(path.join(changeSpecDir, 'spec.md'), specContent); + + await archiveCommand.execute(changeName, { yes: true, noValidate: true }); + + const updatedContent = await fs.readFile( + path.join(tempDir, 'openspec', 'specs', 'pipeline', 'spec.md'), + 'utf-8' + ); + expect(updatedContent).toContain('ingest --> transform --> sink'); + expect(updatedContent).not.toContain('TBD - created by archiving change'); + }); + it('should keep the TBD placeholder when the delta Purpose is only a code fence (issue #1413)', async () => { const changeName = 'new-spec-with-fenced-only-purpose'; const changeSpecDir = path.join(tempDir, 'openspec', 'changes', changeName, 'specs', 'fenced-only'); From ecace715dbc3a39a8e5849f0e3996661229ca875 Mon Sep 17 00:00:00 2001 From: Clay Good Date: Thu, 23 Jul 2026 11:07:56 -0500 Subject: [PATCH 9/9] fix(archive): mask an unterminated HTML comment through end of file alfred caught that maskHtmlComments only matched closed comments, so an unterminated `` spans, preserving line count so indices stay aligned. */ function maskHtmlComments(content: string): string { + const blank = (text: string) => text.replace(/[^\n]/g, ' '); // `--!>` is a comment terminator as well as `-->`. - return content.replace(/` hides + // Only the opener is disqualifying, and only because `maskHtmlComments` + // covers unterminated comments too: a comment starting above the section + // header therefore always masks the header, leaving no body to carry, so a + // body can only hide content behind a `` hides // nothing and renders as text - rejecting it would throw away a Purpose over // prose like "ingest --> transform". if (skeleton.includes(''], + ['unterminated', ''], + ])( + 'should not read a Purpose out of a %s comment that opens above the header (issue #1413)', + async (label, terminator) => { + const changeName = `commented-out-purpose-${label}`; + const changeSpecDir = path.join(tempDir, 'openspec', 'changes', changeName, 'specs', `co-${label}`); + await fs.mkdir(changeSpecDir, { recursive: true }); + + // An unterminated comment runs to end of file, so the header below it is + // commented out just as surely as it is inside a closed comment. + await fs.writeFile( + path.join(changeSpecDir, 'spec.md'), + `