From 5c5176ac39ccf84bea08cd0f66e8ae4bafa9785d Mon Sep 17 00:00:00 2001 From: Clay Good Date: Wed, 19 Aug 2026 10:10:44 -0500 Subject: [PATCH 1/8] fix(archive): never dead-end a capability retirement A change whose delta removes the last requirement a capability has rebuilds the main spec empty, which can never validate. Archive already knows retiring is the fix and names the `retire_capabilities: true` marker that authorises deleting the spec - but only when the marker is the single thing missing. If the spec also holds a line the merge cannot account for (a `## Notes` section, a comment under a requirement - both ordinary), that hint was suppressed, and the hint that names such lines only spoke to authors who had already set the marker. Neither fired, so the archive aborted on "Spec must have at least one requirement" with no guidance at all: the exact dead end the marker exists to close. Archive now names the blocking content in that case. It deliberately does not name the marker there - adding it would not have let this run through, and the marker is only ever named when it really is the one thing missing. Once the content is resolved, the rerun names the marker. Closes #1696 --- .changeset/tidy-moons-smell.md | 5 +++ docs/writing-specs.md | 2 +- src/core/archive.ts | 73 ++++++++++++++++++++++++++-------- test/core/archive.test.ts | 70 ++++++++++++++++++++++++++++++++ 4 files changed, 133 insertions(+), 17 deletions(-) create mode 100644 .changeset/tidy-moons-smell.md diff --git a/.changeset/tidy-moons-smell.md b/.changeset/tidy-moons-smell.md new file mode 100644 index 0000000000..8feec2f206 --- /dev/null +++ b/.changeset/tidy-moons-smell.md @@ -0,0 +1,5 @@ +--- +'@fission-ai/openspec': patch +--- + +archive: tell the author how to retire a capability when the emptied spec also holds content the merge cannot account for. Previously that combination printed only "Spec must have at least one requirement" and no guidance at all. diff --git a/docs/writing-specs.md b/docs/writing-specs.md index a9ff921caf..1e9883a7c5 100644 --- a/docs/writing-specs.md +++ b/docs/writing-specs.md @@ -56,7 +56,7 @@ A change describes its edits to the specs with three section types. Using the ri - **`## MODIFIED Requirements`** — behavior that already existed and is changing. Include the full new version; a short note on what changed helps a reviewer. - **`## REMOVED Requirements`** — behavior going away, with a line on why. -On archive, ADDED gets appended to the main spec, MODIFIED replaces the old version, and REMOVED is dropped from it. Remove the last requirement a capability has and you retire it: rather than leave a spec with nothing in it, archive deletes `openspec/specs//spec.md`. Because that is the one archive step that removes a file, it has to be asked for — add `retire_capabilities: true` to the change's `.openspec.yaml`, alongside the `schema:` that file already needs. Without it the archive aborts and tells you so. For a spec in the caller's checkout, the archive output also names the `git checkout` that restores a committed file; selected stores receive checkout-scoped recovery guidance instead. 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. +On archive, ADDED gets appended to the main spec, MODIFIED replaces the old version, and REMOVED is dropped from it. Remove the last requirement a capability has and you retire it: rather than leave a spec with nothing in it, archive deletes `openspec/specs//spec.md`. Because that is the one archive step that removes a file, it has to be asked for — add `retire_capabilities: true` to the change's `.openspec.yaml`, alongside the `schema:` that file already needs. Without it the archive aborts and tells you so. Retirement deletes the whole file, so it is also refused while the spec holds anything outside its title, `## Purpose`, and its requirement blocks — a `## Notes` section, a comment under a requirement. The abort names those lines; move them into `## Purpose` or a requirement, or delete the spec by hand. For a spec in the caller's checkout, the archive output also names the `git checkout` that restores a committed file; selected stores receive checkout-scoped recovery guidance instead. 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. Here, `` is the directory relative to `specs/`, such as `user-auth` in a flat project or `identity/user-auth` in a project organized by domain. diff --git a/src/core/archive.ts b/src/core/archive.ts index d476c036e5..f2c032ac82 100644 --- a/src/core/archive.ts +++ b/src/core/archive.ts @@ -68,6 +68,32 @@ export async function isRetirableSpec(specName: string, rebuilt: string): Promis ); } +/** + * The sentence that names the marker. Single-sourced because it is now said in + * two places - when the marker is the only thing missing, and when it is + * missing alongside content that would block the retirement anyway (#1696). + */ +function retirementMarkerSentence(specName: string, invalidReason?: string): string { + return ( + `This change removes the last requirement '${specName}' has. To retire the` + + ` capability and delete its spec, add \`retire_capabilities: true\` to the` + + ` change's ${METADATA_FILENAME} (alongside its \`schema:\`, which that file` + + ` requires), then rerun.` + + (invalidReason ? ` The marker present now cannot be honored (${invalidReason}).` : '') + ); +} + +/** + * The first few lines a retirement would delete without being able to name + * them, quoted, with a count for the rest. Capped so a long tail cannot bury + * the rest of the abort. + */ +function describeUnaccountedContent(lines: string[]): string { + const shown = lines.slice(0, 3).map((line) => `"${line}"`).join(', '); + const rest = lines.length > 3 ? `, and ${lines.length - 3} more line(s)` : ''; + return `${shown}${rest}`; +} + /** * What this run should do with a rebuilt spec: write it as usual, retire the * capability because the delta removed its last requirement (#1302), or do @@ -1568,26 +1594,40 @@ export class ArchiveCommand { const specName = p.update.id; const report = await new Validator().validateSpecContent(specName, p.rebuilt); if (!report.valid) { + // This run is what emptied the capability, and "no + // requirements" is the only thing wrong with the spec that + // would be written. Retiring is the fix in every such case; + // what differs is what is still standing in the way. + const emptiedByThisRun = + p.update.exists && + p.counts.removed > 0 && + p.noRequirementBlocks && + (await isRetirableSpec(specName, p.rebuilt)); // The dead end #1302 describes: the rebuilt spec is unwritable // for exactly one reason, and retiring the capability is the // fix - but only the author can authorise deleting the spec, so // the abort names the marker instead of just rejecting. Says so // only when the marker is the ONLY thing missing, so it never // sends someone after a marker that would not have helped. - const retirementWouldFix = - !retirementDeclared && - p.update.exists && - p.counts.removed > 0 && - (await isRetirementCandidate(p.update, p, false)); - const retirementHint = retirementWouldFix - ? `This change removes the last requirement '${specName}' has. To retire the` + - ` capability and delete its spec, add \`retire_capabilities: true\` to the` + - ` change's ${METADATA_FILENAME} (alongside its \`schema:\`, which that file` + - ` requires), then rerun.` + - (retirementMarker.invalidReason - ? ` The marker present now cannot be honored (${retirementMarker.invalidReason}).` - : '') - : undefined; + const retirementHint = + !retirementDeclared && emptiedByThisRun && p.unaccountedContent.length === 0 + ? retirementMarkerSentence(specName, retirementMarker.invalidReason) + : undefined; + // #1696: the marker is missing AND the file holds content a + // retirement cannot account for, so this abort said nothing at + // all - just "must have at least one requirement", with no way + // forward. It names the content instead of the marker, on + // purpose: the marker is only ever named when adding it would + // really let the archive through, and here it would not. Once + // the content is resolved the rerun names the marker. + const blockedRetirementHint = + !retirementDeclared && emptiedByThisRun && p.unaccountedContent.length > 0 + ? `This change removes the last requirement '${specName}' has, so the rebuilt ` + + `spec cannot be written. Retiring the capability is the way through, but the ` + + `spec holds content the merge cannot safely account for and deleting the file ` + + `would take with it: ${describeUnaccountedContent(p.unaccountedContent)}. ` + + 'Move it into `## Purpose` or a canonical requirement, or delete the spec by hand, then rerun.' + : undefined; // The marker was set and retirement was still refused. Saying // nothing left the author who did exactly what the docs asked // back in the original dead end with no signal that their @@ -1600,8 +1640,7 @@ export class ArchiveCommand { (await isRetirableSpec(specName, p.rebuilt)) ? `'${specName}' declares retire_capabilities, but the spec holds content the merge ` + `cannot safely account for and deleting the file would take with it: ` + - `${p.unaccountedContent.slice(0, 3).map((line) => `"${line}"`).join(', ')}` + - `${p.unaccountedContent.length > 3 ? `, and ${p.unaccountedContent.length - 3} more line(s)` : ''}. ` + + `${describeUnaccountedContent(p.unaccountedContent)}. ` + 'Move it into `## Purpose` or a canonical requirement, or delete the spec by hand.' : undefined; if (json) { @@ -1610,6 +1649,7 @@ export class ArchiveCommand { `Rebuilt spec for '${specName}' failed validation. No files were changed.`, refusalReason ?? retirementHint ?? + blockedRetirementHint ?? `Run ${withStoreFlag(root, `openspec validate ${specName}`)} after fixing the change deltas.` ); } @@ -1619,6 +1659,7 @@ export class ArchiveCommand { else if (issue.level === 'WARNING') console.log(chalk.yellow(` ⚠ ${issue.message}`)); } if (retirementHint) console.log(chalk.yellow(` → ${retirementHint}`)); + if (blockedRetirementHint) console.log(chalk.yellow(` → ${blockedRetirementHint}`)); if (refusalReason) console.log(chalk.yellow(` → ${refusalReason}`)); console.log('Aborted. No files were changed.'); process.exitCode = 1; diff --git a/test/core/archive.test.ts b/test/core/archive.test.ts index f64082e6e6..ca735c2cb8 100644 --- a/test/core/archive.test.ts +++ b/test/core/archive.test.ts @@ -3840,6 +3840,76 @@ The system SHALL do the thing differently. expect(JSON.stringify(payload.status)).toContain('retire_capabilities: true'); }); + // #1696: the marker is missing AND the file holds a line the merge cannot + // account for. Both hints were suppressed - the marker hint because + // retirement would still be refused, the refusal reason because it only + // spoke to authors who had already set the marker - so the archive aborted + // on "must have at least one requirement" with no way forward at all. + it('names the content blocking a retirement instead of aborting bare', async () => { + const changeDir = await createChange( + 'retire-unmarked-with-notes', + 'legacy-layer', + REMOVE_ALL, + { declareRetirement: false } + ); + const mainSpecDir = path.join(tempDir, 'openspec', 'specs', 'legacy-layer'); + await fs.mkdir(mainSpecDir, { recursive: true }); + const target = path.join(mainSpecDir, 'spec.md'); + // An ordinary hand-written section. It keeps the spec valid, so the only + // error is still the empty rebuild - but deleting the file would take it. + await fs.writeFile( + target, + `${mainSpec('legacy-layer')}\n## Notes\n\nOwned by the platform team.\n` + ); + const original = await fs.readFile(target, 'utf-8'); + + await archiveCommand.execute('retire-unmarked-with-notes', { yes: true }); + + expect(process.exitCode).toBe(1); + expect(console.log).toHaveBeenCalledWith( + expect.stringContaining(VALIDATION_MESSAGES.SPEC_NO_REQUIREMENTS) + ); + // The abort now says what has to happen: retiring is the way through, + // and here is the line standing in its way. + expect(console.log).toHaveBeenCalledWith( + expect.stringContaining('Retiring the capability is the way through') + ); + expect(console.log).toHaveBeenCalledWith( + expect.stringContaining('"Owned by the platform team."') + ); + // Not the marker, though: adding it would not have let this through, + // and the marker is only ever named when it really is the one thing + // missing. + expect(console.log).not.toHaveBeenCalledWith( + expect.stringContaining('add `retire_capabilities: true`') + ); + // Still a refusal: nothing is written and nothing is deleted. + await expect(fs.readFile(target, 'utf-8')).resolves.toBe(original); + await expect(fs.access(changeDir)).resolves.not.toThrow(); + }); + + it('carries the blocked-retirement guidance into --json', async () => { + await createChange('retire-unmarked-notes-json', 'legacy-layer', REMOVE_ALL, { + declareRetirement: false, + }); + const mainSpecDir = path.join(tempDir, 'openspec', 'specs', 'legacy-layer'); + await fs.mkdir(mainSpecDir, { recursive: true }); + await fs.writeFile( + path.join(mainSpecDir, 'spec.md'), + `${mainSpec('legacy-layer')}\n## Notes\n\nOwned by the platform team.\n` + ); + + await archiveCommand + .execute('retire-unmarked-notes-json', { yes: true, json: true }) + .catch(() => undefined); + + const payload = JSON.parse(lastJsonPayload()); + expect(payload.archive).toBeNull(); + const status = JSON.stringify(payload.status); + expect(status).toContain('Retiring the capability is the way through'); + expect(status).toContain('Owned by the platform team.'); + }); + it('does not name the marker when retirement would not have fixed it', async () => { // A spec broken in some further way is not a retirement candidate, so // pointing at the marker would send the author after the wrong fix. From b72c8effbd87e9ffc86129078cb24deecff56e98 Mon Sep 17 00:00:00 2001 From: Clay Good Date: Wed, 19 Aug 2026 10:28:49 -0500 Subject: [PATCH 2/8] fix(archive): harden the blocked-retirement abort Three follow-ups to the same message. The blocking lines are authored spec content printed verbatim to a terminal, so they now get the treatment `describeChangeName` already gives a change directory name: control characters replaced, since a raw CR could forge a line of its own and an ESC could redraw the screen. Each line is bounded too - one very long line would push the way out of the abort off the reader's screen - and the cut counts code points so it can never leave half a surrogate pair. Both the declared and undeclared branches share the helper, so the marker-declared abort that shipped with #1484 is hardened with it. The wording no longer claims retiring is "the way through". It is not, in the one case this fires on that has a live requirement hiding in a second `## Requirements` section: merging the sections fixes that spec without deleting anything. `openspec/specs/cli-archive/spec.md` records the behavior change - the blocking lines are named whether or not the marker was declared, and the marker is still named only when adding it would let the archive through. --- .changeset/tidy-moons-smell.md | 2 +- openspec/specs/cli-archive/spec.md | 3 ++- src/core/archive.ts | 27 +++++++++++++++++++++---- test/core/archive.test.ts | 32 ++++++++++++++++++++++++++++-- 4 files changed, 56 insertions(+), 8 deletions(-) diff --git a/.changeset/tidy-moons-smell.md b/.changeset/tidy-moons-smell.md index 8feec2f206..edd9b33f10 100644 --- a/.changeset/tidy-moons-smell.md +++ b/.changeset/tidy-moons-smell.md @@ -2,4 +2,4 @@ '@fission-ai/openspec': patch --- -archive: tell the author how to retire a capability when the emptied spec also holds content the merge cannot account for. Previously that combination printed only "Spec must have at least one requirement" and no guidance at all. +archive: tell the author how to retire a capability when the emptied spec also holds content the merge cannot account for. That combination printed only "Spec must have at least one requirement" and no guidance at all; the abort now names the blocking lines, sanitized and length-bounded before they reach the terminal. diff --git a/openspec/specs/cli-archive/spec.md b/openspec/specs/cli-archive/spec.md index 6d13b79721..103f418279 100644 --- a/openspec/specs/cli-archive/spec.md +++ b/openspec/specs/cli-archive/spec.md @@ -196,7 +196,8 @@ A delta whose REMOVED entries cover every requirement a capability has SHALL ret - **WHEN** the spec holds any non-blank line the merge cannot name - anywhere in the file, including above the requirements section and inside a requirement block, where content the parser did not read as a new header rides along - **THEN** refuse the retirement, because deleting the file would take that content with it -- **AND** say which lines stood in the way when the change declared the marker, rather than aborting on the bare validation error +- **AND** say which lines stood in the way whether or not the change declared the marker, rather than aborting on the bare validation error +- **AND** name the marker only when adding it would let the archive through, so an author whose spec still holds such content is pointed at that content first #### Scenario: Main spec is already gone diff --git a/src/core/archive.ts b/src/core/archive.ts index f2c032ac82..b7ea149602 100644 --- a/src/core/archive.ts +++ b/src/core/archive.ts @@ -83,13 +83,31 @@ function retirementMarkerSentence(specName: string, invalidReason?: string): str ); } +/** + * How much of one blocking line the abort is willing to show. A line long + * enough to fill the screen would push the way out of the abort off it. + */ +const UNACCOUNTED_LINE_MAX = 200; + /** * The first few lines a retirement would delete without being able to name * them, quoted, with a count for the rest. Capped so a long tail cannot bury * the rest of the abort. + * + * The lines are authored spec content printed verbatim to a terminal, so they + * get the same treatment as a change directory name (`describeChangeName`): a + * raw CR could forge a line of its own, and an ESC could redraw the screen. + * Truncation counts code points so a cut never leaves half a surrogate pair. */ function describeUnaccountedContent(lines: string[]): string { - const shown = lines.slice(0, 3).map((line) => `"${line}"`).join(', '); + const shown = lines + .slice(0, 3) + .map((line) => { + const safe = [...line.replace(/[\u0000-\u001f\u007f]/g, '?')]; + const clipped = safe.slice(0, UNACCOUNTED_LINE_MAX).join(''); + return `"${safe.length > UNACCOUNTED_LINE_MAX ? `${clipped}\u2026` : clipped}"`; + }) + .join(', '); const rest = lines.length > 3 ? `, and ${lines.length - 3} more line(s)` : ''; return `${shown}${rest}`; } @@ -1623,9 +1641,10 @@ export class ArchiveCommand { const blockedRetirementHint = !retirementDeclared && emptiedByThisRun && p.unaccountedContent.length > 0 ? `This change removes the last requirement '${specName}' has, so the rebuilt ` + - `spec cannot be written. Retiring the capability is the way through, but the ` + - `spec holds content the merge cannot safely account for and deleting the file ` + - `would take with it: ${describeUnaccountedContent(p.unaccountedContent)}. ` + + `spec has none left and cannot be written. Retiring the capability is what ` + + `archive does instead, and it is refused while the spec holds content the ` + + `merge cannot safely account for and deleting the file would take with it: ` + + `${describeUnaccountedContent(p.unaccountedContent)}. ` + 'Move it into `## Purpose` or a canonical requirement, or delete the spec by hand, then rerun.' : undefined; // The marker was set and retirement was still refused. Saying diff --git a/test/core/archive.test.ts b/test/core/archive.test.ts index ca735c2cb8..ef66d74bc3 100644 --- a/test/core/archive.test.ts +++ b/test/core/archive.test.ts @@ -3872,7 +3872,7 @@ The system SHALL do the thing differently. // The abort now says what has to happen: retiring is the way through, // and here is the line standing in its way. expect(console.log).toHaveBeenCalledWith( - expect.stringContaining('Retiring the capability is the way through') + expect.stringContaining('Retiring the capability is what archive does instead') ); expect(console.log).toHaveBeenCalledWith( expect.stringContaining('"Owned by the platform team."') @@ -3888,6 +3888,34 @@ The system SHALL do the thing differently. await expect(fs.access(changeDir)).resolves.not.toThrow(); }); + // The blocking lines are authored file content echoed to a terminal. A + // spec that arrives with a checkout can carry an ESC, and one very long + // line could push the way out of the abort off the screen. + it('renders blocking lines safely and boundedly', async () => { + await createChange('retire-unmarked-hostile', 'legacy-layer', REMOVE_ALL, { + declareRetirement: false, + }); + const mainSpecDir = path.join(tempDir, 'openspec', 'specs', 'legacy-layer'); + await fs.mkdir(mainSpecDir, { recursive: true }); + const longLine = `L${'o'.repeat(400)}ng`; + await fs.writeFile( + path.join(mainSpecDir, 'spec.md'), + `${mainSpec('legacy-layer')}\n## Notes\n\nOwned by \u001b[31mthe platform team.\n\n${longLine}\n` + ); + + await archiveCommand.execute('retire-unmarked-hostile', { yes: true }); + + expect(process.exitCode).toBe(1); + const printed = (console.log as unknown as ReturnType).mock.calls + .map((call) => String(call[0])) + .join('\n'); + // The escape never reaches the terminal, but the line is still findable. + expect(printed).toContain('Owned by ?[31mthe platform team.'); + expect(printed).not.toContain('\u001b[31m'); + // The long line is named, then cut. + expect(printed).toContain(`"L${'o'.repeat(199)}…"`); + }); + it('carries the blocked-retirement guidance into --json', async () => { await createChange('retire-unmarked-notes-json', 'legacy-layer', REMOVE_ALL, { declareRetirement: false, @@ -3906,7 +3934,7 @@ The system SHALL do the thing differently. const payload = JSON.parse(lastJsonPayload()); expect(payload.archive).toBeNull(); const status = JSON.stringify(payload.status); - expect(status).toContain('Retiring the capability is the way through'); + expect(status).toContain('Retiring the capability is what archive does instead'); expect(status).toContain('Owned by the platform team.'); }); From d078330e07ccfdf4bfa394943729977b051c17b3 Mon Sep 17 00:00:00 2001 From: Clay Good Date: Wed, 19 Aug 2026 10:30:46 -0500 Subject: [PATCH 3/8] refactor(archive): drop a helper the revised wording made single-use The marker sentence is said in one place again, so it goes back inline rather than through a function that now has one caller. Also corrects the comment above `emptiedByThisRun`: retiring is not the only fix in every case it covers, which is exactly why the message stopped saying so. --- src/core/archive.ts | 30 ++++++++++++------------------ 1 file changed, 12 insertions(+), 18 deletions(-) diff --git a/src/core/archive.ts b/src/core/archive.ts index b7ea149602..3f3e017299 100644 --- a/src/core/archive.ts +++ b/src/core/archive.ts @@ -68,21 +68,6 @@ export async function isRetirableSpec(specName: string, rebuilt: string): Promis ); } -/** - * The sentence that names the marker. Single-sourced because it is now said in - * two places - when the marker is the only thing missing, and when it is - * missing alongside content that would block the retirement anyway (#1696). - */ -function retirementMarkerSentence(specName: string, invalidReason?: string): string { - return ( - `This change removes the last requirement '${specName}' has. To retire the` + - ` capability and delete its spec, add \`retire_capabilities: true\` to the` + - ` change's ${METADATA_FILENAME} (alongside its \`schema:\`, which that file` + - ` requires), then rerun.` + - (invalidReason ? ` The marker present now cannot be honored (${invalidReason}).` : '') - ); -} - /** * How much of one blocking line the abort is willing to show. A line long * enough to fill the screen would push the way out of the abort off it. @@ -1614,8 +1599,11 @@ export class ArchiveCommand { if (!report.valid) { // This run is what emptied the capability, and "no // requirements" is the only thing wrong with the spec that - // would be written. Retiring is the fix in every such case; - // what differs is what is still standing in the way. + // would be written - so retiring it is what archive would do, + // and what stands in the way of that is worth saying. Not + // always the *only* fix: a live requirement can be hiding in a + // second `## Requirements` section the validator never reaches, + // and merging the sections fixes that spec without a deletion. const emptiedByThisRun = p.update.exists && p.counts.removed > 0 && @@ -1629,7 +1617,13 @@ export class ArchiveCommand { // sends someone after a marker that would not have helped. const retirementHint = !retirementDeclared && emptiedByThisRun && p.unaccountedContent.length === 0 - ? retirementMarkerSentence(specName, retirementMarker.invalidReason) + ? `This change removes the last requirement '${specName}' has. To retire the` + + ` capability and delete its spec, add \`retire_capabilities: true\` to the` + + ` change's ${METADATA_FILENAME} (alongside its \`schema:\`, which that file` + + ` requires), then rerun.` + + (retirementMarker.invalidReason + ? ` The marker present now cannot be honored (${retirementMarker.invalidReason}).` + : '') : undefined; // #1696: the marker is missing AND the file holds content a // retirement cannot account for, so this abort said nothing at From c87f78f22c1604b185a207b47763e51b87cee59f Mon Sep 17 00:00:00 2001 From: Clay Good Date: Wed, 19 Aug 2026 10:41:50 -0500 Subject: [PATCH 4/8] docs(openspec): record the change as a delta, not a direct spec edit Both conventions exist in this repo's history, but the two most recent behavior fixes (#1609, #1616) carry an `openspec/changes/` delta rather than editing the main spec in place, which is also the workflow this project asks of everyone else. The delta reproduces the whole Capability Retirement requirement, so archiving it drops no scenario. Verified by archiving into a scratch copy of `openspec/`: the merged main spec differs from today's by exactly the three added bullets. --- .../.openspec.yaml | 2 + .../proposal.md | 43 ++++++++++++ .../specs/cli-archive/spec.md | 67 +++++++++++++++++++ .../fix-archive-retirement-guidance/tasks.md | 12 ++++ openspec/specs/cli-archive/spec.md | 3 +- 5 files changed, 125 insertions(+), 2 deletions(-) create mode 100644 openspec/changes/fix-archive-retirement-guidance/.openspec.yaml create mode 100644 openspec/changes/fix-archive-retirement-guidance/proposal.md create mode 100644 openspec/changes/fix-archive-retirement-guidance/specs/cli-archive/spec.md create mode 100644 openspec/changes/fix-archive-retirement-guidance/tasks.md diff --git a/openspec/changes/fix-archive-retirement-guidance/.openspec.yaml b/openspec/changes/fix-archive-retirement-guidance/.openspec.yaml new file mode 100644 index 0000000000..41c30bab88 --- /dev/null +++ b/openspec/changes/fix-archive-retirement-guidance/.openspec.yaml @@ -0,0 +1,2 @@ +schema: spec-driven +created: 2026-08-19 diff --git a/openspec/changes/fix-archive-retirement-guidance/proposal.md b/openspec/changes/fix-archive-retirement-guidance/proposal.md new file mode 100644 index 0000000000..9ba56e6f65 --- /dev/null +++ b/openspec/changes/fix-archive-retirement-guidance/proposal.md @@ -0,0 +1,43 @@ +# Never dead-end a capability retirement + +## Why + +A change whose delta removes the last requirement a capability has rebuilds the +main spec empty, and an empty spec can never validate. Retirement is what +archive does instead, and because it deletes a file it has to be asked for: the +change declares `retire_capabilities: true`. The abort names that marker when it +is the single thing missing. + +Retirement is also refused while the spec holds any non-blank line the merge +cannot name — a `## Notes` section, a comment under a requirement. Both are +ordinary things to find in a hand-written spec. When the marker was missing *and* +such a line was present, neither hint fired: the marker hint was suppressed +because adding it would not have let the archive through, and the hint that names +those lines only spoke to authors who had already declared the marker. + +The archive then aborted on a bare "Spec must have at least one requirement" with +no guidance at all — the dead end the marker exists to close, still reachable +(#1696, worked around there with `--skip-specs` plus a hand-applied sync). + +## What Changes + +- When this run emptied the capability, the marker is absent, and the spec holds + content the merge cannot account for, the abort names that content and says + what archive would otherwise do with the spec. +- It still does not name the marker in that case. The marker is named only when + adding it would really let the archive through; a spec with a second + `## Requirements` section holding a live requirement must not be pointed toward + a deletion. Once the content is resolved, the rerun names the marker. +- The blocking lines are authored file content printed to a terminal, so they are + rendered with control characters replaced and their length bounded — the same + treatment a change directory name already gets. This also hardens the + marker-declared refusal, which echoed them verbatim. + +No change to what archive writes, deletes, or refuses. Message paths only. + +## Impact + +- Affected specs: `cli-archive` (MODIFIED: Capability Retirement) +- Affected code: `src/core/archive.ts` +- Affected docs: `docs/writing-specs.md` (states the second refusal condition, + which was true before this change but undocumented) diff --git a/openspec/changes/fix-archive-retirement-guidance/specs/cli-archive/spec.md b/openspec/changes/fix-archive-retirement-guidance/specs/cli-archive/spec.md new file mode 100644 index 0000000000..7d84138929 --- /dev/null +++ b/openspec/changes/fix-archive-retirement-guidance/specs/cli-archive/spec.md @@ -0,0 +1,67 @@ +## MODIFIED Requirements + +### Requirement: Capability Retirement + +A delta whose REMOVED entries cover every requirement a capability has SHALL retire that capability instead of writing a main spec with no requirements, which can never pass validation. + +#### Scenario: Deciding that a rebuilt spec cannot be written + +- **WHEN** applying a delta leaves the rebuilt spec with no requirement blocks, and every other nonblank line in the whole file is accounted for as the title, Purpose, Requirements header, or a canonical requirement's statement, scenarios, or fenced examples +- **THEN** put that rebuilt spec to the spec validator +- **AND** treat it as retirable only when its sole validation error is that the spec has no requirements +- **AND** otherwise write or reject it exactly as any other rebuilt spec, so a spec the validator still accepts, one broken in some further way, and one still holding a `###` heading are all left alone + +#### Scenario: Validation was skipped + +- **WHEN** the archive runs with validation disabled +- **THEN** retire nothing, because no verdict was produced to justify a deletion +- **AND** write the rebuilt spec exactly as an archive without this behavior would + +#### Scenario: Retirement is not declared + +- **WHEN** a rebuilt spec is retirable but the change does not declare `retire_capabilities: true` in its metadata, or declares it in metadata that cannot be honored +- **THEN** write the spec as any other, so the archive aborts on it exactly as it did before this behavior existed +- **AND** name the marker as the fix in that abort, and say when a marker that is present cannot be honored +- **AND** say nothing about the marker when retiring would not have made the spec writable anyway + +#### Scenario: Delta removes the capability's last requirement + +- **WHEN** a retirable rebuilt spec belongs to a capability whose main spec exists +- **AND** at least one requirement was actually removed by this run +- **AND** the change declares `retire_capabilities: true` +- **THEN** delete the capability's `spec.md` instead of writing it +- **AND** refuse to delete when the target resolves outside the real specs root +- **AND** delete any in-root directory the deletion leaves empty, and never the specs root itself +- **AND** count every operation the delta applied in the archive totals +- **AND** record the retirement in the archive warnings, naming what the deleted file held and giving a pasteable Git recovery command only when the spec lived in the caller's checkout + +#### Scenario: Retirement is deferred until every spec is written + +- **WHEN** an archive both retires one capability and updates another +- **THEN** settle the archive destination before touching any spec, so a name collision cannot strand a retirement +- **AND** perform the deletion only after every spec write has succeeded +- **AND** report a destination claimed while the merge ran as the same collision, rather than as a raw filesystem error + +#### Scenario: Capability directory holds other files + +- **WHEN** retiring a capability whose directory still holds other files after `spec.md` is deleted +- **THEN** leave that directory in place + +#### Scenario: Removal was already synced + +- **WHEN** a retirable rebuilt spec removed nothing this run and its main spec exists +- **THEN** leave the file untouched +- **AND** abort the archive with the validation error, as for any other unwritable spec, unless validation was skipped + +#### Scenario: Content the merge cannot account for + +- **WHEN** the spec holds any non-blank line the merge cannot name - anywhere in the file, including above the requirements section and inside a requirement block, where content the parser did not read as a new header rides along +- **THEN** refuse the retirement, because deleting the file would take that content with it +- **AND** say which lines stood in the way whether or not the change declared the marker, rather than aborting on the bare validation error +- **AND** name the marker only when adding it would let the archive through, so an author whose spec still holds such content is pointed at that content first +- **AND** render those lines with control characters replaced and their length bounded, because a spec that redraws the terminal or fills the screen would take the way out of the abort with it + +#### Scenario: Main spec is already gone + +- **WHEN** a REMOVED-only delta targets a capability that has no main spec, and the change declares `retire_capabilities: true` +- **THEN** complete the archive without creating or retiring one diff --git a/openspec/changes/fix-archive-retirement-guidance/tasks.md b/openspec/changes/fix-archive-retirement-guidance/tasks.md new file mode 100644 index 0000000000..2716f411fc --- /dev/null +++ b/openspec/changes/fix-archive-retirement-guidance/tasks.md @@ -0,0 +1,12 @@ +# Tasks + +## 1. Name the blocking content when the marker is absent +- [x] 1.1 Derive "this run emptied the capability" once, and hint on it in both the marker-missing and content-blocked cases +- [x] 1.2 Keep the marker unnamed while content still blocks the retirement + +## 2. Render the blocking lines safely +- [x] 2.1 Replace control characters and bound each line, sharing one helper with the marker-declared refusal +- [x] 2.2 Cover the human abort, the `--json` detail, and the rendering with tests + +## 3. Record the behavior +- [x] 3.1 Update the `cli-archive` spec delta and `docs/writing-specs.md` diff --git a/openspec/specs/cli-archive/spec.md b/openspec/specs/cli-archive/spec.md index 103f418279..6d13b79721 100644 --- a/openspec/specs/cli-archive/spec.md +++ b/openspec/specs/cli-archive/spec.md @@ -196,8 +196,7 @@ A delta whose REMOVED entries cover every requirement a capability has SHALL ret - **WHEN** the spec holds any non-blank line the merge cannot name - anywhere in the file, including above the requirements section and inside a requirement block, where content the parser did not read as a new header rides along - **THEN** refuse the retirement, because deleting the file would take that content with it -- **AND** say which lines stood in the way whether or not the change declared the marker, rather than aborting on the bare validation error -- **AND** name the marker only when adding it would let the archive through, so an author whose spec still holds such content is pointed at that content first +- **AND** say which lines stood in the way when the change declared the marker, rather than aborting on the bare validation error #### Scenario: Main spec is already gone From 563737064a1b6e620a15e1693f511eb4ddd9a54f Mon Sep 17 00:00:00 2001 From: Clay Good Date: Wed, 19 Aug 2026 10:44:32 -0500 Subject: [PATCH 5/8] fix(archive): report an unhonorable marker alongside the blocking content An author who set `retire_capabilities: yes-please` believes they have authorised the deletion. Clearing the blocking content first, only to then learn the marker was never read, is two aborts for one mistake. The abort still never invites the marker to be added while content blocks the retirement - it only reports the one already there. The spec delta records that distinction, which the old bullet ("say nothing about the marker") did not draw. --- .../specs/cli-archive/spec.md | 2 +- src/core/archive.ts | 8 ++++- test/core/archive.test.ts | 36 +++++++++++++++++++ 3 files changed, 44 insertions(+), 2 deletions(-) diff --git a/openspec/changes/fix-archive-retirement-guidance/specs/cli-archive/spec.md b/openspec/changes/fix-archive-retirement-guidance/specs/cli-archive/spec.md index 7d84138929..146498af8a 100644 --- a/openspec/changes/fix-archive-retirement-guidance/specs/cli-archive/spec.md +++ b/openspec/changes/fix-archive-retirement-guidance/specs/cli-archive/spec.md @@ -22,7 +22,7 @@ A delta whose REMOVED entries cover every requirement a capability has SHALL ret - **WHEN** a rebuilt spec is retirable but the change does not declare `retire_capabilities: true` in its metadata, or declares it in metadata that cannot be honored - **THEN** write the spec as any other, so the archive aborts on it exactly as it did before this behavior existed - **AND** name the marker as the fix in that abort, and say when a marker that is present cannot be honored -- **AND** say nothing about the marker when retiring would not have made the spec writable anyway +- **AND** say nothing about adding the marker when retiring would not have made the spec writable anyway, while still reporting a marker that is present but cannot be honored #### Scenario: Delta removes the capability's last requirement diff --git a/src/core/archive.ts b/src/core/archive.ts index 3f3e017299..19f2451a76 100644 --- a/src/core/archive.ts +++ b/src/core/archive.ts @@ -1639,7 +1639,13 @@ export class ArchiveCommand { `archive does instead, and it is refused while the spec holds content the ` + `merge cannot safely account for and deleting the file would take with it: ` + `${describeUnaccountedContent(p.unaccountedContent)}. ` + - 'Move it into `## Purpose` or a canonical requirement, or delete the spec by hand, then rerun.' + 'Move it into `## Purpose` or a canonical requirement, or delete the spec by hand, then rerun.' + + // Said here too, because an author looking at a marker they + // believe authorises the deletion should not have to clear + // the content first to find out it was never read. + (retirementMarker.invalidReason + ? ` A marker is present but cannot be honored (${retirementMarker.invalidReason}).` + : '') : undefined; // The marker was set and retirement was still refused. Saying // nothing left the author who did exactly what the docs asked diff --git a/test/core/archive.test.ts b/test/core/archive.test.ts index ef66d74bc3..6490d6b3db 100644 --- a/test/core/archive.test.ts +++ b/test/core/archive.test.ts @@ -3916,6 +3916,42 @@ The system SHALL do the thing differently. expect(printed).toContain(`"L${'o'.repeat(199)}…"`); }); + // An author who set a marker that cannot be honored believes they have + // authorised the deletion. Clearing the blocking content first, only to + // then learn the marker was never read, is two aborts for one mistake. + it('reports an unhonorable marker alongside the blocking content', async () => { + const changeDir = await createChange( + 'retire-bad-marker-with-notes', + 'legacy-layer', + REMOVE_ALL, + { declareRetirement: false } + ); + await fs.writeFile( + path.join(changeDir, '.openspec.yaml'), + 'schema: spec-driven\nretire_capabilities: yes-please\n' + ); + const mainSpecDir = path.join(tempDir, 'openspec', 'specs', 'legacy-layer'); + await fs.mkdir(mainSpecDir, { recursive: true }); + await fs.writeFile( + path.join(mainSpecDir, 'spec.md'), + `${mainSpec('legacy-layer')}\n## Notes\n\nOwned by the platform team.\n` + ); + + await archiveCommand.execute('retire-bad-marker-with-notes', { yes: true }); + + expect(process.exitCode).toBe(1); + expect(console.log).toHaveBeenCalledWith( + expect.stringContaining('"Owned by the platform team."') + ); + expect(console.log).toHaveBeenCalledWith( + expect.stringContaining('cannot be honored') + ); + // Still no invitation to add one - the content blocks it either way. + expect(console.log).not.toHaveBeenCalledWith( + expect.stringContaining('add `retire_capabilities: true`') + ); + }); + it('carries the blocked-retirement guidance into --json', async () => { await createChange('retire-unmarked-notes-json', 'legacy-layer', REMOVE_ALL, { declareRetirement: false, From 2411b70e1f5114d92c86466f5613b6ba6c481612 Mon Sep 17 00:00:00 2001 From: Clay Good Date: Wed, 19 Aug 2026 10:46:09 -0500 Subject: [PATCH 6/8] style(archive): use one sentence for an unhonorable marker in both aborts --- .changeset/tidy-moons-smell.md | 2 +- src/core/archive.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/.changeset/tidy-moons-smell.md b/.changeset/tidy-moons-smell.md index edd9b33f10..2575934712 100644 --- a/.changeset/tidy-moons-smell.md +++ b/.changeset/tidy-moons-smell.md @@ -2,4 +2,4 @@ '@fission-ai/openspec': patch --- -archive: tell the author how to retire a capability when the emptied spec also holds content the merge cannot account for. That combination printed only "Spec must have at least one requirement" and no guidance at all; the abort now names the blocking lines, sanitized and length-bounded before they reach the terminal. +archive: tell the author how to retire a capability when the emptied spec also holds content the merge cannot account for. That combination printed only "Spec must have at least one requirement" and no guidance at all; the abort now names the blocking lines - sanitized and length-bounded before they reach the terminal - and reports a `retire_capabilities` marker that is present but cannot be honored. diff --git a/src/core/archive.ts b/src/core/archive.ts index 19f2451a76..888a6135a6 100644 --- a/src/core/archive.ts +++ b/src/core/archive.ts @@ -1644,7 +1644,7 @@ export class ArchiveCommand { // believe authorises the deletion should not have to clear // the content first to find out it was never read. (retirementMarker.invalidReason - ? ` A marker is present but cannot be honored (${retirementMarker.invalidReason}).` + ? ` The marker present now cannot be honored (${retirementMarker.invalidReason}).` : '') : undefined; // The marker was set and retirement was still refused. Saying From 48f23a5f448ef4f2955c201e2362488017435424 Mon Sep 17 00:00:00 2001 From: Clay Good Date: Wed, 19 Aug 2026 10:56:51 -0500 Subject: [PATCH 7/8] fix(metadata): strip control characters from an unhonorable marker reason Every reason a boolean change-metadata marker gives quotes something the author wrote - a schema name, a parser message carrying one, a filesystem error carrying a path - and two commands print it straight to a terminal. A schema name carrying a raw ESC, with the marker set, put that ESC on screen through `openspec archive`; `openspec validate` prints the same reason. Fixed at the source in `readBooleanMarker` rather than at either call site, so no consumer has to remember. The reason still quotes the name recognisably; only control characters are replaced. Reported by CodeRabbit on #1699. Pre-existing on main, and this PR would have added a second place it reaches the terminal. --- .changeset/tidy-moons-smell.md | 2 +- .../proposal.md | 10 +++++- .../specs/cli-archive/spec.md | 2 +- .../fix-archive-retirement-guidance/tasks.md | 5 +-- src/utils/change-metadata.ts | 31 +++++++++-------- test/utils/change-metadata.test.ts | 33 +++++++++++++++++++ 6 files changed, 65 insertions(+), 18 deletions(-) diff --git a/.changeset/tidy-moons-smell.md b/.changeset/tidy-moons-smell.md index 2575934712..746b2e47cd 100644 --- a/.changeset/tidy-moons-smell.md +++ b/.changeset/tidy-moons-smell.md @@ -2,4 +2,4 @@ '@fission-ai/openspec': patch --- -archive: tell the author how to retire a capability when the emptied spec also holds content the merge cannot account for. That combination printed only "Spec must have at least one requirement" and no guidance at all; the abort now names the blocking lines - sanitized and length-bounded before they reach the terminal - and reports a `retire_capabilities` marker that is present but cannot be honored. +archive: tell the author how to retire a capability when the emptied spec also holds content the merge cannot account for. That combination printed only "Spec must have at least one requirement" and no guidance at all; the abort now names the blocking lines and reports a `retire_capabilities` marker that is present but cannot be honored. Authored content quoted in those messages - the blocking lines, and the marker's own reason, which `openspec validate` prints too - is stripped of control characters and bounded in length before it reaches the terminal. diff --git a/openspec/changes/fix-archive-retirement-guidance/proposal.md b/openspec/changes/fix-archive-retirement-guidance/proposal.md index 9ba56e6f65..6da2dfac41 100644 --- a/openspec/changes/fix-archive-retirement-guidance/proposal.md +++ b/openspec/changes/fix-archive-retirement-guidance/proposal.md @@ -28,16 +28,24 @@ no guidance at all — the dead end the marker exists to close, still reachable adding it would really let the archive through; a spec with a second `## Requirements` section holding a live requirement must not be pointed toward a deletion. Once the content is resolved, the rerun names the marker. +- A marker that is present but cannot be honored is reported alongside the + blocking content. An author who wrote `retire_capabilities: yes-please` + believes they authorised the deletion; making them clear the content first, + only to then learn the marker was never read, is two aborts for one mistake. - The blocking lines are authored file content printed to a terminal, so they are rendered with control characters replaced and their length bounded — the same treatment a change directory name already gets. This also hardens the marker-declared refusal, which echoed them verbatim. +- The marker's own reason gets the same treatment, at its source in + `readBooleanMarker`, because every reason quotes something the author wrote — + a schema name, a parser message carrying one, a filesystem error carrying a + path. Fixing it there covers `openspec validate`, which prints the same reason. No change to what archive writes, deletes, or refuses. Message paths only. ## Impact - Affected specs: `cli-archive` (MODIFIED: Capability Retirement) -- Affected code: `src/core/archive.ts` +- Affected code: `src/core/archive.ts`, `src/utils/change-metadata.ts` - Affected docs: `docs/writing-specs.md` (states the second refusal condition, which was true before this change but undocumented) diff --git a/openspec/changes/fix-archive-retirement-guidance/specs/cli-archive/spec.md b/openspec/changes/fix-archive-retirement-guidance/specs/cli-archive/spec.md index 146498af8a..a22182d47b 100644 --- a/openspec/changes/fix-archive-retirement-guidance/specs/cli-archive/spec.md +++ b/openspec/changes/fix-archive-retirement-guidance/specs/cli-archive/spec.md @@ -21,7 +21,7 @@ A delta whose REMOVED entries cover every requirement a capability has SHALL ret - **WHEN** a rebuilt spec is retirable but the change does not declare `retire_capabilities: true` in its metadata, or declares it in metadata that cannot be honored - **THEN** write the spec as any other, so the archive aborts on it exactly as it did before this behavior existed -- **AND** name the marker as the fix in that abort, and say when a marker that is present cannot be honored +- **AND** name the marker as the fix in that abort, and say when a marker that is present cannot be honored, with control characters replaced in the reason because it repeats what the author wrote - **AND** say nothing about adding the marker when retiring would not have made the spec writable anyway, while still reporting a marker that is present but cannot be honored #### Scenario: Delta removes the capability's last requirement diff --git a/openspec/changes/fix-archive-retirement-guidance/tasks.md b/openspec/changes/fix-archive-retirement-guidance/tasks.md index 2716f411fc..c9863a1db0 100644 --- a/openspec/changes/fix-archive-retirement-guidance/tasks.md +++ b/openspec/changes/fix-archive-retirement-guidance/tasks.md @@ -2,11 +2,12 @@ ## 1. Name the blocking content when the marker is absent - [x] 1.1 Derive "this run emptied the capability" once, and hint on it in both the marker-missing and content-blocked cases -- [x] 1.2 Keep the marker unnamed while content still blocks the retirement +- [x] 1.2 Keep the marker unnamed while content still blocks the retirement, while still reporting one that cannot be honored ## 2. Render the blocking lines safely - [x] 2.1 Replace control characters and bound each line, sharing one helper with the marker-declared refusal -- [x] 2.2 Cover the human abort, the `--json` detail, and the rendering with tests +- [x] 2.2 Sanitize the marker's own reason at its source, so `validate` is covered too +- [x] 2.3 Cover the human abort, the `--json` detail, and the rendering with tests ## 3. Record the behavior - [x] 3.1 Update the `cli-archive` spec delta and `docs/writing-specs.md` diff --git a/src/utils/change-metadata.ts b/src/utils/change-metadata.ts index 7ad17078dc..1f31a44596 100644 --- a/src/utils/change-metadata.ts +++ b/src/utils/change-metadata.ts @@ -258,6 +258,19 @@ export function readRetireCapabilitiesMarker(changeDir: string): MetadataMarker * name. One body rather than two, so a marker can never drift into honoring * metadata the other rejects - the whole point of the contract described above. */ +/** + * A marker that cannot be honored, with its reason made safe to print. + * + * Every reason quotes something the author wrote - a schema name, a parser + * message carrying one, a filesystem error carrying a path - and callers print + * it straight to a terminal (`openspec archive`, `openspec validate`). A raw CR + * could forge a line of its own and an ESC could redraw the screen, so control + * characters never leave this function. + */ +function unhonorable(reason: string): MetadataMarker { + return { declared: false, invalidReason: reason.replace(/[\u0000-\u001f\u007f]/g, '?') }; +} + function readBooleanMarker( changeDir: string, key: 'skip_specs' | 'retire_capabilities' @@ -275,10 +288,7 @@ function readBooleanMarker( // the change as unmarked while every metadata-reading surface errors. const message = err instanceof Error ? err.message : String(err); - return { - declared: false, - invalidReason: `the metadata file cannot be read (${message})`, - }; + return unhonorable(`the metadata file cannot be read (${message})`); } let parsed: unknown; @@ -288,9 +298,7 @@ function readBooleanMarker( // Anchored so a comment like "# maybe add skip_specs later" does not // claim the marker was set. const mentioned = new RegExp(`^\\s*(['"]?)${key}\\1\\s*:`, 'm').test(raw); - return mentioned - ? { declared: false, invalidReason: 'the file is not valid YAML' } - : { declared: false }; + return mentioned ? unhonorable('the file is not valid YAML') : { declared: false }; } const result = ChangeMetadataSchema.safeParse(parsed); @@ -308,15 +316,12 @@ function readBooleanMarker( try { const projectRoot = path.resolve(changeDir, '../../..'); if (!listSchemas(projectRoot).includes(result.data.schema)) { - return { - declared: false, - invalidReason: `schema: unknown schema '${result.data.schema}'`, - }; + return unhonorable(`schema: unknown schema '${result.data.schema}'`); } resolveSchema(result.data.schema, projectRoot); } catch (err) { const message = err instanceof Error ? err.message : String(err); - return { declared: false, invalidReason: message }; + return unhonorable(message); } return { declared: true }; } @@ -334,7 +339,7 @@ function readBooleanMarker( if (markerMentioned) { const first = result.error.issues[0]; const where = first.path.length > 0 ? `${first.path.join('.')}: ` : ''; - return { declared: false, invalidReason: `${where}${first.message}` }; + return unhonorable(`${where}${first.message}`); } return { declared: false }; } diff --git a/test/utils/change-metadata.test.ts b/test/utils/change-metadata.test.ts index 0082d03d73..c66377bfce 100644 --- a/test/utils/change-metadata.test.ts +++ b/test/utils/change-metadata.test.ts @@ -8,6 +8,7 @@ import { resolveSchemaForChange, validateSchemaName, ChangeMetadataError, + readRetireCapabilitiesMarker, } from '../../src/utils/change-metadata.js'; import { ChangeMetadataSchema } from '../../src/core/change-metadata/index.js'; @@ -382,3 +383,35 @@ describe('validateSchemaName', () => { ); }); }); + +describe('boolean marker reasons', () => { + let tempDir: string; + + beforeEach(async () => { + tempDir = await fs.mkdtemp(path.join(os.tmpdir(), 'openspec-marker-reason-')); + await fs.mkdir(path.join(tempDir, 'openspec', 'changes', 'c'), { recursive: true }); + }); + + afterEach(async () => { + await fs.rm(tempDir, { recursive: true, force: true }); + }); + + // Every reason quotes something the author wrote, and callers print it + // straight to a terminal. A schema name carrying an ESC could redraw the + // screen; a CR could forge a line of its own. + it('strips control characters from a reason that quotes authored content', async () => { + const changeDir = path.join(tempDir, 'openspec', 'changes', 'c'); + await fs.writeFile( + path.join(changeDir, '.openspec.yaml'), + 'schema: "ghost\u001b[31m-schema"\nretire_capabilities: true\n', + 'utf-8' + ); + + const marker = readRetireCapabilitiesMarker(changeDir); + + expect(marker.declared).toBe(false); + // The name is still recognisable, so the author can find what they typed. + expect(marker.invalidReason).toContain("unknown schema 'ghost?[31m-schema'"); + expect(marker.invalidReason).not.toMatch(/[\u0000-\u001f\u007f]/); + }); +}); From 2b90bab636b01f1304206dd6ccf3c48cd06ce7f4 Mon Sep 17 00:00:00 2001 From: Clay Good Date: Wed, 19 Aug 2026 10:58:38 -0500 Subject: [PATCH 8/8] test(archive): fix a comment left behind by the reworded abort --- test/core/archive.test.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/core/archive.test.ts b/test/core/archive.test.ts index 6490d6b3db..b120bb0faf 100644 --- a/test/core/archive.test.ts +++ b/test/core/archive.test.ts @@ -3869,8 +3869,8 @@ The system SHALL do the thing differently. expect(console.log).toHaveBeenCalledWith( expect.stringContaining(VALIDATION_MESSAGES.SPEC_NO_REQUIREMENTS) ); - // The abort now says what has to happen: retiring is the way through, - // and here is the line standing in its way. + // The abort now says what archive would do with the emptied spec, and + // names the line standing in the way of it. expect(console.log).toHaveBeenCalledWith( expect.stringContaining('Retiring the capability is what archive does instead') );