From 457abf3f0bcf9707276c2cec418636a4fd9f84f6 Mon Sep 17 00:00:00 2001 From: Clay Good Date: Thu, 23 Jul 2026 15:47:27 -0500 Subject: [PATCH 1/4] fix(archive): treat early-synced REMOVED deltas as no-ops, plus audit follow-ups Follow-ups from the post-v1.6.0 full-branch audit: - archive: a REMOVED delta whose requirement is already gone from the main spec (early-sync pattern) now warns and continues instead of aborting, matching the ADDED (#1376) and RENAMED (#1386) escapes; spec-update totals now count applied removals only - archive: the has-delta-specs gate matches section headers case-insensitively like the parser, so lowercase headers get the same delta validation errors validate reports - discovery: a symlinked specs//spec.md is resolved instead of being invisible (hasAnyFileUnder and the artifact graph already counted it); dangling links are skipped - show: a plain `openspec show ` no longer warns about the never-passed `scenarios` flag (commander defaults --no-scenarios to true) - parsers: buildCodeFenceMask now has a single implementation in code-fence.ts; requirement-text.ts re-exports it - templates: apply/update/onboard no longer dead-end core-profile users on /opsx:continue and /opsx:new - they name the CLI fallback (openspec status/instructions) for profiles that do not install those workflows - qwen/bob: command bodies and skills reference commands by the hyphen names their files actually answer to (/opsx-), matching opencode/pi/oh-my-pi - specs-apply: remove the dead applySpecs export (no callers, bypassed store-aware roots) Co-Authored-By: Claude Fable 5 --- skills/openspec-apply-change/SKILL.md | 2 +- skills/openspec-onboard/SKILL.md | 6 +- skills/openspec-update-change/SKILL.md | 1 + src/commands/show.ts | 8 +- src/core/archive.ts | 4 +- src/core/command-generation/adapters/qwen.ts | 7 +- src/core/parsers/requirement-text.ts | 58 +------ src/core/specs-apply.ts | 157 ++---------------- src/core/templates/workflows/apply-change.ts | 4 +- src/core/templates/workflows/onboard.ts | 6 +- src/core/templates/workflows/update-change.ts | 6 +- src/utils/command-references.ts | 12 +- src/utils/spec-discovery.ts | 23 ++- test/commands/show.test.ts | 23 ++- test/core/archive.test.ts | 83 ++++++++- test/core/command-generation/adapters.test.ts | 13 ++ .../templates/skill-templates-parity.test.ts | 18 +- test/utils/command-references.test.ts | 16 +- test/utils/spec-discovery.test.ts | 39 +++++ 19 files changed, 241 insertions(+), 245 deletions(-) diff --git a/skills/openspec-apply-change/SKILL.md b/skills/openspec-apply-change/SKILL.md index 49f3b4bd2c..3ea1e37814 100644 --- a/skills/openspec-apply-change/SKILL.md +++ b/skills/openspec-apply-change/SKILL.md @@ -48,7 +48,7 @@ Implement tasks from an OpenSpec change. - Dynamic instruction based on current state **Handle states:** - - If `state: "blocked"` (missing artifacts): show message, suggest using openspec-continue-change + - If `state: "blocked"` (missing artifacts): show message, suggest using openspec-continue-change (if it is not installed, run `openspec status --change "" --json` to see the next artifact and `openspec instructions --change "" --json` for how to create it) - If `state: "all_done"`: congratulate, suggest archive - Otherwise: proceed to implementation diff --git a/skills/openspec-onboard/SKILL.md b/skills/openspec-onboard/SKILL.md index a9b1a7049a..509905985e 100644 --- a/skills/openspec-onboard/SKILL.md +++ b/skills/openspec-onboard/SKILL.md @@ -475,7 +475,7 @@ This same rhythm works for any size change—a small fix or a major feature. | `/openspec-apply-change` | Implement tasks from a change | | `/openspec-archive-change` | Archive a completed change | -**Additional commands:** +**Additional commands** (only if installed - availability depends on your profile): | Command | What it does | |--------------------|----------------------------------------------------------| @@ -503,7 +503,7 @@ If the user says they need to stop, want to pause, or seem disengaged: No problem! Your change is saved at the `changeRoot` reported by `openspec status --change "" --json`. To pick up where we left off later: -- `/openspec-continue-change ` - Resume artifact creation +- `/openspec-continue-change ` - Resume artifact creation (if installed) - `/openspec-apply-change ` - Jump to implementation (if tasks exist) The work won't be lost. Come back whenever you're ready. @@ -527,7 +527,7 @@ If the user says they just want to see the commands or skip the tutorial: | `/openspec-apply-change ` | Implement tasks | | `/openspec-archive-change ` | Archive when done | -**Additional commands:** +**Additional commands** (only if installed - availability depends on your profile): | Command | What it does | |---------------------------|-------------------------------------| diff --git a/skills/openspec-update-change/SKILL.md b/skills/openspec-update-change/SKILL.md index d708818204..b39170da77 100644 --- a/skills/openspec-update-change/SKILL.md +++ b/skills/openspec-update-change/SKILL.md @@ -83,3 +83,4 @@ After each invocation, show: - Do not advance the build frontier: no new artifacts, no new files under glob artifacts - that is `/openspec-continue-change`'s job. - Confirm every edit with the user before writing. - If the request changes the change's *intent* rather than refining it, recommend starting fresh with `/openspec-new-change` (the "Update vs. Start Fresh" heuristic). +- `/openspec-continue-change` and `/openspec-new-change` may not be installed (core profile). When suggesting one that is unavailable, point to the CLI instead: `openspec status --change "" --json` shows the next artifact and `openspec instructions --change "" --json` explains how to create it. diff --git a/src/commands/show.ts b/src/commands/show.ts index 408f11a7ca..bfdb2958dc 100644 --- a/src/commands/show.ts +++ b/src/commands/show.ts @@ -203,10 +203,14 @@ export class ShowCommand { private warnIrrelevantFlags(type: ItemType, options: { [k: string]: any }): boolean { const irrelevant: string[] = []; + // --no-scenarios makes commander default `scenarios` to true, so its + // presence alone does not mean the user passed it — only false does. + const isUserProvided = (k: string) => + k === 'scenarios' ? options[k] === false : k in options; if (type === 'change') { - for (const k of SPEC_FLAG_KEYS) if (k in options) irrelevant.push(k); + for (const k of SPEC_FLAG_KEYS) if (isUserProvided(k)) irrelevant.push(k); } else { - for (const k of CHANGE_FLAG_KEYS) if (k in options) irrelevant.push(k); + for (const k of CHANGE_FLAG_KEYS) if (isUserProvided(k)) irrelevant.push(k); } if (irrelevant.length > 0) { console.error(`Warning: Ignoring flags not applicable to ${type}: ${irrelevant.join(', ')}`); diff --git a/src/core/archive.ts b/src/core/archive.ts index 6c35868c6c..95c738aa03 100644 --- a/src/core/archive.ts +++ b/src/core/archive.ts @@ -323,7 +323,9 @@ export class ArchiveCommand { for (const { specFile } of hasDeltaSpecs ? [] : await discoverSpecFiles(changeSpecsDir)) { try { const content = await fs.readFile(specFile, 'utf-8'); - if (/^##\s+(ADDED|MODIFIED|REMOVED|RENAMED)\s+Requirements/m.test(content)) { + // Case-insensitive to match the delta parser, so a lowercase header + // routes through the same delta validation that validate runs. + if (/^##\s+(ADDED|MODIFIED|REMOVED|RENAMED)\s+Requirements/im.test(content)) { hasDeltaSpecs = true; break; } diff --git a/src/core/command-generation/adapters/qwen.ts b/src/core/command-generation/adapters/qwen.ts index 9d31a07719..a22726ad57 100644 --- a/src/core/command-generation/adapters/qwen.ts +++ b/src/core/command-generation/adapters/qwen.ts @@ -10,6 +10,7 @@ import path from 'path'; import type { CommandContent, ToolCommandAdapter } from '../types.js'; +import { transformToHyphenCommands } from '../../../utils/command-references.js'; /** * Escapes a string value for safe YAML output. @@ -39,11 +40,15 @@ export const qwenAdapter: ToolCommandAdapter = { }, formatFile(content: CommandContent): string { + // Qwen commands are invoked by filename (/opsx-), so cross-references + // must use the hyphen form too. + const transformedBody = transformToHyphenCommands(content.body); + return `--- description: ${escapeYamlValue(content.description)} --- -${content.body} +${transformedBody} `; }, }; diff --git a/src/core/parsers/requirement-text.ts b/src/core/parsers/requirement-text.ts index 8aa0e89567..9841e3ddcf 100644 --- a/src/core/parsers/requirement-text.ts +++ b/src/core/parsers/requirement-text.ts @@ -9,60 +9,10 @@ * `validate `, `validate `, and `archive`. */ -/** - * Build a per-line mask marking lines that fall inside a fenced code block - * (``` ``` ``` or ``` ~~~ ```), including the fence lines themselves. Mirrors the - * fence rules markdown uses: a fence opens on the first ```` ```/~~~ ```` of - * length >= 3 and closes on a line of the same marker whose length is >= the - * opening length, with nothing but whitespace after it. - */ -export function buildCodeFenceMask(lines: string[]): boolean[] { - const mask = new Array(lines.length).fill(false); - let activeFence: { marker: '`' | '~'; length: number } | null = null; - - for (let i = 0; i < lines.length; i++) { - const fence = getFenceMarker(lines[i]); - - if (!activeFence) { - if (fence) { - activeFence = fence; - mask[i] = true; - } - continue; - } - - mask[i] = true; - if (isClosingFence(lines[i], activeFence)) { - activeFence = null; - } - } - - return mask; -} - -function getFenceMarker(line: string): { marker: '`' | '~'; length: number } | null { - const fenceMatch = line.match(/^\s*(`{3,}|~{3,})/); - if (!fenceMatch) { - return null; - } - - return { - marker: fenceMatch[1][0] as '`' | '~', - length: fenceMatch[1].length, - }; -} - -function isClosingFence( - line: string, - activeFence: { marker: '`' | '~'; length: number } -): boolean { - const fenceMatch = line.match(/^\s*(`{3,}|~{3,})\s*$/); - return Boolean( - fenceMatch && - fenceMatch[1][0] === activeFence.marker && - fenceMatch[1].length >= activeFence.length - ); -} +// Re-exported so existing importers keep working; the single implementation +// lives in code-fence.ts. +export { buildCodeFenceMask } from './code-fence.js'; +import { buildCodeFenceMask } from './code-fence.js'; /** Lines that look like `**ID**: ...` / `**Priority**: ...` metadata. */ const METADATA_LINE = /^\*\*[^*]+\*\*:/; diff --git a/src/core/specs-apply.ts b/src/core/specs-apply.ts index 563769b63a..b9e2dfce7f 100644 --- a/src/core/specs-apply.ts +++ b/src/core/specs-apply.ts @@ -17,7 +17,6 @@ import { 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'; @@ -33,26 +32,6 @@ export interface SpecUpdate { exists: boolean; } -export interface ApplyResult { - capability: string; - added: number; - modified: number; - removed: number; - renamed: number; -} - -export interface SpecsApplyOutput { - changeName: string; - capabilities: ApplyResult[]; - totals: { - added: number; - modified: number; - removed: number; - renamed: number; - }; - noChanges: boolean; -} - interface ScenarioBlock { name: string; raw: string; @@ -318,18 +297,26 @@ export async function buildUpdatedSpec( } // REMOVED + let removedApplied = 0; for (const name of plan.removed) { const key = normalizeRequirementName(name); if (!nameToBlock.has(key)) { - // For new specs, REMOVED requirements are already warned about and ignored - // For existing specs, missing requirements are an error - if (!isNewSpec) { - throw new Error(`${specName} REMOVED failed for header "### Requirement: ${name}" - not found`); + // Requirement gone from the baseline means the removal was already + // synced (early-sync pattern) — re-applying it is a no-op, not a + // failure. Unlike RENAMED there is no signal separating that from a + // mistyped header, so warn instead of skipping silently. + // For new specs the skip was already warned about above. + if (!isNewSpec && !options.silent) { + console.log( + chalk.yellow( + `⚠️ Warning: ${specName} - REMOVED requirement "${name}" is not in the current spec; treating it as already removed.` + ) + ); } - // Skip removal for new specs (already warned above) continue; } nameToBlock.delete(key); + removedApplied++; } // MODIFIED @@ -409,7 +396,7 @@ export async function buildUpdatedSpec( counts: { added: addedApplied, modified: plan.modified.length, - removed: plan.removed.length, + removed: removedApplied, renamed: renamedApplied, }, }; @@ -593,119 +580,3 @@ function parseScenarioBlocks(requirementRaw: string): ScenarioBlock[] { return scenarios; } -/** - * Apply all delta specs from a change to main specs. - * - * @param projectRoot - The project root directory - * @param changeName - The name of the change to apply - * @param options - Options for the operation - * @returns Result of the operation with counts - */ -export async function applySpecs( - projectRoot: string, - changeName: string, - options: { - dryRun?: boolean; - skipValidation?: boolean; - silent?: boolean; - } = {} -): Promise { - const changeDir = path.join(projectRoot, 'openspec', 'changes', changeName); - const mainSpecsDir = path.join(projectRoot, 'openspec', 'specs'); - - // Verify change exists - try { - const stat = await fs.stat(changeDir); - if (!stat.isDirectory()) { - throw new Error(`Change '${changeName}' not found.`); - } - } catch { - throw new Error(`Change '${changeName}' not found.`); - } - - // Find specs to update - const specUpdates = await findSpecUpdates(changeDir, mainSpecsDir); - - if (specUpdates.length === 0) { - return { - changeName, - capabilities: [], - totals: { added: 0, modified: 0, removed: 0, renamed: 0 }, - noChanges: true, - }; - } - - // Prepare all updates first (validation pass, no writes) - const prepared: Array<{ - update: SpecUpdate; - rebuilt: string; - counts: { added: number; modified: number; removed: number; renamed: number }; - }> = []; - - for (const update of specUpdates) { - const built = await buildUpdatedSpec(update, changeName); - prepared.push({ update, rebuilt: built.rebuilt, counts: built.counts }); - } - - // Validate rebuilt specs unless validation is skipped - if (!options.skipValidation) { - const validator = new Validator(); - for (const p of prepared) { - const specName = p.update.id; - const report = await validator.validateSpecContent(specName, p.rebuilt); - if (!report.valid) { - const errors = report.issues - .filter((i) => i.level === 'ERROR') - .map((i) => ` ✗ ${i.message}`) - .join('\n'); - throw new Error(`Validation errors in rebuilt spec for ${specName}:\n${errors}`); - } - } - } - - // Build results - const capabilities: ApplyResult[] = []; - const totals = { added: 0, modified: 0, removed: 0, renamed: 0 }; - - for (const p of prepared) { - const capability = p.update.id; - - if (!options.dryRun) { - // Write the updated spec - const targetDir = path.dirname(p.update.target); - await fs.mkdir(targetDir, { recursive: true }); - await fs.writeFile(p.update.target, p.rebuilt); - - if (!options.silent) { - console.log(`Applying changes to openspec/specs/${capability}/spec.md:`); - if (p.counts.added) console.log(` + ${p.counts.added} added`); - if (p.counts.modified) console.log(` ~ ${p.counts.modified} modified`); - if (p.counts.removed) console.log(` - ${p.counts.removed} removed`); - if (p.counts.renamed) console.log(` → ${p.counts.renamed} renamed`); - } - } else if (!options.silent) { - console.log(`Would apply changes to openspec/specs/${capability}/spec.md:`); - if (p.counts.added) console.log(` + ${p.counts.added} added`); - if (p.counts.modified) console.log(` ~ ${p.counts.modified} modified`); - if (p.counts.removed) console.log(` - ${p.counts.removed} removed`); - if (p.counts.renamed) console.log(` → ${p.counts.renamed} renamed`); - } - - capabilities.push({ - capability, - ...p.counts, - }); - - totals.added += p.counts.added; - totals.modified += p.counts.modified; - totals.removed += p.counts.removed; - totals.renamed += p.counts.renamed; - } - - return { - changeName, - capabilities, - totals, - noChanges: false, - }; -} diff --git a/src/core/templates/workflows/apply-change.ts b/src/core/templates/workflows/apply-change.ts index a08b24ddd0..e7c5b6c6a4 100644 --- a/src/core/templates/workflows/apply-change.ts +++ b/src/core/templates/workflows/apply-change.ts @@ -50,7 +50,7 @@ ${STORE_SELECTION_GUIDANCE} - Dynamic instruction based on current state **Handle states:** - - If \`state: "blocked"\` (missing artifacts): show message, suggest using openspec-continue-change + - If \`state: "blocked"\` (missing artifacts): show message, suggest using openspec-continue-change (if it is not installed, run \`openspec status --change "" --json\` to see the next artifact and \`openspec instructions --change "" --json\` for how to create it) - If \`state: "all_done"\`: congratulate, suggest archive - Otherwise: proceed to implementation @@ -210,7 +210,7 @@ ${STORE_SELECTION_GUIDANCE} - Dynamic instruction based on current state **Handle states:** - - If \`state: "blocked"\` (missing artifacts): show message, suggest using \`/opsx:continue\` + - If \`state: "blocked"\` (missing artifacts): show message, suggest using \`/opsx:continue\` (if it is not installed, run \`openspec status --change "" --json\` to see the next artifact and \`openspec instructions --change "" --json\` for how to create it) - If \`state: "all_done"\`: congratulate, suggest archive - Otherwise: proceed to implementation diff --git a/src/core/templates/workflows/onboard.ts b/src/core/templates/workflows/onboard.ts index d175b08322..c52e856027 100644 --- a/src/core/templates/workflows/onboard.ts +++ b/src/core/templates/workflows/onboard.ts @@ -485,7 +485,7 @@ This same rhythm works for any size change—a small fix or a major feature. | \`/opsx:apply\` | Implement tasks from a change | | \`/opsx:archive\` | Archive a completed change | -**Additional commands:** +**Additional commands** (only if installed - availability depends on your profile): | Command | What it does | |--------------------|----------------------------------------------------------| @@ -513,7 +513,7 @@ If the user says they need to stop, want to pause, or seem disengaged: No problem! Your change is saved at the \`changeRoot\` reported by \`openspec status --change "" --json\`. To pick up where we left off later: -- \`/opsx:continue \` - Resume artifact creation +- \`/opsx:continue \` - Resume artifact creation (if installed) - \`/opsx:apply \` - Jump to implementation (if tasks exist) The work won't be lost. Come back whenever you're ready. @@ -537,7 +537,7 @@ If the user says they just want to see the commands or skip the tutorial: | \`/opsx:apply \` | Implement tasks | | \`/opsx:archive \` | Archive when done | -**Additional commands:** +**Additional commands** (only if installed - availability depends on your profile): | Command | What it does | |---------------------------|-------------------------------------| diff --git a/src/core/templates/workflows/update-change.ts b/src/core/templates/workflows/update-change.ts index a5780c4872..551633deb7 100644 --- a/src/core/templates/workflows/update-change.ts +++ b/src/core/templates/workflows/update-change.ts @@ -84,7 +84,8 @@ After each invocation, show: - Edit only the concrete files in \`existingOutputPaths\`; never write to a glob \`resolvedOutputPath\`. - Do not advance the build frontier: no new artifacts, no new files under glob artifacts - that is \`/opsx:continue\`'s job. - Confirm every edit with the user before writing. -- If the request changes the change's *intent* rather than refining it, recommend starting fresh with \`/opsx:new\` (the "Update vs. Start Fresh" heuristic).`, +- If the request changes the change's *intent* rather than refining it, recommend starting fresh with \`/opsx:new\` (the "Update vs. Start Fresh" heuristic). +- \`/opsx:continue\` and \`/opsx:new\` may not be installed (core profile). When suggesting one that is unavailable, point to the CLI instead: \`openspec status --change "" --json\` shows the next artifact and \`openspec instructions --change "" --json\` explains how to create it.`, license: 'MIT', compatibility: 'Requires openspec CLI.', metadata: { author: 'openspec', version: '1.0' }, @@ -170,6 +171,7 @@ After each invocation, show: - Edit only the concrete files in \`existingOutputPaths\`; never write to a glob \`resolvedOutputPath\`. - Do not advance the build frontier: no new artifacts, no new files under glob artifacts - that is \`/opsx:continue\`'s job. - Confirm every edit with the user before writing. -- If the request changes the change's *intent* rather than refining it, recommend starting fresh with \`/opsx:new\` (the "Update vs. Start Fresh" heuristic).` +- If the request changes the change's *intent* rather than refining it, recommend starting fresh with \`/opsx:new\` (the "Update vs. Start Fresh" heuristic). +- \`/opsx:continue\` and \`/opsx:new\` may not be installed (core profile). When suggesting one that is unavailable, point to the CLI instead: \`openspec status --change "" --json\` shows the next artifact and \`openspec instructions --change "" --json\` explains how to create it.` }; } diff --git a/src/utils/command-references.ts b/src/utils/command-references.ts index b3cadf766a..987f7f8634 100644 --- a/src/utils/command-references.ts +++ b/src/utils/command-references.ts @@ -104,8 +104,8 @@ export function getSkillReferenceTransformer(toolId: string): (text: string) => * because the tool has no command surface at all (capability 'none', e.g. * Kimi Code or Mistral Vibe) — so those skills never point at commands * that were not generated. When commands are generated, tools where the - * command filename doubles as the command name (oh-my-pi, opencode, pi) use - * hyphen-based command references. All other cases keep the default + * command filename doubles as the command name (bob, oh-my-pi, opencode, + * pi, qwen) use hyphen-based command references. All other cases keep the default * `/opsx:*` references; notably skills-invocable tools (codex) are * deliberately left untouched here to keep codex output stable while its * reference rewriting is reworked separately. @@ -123,7 +123,13 @@ export function getTransformerForTool( if (delivery === 'skills' || capability === 'none') { return getSkillReferenceTransformer(toolId); } - if (toolId === 'opencode' || toolId === 'pi' || toolId === 'oh-my-pi') { + if ( + toolId === 'bob' || + toolId === 'oh-my-pi' || + toolId === 'opencode' || + toolId === 'pi' || + toolId === 'qwen' + ) { return transformToHyphenCommands; } return undefined; diff --git a/src/utils/spec-discovery.ts b/src/utils/spec-discovery.ts index 7282498041..509f259143 100644 --- a/src/utils/spec-discovery.ts +++ b/src/utils/spec-discovery.ts @@ -13,8 +13,11 @@ export interface DiscoveredSpec { * `specs//spec.md` layout and nested `specs///spec.md` layouts * are found (#1353). A `spec.md` sitting directly in the root is ignored, * matching the historical requirement that specs live in a capability folder. - * Dot-directories are skipped and symlinks are not followed. Results are - * sorted by id for deterministic output. + * Dot-directories are skipped and symlinked directories are not followed. + * A symlinked `spec.md` IS resolved: `hasAnyFileUnder` and the artifact + * graph's globs both count it as content, so dropping it here would silently + * lose the delta on archive; a dangling link is skipped. Results are sorted + * by id for deterministic output. * * A missing root (ENOENT) yields an empty list, but any other read failure * (EACCES, EIO, ...) is thrown rather than swallowed: since this feeds the @@ -35,8 +38,20 @@ export async function discoverSpecFiles(specsRoot: string): Promise 0) { - results.push({ id: segments.join('/'), specFile: path.join(dir, entry.name) }); + } else if (entry.name === 'spec.md' && segments.length > 0) { + if (entry.isFile()) { + results.push({ id: segments.join('/'), specFile: path.join(dir, entry.name) }); + } else if (entry.isSymbolicLink()) { + const specFile = path.join(dir, entry.name); + try { + if ((await fs.stat(specFile)).isFile()) { + results.push({ id: segments.join('/'), specFile }); + } + } catch (err: any) { + // A dangling link is not content; anything else fails loudly. + if (err?.code !== 'ENOENT') throw err; + } + } } } }; diff --git a/test/commands/show.test.ts b/test/commands/show.test.ts index a606b1fe53..19ec6b2820 100644 --- a/test/commands/show.test.ts +++ b/test/commands/show.test.ts @@ -1,7 +1,7 @@ import { describe, it, expect, beforeEach, afterEach } from 'vitest'; import { promises as fs } from 'fs'; import path from 'path'; -import { execFileSync } from 'child_process'; +import { execFileSync, spawnSync } from 'child_process'; describe('top-level show command', () => { const projectRoot = process.cwd(); @@ -64,6 +64,27 @@ describe('top-level show command', () => { } }); + it('does not warn about spec-only flags that were never passed', () => { + // commander defaults `scenarios` to true for --no-scenarios, so a plain + // `show ` must not warn about a flag the user never typed. + const res = spawnSync('node', [openspecBin, 'show', 'demo', '--json'], { + encoding: 'utf-8', + cwd: testDir, + }); + expect(res.status).toBe(0); + expect(res.stderr).not.toContain('not applicable'); + }); + + it('still warns when --no-scenarios is explicitly passed for a change', () => { + const res = spawnSync( + 'node', + [openspecBin, 'show', 'demo', '--json', '--no-scenarios'], + { encoding: 'utf-8', cwd: testDir } + ); + expect(res.status).toBe(0); + expect(res.stderr).toContain('Ignoring flags not applicable to change: scenarios'); + }); + it('auto-detects spec id and supports spec-only flags', () => { const originalCwd = process.cwd(); try { diff --git a/test/core/archive.test.ts b/test/core/archive.test.ts index 0a2b3f4ff3..180b04744f 100644 --- a/test/core/archive.test.ts +++ b/test/core/archive.test.ts @@ -392,6 +392,43 @@ Then expected result happens`; expect(untouched).toBe(mainSpecContent); }); + it('should archive when REMOVED requirements were already synced to the baseline', async () => { + const changeName = 'early-synced-removal'; + const changeDir = path.join(tempDir, 'openspec', 'changes', changeName); + const changeSpecDir = path.join(changeDir, 'specs', 'core-layer'); + await fs.mkdir(changeSpecDir, { recursive: true }); + + await fs.writeFile( + path.join(changeSpecDir, 'spec.md'), + `# Core Layer - Changes\n\n## REMOVED Requirements\n\n### Requirement: The system SHALL provide a legacy layer\n**Reason**: Replaced by the core abstraction layer.\n` + ); + + // Early-sync pattern: the requirement was already removed from the main spec. + const keptBlock = `### Requirement: The system SHALL provide a core abstraction layer\n\n#### Scenario: Layer is available\n- **WHEN** a consumer imports the layer\n- **THEN** the abstraction is available`; + const mainSpecDir = path.join(tempDir, 'openspec', 'specs', 'core-layer'); + await fs.mkdir(mainSpecDir, { recursive: true }); + await fs.writeFile( + path.join(mainSpecDir, 'spec.md'), + `# core-layer Specification\n\n## Purpose\nCore abstraction layer.\n\n## Requirements\n\n${keptBlock}\n` + ); + + await archiveCommand.execute(changeName, { yes: true, noValidate: true }); + + // Archive succeeds with a warning instead of aborting + expect(console.log).toHaveBeenCalledWith( + expect.stringContaining('REMOVED requirement "The system SHALL provide a legacy layer" is not in the current spec') + ); + // The skipped removal is not reported as applied + expect(console.log).not.toHaveBeenCalledWith(expect.stringContaining('- 1 removed')); + const updatedContent = await fs.readFile(path.join(mainSpecDir, 'spec.md'), 'utf-8'); + expect(updatedContent).toContain('SHALL provide a core abstraction layer'); + expect(updatedContent).not.toContain('SHALL provide a legacy layer'); + + const archives = await fs.readdir(path.join(tempDir, 'openspec', 'changes', 'archive')); + expect(archives.some(a => a.includes(changeName))).toBe(true); + expect(process.exitCode).toBeUndefined(); + }); + it('should merge nested delta specs into the same relative path (#1353)', async () => { const changeName = 'nested-spec-feature'; const changeDir = path.join(tempDir, 'openspec', 'changes', changeName); @@ -472,6 +509,9 @@ The system SHALL support logo and backgroundColor fields for gift cards. expect(console.log).toHaveBeenCalledWith( expect.stringContaining('Warning: gift-card - 2 REMOVED requirement(s) ignored for new spec (nothing to remove).') ); + + // The ignored removals are not reported as applied + expect(console.log).not.toHaveBeenCalledWith(expect.stringContaining('- 2 removed')); // Verify spec was created with only ADDED requirements const mainSpecPath = path.join(tempDir, 'openspec', 'specs', 'gift-card', 'spec.md'); @@ -1679,7 +1719,7 @@ content D`; expect(updated).not.toContain('### Requirement: B'); }); - it('should abort with error when MODIFIED/REMOVED reference non-existent requirements', async () => { + it('should abort with error when MODIFIED references non-existent requirements', async () => { const changeName = 'validate-missing'; const changeDir = path.join(tempDir, 'openspec', 'changes', changeName); const changeSpecDir = path.join(changeDir, 'specs', 'gamma'); @@ -1696,15 +1736,12 @@ Gamma purpose. ## Requirements`; await fs.writeFile(path.join(mainSpecDir, 'spec.md'), mainContent); - // Delta tries to modify and remove non-existent requirement + // Delta tries to modify a non-existent requirement const deltaContent = `# Gamma - Changes ## MODIFIED Requirements ### Requirement: Missing -new text - -## REMOVED Requirements -### Requirement: Another Missing`; +new text`; await fs.writeFile(path.join(changeSpecDir, 'spec.md'), deltaContent); await archiveCommand.execute(changeName, { yes: true, noValidate: true }); @@ -2016,7 +2053,7 @@ Zeta purpose. ### Requirement: Z1 z1`); - // Delta: epsilon is valid modification; zeta tries to remove non-existent -> should abort both + // Delta: epsilon is valid modification; zeta tries to modify non-existent -> should abort both await fs.writeFile(path.join(spec1Dir, 'spec.md'), `# Epsilon - Changes ## MODIFIED Requirements @@ -2025,8 +2062,9 @@ E1 updated`); await fs.writeFile(path.join(spec2Dir, 'spec.md'), `# Zeta - Changes -## REMOVED Requirements -### Requirement: Missing`); +## MODIFIED Requirements +### Requirement: Missing +missing body`); await archiveCommand.execute(changeName, { yes: true, noValidate: true }); @@ -2073,6 +2111,33 @@ E1 updated`); // Regression for the silent-exit-0 bug: when archive is blocked in // human mode it must set a non-zero exit code so scripts/CI can detect // the failure, mirroring the JSON-mode behavior. + it('runs delta spec validation for lowercase delta headers (parity with validate)', async () => { + const changeName = 'exit-lowercase-delta'; + const changeDir = path.join(tempDir, 'openspec', 'changes', changeName); + const changeSpecDir = path.join(changeDir, 'specs', 'lower-capability'); + await fs.mkdir(changeSpecDir, { recursive: true }); + + // Lowercase section header: the parser reads it case-insensitively, so + // the archive gate must route it into delta validation the same way + // validate does instead of falling through to the rebuilt-spec check. + const specContent = `# Lower Capability - Changes + +## added requirements + +### Requirement: Logging Feature +The system SHALL log all events.`; + await fs.writeFile(path.join(changeSpecDir, 'spec.md'), specContent); + await fs.writeFile(path.join(changeDir, 'tasks.md'), '- [x] Task 1\n'); + + await archiveCommand.execute(changeName, { yes: true }); + + expect(process.exitCode).toBe(1); + expect(console.log).toHaveBeenCalledWith( + expect.stringContaining('must include at least one scenario') + ); + await expect(fs.access(changeDir)).resolves.not.toThrow(); + }); + it('sets exit code 1 when delta spec validation fails', async () => { const changeName = 'exit-delta-fail'; const changeDir = path.join(tempDir, 'openspec', 'changes', changeName); diff --git a/test/core/command-generation/adapters.test.ts b/test/core/command-generation/adapters.test.ts index f758305704..75d63bc6b8 100644 --- a/test/core/command-generation/adapters.test.ts +++ b/test/core/command-generation/adapters.test.ts @@ -544,6 +544,19 @@ describe('command-generation/adapters', () => { }); expect(output).toContain('description: "Review: plan & apply \\"changes\\""'); }); + + it('should transform colon command references to hyphen format', () => { + // Qwen commands are invoked by filename (/opsx-), like bob/opencode. + const contentWithRefs: CommandContent = { + ...sampleContent, + body: 'Run /opsx:apply to implement. Then use /opsx:archive.', + }; + const output = qwenAdapter.formatFile(contentWithRefs); + expect(output).toContain('/opsx-apply'); + expect(output).toContain('/opsx-archive'); + expect(output).not.toContain('/opsx:apply'); + expect(output).not.toContain('/opsx:archive'); + }); }); describe('piAdapter', () => { diff --git a/test/core/templates/skill-templates-parity.test.ts b/test/core/templates/skill-templates-parity.test.ts index ab84ea92dc..7a1c7f7f97 100644 --- a/test/core/templates/skill-templates-parity.test.ts +++ b/test/core/templates/skill-templates-parity.test.ts @@ -40,43 +40,43 @@ const EXPECTED_FUNCTION_HASHES: Record = { getExploreSkillTemplate: 'a7eb6fabdc05a5b90a4773ba93320a60edffea88e9b27985668a2959dcec2e3d', getNewChangeSkillTemplate: '39663a6d2037e6697020393a66f6327506e3e3bc573b7a3556dcb7f9457dc51d', getContinueChangeSkillTemplate: '5cc6cf74c055ae67b08373421d934ece65dacbccafbc7452ab5636df3eb9e862', - getApplyChangeSkillTemplate: '0f5a15fc7fb9ad6059a5643d0e01365d27642637a4aaebf182f9eabb45348197', + getApplyChangeSkillTemplate: '3d52b852f3c5f87c3c88aeb4915c78604d97cc75d33aaac8f7e174d365b49971', getFfChangeSkillTemplate: '097a9ff9533900f227cac0523289eae4e19f06a081e5f355a8374dbecf3ff55d', getSyncSpecsSkillTemplate: '8a0e6a41250d9e5f893dd016c375ffb5773823693cb4e481ca74775bbfb9bfb9', - getOnboardSkillTemplate: 'bc2216b72724b01c3a733e63b8bf4aff457f561c0e9ff7288bdacc39780a37a7', + getOnboardSkillTemplate: '60a9df019a86c576bce9f26725925ea1324542cf6cb1e081b68eb0fe4285095a', getOpsxExploreCommandTemplate: 'eef1f8b4fd90ade6d70be46f0f8c3e6722f221fed175a6f9cf626287ef504a94', getOpsxNewCommandTemplate: '57c600cce318d16b9b4308a18d0d983ea3c0673034e606a7cceec07b4c705e87', getOpsxContinueCommandTemplate: '5c3968174001c20737ba39d2473ecec0f3b76591a80f7e2fc3974904d3da9dcd', - getOpsxApplyCommandTemplate: 'daeb507206707169de73c828e199648dde5732cbc17791ef2a027adffd028574', + getOpsxApplyCommandTemplate: '147408d7085b468981a400cc725804252c3fd84e519c57c5f6f83562e32606ee', getOpsxFfCommandTemplate: '264b514cc4849f91fb4414f639484c4181f1e5850d0d788ef276c851efa92859', getArchiveChangeSkillTemplate: '206a22b6778e97c30da9145ef51fdad449b8c995538f6fc25752ef551a37b675', getBulkArchiveChangeSkillTemplate: '2b74b1f73380ff32e35f580734780d843c6161a2748c39edb07f1e00453771b4', getOpsxSyncCommandTemplate: 'df0240a79f7b4943a54c7413ab088ee48f5bf5fe19f9347c170d695c8ec777a4', getVerifyChangeSkillTemplate: 'cab4db01b5d2b1243d63d90c53747d8b39e488c60f76eba3fe8b994467f69267', getOpsxArchiveCommandTemplate: '7dea65d0e2e17db366bb666ba6ae5e205ea02707b8c5c7707565200875c78916', - getOpsxOnboardCommandTemplate: '9430a0fb6530791ab720e068f4b172bc3dfc4e96a1ae29102bee0b92c2afe7b5', + getOpsxOnboardCommandTemplate: 'b1e8e48a7588ced934a6397460597a497009a75d97304124ea763168abe9cfec', getOpsxBulkArchiveCommandTemplate: 'da7be1a7318f15b915f5aae8eb638797a8a24a31e5fc7fc0a2bad01bba137686', getOpsxVerifyCommandTemplate: 'f01c0c0cef53be0956de52363d955d4ace131b1b2d77adf902f35fead9a1486d', getOpsxProposeSkillTemplate: '57fb556a060e2eb246b500922837af7573a6e100a6ed7dfaa7bd4ce0f5daffd3', getOpsxProposeCommandTemplate: '434cae3ee20835725bb1d2ccb9698310a850c5b95ed669ea15fc7a0125371c59', getFeedbackSkillTemplate: 'd7d83c5f7fc2b92fe8f4588a5bf2d9cb315e4c73ec19bcd5ef28270906319a0d', - getUpdateChangeSkillTemplate: 'a30e5bc2ce1e6ba97db22fd7773797ef1760309ee9f4fc28ca46e63486b5e9dd', - getOpsxUpdateCommandTemplate: 'd4eafd808ad614b7d3f188cbe8d8c5fff36504fd63f9b2903dc7aa6fc0f1201d', + getUpdateChangeSkillTemplate: 'd885847ea1af48a2ef41a08f6319888d058d50b81cf5511bda768cd4b59359ee', + getOpsxUpdateCommandTemplate: 'cf43a6bdcdc549180970ddde40893223493a55e171a39290731e0339df530975', }; const EXPECTED_GENERATED_SKILL_CONTENT_HASHES: Record = { 'openspec-explore': 'c8de6033b2c78009647647c65a504e4ada1a3bdcee31aed38a4bf7d629513f6e', 'openspec-new-change': 'd5b8909bea70a33b7a312b38ce204a91f40b6bb2bff12c4c06b3e11641b6a689', 'openspec-continue-change': '02ec4de061ad6277866b877497a1e66142ba364e12b83dd7dedb838579ea88db', - 'openspec-apply-change': '09c0e1cdf5ccc82416d0969d6bd715cc70616bdbc3531358a5c36057f78be55a', + 'openspec-apply-change': '2f7a8e7a7528d9f8d89b508a8cbc909ba47bdff473db19317008d156b9ba5893', 'openspec-ff-change': 'ff3bd3eac427a1e50071ad7c70f73b556cffa3db43e90da2726e96849c3fc886', 'openspec-sync-specs': '74de778dd8a8fd4987a09621147358cc32505bb58110492ab2b4ffe7f35aa48f', 'openspec-archive-change': '64b1611dd7aee04ca268820d1b193e8bf0a39ff3672ec6ba21fb0a1bcb1786c2', 'openspec-bulk-archive-change': '49d410bda408c0411decd584be9c2355335e3b3db760fc6a0adcd82c172a280f', 'openspec-verify-change': '57693d22940f06080c6cf8d590ac2f48240d4a5e9ce7074dacd0f8d3c9945afa', - 'openspec-onboard': '76225d10352454a304e56566997811d16f91de1b37653816f2bc5d8ec976febc', + 'openspec-onboard': '656c0ab1492611ae0a2ffcdebc52d1cdaabfdec9d99612a289907f15c996f87a', 'openspec-propose': '4638400113946f4f1ee9f0bd0e965aafb200bd89b64ec7f5406ef5e948e8e218', - 'openspec-update-change': '6b37268bca94856d5533515762821274664b8dc9f2644b6c081ea6cc0205eda7', + 'openspec-update-change': '4e6669540bc5332b72db7dd432625cc4b45234ae7674f9b46fcd1309b9697b0d', }; // Intentionally excludes getFeedbackSkillTemplate: this list only models templates diff --git a/test/utils/command-references.test.ts b/test/utils/command-references.test.ts index 1f2367e517..8a9d7dced1 100644 --- a/test/utils/command-references.test.ts +++ b/test/utils/command-references.test.ts @@ -206,13 +206,15 @@ describe('getTransformerForTool', () => { } }); - it('selects hyphen commands for opencode, pi, and oh-my-pi when commands are generated', () => { - expect(getTransformerForTool('opencode', 'both', 'adapter-backed')).toBe(transformToHyphenCommands); - expect(getTransformerForTool('opencode', 'commands', 'adapter-backed')).toBe(transformToHyphenCommands); - expect(getTransformerForTool('pi', 'both', 'adapter-backed')).toBe(transformToHyphenCommands); - expect(getTransformerForTool('pi', 'commands', 'adapter-backed')).toBe(transformToHyphenCommands); - expect(getTransformerForTool('oh-my-pi', 'both', 'adapter-backed')).toBe(transformToHyphenCommands); - expect(getTransformerForTool('oh-my-pi', 'commands', 'adapter-backed')).toBe(transformToHyphenCommands); + it('selects hyphen commands for bob, oh-my-pi, opencode, pi, and qwen when commands are generated', () => { + // These tools invoke commands by filename (/opsx-), so skills must + // reference the hyphen form their command files actually answer to. + for (const toolId of ['bob', 'oh-my-pi', 'opencode', 'pi', 'qwen'] as const) { + expect(getTransformerForTool(toolId, 'both', 'adapter-backed')).toBe(transformToHyphenCommands); + expect(getTransformerForTool(toolId, 'commands', 'adapter-backed')).toBe(transformToHyphenCommands); + // ...but must not fall back to hyphen commands when no commands are generated + expect(getTransformerForTool(toolId, 'skills', 'adapter-backed')).toBe(transformToSkillReferences); + } }); it('selects no transformer for adapter-backed and skills-invocable tools when commands are generated', () => { diff --git a/test/utils/spec-discovery.test.ts b/test/utils/spec-discovery.test.ts index e8c18e75a5..1fb6713a77 100644 --- a/test/utils/spec-discovery.test.ts +++ b/test/utils/spec-discovery.test.ts @@ -108,6 +108,45 @@ describe('discoverSpecFiles', () => { }); }); + it('discovers a symlinked spec.md file', async () => { + await withTempDir(async (dir) => { + // hasAnyFileUnder and the artifact graph's globs both count a symlinked + // spec.md as content, so discovery must not silently drop it. + const target = path.join(dir, 'shared-delta.md'); + await fs.writeFile(target, '# Spec\n', 'utf8'); + await fs.mkdir(path.join(dir, 'auth'), { recursive: true }); + try { + await fs.symlink(target, path.join(dir, 'auth', 'spec.md'), 'file'); + } catch { + // Symlink creation can be unavailable (e.g. Windows without dev mode). + return; + } + + const found = await discoverSpecFiles(dir); + expect(found.map((s) => s.id)).toEqual(['auth']); + expect(found[0].specFile).toBe(path.join(dir, 'auth', 'spec.md')); + }); + }); + + it('skips a dangling spec.md symlink', async () => { + await withTempDir(async (dir) => { + await writeSpec(dir, 'real'); + await fs.mkdir(path.join(dir, 'ghost'), { recursive: true }); + try { + await fs.symlink( + path.join(dir, 'missing-target.md'), + path.join(dir, 'ghost', 'spec.md'), + 'file' + ); + } catch { + return; + } + + const found = await discoverSpecFiles(dir); + expect(found.map((s) => s.id)).toEqual(['real']); + }); + }); + it('does not follow symlinked directories', async () => { await withTempDir(async (dir) => { await writeSpec(dir, 'real'); From d5b55ccbb95b9aae576a832ca02bffebba2721b0 Mon Sep 17 00:00:00 2001 From: Clay Good Date: Thu, 23 Jul 2026 16:47:13 -0500 Subject: [PATCH 2/4] fix(archive): reject RENAMED+REMOVED conflicts, surface JSON warnings, skip no-op writes Adversarial-review round for #1437: - a delta that both RENAMEs and REMOVEs the same requirement is rejected explicitly by both validate and archive - the warn-and-continue REMOVED path would otherwise have masked the contradiction that previously failed incidentally at apply time - buildUpdatedSpec collects its warnings and archive --json carries them in a new optional `warnings` array, so agent flows see the same skipped-REMOVED signal humans get on stdout - archive skips rewriting a spec whose operations were all already synced, instead of churning normalization differences into the file (and no longer materializes an empty skeleton for a REMOVED-only new spec) - init's getting-started hint uses each tool's real invocation form (/opsx-propose for qwen/bob/opencode/pi/oh-my-pi) - onboard's pause guidance names the CLI fallback when /opsx:continue is not installed (CodeRabbit) - openspec-conventions spec updated to state the idempotent archive semantics; changeset added Co-Authored-By: Claude Fable 5 --- .changeset/archive-early-synced-removed.md | 5 ++ openspec/specs/openspec-conventions/spec.md | 6 +- skills/openspec-onboard/SKILL.md | 2 +- src/core/archive.ts | 21 +++++- src/core/init.ts | 9 ++- src/core/specs-apply.ts | 71 ++++++++++-------- src/core/templates/workflows/onboard.ts | 2 +- src/core/validation/validator.ts | 3 + test/core/archive.test.ts | 75 +++++++++++++++++-- test/core/init.test.ts | 16 ++++ .../templates/skill-templates-parity.test.ts | 6 +- test/core/validation.test.ts | 28 +++++++ 12 files changed, 196 insertions(+), 48 deletions(-) create mode 100644 .changeset/archive-early-synced-removed.md diff --git a/.changeset/archive-early-synced-removed.md b/.changeset/archive-early-synced-removed.md new file mode 100644 index 0000000000..ed99233d0a --- /dev/null +++ b/.changeset/archive-early-synced-removed.md @@ -0,0 +1,5 @@ +--- +'@fission-ai/openspec': patch +--- + +`openspec archive` no longer aborts when a REMOVED delta's requirement is already gone from the main spec (the early-sync pattern the sync skill teaches): it warns, treats the removal as already applied, and reports applied-only totals. In `--json` mode those warnings are carried in a new optional `warnings` array on the archive result. When every operation for a spec was already synced, archive skips rewriting that file instead of churning normalization differences into it. A delta that both RENAMEs and REMOVEs the same requirement is now rejected explicitly, by both `validate` and `archive`. Also fixed: the archive delta gate matches section headers case-insensitively like the parser; symlinked `specs//spec.md` files are discovered instead of silently dropped; `openspec show ` no longer prints a spurious "scenarios" flag warning; qwen and bob generated files reference commands by their real hyphenated names (`/opsx-`), and init's getting-started hint follows suit; apply/update/onboard guidance names the CLI fallback for profiles that don't install `/opsx:continue` or `/opsx:new`. diff --git a/openspec/specs/openspec-conventions/spec.md b/openspec/specs/openspec-conventions/spec.md index b47a98eb3e..ab81e8dfbc 100644 --- a/openspec/specs/openspec-conventions/spec.md +++ b/openspec/specs/openspec-conventions/spec.md @@ -183,8 +183,10 @@ The archive process SHALL programmatically apply delta changes to current specif 2. Parse REMOVED sections and remove by normalized header match 3. Parse MODIFIED sections and replace by normalized header match (using new names if renamed) 4. Parse ADDED sections and append new requirements -- **AND** validate that all MODIFIED/REMOVED headers exist in current spec -- **AND** validate that ADDED headers don't already exist +- **AND** validate that all MODIFIED headers exist in current spec +- **AND** treat a REMOVED header that is already absent as already removed (warn and continue; a REMOVED header that names the FROM side of a RENAMED in the same delta is a conflict) +- **AND** treat an ADDED header that already exists with identical content as already synced (differing content is a conflict) +- **AND** treat a RENAMED whose source is gone but target present as already synced - **AND** generate the updated spec in the main specs/ directory #### Scenario: Handling conflicts during archive diff --git a/skills/openspec-onboard/SKILL.md b/skills/openspec-onboard/SKILL.md index 509905985e..9a4a3887f0 100644 --- a/skills/openspec-onboard/SKILL.md +++ b/skills/openspec-onboard/SKILL.md @@ -503,7 +503,7 @@ If the user says they need to stop, want to pause, or seem disengaged: No problem! Your change is saved at the `changeRoot` reported by `openspec status --change "" --json`. To pick up where we left off later: -- `/openspec-continue-change ` - Resume artifact creation (if installed) +- `/openspec-continue-change ` - Resume artifact creation (if installed; otherwise `openspec status --change "" --json` shows the next artifact) - `/openspec-apply-change ` - Jump to implementation (if tasks exist) The work won't be lost. Come back whenever you're ready. diff --git a/src/core/archive.ts b/src/core/archive.ts index 95c738aa03..1fab2b2fd3 100644 --- a/src/core/archive.ts +++ b/src/core/archive.ts @@ -74,6 +74,8 @@ interface ArchiveResult { path: string; specsUpdated: boolean; totals?: { added: number; modified: number; removed: number; renamed: number }; + /** Non-blocking spec-merge warnings (e.g. a REMOVED requirement that was already gone). */ + warnings?: string[]; } /** @@ -426,6 +428,7 @@ export class ArchiveCommand { // Handle spec updates unless skipSpecs flag is set let specsUpdated = false; let totals: ArchiveResult['totals']; + const specWarnings: string[] = []; if (options.skipSpecs) { if (!json) { console.log('Skipping spec updates (--skip-specs flag provided).'); @@ -470,6 +473,9 @@ export class ArchiveCommand { for (const update of specUpdates) { const built = await buildUpdatedSpec(update, changeName!, { silent: json }); prepared.push({ update, rebuilt: built.rebuilt, counts: built.counts }); + // In JSON mode nothing was printed, so carry the warnings into + // the result instead of dropping them. + specWarnings.push(...built.warnings); } } catch (err: any) { if (json) { @@ -514,15 +520,21 @@ export class ArchiveCommand { // All validations passed; write files and display counts const writeTotals = { added: 0, modified: 0, removed: 0, renamed: 0 }; for (const p of prepared) { + const { added, modified, removed, renamed } = p.counts; + if (added + modified + removed + renamed === 0) { + // Every operation was already synced: rewriting the file would + // only churn normalization differences into it. + continue; + } await writeUpdatedSpec(p.update, p.rebuilt, p.counts, { silent: json, // Cross-root paths must be absolute when a store is selected. ...(isStoreSelectedRoot(root) ? { displayPath: p.update.target } : {}), }); - writeTotals.added += p.counts.added; - writeTotals.modified += p.counts.modified; - writeTotals.removed += p.counts.removed; - writeTotals.renamed += p.counts.renamed; + writeTotals.added += added; + writeTotals.modified += modified; + writeTotals.removed += removed; + writeTotals.renamed += renamed; } specsUpdated = true; totals = writeTotals; @@ -575,6 +587,7 @@ export class ArchiveCommand { path: archivePath, specsUpdated, ...(totals ? { totals } : {}), + ...(specWarnings.length > 0 ? { warnings: specWarnings } : {}), }; } diff --git a/src/core/init.ts b/src/core/init.ts index 48774602d7..faf83f658c 100644 --- a/src/core/init.ts +++ b/src/core/init.ts @@ -897,7 +897,14 @@ export class InitCommand { for (const tool of successfulTools) { let hint: string; if (shouldGenerateCommandsForTool(tool.value, activeDelivery)) { - hint = `Start your first change: ${command} "your idea"`; + // Tools that invoke commands by filename (bob, qwen, ...) need the + // hyphen form here too, not just inside generated bodies. + const transformer = getTransformerForTool( + tool.value, + activeDelivery, + resolveCommandSurfaceCapability(tool.value) + ); + hint = `Start your first change: ${transformer ? transformer(command) : command} "your idea"`; } else if (shouldGenerateSkillsForTool(tool.value, activeDelivery)) { hint = resolveCommandSurfaceCapability(tool.value) === 'skills-invocable' diff --git a/src/core/specs-apply.ts b/src/core/specs-apply.ts index b9e2dfce7f..78c2fb021b 100644 --- a/src/core/specs-apply.ts +++ b/src/core/specs-apply.ts @@ -84,7 +84,20 @@ export async function buildUpdatedSpec( update: SpecUpdate, changeName: string, options: { silent?: boolean } = {} -): Promise<{ rebuilt: string; counts: { added: number; modified: number; removed: number; renamed: number } }> { +): Promise<{ + rebuilt: string; + counts: { added: number; modified: number; removed: number; renamed: number }; + warnings: string[]; +}> { + // Collected so silent (JSON) callers can surface them; printed live for + // human callers at the point they occur. + const warnings: string[] = []; + const warn = (message: string): void => { + warnings.push(message); + if (!options.silent) { + console.log(chalk.yellow(`⚠️ Warning: ${message}`)); + } + }; // Read change spec content (delta-format expected) const changeContent = await fs.readFile(update.source, 'utf-8'); @@ -155,6 +168,15 @@ export async function buildUpdatedSpec( for (const { from, to } of plan.renamed) { const fromNorm = normalizeRequirementName(from); const toNorm = normalizeRequirementName(to); + // A REMOVED naming the FROM side contradicts the rename. This used to + // fail incidentally at apply time (the rename consumed the old header, + // so REMOVED hit "not found"); now that a missing REMOVED target is a + // no-op, the conflict must be rejected explicitly. + if (removedNamesSet.has(fromNorm)) { + throw new Error( + `${specName} validation failed - requirement present in multiple sections (RENAMED and REMOVED) for header "### Requirement: ${from}"` + ); + } if (modifiedNames.has(fromNorm)) { throw new Error( `${specName} validation failed - when a rename exists, MODIFIED must reference the NEW header "### Requirement: ${to}"` @@ -193,14 +215,12 @@ export async function buildUpdatedSpec( // 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) { + if (deltaPurpose) { 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.` - ) + warn( + `${specName} - delta Purpose ignored; ${specName} already has one. ` + + `Edit ${update.target} directly to change it.` ); } } @@ -213,11 +233,9 @@ export async function buildUpdatedSpec( ); } // Warn about REMOVED requirements being ignored for new specs - if (plan.removed.length > 0 && !options.silent) { - console.log( - chalk.yellow( - `⚠️ Warning: ${specName} - ${plan.removed.length} REMOVED requirement(s) ignored for new spec (nothing to remove).` - ) + if (plan.removed.length > 0) { + warn( + `${specName} - ${plan.removed.length} REMOVED requirement(s) ignored for new spec (nothing to remove).` ); } isNewSpec = true; @@ -227,22 +245,16 @@ export async function buildUpdatedSpec( // 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( - chalk.yellow( - `⚠️ Warning: ${specName} - delta Purpose ignored (it would leave the new spec unreadable); wrote the placeholder Purpose instead.` - ) - ); - } - } else if (overview && overview.length < MIN_PURPOSE_LENGTH && !options.silent) { + warn( + `${specName} - delta Purpose ignored (it would leave the new spec unreadable); wrote the placeholder Purpose instead.` + ); + } else if (overview && overview.length < MIN_PURPOSE_LENGTH) { // 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; ` + - `openspec validate --strict reports it as too brief.` - ) + warn( + `${specName} - carried Purpose is under ${MIN_PURPOSE_LENGTH} characters; ` + + `openspec validate --strict reports it as too brief.` ); } } @@ -306,11 +318,9 @@ export async function buildUpdatedSpec( // failure. Unlike RENAMED there is no signal separating that from a // mistyped header, so warn instead of skipping silently. // For new specs the skip was already warned about above. - if (!isNewSpec && !options.silent) { - console.log( - chalk.yellow( - `⚠️ Warning: ${specName} - REMOVED requirement "${name}" is not in the current spec; treating it as already removed.` - ) + if (!isNewSpec) { + warn( + `${specName} - REMOVED requirement "${name}" is not in the current spec; treating it as already removed.` ); } continue; @@ -399,6 +409,7 @@ export async function buildUpdatedSpec( removed: removedApplied, renamed: renamedApplied, }, + warnings, }; } diff --git a/src/core/templates/workflows/onboard.ts b/src/core/templates/workflows/onboard.ts index c52e856027..82a54395c0 100644 --- a/src/core/templates/workflows/onboard.ts +++ b/src/core/templates/workflows/onboard.ts @@ -513,7 +513,7 @@ If the user says they need to stop, want to pause, or seem disengaged: No problem! Your change is saved at the \`changeRoot\` reported by \`openspec status --change "" --json\`. To pick up where we left off later: -- \`/opsx:continue \` - Resume artifact creation (if installed) +- \`/opsx:continue \` - Resume artifact creation (if installed; otherwise \`openspec status --change "" --json\` shows the next artifact) - \`/opsx:apply \` - Jump to implementation (if tasks exist) The work won't be lost. Come back whenever you're ready. diff --git a/src/core/validation/validator.ts b/src/core/validation/validator.ts index 4b59ed6cb1..802e1f9ec5 100644 --- a/src/core/validation/validator.ts +++ b/src/core/validation/validator.ts @@ -318,6 +318,9 @@ export class Validator { if (addedNames.has(toKey)) { issues.push({ level: 'ERROR', path: entryPath, message: `RENAMED TO collides with ADDED for "${to}"` }); } + if (removedNames.has(fromKey)) { + issues.push({ level: 'ERROR', path: entryPath, message: `Requirement present in both RENAMED and REMOVED: "${from}"` }); + } } } } catch { diff --git a/test/core/archive.test.ts b/test/core/archive.test.ts index 180b04744f..17cacb34a8 100644 --- a/test/core/archive.test.ts +++ b/test/core/archive.test.ts @@ -392,6 +392,37 @@ Then expected result happens`; expect(untouched).toBe(mainSpecContent); }); + it('should abort when REMOVED names the FROM side of a RENAMED in the same delta', async () => { + // Contradictory delta: you cannot both rename and remove the same + // requirement. This used to fail incidentally at apply time (the rename + // consumed the old header, so REMOVED hit "not found"); now that a + // missing REMOVED target is treated as already synced, the conflict has + // to be rejected explicitly. + const changeName = 'rename-and-remove'; + const changeDir = path.join(tempDir, 'openspec', 'changes', changeName); + const changeSpecDir = path.join(changeDir, 'specs', 'core-layer'); + await fs.mkdir(changeSpecDir, { recursive: true }); + + await fs.writeFile( + path.join(changeSpecDir, 'spec.md'), + `# Core Layer - Changes\n\n## RENAMED Requirements\n\n- FROM: \`### Requirement: Old name\`\n- TO: \`### Requirement: New name\`\n\n## REMOVED Requirements\n\n### Requirement: Old name\n` + ); + + const mainSpecDir = path.join(tempDir, 'openspec', 'specs', 'core-layer'); + await fs.mkdir(mainSpecDir, { recursive: true }); + const mainSpecContent = `# core-layer Specification\n\n## Purpose\nCore abstraction layer.\n\n## Requirements\n\n### Requirement: Old name\n\n#### Scenario: Works\n- **WHEN** it runs\n- **THEN** it works\n`; + await fs.writeFile(path.join(mainSpecDir, 'spec.md'), mainSpecContent); + + await archiveCommand.execute(changeName, { yes: true, noValidate: true }); + + expect(console.log).toHaveBeenCalledWith( + expect.stringContaining('requirement present in multiple sections (RENAMED and REMOVED) for header "### Requirement: Old name"') + ); + expect(process.exitCode).toBe(1); + const untouched = await fs.readFile(path.join(mainSpecDir, 'spec.md'), 'utf-8'); + expect(untouched).toBe(mainSpecContent); + }); + it('should archive when REMOVED requirements were already synced to the baseline', async () => { const changeName = 'early-synced-removal'; const changeDir = path.join(tempDir, 'openspec', 'changes', changeName); @@ -407,10 +438,8 @@ Then expected result happens`; const keptBlock = `### Requirement: The system SHALL provide a core abstraction layer\n\n#### Scenario: Layer is available\n- **WHEN** a consumer imports the layer\n- **THEN** the abstraction is available`; const mainSpecDir = path.join(tempDir, 'openspec', 'specs', 'core-layer'); await fs.mkdir(mainSpecDir, { recursive: true }); - await fs.writeFile( - path.join(mainSpecDir, 'spec.md'), - `# core-layer Specification\n\n## Purpose\nCore abstraction layer.\n\n## Requirements\n\n${keptBlock}\n` - ); + const mainSpecContent = `# core-layer Specification\n\n## Purpose\nCore abstraction layer.\n\n## Requirements\n\n${keptBlock}\n`; + await fs.writeFile(path.join(mainSpecDir, 'spec.md'), mainSpecContent); await archiveCommand.execute(changeName, { yes: true, noValidate: true }); @@ -420,15 +449,49 @@ Then expected result happens`; ); // The skipped removal is not reported as applied expect(console.log).not.toHaveBeenCalledWith(expect.stringContaining('- 1 removed')); + // A no-op update must not churn the file with normalization differences const updatedContent = await fs.readFile(path.join(mainSpecDir, 'spec.md'), 'utf-8'); - expect(updatedContent).toContain('SHALL provide a core abstraction layer'); - expect(updatedContent).not.toContain('SHALL provide a legacy layer'); + expect(updatedContent).toBe(mainSpecContent); const archives = await fs.readdir(path.join(tempDir, 'openspec', 'changes', 'archive')); expect(archives.some(a => a.includes(changeName))).toBe(true); expect(process.exitCode).toBeUndefined(); }); + it('should surface the skipped REMOVED as a warning in --json output', async () => { + const changeName = 'early-synced-removal-json'; + const changeDir = path.join(tempDir, 'openspec', 'changes', changeName); + const changeSpecDir = path.join(changeDir, 'specs', 'core-layer'); + await fs.mkdir(changeSpecDir, { recursive: true }); + + await fs.writeFile( + path.join(changeSpecDir, 'spec.md'), + `# Core Layer - Changes\n\n## REMOVED Requirements\n\n### Requirement: The system SHALL provide a legacy layer\n**Reason**: Replaced.\n` + ); + + const keptBlock = `### Requirement: The system SHALL provide a core abstraction layer\n\n#### Scenario: Layer is available\n- **WHEN** a consumer imports the layer\n- **THEN** the abstraction is available`; + const mainSpecDir = path.join(tempDir, 'openspec', 'specs', 'core-layer'); + await fs.mkdir(mainSpecDir, { recursive: true }); + await fs.writeFile( + path.join(mainSpecDir, 'spec.md'), + `# core-layer Specification\n\n## Purpose\nCore abstraction layer.\n\n## Requirements\n\n${keptBlock}\n` + ); + + await archiveCommand.execute(changeName, { yes: true, noValidate: true, json: true }); + + expect(process.exitCode).toBeUndefined(); + const logCalls = (console.log as unknown as { mock: { calls: unknown[][] } }).mock.calls.flat().map(String); + const jsonLine = logCalls.find((entry) => entry.trimStart().startsWith('{')); + expect(jsonLine).toBeDefined(); + const parsed = JSON.parse(jsonLine!); + expect(parsed.archive.totals.removed).toBe(0); + // The silent path must not swallow the skip: agents reading JSON get + // the same signal humans get on stdout. + expect(parsed.archive.warnings).toEqual([ + expect.stringContaining('REMOVED requirement "The system SHALL provide a legacy layer" is not in the current spec'), + ]); + }); + it('should merge nested delta specs into the same relative path (#1353)', async () => { const changeName = 'nested-spec-feature'; const changeDir = path.join(tempDir, 'openspec', 'changes', changeName); diff --git a/test/core/init.test.ts b/test/core/init.test.ts index 492a23c6a4..3b4b5570a9 100644 --- a/test/core/init.test.ts +++ b/test/core/init.test.ts @@ -1064,6 +1064,22 @@ describe('InitCommand - profile and detection features', () => { } }); + it('should print the hyphen command hint for filename-invoked tools (claude+qwen)', async () => { + const initCommand = new InitCommand({ tools: 'claude,qwen', force: true }); + await initCommand.execute(testDir); + + const logCalls = (console.log as unknown as { mock: { calls: unknown[][] } }).mock.calls.flat().map(String); + const startHints = logCalls.filter((entry) => entry.includes('Start your first change')); + // Qwen invokes commands by filename (/opsx-propose), so it must not share + // Claude's /opsx:propose line + expect(startHints).toHaveLength(2); + const claudeHint = startHints.find((entry) => entry.includes('Claude Code')); + const qwenHint = startHints.find((entry) => entry.includes('Qwen Code')); + expect(claudeHint).toContain('/opsx:propose'); + expect(qwenHint).toContain('/opsx-propose'); + expect(qwenHint).not.toContain('/opsx:propose'); + }); + it('should not advertise an instruction for a tool that got no skills (delivery=commands, codex+kimi)', async () => { saveGlobalConfig({ featureFlags: {}, diff --git a/test/core/templates/skill-templates-parity.test.ts b/test/core/templates/skill-templates-parity.test.ts index 7a1c7f7f97..a9b2e6fdb2 100644 --- a/test/core/templates/skill-templates-parity.test.ts +++ b/test/core/templates/skill-templates-parity.test.ts @@ -43,7 +43,7 @@ const EXPECTED_FUNCTION_HASHES: Record = { getApplyChangeSkillTemplate: '3d52b852f3c5f87c3c88aeb4915c78604d97cc75d33aaac8f7e174d365b49971', getFfChangeSkillTemplate: '097a9ff9533900f227cac0523289eae4e19f06a081e5f355a8374dbecf3ff55d', getSyncSpecsSkillTemplate: '8a0e6a41250d9e5f893dd016c375ffb5773823693cb4e481ca74775bbfb9bfb9', - getOnboardSkillTemplate: '60a9df019a86c576bce9f26725925ea1324542cf6cb1e081b68eb0fe4285095a', + getOnboardSkillTemplate: 'f9988a9ef9ab7c09a16f64847902b2a082499f8a2d5c0533856cefb1d68f2318', getOpsxExploreCommandTemplate: 'eef1f8b4fd90ade6d70be46f0f8c3e6722f221fed175a6f9cf626287ef504a94', getOpsxNewCommandTemplate: '57c600cce318d16b9b4308a18d0d983ea3c0673034e606a7cceec07b4c705e87', getOpsxContinueCommandTemplate: '5c3968174001c20737ba39d2473ecec0f3b76591a80f7e2fc3974904d3da9dcd', @@ -54,7 +54,7 @@ const EXPECTED_FUNCTION_HASHES: Record = { getOpsxSyncCommandTemplate: 'df0240a79f7b4943a54c7413ab088ee48f5bf5fe19f9347c170d695c8ec777a4', getVerifyChangeSkillTemplate: 'cab4db01b5d2b1243d63d90c53747d8b39e488c60f76eba3fe8b994467f69267', getOpsxArchiveCommandTemplate: '7dea65d0e2e17db366bb666ba6ae5e205ea02707b8c5c7707565200875c78916', - getOpsxOnboardCommandTemplate: 'b1e8e48a7588ced934a6397460597a497009a75d97304124ea763168abe9cfec', + getOpsxOnboardCommandTemplate: '16a68b8c9819e2a7bab013c3b49a3e49ea258b68c4e7f47f0d598e30815e0a80', getOpsxBulkArchiveCommandTemplate: 'da7be1a7318f15b915f5aae8eb638797a8a24a31e5fc7fc0a2bad01bba137686', getOpsxVerifyCommandTemplate: 'f01c0c0cef53be0956de52363d955d4ace131b1b2d77adf902f35fead9a1486d', getOpsxProposeSkillTemplate: '57fb556a060e2eb246b500922837af7573a6e100a6ed7dfaa7bd4ce0f5daffd3', @@ -74,7 +74,7 @@ const EXPECTED_GENERATED_SKILL_CONTENT_HASHES: Record = { 'openspec-archive-change': '64b1611dd7aee04ca268820d1b193e8bf0a39ff3672ec6ba21fb0a1bcb1786c2', 'openspec-bulk-archive-change': '49d410bda408c0411decd584be9c2355335e3b3db760fc6a0adcd82c172a280f', 'openspec-verify-change': '57693d22940f06080c6cf8d590ac2f48240d4a5e9ce7074dacd0f8d3c9945afa', - 'openspec-onboard': '656c0ab1492611ae0a2ffcdebc52d1cdaabfdec9d99612a289907f15c996f87a', + 'openspec-onboard': '1d581c12d4928d751eb79de099e275dabe9c99fc15dc1f502abebd99ad7cb7d2', 'openspec-propose': '4638400113946f4f1ee9f0bd0e965aafb200bd89b64ec7f5406ef5e948e8e218', 'openspec-update-change': '4e6669540bc5332b72db7dd432625cc4b45234ae7674f9b46fcd1309b9697b0d', }; diff --git a/test/core/validation.test.ts b/test/core/validation.test.ts index a443e4ab0d..d04c49361e 100644 --- a/test/core/validation.test.ts +++ b/test/core/validation.test.ts @@ -446,6 +446,34 @@ Then result`; }); describe('validateChangeDeltaSpecs with metadata', () => { + it('rejects a delta that both renames and removes the same requirement', async () => { + // Parity with archive: apply-time rejects this contradiction, so + // validate must flag it too instead of reporting the change as valid. + const changeDir = path.join(testDir, 'rename-remove-conflict'); + const specsDir = path.join(changeDir, 'specs', 'test-spec'); + await fs.mkdir(specsDir, { recursive: true }); + + const deltaSpec = `# Test Spec + +## RENAMED Requirements + +- FROM: \`### Requirement: Old name\` +- TO: \`### Requirement: New name\` + +## REMOVED Requirements + +### Requirement: Old name`; + + await fs.writeFile(path.join(specsDir, 'spec.md'), deltaSpec); + + const validator = new Validator(true); + const report = await validator.validateChangeDeltaSpecs(changeDir); + + expect(report.valid).toBe(false); + const msg = report.issues.map((i) => i.message).join('\n'); + expect(msg).toContain('Requirement present in both RENAMED and REMOVED: "Old name"'); + }); + it('should validate requirement with metadata before SHALL/MUST text', async () => { const changeDir = path.join(testDir, 'test-change'); const specsDir = path.join(changeDir, 'specs', 'test-spec'); From 74a1026a4d46e3f38b1d85adeb7cc5f853af8448 Mon Sep 17 00:00:00 2001 From: Clay Good Date: Thu, 23 Jul 2026 17:13:21 -0500 Subject: [PATCH 3/4] fix(archive): abort on near-miss REMOVED typos, honest specsUpdated for no-op archives Round-2 adversarial review for #1437: - a REMOVED header that differs only in case or interior whitespace from an existing requirement is a typo, not an early sync - it stays a hard abort naming the near-miss, instead of degrading to warn-and-continue - specsUpdated is true only when a spec file was actually written; a fully-already-synced change prints "Specs already in sync; no files changed." and reports specsUpdated: false in JSON (CodeRabbit) - agent-contract documents the archive warnings field and specsUpdated semantics; changeset wording fixed (CodeRabbit) Co-Authored-By: Claude Fable 5 --- .changeset/archive-early-synced-removed.md | 2 +- docs/agent-contract.md | 2 +- openspec/specs/openspec-conventions/spec.md | 2 +- src/core/archive.ts | 14 ++++++--- src/core/specs-apply.ts | 16 ++++++++-- test/core/archive.test.ts | 34 +++++++++++++++++++++ 6 files changed, 61 insertions(+), 9 deletions(-) diff --git a/.changeset/archive-early-synced-removed.md b/.changeset/archive-early-synced-removed.md index ed99233d0a..6099dc48f2 100644 --- a/.changeset/archive-early-synced-removed.md +++ b/.changeset/archive-early-synced-removed.md @@ -2,4 +2,4 @@ '@fission-ai/openspec': patch --- -`openspec archive` no longer aborts when a REMOVED delta's requirement is already gone from the main spec (the early-sync pattern the sync skill teaches): it warns, treats the removal as already applied, and reports applied-only totals. In `--json` mode those warnings are carried in a new optional `warnings` array on the archive result. When every operation for a spec was already synced, archive skips rewriting that file instead of churning normalization differences into it. A delta that both RENAMEs and REMOVEs the same requirement is now rejected explicitly, by both `validate` and `archive`. Also fixed: the archive delta gate matches section headers case-insensitively like the parser; symlinked `specs//spec.md` files are discovered instead of silently dropped; `openspec show ` no longer prints a spurious "scenarios" flag warning; qwen and bob generated files reference commands by their real hyphenated names (`/opsx-`), and init's getting-started hint follows suit; apply/update/onboard guidance names the CLI fallback for profiles that don't install `/opsx:continue` or `/opsx:new`. +`openspec archive` no longer aborts when a REMOVED delta's requirement is already gone from the main spec (the early-sync pattern the sync skill teaches): it warns, treats the removal as already applied, and reports applied-only totals. In `--json` mode those warnings are carried in a new optional `warnings` array on the archive result. When every operation for a spec was already synced, archive skips rewriting that file instead of churning normalization differences into it. A delta that both RENAMEs and REMOVEs the same requirement is now rejected explicitly, by both `validate` and `archive`, and a REMOVED header that differs only in case or whitespace from an existing requirement still aborts (that is a typo, not an early sync). Also fixed: the archive delta gate matches section headers case-insensitively like the parser; symlinked `specs//spec.md` files are discovered instead of silently dropped; `openspec show ` no longer prints a spurious "scenarios" flag warning; files generated for qwen and bob reference commands by their real hyphenated names (`/opsx-`), and init's getting-started hint follows suit; apply/update/onboard guidance names the CLI fallback for profiles that don't install `/opsx:continue` or `/opsx:new`. diff --git a/docs/agent-contract.md b/docs/agent-contract.md index c2429d10eb..e88d3d4795 100644 --- a/docs/agent-contract.md +++ b/docs/agent-contract.md @@ -69,7 +69,7 @@ Change: `{ "id", "title", "deltaCount", "deltas": [...], "root" }`. Spec: `{ "id Success: `{ "change": { "id", "path", "metadataPath", "schema" }, "root" }`. Failure: `{ "change": null, "status": [d] }`, exit 1. ### 4.8 `archive --json` -Success: `{ "archive": { "change", "archivedAs": "YYYY-MM-DD-name", "path", "specsUpdated", "totals"? }, "root" }`. Failure: `{ "archive": null, "root"?, "status": [d] }`, exit 1. JSON mode is strictly non-interactive: every prompt point becomes an `archive_*` code. +Success: `{ "archive": { "change", "archivedAs": "YYYY-MM-DD-name", "path", "specsUpdated", "totals"?, "warnings"? }, "root" }`. Failure: `{ "archive": null, "root"?, "status": [d] }`, exit 1. `specsUpdated` is true only when at least one spec file was written; an already-synced change archives with all-zero totals and the skips listed in `warnings`. JSON mode is strictly non-interactive: every prompt point becomes an `archive_*` code. ### 4.9 `doctor --json` `{ "root": { "path", "source", "store_id"?, "healthy", "status": [] }, "store": { "id", "metadata": {present,valid,remote?}, "origin_url"?, "drift"?: {ahead,behind}, "status": [] } | null, "references": [...], "status": [] }`. `drift` (present only for a git-backed store checkout that has an upstream tracking ref) is ahead/behind counts against the last-fetched upstream, not the live remote. Health findings of any severity exit 0. Failure payload: `{ "root": null, "store": null, "references": [], "status": [d] }`, exit 1. diff --git a/openspec/specs/openspec-conventions/spec.md b/openspec/specs/openspec-conventions/spec.md index ab81e8dfbc..c8f847d929 100644 --- a/openspec/specs/openspec-conventions/spec.md +++ b/openspec/specs/openspec-conventions/spec.md @@ -184,7 +184,7 @@ The archive process SHALL programmatically apply delta changes to current specif 3. Parse MODIFIED sections and replace by normalized header match (using new names if renamed) 4. Parse ADDED sections and append new requirements - **AND** validate that all MODIFIED headers exist in current spec -- **AND** treat a REMOVED header that is already absent as already removed (warn and continue; a REMOVED header that names the FROM side of a RENAMED in the same delta is a conflict) +- **AND** treat a REMOVED header that is already absent as already removed (warn and continue; a REMOVED header that names the FROM side of a RENAMED in the same delta, or that differs only in case or whitespace from an existing requirement, is a conflict) - **AND** treat an ADDED header that already exists with identical content as already synced (differing content is a conflict) - **AND** treat a RENAMED whose source is gone but target present as already synced - **AND** generate the updated spec in the main specs/ directory diff --git a/src/core/archive.ts b/src/core/archive.ts index 1fab2b2fd3..f0f6013b3e 100644 --- a/src/core/archive.ts +++ b/src/core/archive.ts @@ -473,8 +473,8 @@ export class ArchiveCommand { for (const update of specUpdates) { const built = await buildUpdatedSpec(update, changeName!, { silent: json }); prepared.push({ update, rebuilt: built.rebuilt, counts: built.counts }); - // In JSON mode nothing was printed, so carry the warnings into - // the result instead of dropping them. + // Carried into the result so JSON mode (where nothing was + // printed) still surfaces them; human mode discards the result. specWarnings.push(...built.warnings); } } catch (err: any) { @@ -519,6 +519,7 @@ export class ArchiveCommand { // All validations passed; write files and display counts const writeTotals = { added: 0, modified: 0, removed: 0, renamed: 0 }; + let wroteAny = false; for (const p of prepared) { const { added, modified, removed, renamed } = p.counts; if (added + modified + removed + renamed === 0) { @@ -531,18 +532,23 @@ export class ArchiveCommand { // Cross-root paths must be absolute when a store is selected. ...(isStoreSelectedRoot(root) ? { displayPath: p.update.target } : {}), }); + wroteAny = true; writeTotals.added += added; writeTotals.modified += modified; writeTotals.removed += removed; writeTotals.renamed += renamed; } - specsUpdated = true; + specsUpdated = wroteAny; totals = writeTotals; if (!json) { console.log( `Totals: + ${writeTotals.added}, ~ ${writeTotals.modified}, - ${writeTotals.removed}, → ${writeTotals.renamed}` ); - console.log('Specs updated successfully.'); + console.log( + wroteAny + ? 'Specs updated successfully.' + : 'Specs already in sync; no files changed.' + ); } } } diff --git a/src/core/specs-apply.ts b/src/core/specs-apply.ts index 78c2fb021b..7ee2ae2928 100644 --- a/src/core/specs-apply.ts +++ b/src/core/specs-apply.ts @@ -315,10 +315,17 @@ export async function buildUpdatedSpec( if (!nameToBlock.has(key)) { // Requirement gone from the baseline means the removal was already // synced (early-sync pattern) — re-applying it is a no-op, not a - // failure. Unlike RENAMED there is no signal separating that from a - // mistyped header, so warn instead of skipping silently. + // failure. One signal does separate that from a mistyped header: a + // requirement that differs only in case or interior whitespace still + // being present. That is a typo, and stays a hard abort. // For new specs the skip was already warned about above. if (!isNewSpec) { + const nearMiss = [...nameToBlock.keys()].find((k) => foldRequirementName(k) === foldRequirementName(key)); + if (nearMiss !== undefined) { + throw new Error( + `${specName} REMOVED failed for header "### Requirement: ${name}" - not found, but "### Requirement: ${nameToBlock.get(nearMiss)!.name}" exists; fix the header to match it exactly` + ); + } warn( `${specName} - REMOVED requirement "${name}" is not in the current spec; treating it as already removed.` ); @@ -417,6 +424,11 @@ function normalizeBlockRaw(raw: string): string { return raw.replace(/\r\n?/g, '\n').trim(); } +/** Case- and whitespace-insensitive fold used only for near-miss detection. */ +function foldRequirementName(name: string): string { + return name.toLowerCase().replace(/\s+/g, ' '); +} + /** * Write an updated spec to disk. */ diff --git a/test/core/archive.test.ts b/test/core/archive.test.ts index 17cacb34a8..fb1b49357f 100644 --- a/test/core/archive.test.ts +++ b/test/core/archive.test.ts @@ -452,12 +452,44 @@ Then expected result happens`; // A no-op update must not churn the file with normalization differences const updatedContent = await fs.readFile(path.join(mainSpecDir, 'spec.md'), 'utf-8'); expect(updatedContent).toBe(mainSpecContent); + // ...and must not claim an update happened + expect(console.log).toHaveBeenCalledWith('Specs already in sync; no files changed.'); + expect(console.log).not.toHaveBeenCalledWith('Specs updated successfully.'); const archives = await fs.readdir(path.join(tempDir, 'openspec', 'changes', 'archive')); expect(archives.some(a => a.includes(changeName))).toBe(true); expect(process.exitCode).toBeUndefined(); }); + it('should abort when a REMOVED header near-misses an existing requirement (case/whitespace typo)', async () => { + // A fold-insensitive match in the current spec means the header is a + // typo, not an early-synced removal - that case must stay a hard abort. + const changeName = 'typo-removal'; + const changeDir = path.join(tempDir, 'openspec', 'changes', changeName); + const changeSpecDir = path.join(changeDir, 'specs', 'core-layer'); + await fs.mkdir(changeSpecDir, { recursive: true }); + + await fs.writeFile( + path.join(changeSpecDir, 'spec.md'), + `# Core Layer - Changes\n\n## REMOVED Requirements\n\n### Requirement: legacy layer\n**Reason**: Replaced.\n` + ); + + const mainSpecDir = path.join(tempDir, 'openspec', 'specs', 'core-layer'); + await fs.mkdir(mainSpecDir, { recursive: true }); + const mainSpecContent = `# core-layer Specification\n\n## Purpose\nCore abstraction layer.\n\n## Requirements\n\n### Requirement: Legacy Layer\n\n#### Scenario: Works\n- **WHEN** it runs\n- **THEN** it works\n`; + await fs.writeFile(path.join(mainSpecDir, 'spec.md'), mainSpecContent); + + await archiveCommand.execute(changeName, { yes: true, noValidate: true }); + + expect(console.log).toHaveBeenCalledWith( + expect.stringContaining('REMOVED failed for header "### Requirement: legacy layer" - not found, but "### Requirement: Legacy Layer" exists') + ); + expect(process.exitCode).toBe(1); + await expect(fs.access(changeDir)).resolves.not.toThrow(); + const untouched = await fs.readFile(path.join(mainSpecDir, 'spec.md'), 'utf-8'); + expect(untouched).toBe(mainSpecContent); + }); + it('should surface the skipped REMOVED as a warning in --json output', async () => { const changeName = 'early-synced-removal-json'; const changeDir = path.join(tempDir, 'openspec', 'changes', changeName); @@ -485,6 +517,8 @@ Then expected result happens`; expect(jsonLine).toBeDefined(); const parsed = JSON.parse(jsonLine!); expect(parsed.archive.totals.removed).toBe(0); + // No file was written, so the result must not claim an update + expect(parsed.archive.specsUpdated).toBe(false); // The silent path must not swallow the skip: agents reading JSON get // the same signal humans get on stdout. expect(parsed.archive.warnings).toEqual([ From 33ebba59b3eb12bfd967a0002ce252f83572f1c1 Mon Sep 17 00:00:00 2001 From: Clay Good Date: Thu, 23 Jul 2026 17:33:37 -0500 Subject: [PATCH 4/4] fix(archive): compare the RENAMED+REMOVED conflict case- and whitespace-insensitively Addresses alfred's review on #1437: `RENAMED FROM: Old Name` plus `REMOVED: old name` slipped past the exact-match cross-section guard, so validate passed, archive renamed the requirement, reported the removal as already synced, and archived the change. Both the validator and the apply-side guard now compare the two spellings with the shared foldRequirementName (lowercase, collapsed whitespace), and the error names the variant spelling when it differs. Focused regressions cover both paths; requirement matching everywhere else stays case-sensitive. Co-Authored-By: Claude Fable 5 --- .changeset/archive-early-synced-removed.md | 2 +- openspec/specs/openspec-conventions/spec.md | 2 +- src/core/parsers/requirement-blocks.ts | 11 ++++++++ src/core/specs-apply.ts | 18 +++++++------ src/core/validation/validator.ts | 17 +++++++++--- test/core/archive.test.ts | 27 +++++++++++++++++++ test/core/validation.test.ts | 29 +++++++++++++++++++++ 7 files changed, 93 insertions(+), 13 deletions(-) diff --git a/.changeset/archive-early-synced-removed.md b/.changeset/archive-early-synced-removed.md index 6099dc48f2..25ed4dba06 100644 --- a/.changeset/archive-early-synced-removed.md +++ b/.changeset/archive-early-synced-removed.md @@ -2,4 +2,4 @@ '@fission-ai/openspec': patch --- -`openspec archive` no longer aborts when a REMOVED delta's requirement is already gone from the main spec (the early-sync pattern the sync skill teaches): it warns, treats the removal as already applied, and reports applied-only totals. In `--json` mode those warnings are carried in a new optional `warnings` array on the archive result. When every operation for a spec was already synced, archive skips rewriting that file instead of churning normalization differences into it. A delta that both RENAMEs and REMOVEs the same requirement is now rejected explicitly, by both `validate` and `archive`, and a REMOVED header that differs only in case or whitespace from an existing requirement still aborts (that is a typo, not an early sync). Also fixed: the archive delta gate matches section headers case-insensitively like the parser; symlinked `specs//spec.md` files are discovered instead of silently dropped; `openspec show ` no longer prints a spurious "scenarios" flag warning; files generated for qwen and bob reference commands by their real hyphenated names (`/opsx-`), and init's getting-started hint follows suit; apply/update/onboard guidance names the CLI fallback for profiles that don't install `/opsx:continue` or `/opsx:new`. +`openspec archive` no longer aborts when a REMOVED delta's requirement is already gone from the main spec (the early-sync pattern the sync skill teaches): it warns, treats the removal as already applied, and reports applied-only totals. In `--json` mode those warnings are carried in a new optional `warnings` array on the archive result. When every operation for a spec was already synced, archive skips rewriting that file instead of churning normalization differences into it. A delta that both RENAMEs and REMOVEs the same requirement is now rejected explicitly, by both `validate` and `archive` — the two spellings are compared case- and whitespace-insensitively — and a REMOVED header that differs only in case or whitespace from an existing requirement still aborts (that is a typo, not an early sync). Also fixed: the archive delta gate matches section headers case-insensitively like the parser; symlinked `specs//spec.md` files are discovered instead of silently dropped; `openspec show ` no longer prints a spurious "scenarios" flag warning; files generated for qwen and bob reference commands by their real hyphenated names (`/opsx-`), and init's getting-started hint follows suit; apply/update/onboard guidance names the CLI fallback for profiles that don't install `/opsx:continue` or `/opsx:new`. diff --git a/openspec/specs/openspec-conventions/spec.md b/openspec/specs/openspec-conventions/spec.md index c8f847d929..85ad36d619 100644 --- a/openspec/specs/openspec-conventions/spec.md +++ b/openspec/specs/openspec-conventions/spec.md @@ -184,7 +184,7 @@ The archive process SHALL programmatically apply delta changes to current specif 3. Parse MODIFIED sections and replace by normalized header match (using new names if renamed) 4. Parse ADDED sections and append new requirements - **AND** validate that all MODIFIED headers exist in current spec -- **AND** treat a REMOVED header that is already absent as already removed (warn and continue; a REMOVED header that names the FROM side of a RENAMED in the same delta, or that differs only in case or whitespace from an existing requirement, is a conflict) +- **AND** treat a REMOVED header that is already absent as already removed (warn and continue; a REMOVED header that names the FROM side of a RENAMED in the same delta — compared case- and whitespace-insensitively — or that differs only in case or whitespace from an existing requirement, is a conflict) - **AND** treat an ADDED header that already exists with identical content as already synced (differing content is a conflict) - **AND** treat a RENAMED whose source is gone but target present as already synced - **AND** generate the updated spec in the main specs/ directory diff --git a/src/core/parsers/requirement-blocks.ts b/src/core/parsers/requirement-blocks.ts index 6bc4e15109..b47b9ecd45 100644 --- a/src/core/parsers/requirement-blocks.ts +++ b/src/core/parsers/requirement-blocks.ts @@ -18,6 +18,17 @@ export function normalizeRequirementName(name: string): string { return name.trim(); } +/** + * Case- and whitespace-insensitive fold of a requirement name. Requirement + * matching itself is case-sensitive (normalizeRequirementName); this fold + * exists only for typo detection - near-miss REMOVED headers and the + * RENAMED+REMOVED cross-section conflict - where two spellings that differ + * only in case or interior whitespace mean a mistake, never two requirements. + */ +export function foldRequirementName(name: string): string { + return normalizeRequirementName(name).toLowerCase().replace(/\s+/g, ' '); +} + /** The canonical requirement header the delta reader recognizes. */ const REQUIREMENT_HEADER_REGEX = /^###\s*Requirement:\s*(.+)\s*$/i; diff --git a/src/core/specs-apply.ts b/src/core/specs-apply.ts index 7ee2ae2928..db80ae8bf5 100644 --- a/src/core/specs-apply.ts +++ b/src/core/specs-apply.ts @@ -10,6 +10,7 @@ import path from 'path'; import chalk from 'chalk'; import { extractRequirementsSection, + foldRequirementName, parseDeltaSpec, normalizeRequirementName, type RequirementBlock, @@ -171,10 +172,16 @@ export async function buildUpdatedSpec( // A REMOVED naming the FROM side contradicts the rename. This used to // fail incidentally at apply time (the rename consumed the old header, // so REMOVED hit "not found"); now that a missing REMOVED target is a - // no-op, the conflict must be rejected explicitly. - if (removedNamesSet.has(fromNorm)) { + // no-op, the conflict must be rejected explicitly. Compared folded, so + // a case/whitespace variant cannot slip past the guard and degrade + // into a warned no-op. + const removedFoldMatch = [...removedNamesSet].find( + (r) => foldRequirementName(r) === foldRequirementName(fromNorm) + ); + if (removedFoldMatch !== undefined) { throw new Error( - `${specName} validation failed - requirement present in multiple sections (RENAMED and REMOVED) for header "### Requirement: ${from}"` + `${specName} validation failed - requirement present in multiple sections (RENAMED and REMOVED) for header "### Requirement: ${from}"` + + (removedFoldMatch === fromNorm ? '' : ` (REMOVED spells it "${removedFoldMatch}")`) ); } if (modifiedNames.has(fromNorm)) { @@ -424,11 +431,6 @@ function normalizeBlockRaw(raw: string): string { return raw.replace(/\r\n?/g, '\n').trim(); } -/** Case- and whitespace-insensitive fold used only for near-miss detection. */ -function foldRequirementName(name: string): string { - return name.toLowerCase().replace(/\s+/g, ' '); -} - /** * Write an updated spec to disk. */ diff --git a/src/core/validation/validator.ts b/src/core/validation/validator.ts index 802e1f9ec5..25989f86e2 100644 --- a/src/core/validation/validator.ts +++ b/src/core/validation/validator.ts @@ -10,7 +10,7 @@ import { MAX_REQUIREMENT_TEXT_LENGTH, VALIDATION_MESSAGES } from './constants.js'; -import { parseDeltaSpec, normalizeRequirementName, extractRequirementsSection } from '../parsers/requirement-blocks.js'; +import { parseDeltaSpec, foldRequirementName, normalizeRequirementName, extractRequirementsSection } from '../parsers/requirement-blocks.js'; import { extractRequirementBody as extractRequirementBodyShared, containsShallOrMust as containsShallOrMustShared, @@ -318,8 +318,19 @@ export class Validator { if (addedNames.has(toKey)) { issues.push({ level: 'ERROR', path: entryPath, message: `RENAMED TO collides with ADDED for "${to}"` }); } - if (removedNames.has(fromKey)) { - issues.push({ level: 'ERROR', path: entryPath, message: `Requirement present in both RENAMED and REMOVED: "${from}"` }); + // Folded comparison: a case/whitespace variant of the FROM header + // in REMOVED is the same contradiction, not a different name. + const removedFoldMatch = [...removedNames].find( + (r) => foldRequirementName(r) === foldRequirementName(fromKey) + ); + if (removedFoldMatch !== undefined) { + issues.push({ + level: 'ERROR', + path: entryPath, + message: + `Requirement present in both RENAMED and REMOVED: "${from}"` + + (removedFoldMatch === fromKey ? '' : ` (REMOVED spells it "${removedFoldMatch}")`), + }); } } } diff --git a/test/core/archive.test.ts b/test/core/archive.test.ts index fb1b49357f..fd5a5d3132 100644 --- a/test/core/archive.test.ts +++ b/test/core/archive.test.ts @@ -423,6 +423,33 @@ Then expected result happens`; expect(untouched).toBe(mainSpecContent); }); + it('should abort when REMOVED spells the renamed FROM header with different case', async () => { + const changeName = 'rename-and-remove-case'; + const changeDir = path.join(tempDir, 'openspec', 'changes', changeName); + const changeSpecDir = path.join(changeDir, 'specs', 'core-layer'); + await fs.mkdir(changeSpecDir, { recursive: true }); + + await fs.writeFile( + path.join(changeSpecDir, 'spec.md'), + `# Core Layer - Changes\n\n## RENAMED Requirements\n\n- FROM: \`### Requirement: Old Name\`\n- TO: \`### Requirement: New Name\`\n\n## REMOVED Requirements\n\n### Requirement: old name\n` + ); + + const mainSpecDir = path.join(tempDir, 'openspec', 'specs', 'core-layer'); + await fs.mkdir(mainSpecDir, { recursive: true }); + const mainSpecContent = `# core-layer Specification\n\n## Purpose\nCore abstraction layer.\n\n## Requirements\n\n### Requirement: Old Name\n\n#### Scenario: Works\n- **WHEN** it runs\n- **THEN** it works\n`; + await fs.writeFile(path.join(mainSpecDir, 'spec.md'), mainSpecContent); + + await archiveCommand.execute(changeName, { yes: true, noValidate: true }); + + expect(console.log).toHaveBeenCalledWith( + expect.stringContaining('requirement present in multiple sections (RENAMED and REMOVED) for header "### Requirement: Old Name" (REMOVED spells it "old name")') + ); + expect(process.exitCode).toBe(1); + await expect(fs.access(changeDir)).resolves.not.toThrow(); + const untouched = await fs.readFile(path.join(mainSpecDir, 'spec.md'), 'utf-8'); + expect(untouched).toBe(mainSpecContent); + }); + it('should archive when REMOVED requirements were already synced to the baseline', async () => { const changeName = 'early-synced-removal'; const changeDir = path.join(tempDir, 'openspec', 'changes', changeName); diff --git a/test/core/validation.test.ts b/test/core/validation.test.ts index d04c49361e..04c63d943e 100644 --- a/test/core/validation.test.ts +++ b/test/core/validation.test.ts @@ -474,6 +474,35 @@ Then result`; expect(msg).toContain('Requirement present in both RENAMED and REMOVED: "Old name"'); }); + it('rejects a case/whitespace variant of the renamed FROM header in REMOVED', async () => { + // The contradiction is the same when REMOVED spells the FROM header + // with different case or spacing - the folded identity must catch it. + const changeDir = path.join(testDir, 'rename-remove-case-conflict'); + const specsDir = path.join(changeDir, 'specs', 'test-spec'); + await fs.mkdir(specsDir, { recursive: true }); + + const deltaSpec = `# Test Spec + +## RENAMED Requirements + +- FROM: \`### Requirement: Old Name\` +- TO: \`### Requirement: New Name\` + +## REMOVED Requirements + +### Requirement: old name`; + + await fs.writeFile(path.join(specsDir, 'spec.md'), deltaSpec); + + const validator = new Validator(true); + const report = await validator.validateChangeDeltaSpecs(changeDir); + + expect(report.valid).toBe(false); + const msg = report.issues.map((i) => i.message).join('\n'); + expect(msg).toContain('Requirement present in both RENAMED and REMOVED: "Old Name"'); + expect(msg).toContain('(REMOVED spells it "old name")'); + }); + it('should validate requirement with metadata before SHALL/MUST text', async () => { const changeDir = path.join(testDir, 'test-change'); const specsDir = path.join(changeDir, 'specs', 'test-spec');