diff --git a/.changeset/tidy-moons-smell.md b/.changeset/tidy-moons-smell.md new file mode 100644 index 0000000000..746b2e47cd --- /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. 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/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/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..6da2dfac41 --- /dev/null +++ b/openspec/changes/fix-archive-retirement-guidance/proposal.md @@ -0,0 +1,51 @@ +# 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. +- 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`, `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 new file mode 100644 index 0000000000..a22182d47b --- /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, 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 + +- **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..c9863a1db0 --- /dev/null +++ b/openspec/changes/fix-archive-retirement-guidance/tasks.md @@ -0,0 +1,13 @@ +# 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, 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 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/core/archive.ts b/src/core/archive.ts index d476c036e5..888a6135a6 100644 --- a/src/core/archive.ts +++ b/src/core/archive.ts @@ -68,6 +68,35 @@ export async function isRetirableSpec(specName: string, rebuilt: string): Promis ); } +/** + * 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) => { + 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}`; +} + /** * 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 +1597,56 @@ 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 - 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 && + 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 + ? `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 + // 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 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.' + + // 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 + ? ` The marker present now 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 // back in the original dead end with no signal that their @@ -1600,8 +1659,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 +1668,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 +1678,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/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/core/archive.test.ts b/test/core/archive.test.ts index f64082e6e6..b120bb0faf 100644 --- a/test/core/archive.test.ts +++ b/test/core/archive.test.ts @@ -3840,6 +3840,140 @@ 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 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') + ); + 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(); + }); + + // 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)}…"`); + }); + + // 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, + }); + 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 what archive does instead'); + 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. 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]/); + }); +});