From c8fe7fe618b9db0a27a77eba41f56fe5e64405a9 Mon Sep 17 00:00:00 2001 From: Henry Su Date: Sun, 26 Jul 2026 11:23:27 -0500 Subject: [PATCH 1/6] fix(adapters): escape YAML frontmatter values consistently across all command adapters --- .../command-generation/adapters/amazon-q.ts | 3 +- .../adapters/antigravity.ts | 3 +- .../command-generation/adapters/auggie.ts | 3 +- src/core/command-generation/adapters/bob.ts | 7 ++- .../command-generation/adapters/claude.ts | 10 +--- .../command-generation/adapters/codebuddy.ts | 5 +- .../command-generation/adapters/continue.ts | 3 +- .../command-generation/adapters/costrict.ts | 3 +- src/core/command-generation/adapters/crush.ts | 10 ++-- .../command-generation/adapters/factory.ts | 3 +- .../adapters/github-copilot.ts | 3 +- src/core/command-generation/adapters/iflow.ts | 5 +- src/core/command-generation/adapters/junie.ts | 3 +- src/core/command-generation/adapters/kiro.ts | 3 +- .../command-generation/adapters/lingma.ts | 10 ++-- .../command-generation/adapters/opencode.ts | 3 +- src/core/command-generation/adapters/qoder.ts | 10 ++-- src/core/command-generation/adapters/qwen.ts | 16 +---- src/core/command-generation/adapters/trae.ts | 23 +------- .../command-generation/adapters/windsurf.ts | 10 +--- src/core/command-generation/adapters/zcode.ts | 24 +------- src/core/command-generation/yaml.ts | 14 +++++ test/core/command-generation/adapters.test.ts | 58 ++++++++++++++++++- 23 files changed, 120 insertions(+), 112 deletions(-) diff --git a/src/core/command-generation/adapters/amazon-q.ts b/src/core/command-generation/adapters/amazon-q.ts index 0131c0638f..c75bd2ee58 100644 --- a/src/core/command-generation/adapters/amazon-q.ts +++ b/src/core/command-generation/adapters/amazon-q.ts @@ -6,6 +6,7 @@ import path from 'path'; import type { CommandContent, ToolCommandAdapter } from '../types.js'; +import { escapeYamlValue } from '../yaml.js'; /** * Amazon Q adapter for command generation. @@ -21,7 +22,7 @@ export const amazonQAdapter: ToolCommandAdapter = { formatFile(content: CommandContent): string { return `--- -description: ${content.description} +description: ${escapeYamlValue(content.description)} --- ${content.body} diff --git a/src/core/command-generation/adapters/antigravity.ts b/src/core/command-generation/adapters/antigravity.ts index e7a5d4919d..b0c3035a52 100644 --- a/src/core/command-generation/adapters/antigravity.ts +++ b/src/core/command-generation/adapters/antigravity.ts @@ -6,6 +6,7 @@ import path from 'path'; import type { CommandContent, ToolCommandAdapter } from '../types.js'; +import { escapeYamlValue } from '../yaml.js'; /** * Antigravity adapter for command generation. @@ -21,7 +22,7 @@ export const antigravityAdapter: ToolCommandAdapter = { formatFile(content: CommandContent): string { return `--- -description: ${content.description} +description: ${escapeYamlValue(content.description)} --- ${content.body} diff --git a/src/core/command-generation/adapters/auggie.ts b/src/core/command-generation/adapters/auggie.ts index 2a52104c07..b790c04f51 100644 --- a/src/core/command-generation/adapters/auggie.ts +++ b/src/core/command-generation/adapters/auggie.ts @@ -6,6 +6,7 @@ import path from 'path'; import type { CommandContent, ToolCommandAdapter } from '../types.js'; +import { escapeYamlValue } from '../yaml.js'; /** * Auggie adapter for command generation. @@ -21,7 +22,7 @@ export const auggieAdapter: ToolCommandAdapter = { formatFile(content: CommandContent): string { return `--- -description: ${content.description} +description: ${escapeYamlValue(content.description)} argument-hint: command arguments --- diff --git a/src/core/command-generation/adapters/bob.ts b/src/core/command-generation/adapters/bob.ts index 8acb32bebc..3e81ded345 100644 --- a/src/core/command-generation/adapters/bob.ts +++ b/src/core/command-generation/adapters/bob.ts @@ -13,7 +13,11 @@ import { escapeYamlValue } from '../yaml.js'; /** * Bob Shell adapter for command generation. * File path: .bob/commands/opsx-.md - * Frontmatter: description, argument-hint + * Frontmatter: description + * + * Bob uses the filename (minus .md) as the slash command name, so + * opsx-propose.md → /opsx-propose. Command references in the body + * are transformed from /opsx: to /opsx- for consistency. */ export const bobAdapter: ToolCommandAdapter = { toolId: 'bob', @@ -23,7 +27,6 @@ export const bobAdapter: ToolCommandAdapter = { }, formatFile(content: CommandContent): string { - // Transform command references from colon to hyphen format for Bob const transformedBody = transformToHyphenCommands(content.body); return `--- diff --git a/src/core/command-generation/adapters/claude.ts b/src/core/command-generation/adapters/claude.ts index 6211195913..17a5f3b6bd 100644 --- a/src/core/command-generation/adapters/claude.ts +++ b/src/core/command-generation/adapters/claude.ts @@ -6,17 +6,9 @@ import path from 'path'; import type { CommandContent, ToolCommandAdapter } from '../types.js'; -import { escapeYamlValue } from '../yaml.js'; +import { escapeYamlValue, formatTagsArray } from '../yaml.js'; import { OPENSPEC_CLI_ALLOWED_TOOLS } from '../../shared/allowed-tools.js'; -/** - * Formats a tags array as a YAML array with proper escaping. - */ -function formatTagsArray(tags: string[]): string { - const escapedTags = tags.map((tag) => escapeYamlValue(tag)); - return `[${escapedTags.join(', ')}]`; -} - /** * Claude Code adapter for command generation. * File path: .claude/commands/opsx/.md diff --git a/src/core/command-generation/adapters/codebuddy.ts b/src/core/command-generation/adapters/codebuddy.ts index 54b7eebdcf..51657e7664 100644 --- a/src/core/command-generation/adapters/codebuddy.ts +++ b/src/core/command-generation/adapters/codebuddy.ts @@ -6,6 +6,7 @@ import path from 'path'; import type { CommandContent, ToolCommandAdapter } from '../types.js'; +import { escapeYamlValue } from '../yaml.js'; /** * CodeBuddy adapter for command generation. @@ -21,8 +22,8 @@ export const codebuddyAdapter: ToolCommandAdapter = { formatFile(content: CommandContent): string { return `--- -name: ${content.name} -description: "${content.description}" +name: ${escapeYamlValue(content.name)} +description: ${escapeYamlValue(content.description)} argument-hint: "[command arguments]" --- diff --git a/src/core/command-generation/adapters/continue.ts b/src/core/command-generation/adapters/continue.ts index f6aac08b00..cde510b1fc 100644 --- a/src/core/command-generation/adapters/continue.ts +++ b/src/core/command-generation/adapters/continue.ts @@ -6,6 +6,7 @@ import path from 'path'; import type { CommandContent, ToolCommandAdapter } from '../types.js'; +import { escapeYamlValue } from '../yaml.js'; /** * Continue adapter for command generation. @@ -22,7 +23,7 @@ export const continueAdapter: ToolCommandAdapter = { formatFile(content: CommandContent): string { return `--- name: opsx-${content.id} -description: ${content.description} +description: ${escapeYamlValue(content.description)} invokable: true --- diff --git a/src/core/command-generation/adapters/costrict.ts b/src/core/command-generation/adapters/costrict.ts index 17628a1241..82a4aea6bd 100644 --- a/src/core/command-generation/adapters/costrict.ts +++ b/src/core/command-generation/adapters/costrict.ts @@ -6,6 +6,7 @@ import path from 'path'; import type { CommandContent, ToolCommandAdapter } from '../types.js'; +import { escapeYamlValue } from '../yaml.js'; /** * CoStrict adapter for command generation. @@ -21,7 +22,7 @@ export const costrictAdapter: ToolCommandAdapter = { formatFile(content: CommandContent): string { return `--- -description: "${content.description}" +description: ${escapeYamlValue(content.description)} argument-hint: command arguments --- diff --git a/src/core/command-generation/adapters/crush.ts b/src/core/command-generation/adapters/crush.ts index b4d1a0b9dd..e1f3aae299 100644 --- a/src/core/command-generation/adapters/crush.ts +++ b/src/core/command-generation/adapters/crush.ts @@ -6,6 +6,7 @@ import path from 'path'; import type { CommandContent, ToolCommandAdapter } from '../types.js'; +import { escapeYamlValue, formatTagsArray } from '../yaml.js'; /** * Crush adapter for command generation. @@ -20,12 +21,11 @@ export const crushAdapter: ToolCommandAdapter = { }, formatFile(content: CommandContent): string { - const tagsStr = content.tags.join(', '); return `--- -name: ${content.name} -description: ${content.description} -category: ${content.category} -tags: [${tagsStr}] +name: ${escapeYamlValue(content.name)} +description: ${escapeYamlValue(content.description)} +category: ${escapeYamlValue(content.category)} +tags: ${formatTagsArray(content.tags)} --- ${content.body} diff --git a/src/core/command-generation/adapters/factory.ts b/src/core/command-generation/adapters/factory.ts index 5031d5dc79..383d36844f 100644 --- a/src/core/command-generation/adapters/factory.ts +++ b/src/core/command-generation/adapters/factory.ts @@ -6,6 +6,7 @@ import path from 'path'; import type { CommandContent, ToolCommandAdapter } from '../types.js'; +import { escapeYamlValue } from '../yaml.js'; /** * Factory adapter for command generation. @@ -21,7 +22,7 @@ export const factoryAdapter: ToolCommandAdapter = { formatFile(content: CommandContent): string { return `--- -description: ${content.description} +description: ${escapeYamlValue(content.description)} argument-hint: command arguments --- diff --git a/src/core/command-generation/adapters/github-copilot.ts b/src/core/command-generation/adapters/github-copilot.ts index 4eac7f1b69..cd71b87467 100644 --- a/src/core/command-generation/adapters/github-copilot.ts +++ b/src/core/command-generation/adapters/github-copilot.ts @@ -6,6 +6,7 @@ import path from 'path'; import type { CommandContent, ToolCommandAdapter } from '../types.js'; +import { escapeYamlValue } from '../yaml.js'; /** * GitHub Copilot adapter for command generation. @@ -21,7 +22,7 @@ export const githubCopilotAdapter: ToolCommandAdapter = { formatFile(content: CommandContent): string { return `--- -description: ${content.description} +description: ${escapeYamlValue(content.description)} --- ${content.body} diff --git a/src/core/command-generation/adapters/iflow.ts b/src/core/command-generation/adapters/iflow.ts index d60a3f0b1e..e92c14f247 100644 --- a/src/core/command-generation/adapters/iflow.ts +++ b/src/core/command-generation/adapters/iflow.ts @@ -6,6 +6,7 @@ import path from 'path'; import type { CommandContent, ToolCommandAdapter } from '../types.js'; +import { escapeYamlValue } from '../yaml.js'; /** * iFlow adapter for command generation. @@ -23,8 +24,8 @@ export const iflowAdapter: ToolCommandAdapter = { return `--- name: /opsx-${content.id} id: opsx-${content.id} -category: ${content.category} -description: ${content.description} +category: ${escapeYamlValue(content.category)} +description: ${escapeYamlValue(content.description)} --- ${content.body} diff --git a/src/core/command-generation/adapters/junie.ts b/src/core/command-generation/adapters/junie.ts index 907ca46982..69c0a53484 100644 --- a/src/core/command-generation/adapters/junie.ts +++ b/src/core/command-generation/adapters/junie.ts @@ -6,6 +6,7 @@ import path from 'path'; import type { CommandContent, ToolCommandAdapter } from '../types.js'; +import { escapeYamlValue } from '../yaml.js'; /** * Junie adapter for command generation. @@ -21,7 +22,7 @@ export const junieAdapter: ToolCommandAdapter = { formatFile(content: CommandContent): string { return `--- -description: ${content.description} +description: ${escapeYamlValue(content.description)} --- ${content.body} diff --git a/src/core/command-generation/adapters/kiro.ts b/src/core/command-generation/adapters/kiro.ts index 2e8a4ca4c5..8d52d47cc4 100644 --- a/src/core/command-generation/adapters/kiro.ts +++ b/src/core/command-generation/adapters/kiro.ts @@ -6,6 +6,7 @@ import path from 'path'; import type { CommandContent, ToolCommandAdapter } from '../types.js'; +import { escapeYamlValue } from '../yaml.js'; /** * Kiro adapter for command generation. @@ -21,7 +22,7 @@ export const kiroAdapter: ToolCommandAdapter = { formatFile(content: CommandContent): string { return `--- -description: ${content.description} +description: ${escapeYamlValue(content.description)} --- ${content.body} diff --git a/src/core/command-generation/adapters/lingma.ts b/src/core/command-generation/adapters/lingma.ts index cf9bcc88b2..e6e15ba1c1 100644 --- a/src/core/command-generation/adapters/lingma.ts +++ b/src/core/command-generation/adapters/lingma.ts @@ -6,6 +6,7 @@ import path from 'path'; import type { CommandContent, ToolCommandAdapter } from '../types.js'; +import { escapeYamlValue, formatTagsArray } from '../yaml.js'; /** * Lingma adapter for command generation. @@ -20,12 +21,11 @@ export const lingmaAdapter: ToolCommandAdapter = { }, formatFile(content: CommandContent): string { - const tagsStr = content.tags.join(', '); return `--- -name: ${content.name} -description: ${content.description} -category: ${content.category} -tags: [${tagsStr}] +name: ${escapeYamlValue(content.name)} +description: ${escapeYamlValue(content.description)} +category: ${escapeYamlValue(content.category)} +tags: ${formatTagsArray(content.tags)} --- ${content.body} diff --git a/src/core/command-generation/adapters/opencode.ts b/src/core/command-generation/adapters/opencode.ts index 301664b47f..15d88dfc02 100644 --- a/src/core/command-generation/adapters/opencode.ts +++ b/src/core/command-generation/adapters/opencode.ts @@ -7,6 +7,7 @@ import path from 'path'; import type { CommandContent, ToolCommandAdapter } from '../types.js'; import { transformToHyphenCommands } from '../../../utils/command-references.js'; +import { escapeYamlValue } from '../yaml.js'; /** * OpenCode adapter for command generation. @@ -25,7 +26,7 @@ export const opencodeAdapter: ToolCommandAdapter = { const transformedBody = transformToHyphenCommands(content.body); return `--- -description: ${content.description} +description: ${escapeYamlValue(content.description)} --- ${transformedBody} diff --git a/src/core/command-generation/adapters/qoder.ts b/src/core/command-generation/adapters/qoder.ts index 608fc9ae25..9fa78f9e3d 100644 --- a/src/core/command-generation/adapters/qoder.ts +++ b/src/core/command-generation/adapters/qoder.ts @@ -6,6 +6,7 @@ import path from 'path'; import type { CommandContent, ToolCommandAdapter } from '../types.js'; +import { escapeYamlValue, formatTagsArray } from '../yaml.js'; /** * Qoder adapter for command generation. @@ -20,12 +21,11 @@ export const qoderAdapter: ToolCommandAdapter = { }, formatFile(content: CommandContent): string { - const tagsStr = content.tags.join(', '); return `--- -name: ${content.name} -description: ${content.description} -category: ${content.category} -tags: [${tagsStr}] +name: ${escapeYamlValue(content.name)} +description: ${escapeYamlValue(content.description)} +category: ${escapeYamlValue(content.category)} +tags: ${formatTagsArray(content.tags)} --- ${content.body} diff --git a/src/core/command-generation/adapters/qwen.ts b/src/core/command-generation/adapters/qwen.ts index a22726ad57..44d55371d9 100644 --- a/src/core/command-generation/adapters/qwen.ts +++ b/src/core/command-generation/adapters/qwen.ts @@ -11,21 +11,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. - * Quotes the string if it contains special YAML characters. - */ -function escapeYamlValue(value: string): string { - // Check if value needs quoting (contains special YAML characters or starts/ends with whitespace) - const needsQuoting = /[:\n\r#{}[\],&*!|>'"%@`]|^\s|\s$/.test(value); - if (needsQuoting) { - // Use double quotes and escape internal double quotes and backslashes - const escaped = value.replace(/\\/g, '\\\\').replace(/"/g, '\\"').replace(/\n/g, '\\n'); - return `"${escaped}"`; - } - return value; -} +import { escapeYamlValue } from '../yaml.js'; /** * Qwen adapter for command generation. diff --git a/src/core/command-generation/adapters/trae.ts b/src/core/command-generation/adapters/trae.ts index 6db48e6088..3052961582 100644 --- a/src/core/command-generation/adapters/trae.ts +++ b/src/core/command-generation/adapters/trae.ts @@ -6,28 +6,7 @@ import path from 'path'; import type { CommandContent, ToolCommandAdapter } from '../types.js'; - -/** - * Escapes a string value for safe YAML output. - * Quotes the string if it contains special YAML characters. - */ -function escapeYamlValue(value: string): string { - if (value === '') { - return '""'; - } - // Check if value needs quoting (contains special YAML characters or starts/ends with whitespace) - const needsQuoting = /[:\n\r#{}[\],&*!|>'"%@`]|^\s|\s$/.test(value); - if (needsQuoting) { - // Use double quotes and escape internal double quotes, backslashes, and newlines - const escaped = value - .replace(/\\/g, '\\\\') - .replace(/"/g, '\\"') - .replace(/\n/g, '\\n') - .replace(/\r/g, '\\r'); - return `"${escaped}"`; - } - return value; -} +import { escapeYamlValue } from '../yaml.js'; /** * Trae adapter for command generation. diff --git a/src/core/command-generation/adapters/windsurf.ts b/src/core/command-generation/adapters/windsurf.ts index a7fe4febe2..2497e2a21f 100644 --- a/src/core/command-generation/adapters/windsurf.ts +++ b/src/core/command-generation/adapters/windsurf.ts @@ -7,15 +7,7 @@ import path from 'path'; import type { CommandContent, ToolCommandAdapter } from '../types.js'; -import { escapeYamlValue } from '../yaml.js'; - -/** - * Formats a tags array as a YAML array with proper escaping. - */ -function formatTagsArray(tags: string[]): string { - const escapedTags = tags.map((tag) => escapeYamlValue(tag)); - return `[${escapedTags.join(', ')}]`; -} +import { escapeYamlValue, formatTagsArray } from '../yaml.js'; /** * Windsurf adapter for command generation. diff --git a/src/core/command-generation/adapters/zcode.ts b/src/core/command-generation/adapters/zcode.ts index 1712ba19e6..0121debba0 100644 --- a/src/core/command-generation/adapters/zcode.ts +++ b/src/core/command-generation/adapters/zcode.ts @@ -9,29 +9,7 @@ import path from 'path'; import type { CommandContent, ToolCommandAdapter } from '../types.js'; - -/** - * Escapes a string value for safe YAML output. - * Quotes the string if it contains special YAML characters. - */ -function escapeYamlValue(value: string): string { - // Check if value needs quoting (contains special YAML characters or starts/ends with whitespace) - const needsQuoting = /[:\n\r#{}[\],&*!|>'"%@`]|^\s|\s$/.test(value); - if (needsQuoting) { - // Use double quotes and escape internal double quotes and backslashes - const escaped = value.replace(/\\/g, '\\\\').replace(/"/g, '\\"').replace(/\n/g, '\\n'); - return `"${escaped}"`; - } - return value; -} - -/** - * Formats a tags array as a YAML array with proper escaping. - */ -function formatTagsArray(tags: string[]): string { - const escapedTags = tags.map((tag) => escapeYamlValue(tag)); - return `[${escapedTags.join(', ')}]`; -} +import { escapeYamlValue, formatTagsArray } from '../yaml.js'; /** * ZCode adapter for command generation. diff --git a/src/core/command-generation/yaml.ts b/src/core/command-generation/yaml.ts index dc4354add8..80699868e4 100644 --- a/src/core/command-generation/yaml.ts +++ b/src/core/command-generation/yaml.ts @@ -20,6 +20,9 @@ * @returns The value, double-quoted and escaped when necessary. */ export function escapeYamlValue(value: string): string { + if (value === '') { + return '""'; + } // Check if value needs quoting (contains special YAML characters or starts/ends with whitespace) const needsQuoting = /[:\n\r#{}[\],&*!|>'"%@`]|^\s|\s$/.test(value); if (needsQuoting) { @@ -36,3 +39,14 @@ export function escapeYamlValue(value: string): string { } return value; } + +/** + * Formats a tags array as a YAML array with proper escaping. + * + * @param tags - Array of tag strings. + * @returns Formatted YAML array string, e.g. '[tag1, tag2]'. + */ +export function formatTagsArray(tags: string[]): string { + const escapedTags = tags.map((tag) => escapeYamlValue(tag)); + return `[${escapedTags.join(', ')}]`; +} diff --git a/test/core/command-generation/adapters.test.ts b/test/core/command-generation/adapters.test.ts index 75d63bc6b8..8d6a2274f7 100644 --- a/test/core/command-generation/adapters.test.ts +++ b/test/core/command-generation/adapters.test.ts @@ -15,7 +15,10 @@ import { factoryAdapter } from '../../../src/core/command-generation/adapters/fa import { geminiAdapter } from '../../../src/core/command-generation/adapters/gemini.js'; import { githubCopilotAdapter } from '../../../src/core/command-generation/adapters/github-copilot.js'; import { iflowAdapter } from '../../../src/core/command-generation/adapters/iflow.js'; +import { junieAdapter } from '../../../src/core/command-generation/adapters/junie.js'; import { kilocodeAdapter } from '../../../src/core/command-generation/adapters/kilocode.js'; +import { kiroAdapter } from '../../../src/core/command-generation/adapters/kiro.js'; +import { lingmaAdapter } from '../../../src/core/command-generation/adapters/lingma.js'; import { ohMyPiAdapter } from '../../../src/core/command-generation/adapters/oh-my-pi.js'; import { opencodeAdapter } from '../../../src/core/command-generation/adapters/opencode.js'; import { piAdapter } from '../../../src/core/command-generation/adapters/pi.js'; @@ -247,7 +250,7 @@ describe('command-generation/adapters', () => { description: '', }; const output = bobAdapter.formatFile(contentEmptyDesc); - expect(output).toContain('description: \n'); + expect(output).toContain('description: ""'); }); }); @@ -284,7 +287,7 @@ describe('command-generation/adapters', () => { const output = codebuddyAdapter.formatFile(sampleContent); expect(output).toContain('---\n'); expect(output).toContain('name: OpenSpec Explore'); - expect(output).toContain('description: "Enter explore mode for thinking"'); + expect(output).toContain('description: Enter explore mode for thinking'); expect(output).toContain('argument-hint: "[command arguments]"'); expect(output).toContain('---\n\n'); expect(output).toContain('This is the command body.'); @@ -325,7 +328,7 @@ describe('command-generation/adapters', () => { it('should format file with description and argument-hint', () => { const output = costrictAdapter.formatFile(sampleContent); expect(output).toContain('---\n'); - expect(output).toContain('description: "Enter explore mode for thinking"'); + expect(output).toContain('description: Enter explore mode for thinking'); expect(output).toContain('argument-hint: command arguments'); expect(output).toContain('---\n\n'); expect(output).toContain('This is the command body.'); @@ -944,4 +947,53 @@ describe('command-generation/adapters', () => { } }); }); + + describe('YAML frontmatter escaping across adapters', () => { + const yamlAdapters = [ + amazonQAdapter, + antigravityAdapter, + auggieAdapter, + bobAdapter, + claudeAdapter, + codebuddyAdapter, + continueAdapter, + costrictAdapter, + crushAdapter, + cursorAdapter, + factoryAdapter, + githubCopilotAdapter, + iflowAdapter, + junieAdapter, + ohMyPiAdapter, + opencodeAdapter, + piAdapter, + qoderAdapter, + qwenAdapter, + traeAdapter, + windsurfAdapter, + zcodeAdapter, + ]; + + it.each(yamlAdapters)('$toolId formats valid YAML frontmatter when description contains colons and quotes', (adapter) => { + const specialContent: CommandContent = { + id: 'explore', + name: 'OpenSpec: Explore', + description: 'Explore mode: "thinking" & planning (e.g. feature: dark-mode)', + category: 'Workflow: Core', + tags: ['workflow:core', 'explore:mode'], + body: 'Body text', + }; + + const fileContent = adapter.formatFile(specialContent); + const frontmatterMatch = fileContent.match(/^---\n([\s\S]*?)\n---/); + expect(frontmatterMatch).not.toBeNull(); + + // Ensure frontmatter is valid YAML that parses without throwing + const { parse: parseYaml } = require('yaml'); + expect(() => parseYaml(frontmatterMatch![1])).not.toThrow(); + + const parsed = parseYaml(frontmatterMatch![1]); + expect(parsed.description).toBe(specialContent.description); + }); + }); }); From 456f006ed1d8d50025f0332cd9853f5bcc144413 Mon Sep 17 00:00:00 2001 From: Henry Su Date: Mon, 27 Jul 2026 14:42:15 -0500 Subject: [PATCH 2/6] fix(yaml): safely double-quote all frontmatter string values and expand table-driven adapter coverage --- .../command-generation/adapters/continue.ts | 2 +- .../command-generation/adapters/cursor.ts | 4 +- src/core/command-generation/adapters/iflow.ts | 4 +- src/core/command-generation/yaml.ts | 24 +-- test/commands/artifact-workflow.test.ts | 2 +- test/core/command-generation/adapters.test.ts | 183 +++++++++++------- .../core/command-generation/generator.test.ts | 16 +- test/core/command-generation/yaml.test.ts | 4 +- test/core/init.test.ts | 2 +- 9 files changed, 138 insertions(+), 103 deletions(-) diff --git a/src/core/command-generation/adapters/continue.ts b/src/core/command-generation/adapters/continue.ts index cde510b1fc..b3bdedea68 100644 --- a/src/core/command-generation/adapters/continue.ts +++ b/src/core/command-generation/adapters/continue.ts @@ -22,7 +22,7 @@ export const continueAdapter: ToolCommandAdapter = { formatFile(content: CommandContent): string { return `--- -name: opsx-${content.id} +name: ${escapeYamlValue(`opsx-${content.id}`)} description: ${escapeYamlValue(content.description)} invokable: true --- diff --git a/src/core/command-generation/adapters/cursor.ts b/src/core/command-generation/adapters/cursor.ts index d540a479b9..7ee77a1e72 100644 --- a/src/core/command-generation/adapters/cursor.ts +++ b/src/core/command-generation/adapters/cursor.ts @@ -23,8 +23,8 @@ export const cursorAdapter: ToolCommandAdapter = { formatFile(content: CommandContent): string { return `--- -name: /opsx-${content.id} -id: opsx-${content.id} +name: ${escapeYamlValue(`/opsx-${content.id}`)} +id: ${escapeYamlValue(`opsx-${content.id}`)} category: ${escapeYamlValue(content.category)} description: ${escapeYamlValue(content.description)} --- diff --git a/src/core/command-generation/adapters/iflow.ts b/src/core/command-generation/adapters/iflow.ts index e92c14f247..8d94cc112a 100644 --- a/src/core/command-generation/adapters/iflow.ts +++ b/src/core/command-generation/adapters/iflow.ts @@ -22,8 +22,8 @@ export const iflowAdapter: ToolCommandAdapter = { formatFile(content: CommandContent): string { return `--- -name: /opsx-${content.id} -id: opsx-${content.id} +name: ${escapeYamlValue(`/opsx-${content.id}`)} +id: ${escapeYamlValue(`opsx-${content.id}`)} category: ${escapeYamlValue(content.category)} description: ${escapeYamlValue(content.description)} --- diff --git a/src/core/command-generation/yaml.ts b/src/core/command-generation/yaml.ts index 80699868e4..cb1d44e86e 100644 --- a/src/core/command-generation/yaml.ts +++ b/src/core/command-generation/yaml.ts @@ -20,24 +20,12 @@ * @returns The value, double-quoted and escaped when necessary. */ export function escapeYamlValue(value: string): string { - if (value === '') { - return '""'; - } - // Check if value needs quoting (contains special YAML characters or starts/ends with whitespace) - const needsQuoting = /[:\n\r#{}[\],&*!|>'"%@`]|^\s|\s$/.test(value); - if (needsQuoting) { - // Use double quotes and escape characters that are not safe to emit - // verbatim inside a double-quoted YAML scalar. Carriage returns must be - // escaped too: a literal CR inside double quotes is subject to YAML line - // folding/normalization and would silently corrupt the round-tripped value. - const escaped = value - .replace(/\\/g, '\\\\') - .replace(/"/g, '\\"') - .replace(/\n/g, '\\n') - .replace(/\r/g, '\\r'); - return `"${escaped}"`; - } - return value; + const escaped = value + .replace(/\\/g, '\\\\') + .replace(/"/g, '\\"') + .replace(/\n/g, '\\n') + .replace(/\r/g, '\\r'); + return `"${escaped}"`; } /** diff --git a/test/commands/artifact-workflow.test.ts b/test/commands/artifact-workflow.test.ts index 90f80edc62..3af601b684 100644 --- a/test/commands/artifact-workflow.test.ts +++ b/test/commands/artifact-workflow.test.ts @@ -800,7 +800,7 @@ artifacts: // Verify commands were created with Cursor format const commandFile = path.join(tempDir, '.cursor', 'commands', 'opsx-explore.md'); const content = await fs.readFile(commandFile, 'utf-8'); - expect(content).toContain('name: /opsx-explore'); + expect(content).toContain('name: "/opsx-explore"'); }); it('creates skills for Windsurf tool', async () => { diff --git a/test/core/command-generation/adapters.test.ts b/test/core/command-generation/adapters.test.ts index 8d6a2274f7..ec87ad5f72 100644 --- a/test/core/command-generation/adapters.test.ts +++ b/test/core/command-generation/adapters.test.ts @@ -59,11 +59,11 @@ describe('command-generation/adapters', () => { const output = claudeAdapter.formatFile(sampleContent); expect(output).toContain('---\n'); - expect(output).toContain('name: OpenSpec Explore'); - expect(output).toContain('description: Enter explore mode for thinking'); + expect(output).toContain('name: "OpenSpec Explore"'); + expect(output).toContain('description: "Enter explore mode for thinking"'); expect(output).toContain('allowed-tools: Bash(openspec:*)'); - expect(output).toContain('category: Workflow'); - expect(output).toContain('tags: [workflow, explore, experimental]'); + expect(output).toContain('category: "Workflow"'); + expect(output).toContain('tags: ["workflow", "explore", "experimental"]'); expect(output).toContain('---\n\n'); expect(output).toContain('This is the command body.\n\nWith multiple lines.'); }); @@ -94,10 +94,10 @@ describe('command-generation/adapters', () => { const output = cursorAdapter.formatFile(sampleContent); expect(output).toContain('---\n'); - expect(output).toContain('name: /opsx-explore'); - expect(output).toContain('id: opsx-explore'); - expect(output).toContain('category: Workflow'); - expect(output).toContain('description: Enter explore mode for thinking'); + expect(output).toContain('name: "/opsx-explore"'); + expect(output).toContain('id: "opsx-explore"'); + expect(output).toContain('category: "Workflow"'); + expect(output).toContain('description: "Enter explore mode for thinking"'); expect(output).toContain('---\n\n'); expect(output).toContain('This is the command body.'); }); @@ -122,10 +122,10 @@ describe('command-generation/adapters', () => { const output = windsurfAdapter.formatFile(sampleContent); expect(output).toContain('---\n'); - expect(output).toContain('name: OpenSpec Explore'); - expect(output).toContain('description: Enter explore mode for thinking'); - expect(output).toContain('category: Workflow'); - expect(output).toContain('tags: [workflow, explore, experimental]'); + expect(output).toContain('name: "OpenSpec Explore"'); + expect(output).toContain('description: "Enter explore mode for thinking"'); + expect(output).toContain('category: "Workflow"'); + expect(output).toContain('tags: ["workflow", "explore", "experimental"]'); expect(output).toContain('---\n\n'); expect(output).toContain('This is the command body.'); }); @@ -144,7 +144,7 @@ describe('command-generation/adapters', () => { it('should format file with description frontmatter', () => { const output = amazonQAdapter.formatFile(sampleContent); expect(output).toContain('---\n'); - expect(output).toContain('description: Enter explore mode for thinking'); + expect(output).toContain('description: "Enter explore mode for thinking"'); expect(output).toContain('---\n\n'); expect(output).toContain('This is the command body.'); }); @@ -163,7 +163,7 @@ describe('command-generation/adapters', () => { it('should format file with description frontmatter', () => { const output = antigravityAdapter.formatFile(sampleContent); expect(output).toContain('---\n'); - expect(output).toContain('description: Enter explore mode for thinking'); + expect(output).toContain('description: "Enter explore mode for thinking"'); expect(output).toContain('---\n\n'); expect(output).toContain('This is the command body.'); }); @@ -182,7 +182,7 @@ describe('command-generation/adapters', () => { it('should format file with description and argument-hint', () => { const output = auggieAdapter.formatFile(sampleContent); expect(output).toContain('---\n'); - expect(output).toContain('description: Enter explore mode for thinking'); + expect(output).toContain('description: "Enter explore mode for thinking"'); expect(output).toContain('argument-hint: command arguments'); expect(output).toContain('---\n\n'); expect(output).toContain('This is the command body.'); @@ -208,7 +208,7 @@ describe('command-generation/adapters', () => { it('should format file with description and argument-hint frontmatter', () => { const output = bobAdapter.formatFile(sampleContent); expect(output).toContain('---\n'); - expect(output).toContain('description: Enter explore mode for thinking'); + expect(output).toContain('description: "Enter explore mode for thinking"'); expect(output).toContain('argument-hint: command arguments'); expect(output).toContain('---\n\n'); expect(output).toContain('This is the command body.\n\nWith multiple lines.'); @@ -286,8 +286,8 @@ describe('command-generation/adapters', () => { it('should format file with name, description, and argument-hint', () => { const output = codebuddyAdapter.formatFile(sampleContent); expect(output).toContain('---\n'); - expect(output).toContain('name: OpenSpec Explore'); - expect(output).toContain('description: Enter explore mode for thinking'); + expect(output).toContain('name: "OpenSpec Explore"'); + expect(output).toContain('description: "Enter explore mode for thinking"'); expect(output).toContain('argument-hint: "[command arguments]"'); expect(output).toContain('---\n\n'); expect(output).toContain('This is the command body.'); @@ -307,8 +307,8 @@ describe('command-generation/adapters', () => { it('should format file with name, description, and invokable', () => { const output = continueAdapter.formatFile(sampleContent); expect(output).toContain('---\n'); - expect(output).toContain('name: opsx-explore'); - expect(output).toContain('description: Enter explore mode for thinking'); + expect(output).toContain('name: "opsx-explore"'); + expect(output).toContain('description: "Enter explore mode for thinking"'); expect(output).toContain('invokable: true'); expect(output).toContain('---\n\n'); expect(output).toContain('This is the command body.'); @@ -328,7 +328,7 @@ describe('command-generation/adapters', () => { it('should format file with description and argument-hint', () => { const output = costrictAdapter.formatFile(sampleContent); expect(output).toContain('---\n'); - expect(output).toContain('description: Enter explore mode for thinking'); + expect(output).toContain('description: "Enter explore mode for thinking"'); expect(output).toContain('argument-hint: command arguments'); expect(output).toContain('---\n\n'); expect(output).toContain('This is the command body.'); @@ -348,10 +348,10 @@ describe('command-generation/adapters', () => { it('should format file with name, description, category, and tags', () => { const output = crushAdapter.formatFile(sampleContent); expect(output).toContain('---\n'); - expect(output).toContain('name: OpenSpec Explore'); - expect(output).toContain('description: Enter explore mode for thinking'); - expect(output).toContain('category: Workflow'); - expect(output).toContain('tags: [workflow, explore, experimental]'); + expect(output).toContain('name: "OpenSpec Explore"'); + expect(output).toContain('description: "Enter explore mode for thinking"'); + expect(output).toContain('category: "Workflow"'); + expect(output).toContain('tags: ["workflow", "explore", "experimental"]'); expect(output).toContain('---\n\n'); expect(output).toContain('This is the command body.'); }); @@ -370,7 +370,7 @@ describe('command-generation/adapters', () => { it('should format file with description and argument-hint', () => { const output = factoryAdapter.formatFile(sampleContent); expect(output).toContain('---\n'); - expect(output).toContain('description: Enter explore mode for thinking'); + expect(output).toContain('description: "Enter explore mode for thinking"'); expect(output).toContain('argument-hint: command arguments'); expect(output).toContain('---\n\n'); expect(output).toContain('This is the command body.'); @@ -409,7 +409,7 @@ describe('command-generation/adapters', () => { it('should format file with description frontmatter', () => { const output = githubCopilotAdapter.formatFile(sampleContent); expect(output).toContain('---\n'); - expect(output).toContain('description: Enter explore mode for thinking'); + expect(output).toContain('description: "Enter explore mode for thinking"'); expect(output).toContain('---\n\n'); expect(output).toContain('This is the command body.'); }); @@ -428,10 +428,10 @@ describe('command-generation/adapters', () => { it('should format file with name, id, category, and description', () => { const output = iflowAdapter.formatFile(sampleContent); expect(output).toContain('---\n'); - expect(output).toContain('name: /opsx-explore'); - expect(output).toContain('id: opsx-explore'); - expect(output).toContain('category: Workflow'); - expect(output).toContain('description: Enter explore mode for thinking'); + expect(output).toContain('name: "/opsx-explore"'); + expect(output).toContain('id: "opsx-explore"'); + expect(output).toContain('category: "Workflow"'); + expect(output).toContain('description: "Enter explore mode for thinking"'); expect(output).toContain('---\n\n'); expect(output).toContain('This is the command body.'); }); @@ -467,7 +467,7 @@ describe('command-generation/adapters', () => { it('should format file with description frontmatter', () => { const output = opencodeAdapter.formatFile(sampleContent); expect(output).toContain('---\n'); - expect(output).toContain('description: Enter explore mode for thinking'); + expect(output).toContain('description: "Enter explore mode for thinking"'); expect(output).toContain('---\n\n'); expect(output).toContain('This is the command body.'); }); @@ -513,10 +513,10 @@ describe('command-generation/adapters', () => { it('should format file with name, description, category, and tags', () => { const output = qoderAdapter.formatFile(sampleContent); expect(output).toContain('---\n'); - expect(output).toContain('name: OpenSpec Explore'); - expect(output).toContain('description: Enter explore mode for thinking'); - expect(output).toContain('category: Workflow'); - expect(output).toContain('tags: [workflow, explore, experimental]'); + expect(output).toContain('name: "OpenSpec Explore"'); + expect(output).toContain('description: "Enter explore mode for thinking"'); + expect(output).toContain('category: "Workflow"'); + expect(output).toContain('tags: ["workflow", "explore", "experimental"]'); expect(output).toContain('---\n\n'); expect(output).toContain('This is the command body.'); }); @@ -535,7 +535,7 @@ describe('command-generation/adapters', () => { it('should format file with description frontmatter', () => { const output = qwenAdapter.formatFile(sampleContent); expect(output).toContain('---\n'); - expect(output).toContain('description: Enter explore mode for thinking'); + expect(output).toContain('description: "Enter explore mode for thinking"'); expect(output).toContain('---\n\n'); expect(output).toContain('This is the command body.'); }); @@ -580,7 +580,7 @@ describe('command-generation/adapters', () => { it('should format file with description frontmatter', () => { const output = piAdapter.formatFile(sampleContent); expect(output).toContain('---\n'); - expect(output).toContain('description: Enter explore mode for thinking'); + expect(output).toContain('description: "Enter explore mode for thinking"'); expect(output).toContain('---\n\n'); expect(output).toContain('This is the command body.'); }); @@ -644,7 +644,7 @@ describe('command-generation/adapters', () => { it('should format file with description frontmatter', () => { const output = ohMyPiAdapter.formatFile(sampleContent); expect(output).toContain('---\n'); - expect(output).toContain('description: Enter explore mode for thinking'); + expect(output).toContain('description: "Enter explore mode for thinking"'); expect(output).toContain('---\n\n'); expect(output).toContain('This is the command body.'); }); @@ -754,8 +754,8 @@ describe('command-generation/adapters', () => { const output = traeAdapter.formatFile(sampleContent); expect(output).toContain('---\n'); - expect(output).toContain('name: OpenSpec Explore'); - expect(output).toContain('description: Enter explore mode for thinking'); + expect(output).toContain('name: "OpenSpec Explore"'); + expect(output).toContain('description: "Enter explore mode for thinking"'); expect(output).toContain('---\n\n'); expect(output).toContain('This is the command body.\n\nWith multiple lines.'); }); @@ -833,10 +833,10 @@ describe('command-generation/adapters', () => { const output = zcodeAdapter.formatFile(sampleContent); expect(output).toContain('---\n'); - expect(output).toContain('name: OpenSpec Explore'); - expect(output).toContain('description: Enter explore mode for thinking'); - expect(output).toContain('category: Workflow'); - expect(output).toContain('tags: [workflow, explore, experimental]'); + expect(output).toContain('name: "OpenSpec Explore"'); + expect(output).toContain('description: "Enter explore mode for thinking"'); + expect(output).toContain('category: "Workflow"'); + expect(output).toContain('tags: ["workflow", "explore", "experimental"]'); expect(output).toContain('---\n\n'); expect(output).toContain('This is the command body.\n\nWith multiple lines.'); }); @@ -891,7 +891,7 @@ describe('command-generation/adapters', () => { ...sampleContent, tags: ['workflow', 'explore:1', 'experimental'], }); - expect(output).toContain('tags: [workflow, "explore:1", experimental]'); + expect(output).toContain('tags: ["workflow", "explore:1", "experimental"]'); }); it('should escape backslashes when quoting is triggered by another special char', () => { @@ -937,8 +937,9 @@ describe('command-generation/adapters', () => { amazonQAdapter, antigravityAdapter, auggieAdapter, bobAdapter, clineAdapter, codebuddyAdapter, continueAdapter, costrictAdapter, crushAdapter, factoryAdapter, geminiAdapter, githubCopilotAdapter, - iflowAdapter, kilocodeAdapter, ohMyPiAdapter, opencodeAdapter, piAdapter, qoderAdapter, - qwenAdapter, roocodeAdapter, traeAdapter, zcodeAdapter + iflowAdapter, kilocodeAdapter, kiroAdapter, lingmaAdapter, ohMyPiAdapter, + opencodeAdapter, piAdapter, qoderAdapter, qwenAdapter, roocodeAdapter, + traeAdapter, zcodeAdapter ]; for (const adapter of adapters) { const filePath = adapter.getFilePath('test'); @@ -964,6 +965,8 @@ describe('command-generation/adapters', () => { githubCopilotAdapter, iflowAdapter, junieAdapter, + kiroAdapter, + lingmaAdapter, ohMyPiAdapter, opencodeAdapter, piAdapter, @@ -974,26 +977,70 @@ describe('command-generation/adapters', () => { zcodeAdapter, ]; - it.each(yamlAdapters)('$toolId formats valid YAML frontmatter when description contains colons and quotes', (adapter) => { - const specialContent: CommandContent = { - id: 'explore', - name: 'OpenSpec: Explore', - description: 'Explore mode: "thinking" & planning (e.g. feature: dark-mode)', - category: 'Workflow: Core', - tags: ['workflow:core', 'explore:mode'], - body: 'Body text', - }; - - const fileContent = adapter.formatFile(specialContent); - const frontmatterMatch = fileContent.match(/^---\n([\s\S]*?)\n---/); - expect(frontmatterMatch).not.toBeNull(); - - // Ensure frontmatter is valid YAML that parses without throwing - const { parse: parseYaml } = require('yaml'); - expect(() => parseYaml(frontmatterMatch![1])).not.toThrow(); + const roundTripCases: Array<[string, string]> = [ + ['plain text', 'Enter explore mode for thinking'], + ['empty string', ''], + ['colon and quotes', 'Explore mode: "thinking" & planning (e.g. feature: dark-mode)'], + ['block literal |', '|'], + ['block literal |-', '|-'], + ['block literal |+', '|+'], + ['block folded >', '>'], + ['block folded >-', '>-'], + ['block folded >+', '>+'], + ['block with text', '| block text'], + ['folded with text', '> folded text'], + ['boolean true', 'true'], + ['boolean false', 'false'], + ['boolean yes', 'yes'], + ['boolean no', 'no'], + ['boolean on', 'on'], + ['boolean off', 'off'], + ['null string', 'null'], + ['tilde null', '~'], + ['integer', '123'], + ['zero', '0'], + ['negative int', '-10'], + ['float', '1.23'], + ['scientific notation', '1e5'], + ['hex integer', '0x12'], + ['octal integer', '077'], + ['binary integer', '0b101'], + ['infinity', '.inf'], + ['nan', '.nan'], + ['special characters', '# comment: [a, b] {c: d} - item ? key *ref &anc !tag @at `cmd`'], + ['leading space', ' leading'], + ['trailing space', 'trailing '], + ['multiple spaces', ' '], + ]; - const parsed = parseYaml(frontmatterMatch![1]); - expect(parsed.description).toBe(specialContent.description); - }); + for (const adapter of yamlAdapters) { + describe(`${adapter.toolId} adapter table-driven round-trip`, () => { + for (const [label, testVal] of roundTripCases) { + it(`preserves description value and string type for ${label}`, () => { + const content: CommandContent = { + id: 'explore', + name: 'OpenSpec Explore', + description: testVal, + category: 'Workflow', + tags: ['workflow', 'explore'], + body: 'Body text', + }; + + const fileContent = adapter.formatFile(content); + const frontmatterMatch = fileContent.match(/^---\n([\s\S]*?)\n---/); + expect(frontmatterMatch).not.toBeNull(); + + const { parse: parseYaml } = require('yaml'); + let parsed: any; + expect(() => { + parsed = parseYaml(frontmatterMatch![1]); + }).not.toThrow(); + + expect(parsed.description).toBe(testVal); + expect(typeof parsed.description).toBe('string'); + }); + } + }); + } }); }); diff --git a/test/core/command-generation/generator.test.ts b/test/core/command-generation/generator.test.ts index 903aac3e1d..e7a5c8fb66 100644 --- a/test/core/command-generation/generator.test.ts +++ b/test/core/command-generation/generator.test.ts @@ -20,17 +20,17 @@ describe('command-generation/generator', () => { expect(result.path).toContain('.claude'); expect(result.path).toContain('explore.md'); - expect(result.fileContent).toContain('name: OpenSpec Explore'); + expect(result.fileContent).toContain('name: "OpenSpec Explore"'); expect(result.fileContent).toContain('Command body here.'); }); - it('should generate command with path and content using Cursor adapter', () => { + it('should generate command for Cursor adapter', () => { const result = generateCommand(sampleContent, cursorAdapter); expect(result.path).toContain('.cursor'); expect(result.path).toContain('opsx-explore.md'); - expect(result.fileContent).toContain('name: /opsx-explore'); - expect(result.fileContent).toContain('id: opsx-explore'); + expect(result.fileContent).toContain('name: "/opsx-explore"'); + expect(result.fileContent).toContain('id: "opsx-explore"'); expect(result.fileContent).toContain('Command body here.'); }); @@ -98,13 +98,13 @@ describe('command-generation/generator', () => { const results = generateCommands(contents, claudeAdapter); - expect(results[0].fileContent).toContain('name: A'); + expect(results[0].fileContent).toContain('name: "A"'); expect(results[0].fileContent).toContain('B1'); - expect(results[0].fileContent).not.toContain('name: B'); + expect(results[0].fileContent).not.toContain('name: "B"'); - expect(results[1].fileContent).toContain('name: B'); + expect(results[1].fileContent).toContain('name: "B"'); expect(results[1].fileContent).toContain('B2'); - expect(results[1].fileContent).not.toContain('name: A'); + expect(results[1].fileContent).not.toContain('name: "A"'); }); }); }); diff --git a/test/core/command-generation/yaml.test.ts b/test/core/command-generation/yaml.test.ts index 937209ae1e..21b2944e4a 100644 --- a/test/core/command-generation/yaml.test.ts +++ b/test/core/command-generation/yaml.test.ts @@ -14,9 +14,9 @@ function roundTrip(value: string): unknown { } describe('command-generation/yaml escapeYamlValue', () => { - it('returns the value unquoted when no special characters are present', () => { + it('quotes plain values for safe string serialization', () => { expect(escapeYamlValue('Enter explore mode for thinking')).toBe( - 'Enter explore mode for thinking' + '"Enter explore mode for thinking"' ); }); diff --git a/test/core/init.test.ts b/test/core/init.test.ts index 3b4b5570a9..d13db2f23d 100644 --- a/test/core/init.test.ts +++ b/test/core/init.test.ts @@ -615,7 +615,7 @@ describe('InitCommand', () => { expect(await fileExists(cmdFile)).toBe(true); const content = await fs.readFile(cmdFile, 'utf-8'); - expect(content).toContain('name: opsx-explore'); + expect(content).toContain('name: "opsx-explore"'); expect(content).toContain('invokable: true'); }); From 206522d0d8f131545241d2526bbe8567ec4c5818 Mon Sep 17 00:00:00 2001 From: Henry Su Date: Mon, 27 Jul 2026 14:44:19 -0500 Subject: [PATCH 3/6] fix(archive): make command and bulk archive paths root-aware, synchronous, and verified --- skills/openspec-bulk-archive-change/SKILL.md | 29 ++++++--- .../templates/workflows/archive-change.ts | 4 +- .../workflows/bulk-archive-change.ts | 62 +++++++++++++------ .../templates/skill-templates-parity.test.ts | 53 ++++++++++++---- 4 files changed, 105 insertions(+), 43 deletions(-) diff --git a/skills/openspec-bulk-archive-change/SKILL.md b/skills/openspec-bulk-archive-change/SKILL.md index 87dd2205a0..3541595ab4 100644 --- a/skills/openspec-bulk-archive-change/SKILL.md +++ b/skills/openspec-bulk-archive-change/SKILL.md @@ -122,7 +122,7 @@ This skill allows you to batch-archive changes, handling spec conflicts intellig so match what the user picked rather than the wording above: - "Cancel" — stop, do not archive. Report that nothing was archived and skip the remaining steps. - The archive-everything option — proceed with every selected change - - The ready-only option — proceed with only the changes the step 6 table marks `Ready` or `Ready*`, and record the rest as Skipped in step 8c. If a `Ready*` change's conflict partner is skipped, re-derive that conflict's resolution using only the changes being archived. + - The ready-only option — proceed with only the changes the step 6 table marks `Ready` or `Ready*`, and record the rest as Skipped in step 8d. If a `Ready*` change's conflict partner is skipped, re-derive that conflict's resolution using only the changes being archived. - Anything else — ask again rather than archiving 8. **Execute archive for each confirmed change** @@ -130,11 +130,20 @@ This skill allows you to batch-archive changes, handling spec conflicts intellig Process changes in the determined order (respecting conflict resolution): a. **Sync specs** if delta specs exist: - - Use the openspec-sync-specs approach (agent-driven intelligent merge) - - For conflicts, apply in resolved order - - Track if sync was done + - Run the `openspec-sync-specs` workflow inline (agent-driven intelligent merge) for each change, passing the delta spec analysis, and wait for it to finish. + - For conflicts, apply in resolved order. + - Do not delegate to a background task — step 8c would move `changeRoot` out from under a sync that is still reading it. - b. **Perform the archive**: + b. **Verify main specs before moving changeRoot**: + - Re-run the comparison against every capability that has a delta spec in `artifactPaths.specs.existingOutputPaths` against main spec at `/openspec/specs//spec.md` (use the store-aware `planningHome.root` from step 3 status JSON, not a hardcoded repo path). + - Verify that main specs are updated: + - ADDED requirements present + - MODIFIED requirements carrying scenario and description changes named in the delta, with their other scenarios intact + - REMOVED requirements gone + - RENAMED requirements present under the new name and absent under the old one + - If sync failed or any capability does not match verification, report what differs and fail/skip moving that change's `changeRoot` — do not archive that change. `changeRoot` remains intact. + + c. **Perform the archive**: Target name: use the change name as-is when it already starts with a `YYYY-MM-DD-` prefix; otherwise prepend the current date as `YYYY-MM-DD-` (same rule as `openspec archive`). @@ -143,9 +152,9 @@ This skill allows you to batch-archive changes, handling spec conflicts intellig mv "" "/archive/" ``` - c. **Track outcome** for each change: + d. **Track outcome** for each change: - Success: archived successfully - - Failed: error during archive (record error) + - Failed: error during archive or spec verification (record error) - Skipped: user chose not to archive (if applicable) 9. **Display summary** @@ -178,7 +187,7 @@ This skill allows you to batch-archive changes, handling spec conflicts intellig Example 1: Only one implemented ```text -Conflict: specs/auth/spec.md touched by [add-oauth, add-jwt] +Conflict: /openspec/specs/auth/spec.md touched by [add-oauth, add-jwt] Checking add-oauth: - Delta adds "OAuth Provider Integration" requirement @@ -193,7 +202,7 @@ Resolution: Only add-oauth is implemented. Will sync add-oauth specs only. Example 2: Both implemented ```text -Conflict: specs/api/spec.md touched by [add-rest-api, add-graphql] +Conflict: /openspec/specs/api/spec.md touched by [add-rest-api, add-graphql] Checking add-rest-api (created 2026-01-10): - Delta adds "REST Endpoints" requirement @@ -257,3 +266,5 @@ No active changes found. Create a new change to get started. - Preserve .openspec.yaml when moving to archive - Archive directory target uses current date: YYYY-MM-DD-; a name that already starts with a `YYYY-MM-DD-` prefix is used as-is (never stack a second date) - If archive target exists, fail that change but continue with others +- If sync is requested, run the `openspec-sync-specs` workflow inline (agent-driven) for each change +- Never archive a change while a spec sync is still in flight — run the sync inline and verify main specs at `/openspec/specs//spec.md` before moving `changeRoot` diff --git a/src/core/templates/workflows/archive-change.ts b/src/core/templates/workflows/archive-change.ts index 85441751d0..be12ea37e8 100644 --- a/src/core/templates/workflows/archive-change.ts +++ b/src/core/templates/workflows/archive-change.ts @@ -207,7 +207,7 @@ ${STORE_SELECTION_GUIDANCE} - "Sync now" or "Sync anyway" — sync, then verify (below) - Anything else — ask again rather than archiving - To sync, run the \`openspec-sync-specs\` workflow inline (agent-driven intelligent merge) for change '', passing the delta spec analysis from above, and wait for it to finish. Do not delegate it to a background task — step 5 would move \`changeRoot\` out from under a sync that is still reading it, leaving the change archived and the main specs never updated. If your agent can only run it by delegation, delegate synchronously and wait for the result. + To sync, run the \`/opsx:sync\` workflow inline (agent-driven intelligent merge) for change '', passing the delta spec analysis from above, and wait for it to finish. Do not delegate it to a background task — step 5 would move \`changeRoot\` out from under a sync that is still reading it, leaving the change archived and the main specs never updated. If your agent can only run it by delegation, delegate synchronously and wait for the result. Then re-run the comparison from the top of this step against every capability that has a delta spec in \`artifactPaths.specs.existingOutputPaths\` — not only the ones the sync reports it touched. A successful sync leaves nothing left to apply, so each capability must now read as already synced: - ADDED requirements present @@ -309,7 +309,7 @@ Target archive directory already exists. - Don't block archive on warnings - just inform and confirm - Preserve .openspec.yaml when moving to archive (it moves with the directory) - Show clear summary of what happened -- If sync is requested, run the \`openspec-sync-specs\` workflow inline (agent-driven) +- If sync is requested, run the \`/opsx:sync\` workflow inline (agent-driven) - Never archive while a spec sync is still in flight — run the sync inline and verify the main specs before moving \`changeRoot\` - If delta specs exist, always run the sync assessment and show the combined summary before prompting` }; diff --git a/src/core/templates/workflows/bulk-archive-change.ts b/src/core/templates/workflows/bulk-archive-change.ts index 3cca28b022..205d61d6f0 100644 --- a/src/core/templates/workflows/bulk-archive-change.ts +++ b/src/core/templates/workflows/bulk-archive-change.ts @@ -124,7 +124,7 @@ ${STORE_SELECTION_GUIDANCE} so match what the user picked rather than the wording above: - "Cancel" — stop, do not archive. Report that nothing was archived and skip the remaining steps. - The archive-everything option — proceed with every selected change - - The ready-only option — proceed with only the changes the step 6 table marks \`Ready\` or \`Ready*\`, and record the rest as Skipped in step 8c. If a \`Ready*\` change's conflict partner is skipped, re-derive that conflict's resolution using only the changes being archived. + - The ready-only option — proceed with only the changes the step 6 table marks \`Ready\` or \`Ready*\`, and record the rest as Skipped in step 8d. If a \`Ready*\` change's conflict partner is skipped, re-derive that conflict's resolution using only the changes being archived. - Anything else — ask again rather than archiving 8. **Execute archive for each confirmed change** @@ -132,11 +132,20 @@ ${STORE_SELECTION_GUIDANCE} Process changes in the determined order (respecting conflict resolution): a. **Sync specs** if delta specs exist: - - Use the openspec-sync-specs approach (agent-driven intelligent merge) - - For conflicts, apply in resolved order - - Track if sync was done + - Run the \`openspec-sync-specs\` workflow inline (agent-driven intelligent merge) for each change, passing the delta spec analysis, and wait for it to finish. + - For conflicts, apply in resolved order. + - Do not delegate to a background task — step 8c would move \`changeRoot\` out from under a sync that is still reading it. - b. **Perform the archive**: + b. **Verify main specs before moving changeRoot**: + - Re-run the comparison against every capability that has a delta spec in \`artifactPaths.specs.existingOutputPaths\` against main spec at \`/openspec/specs//spec.md\` (use the store-aware \`planningHome.root\` from step 3 status JSON, not a hardcoded repo path). + - Verify that main specs are updated: + - ADDED requirements present + - MODIFIED requirements carrying scenario and description changes named in the delta, with their other scenarios intact + - REMOVED requirements gone + - RENAMED requirements present under the new name and absent under the old one + - If sync failed or any capability does not match verification, report what differs and fail/skip moving that change's \`changeRoot\` — do not archive that change. \`changeRoot\` remains intact. + + c. **Perform the archive**: Target name: use the change name as-is when it already starts with a \`YYYY-MM-DD-\` prefix; otherwise prepend the current date as \`YYYY-MM-DD-\` (same rule as \`openspec archive\`). @@ -145,9 +154,9 @@ ${STORE_SELECTION_GUIDANCE} mv "" "/archive/" \`\`\` - c. **Track outcome** for each change: + d. **Track outcome** for each change: - Success: archived successfully - - Failed: error during archive (record error) + - Failed: error during archive or spec verification (record error) - Skipped: user chose not to archive (if applicable) 9. **Display summary** @@ -180,7 +189,7 @@ ${STORE_SELECTION_GUIDANCE} Example 1: Only one implemented \`\`\`text -Conflict: specs/auth/spec.md touched by [add-oauth, add-jwt] +Conflict: /openspec/specs/auth/spec.md touched by [add-oauth, add-jwt] Checking add-oauth: - Delta adds "OAuth Provider Integration" requirement @@ -195,7 +204,7 @@ Resolution: Only add-oauth is implemented. Will sync add-oauth specs only. Example 2: Both implemented \`\`\`text -Conflict: specs/api/spec.md touched by [add-rest-api, add-graphql] +Conflict: /openspec/specs/api/spec.md touched by [add-rest-api, add-graphql] Checking add-rest-api (created 2026-01-10): - Delta adds "REST Endpoints" requirement @@ -258,7 +267,9 @@ No active changes found. Create a new change to get started. - Track and report all outcomes (success/skip/fail) - Preserve .openspec.yaml when moving to archive - Archive directory target uses current date: YYYY-MM-DD-; a name that already starts with a \`YYYY-MM-DD-\` prefix is used as-is (never stack a second date) -- If archive target exists, fail that change but continue with others`, +- If archive target exists, fail that change but continue with others +- If sync is requested, run the \`openspec-sync-specs\` workflow inline (agent-driven) for each change +- Never archive a change while a spec sync is still in flight — run the sync inline and verify main specs at \`/openspec/specs//spec.md\` before moving \`changeRoot\``, license: 'MIT', compatibility: 'Requires openspec CLI.', metadata: { author: 'openspec', version: '1.0' }, @@ -384,7 +395,7 @@ ${STORE_SELECTION_GUIDANCE} so match what the user picked rather than the wording above: - "Cancel" — stop, do not archive. Report that nothing was archived and skip the remaining steps. - The archive-everything option — proceed with every selected change - - The ready-only option — proceed with only the changes the step 6 table marks \`Ready\` or \`Ready*\`, and record the rest as Skipped in step 8c. If a \`Ready*\` change's conflict partner is skipped, re-derive that conflict's resolution using only the changes being archived. + - The ready-only option — proceed with only the changes the step 6 table marks \`Ready\` or \`Ready*\`, and record the rest as Skipped in step 8d. If a \`Ready*\` change's conflict partner is skipped, re-derive that conflict's resolution using only the changes being archived. - Anything else — ask again rather than archiving 8. **Execute archive for each confirmed change** @@ -392,11 +403,20 @@ ${STORE_SELECTION_GUIDANCE} Process changes in the determined order (respecting conflict resolution): a. **Sync specs** if delta specs exist: - - Use the openspec-sync-specs approach (agent-driven intelligent merge) - - For conflicts, apply in resolved order - - Track if sync was done + - Run the \`/opsx:sync\` workflow inline (agent-driven intelligent merge) for each change, passing the delta spec analysis, and wait for it to finish. + - For conflicts, apply in resolved order. + - Do not delegate to a background task — step 8c would move \`changeRoot\` out from under a sync that is still reading it. + + b. **Verify main specs before moving changeRoot**: + - Re-run the comparison against every capability that has a delta spec in \`artifactPaths.specs.existingOutputPaths\` against main spec at \`/openspec/specs//spec.md\` (use the store-aware \`planningHome.root\` from step 3 status JSON, not a hardcoded repo path). + - Verify that main specs are updated: + - ADDED requirements present + - MODIFIED requirements carrying scenario and description changes named in the delta, with their other scenarios intact + - REMOVED requirements gone + - RENAMED requirements present under the new name and absent under the old one + - If sync failed or any capability does not match verification, report what differs and fail/skip moving that change's \`changeRoot\` — do not archive that change. \`changeRoot\` remains intact. - b. **Perform the archive**: + c. **Perform the archive**: Target name: use the change name as-is when it already starts with a \`YYYY-MM-DD-\` prefix; otherwise prepend the current date as \`YYYY-MM-DD-\` (same rule as \`openspec archive\`). @@ -405,9 +425,9 @@ ${STORE_SELECTION_GUIDANCE} mv "" "/archive/" \`\`\` - c. **Track outcome** for each change: + d. **Track outcome** for each change: - Success: archived successfully - - Failed: error during archive (record error) + - Failed: error during archive or spec verification (record error) - Skipped: user chose not to archive (if applicable) 9. **Display summary** @@ -440,7 +460,7 @@ ${STORE_SELECTION_GUIDANCE} Example 1: Only one implemented \`\`\`text -Conflict: specs/auth/spec.md touched by [add-oauth, add-jwt] +Conflict: /openspec/specs/auth/spec.md touched by [add-oauth, add-jwt] Checking add-oauth: - Delta adds "OAuth Provider Integration" requirement @@ -455,7 +475,7 @@ Resolution: Only add-oauth is implemented. Will sync add-oauth specs only. Example 2: Both implemented \`\`\`text -Conflict: specs/api/spec.md touched by [add-rest-api, add-graphql] +Conflict: /openspec/specs/api/spec.md touched by [add-rest-api, add-graphql] Checking add-rest-api (created 2026-01-10): - Delta adds "REST Endpoints" requirement @@ -518,6 +538,8 @@ No active changes found. Create a new change to get started. - Track and report all outcomes (success/skip/fail) - Preserve .openspec.yaml when moving to archive - Archive directory target uses current date: YYYY-MM-DD-; a name that already starts with a \`YYYY-MM-DD-\` prefix is used as-is (never stack a second date) -- If archive target exists, fail that change but continue with others` +- If archive target exists, fail that change but continue with others +- If sync is requested, run the \`/opsx:sync\` workflow inline (agent-driven) for each change +- Never archive a change while a spec sync is still in flight — run the sync inline and verify main specs at \`/openspec/specs//spec.md\` before moving \`changeRoot\`` }; } diff --git a/test/core/templates/skill-templates-parity.test.ts b/test/core/templates/skill-templates-parity.test.ts index a9b2e6fdb2..fb3c43c5a8 100644 --- a/test/core/templates/skill-templates-parity.test.ts +++ b/test/core/templates/skill-templates-parity.test.ts @@ -50,12 +50,12 @@ const EXPECTED_FUNCTION_HASHES: Record = { getOpsxApplyCommandTemplate: '147408d7085b468981a400cc725804252c3fd84e519c57c5f6f83562e32606ee', getOpsxFfCommandTemplate: '264b514cc4849f91fb4414f639484c4181f1e5850d0d788ef276c851efa92859', getArchiveChangeSkillTemplate: '206a22b6778e97c30da9145ef51fdad449b8c995538f6fc25752ef551a37b675', - getBulkArchiveChangeSkillTemplate: '2b74b1f73380ff32e35f580734780d843c6161a2748c39edb07f1e00453771b4', + getBulkArchiveChangeSkillTemplate: 'e8f127adbbc39fabac09a389688126d043f4a88acb97c8d8191a47f2440fa05d', getOpsxSyncCommandTemplate: 'df0240a79f7b4943a54c7413ab088ee48f5bf5fe19f9347c170d695c8ec777a4', getVerifyChangeSkillTemplate: 'cab4db01b5d2b1243d63d90c53747d8b39e488c60f76eba3fe8b994467f69267', - getOpsxArchiveCommandTemplate: '7dea65d0e2e17db366bb666ba6ae5e205ea02707b8c5c7707565200875c78916', + getOpsxArchiveCommandTemplate: '2f337c6dfb5f988cf994e5637433c5a90d5d3b66ec20ecc1a2b723050a872efd', getOpsxOnboardCommandTemplate: '16a68b8c9819e2a7bab013c3b49a3e49ea258b68c4e7f47f0d598e30815e0a80', - getOpsxBulkArchiveCommandTemplate: 'da7be1a7318f15b915f5aae8eb638797a8a24a31e5fc7fc0a2bad01bba137686', + getOpsxBulkArchiveCommandTemplate: 'f26699de74499c5699dfafa5a3678f05afc9284c9a9891220f30e0a2c3e2fcc6', getOpsxVerifyCommandTemplate: 'f01c0c0cef53be0956de52363d955d4ace131b1b2d77adf902f35fead9a1486d', getOpsxProposeSkillTemplate: '57fb556a060e2eb246b500922837af7573a6e100a6ed7dfaa7bd4ce0f5daffd3', getOpsxProposeCommandTemplate: '434cae3ee20835725bb1d2ccb9698310a850c5b95ed669ea15fc7a0125371c59', @@ -72,7 +72,7 @@ const EXPECTED_GENERATED_SKILL_CONTENT_HASHES: Record = { 'openspec-ff-change': 'ff3bd3eac427a1e50071ad7c70f73b556cffa3db43e90da2726e96849c3fc886', 'openspec-sync-specs': '74de778dd8a8fd4987a09621147358cc32505bb58110492ab2b4ffe7f35aa48f', 'openspec-archive-change': '64b1611dd7aee04ca268820d1b193e8bf0a39ff3672ec6ba21fb0a1bcb1786c2', - 'openspec-bulk-archive-change': '49d410bda408c0411decd584be9c2355335e3b3db760fc6a0adcd82c172a280f', + 'openspec-bulk-archive-change': '8a660127e6b889a039eca702ffa8c0358ad97c61a18a8381c3ed2f212b94147f', 'openspec-verify-change': '57693d22940f06080c6cf8d590ac2f48240d4a5e9ce7074dacd0f8d3c9945afa', 'openspec-onboard': '1d581c12d4928d751eb79de099e275dabe9c99fc15dc1f502abebd99ad7cb7d2', 'openspec-propose': '4638400113946f4f1ee9f0bd0e965aafb200bd89b64ec7f5406ef5e948e8e218', @@ -224,28 +224,57 @@ describe('skill templates split parity', () => { const generatedSkill = generateSkillContent(getArchiveChangeSkillTemplate(), 'PARITY-BASELINE'); const commandContent = getOpsxArchiveCommandTemplate().content; + // The single archive skill references openspec-sync-specs; opsx command references /opsx:sync. + expect(generatedSkill, 'skill').toContain('run the `openspec-sync-specs` workflow inline'); + expect(commandContent, 'opsx command').toContain('run the `/opsx:sync` workflow inline'); + const variants: Array<[string, string]> = [ ['skill', generatedSkill], ['opsx command', commandContent], ]; for (const [variant, content] of variants) { - // The sync must run inline: delegating it to a background task lets step 5 - // move changeRoot out from under a sync that is still reading it. - expect(content, variant).toContain('run the `openspec-sync-specs` workflow inline'); expect(content, variant).toContain('Do not delegate it to a background task'); expect(content, variant).toContain('Never archive while a spec sync is still in flight'); - // Verification must follow delta semantics. Asserting presence alone would - // read a correct REMOVED-only sync as a failure, and would pass a no-op - // sync for a MODIFIED-only delta (those requirements already exist). + // Verification must follow delta semantics. expect(content, variant).toContain('MODIFIED requirements carrying the scenario and description changes'); expect(content, variant).toContain('REMOVED requirements gone'); expect(content, variant).toContain('RENAMED requirements present under the new name and absent under the old one'); - // Verification is bound to the delta specs on disk, not to whatever the - // sync reports it touched — a silently skipped capability must not escape. + // Verification is bound to the delta specs on disk, not to whatever the sync reports it touched. expect(content, variant).toContain('not only the ones the sync reports it touched'); + + // Main spec paths are store-root aware + expect(content, variant).toContain('/openspec/specs//spec.md'); + } + }); + + it('gates bulk archive on inline synchronous spec sync and verification before moving change root', () => { + const generatedSkill = generateSkillContent(getBulkArchiveChangeSkillTemplate(), 'PARITY-BASELINE'); + const commandContent = getOpsxBulkArchiveCommandTemplate().content; + + // The bulk archive skill references openspec-sync-specs; opsx command references /opsx:sync. + expect(generatedSkill, 'bulk skill').toContain('run the `openspec-sync-specs` workflow inline'); + expect(commandContent, 'bulk opsx command').toContain('run the `/opsx:sync` workflow inline'); + + const variants: Array<[string, string]> = [ + ['bulk skill', generatedSkill], + ['bulk opsx command', commandContent], + ]; + + for (const [variant, content] of variants) { + expect(content, variant).toContain('Do not delegate to a background task'); + expect(content, variant).toContain('Never archive a change while a spec sync is still in flight'); + expect(content, variant).toContain('Verify main specs before moving changeRoot'); + + // Verification must follow delta semantics. + expect(content, variant).toContain('MODIFIED requirements carrying scenario and description changes'); + expect(content, variant).toContain('REMOVED requirements gone'); + expect(content, variant).toContain('RENAMED requirements present under the new name and absent under the old one'); + + // Main spec paths are store-root aware + expect(content, variant).toContain('/openspec/specs//spec.md'); } }); From b3b037b5176a6ffe0adae69fbebff0276f78353f Mon Sep 17 00:00:00 2001 From: Henry Su Date: Mon, 27 Jul 2026 20:14:57 -0500 Subject: [PATCH 4/6] fix(archive): honor bulk sync inclusion decisions --- skills/openspec-bulk-archive-change/SKILL.md | 28 +++++++--- .../workflows/bulk-archive-change.ts | 56 +++++++++++++------ .../templates/skill-templates-parity.test.ts | 43 ++++++++++++-- 3 files changed, 99 insertions(+), 28 deletions(-) diff --git a/skills/openspec-bulk-archive-change/SKILL.md b/skills/openspec-bulk-archive-change/SKILL.md index 3541595ab4..8c1b0ae453 100644 --- a/skills/openspec-bulk-archive-change/SKILL.md +++ b/skills/openspec-bulk-archive-change/SKILL.md @@ -77,8 +77,9 @@ This skill allows you to batch-archive changes, handling spec conflicts intellig - If neither implemented -> skip spec sync, warn user d. **Record resolution** for each conflict: - - Which change's specs to apply - - In what order (if both) + - An inclusion or exclusion decision for every delta spec, keyed by change and capability + - Which included delta specs to apply and in what order + - Which delta specs to exclude from sync because their implementation is missing - Rationale (what was found in codebase) 6. **Show consolidated status table** @@ -127,20 +128,27 @@ This skill allows you to batch-archive changes, handling spec conflicts intellig 8. **Execute archive for each confirmed change** + Before processing, carry the recorded decisions from step 5 (after any step 7 re-derivation) into two per-delta sets: + - `includedDeltas`: all non-conflicting delta specs from confirmed changes plus conflict deltas selected for sync + - `excludedDeltas`: conflict deltas from confirmed changes excluded because their implementation is missing + - A single change can have both included and excluded delta specs. Keep the decision per delta; do not collapse it into a per-change sync flag. + Process changes in the determined order (respecting conflict resolution): - a. **Sync specs** if delta specs exist: - - Run the `openspec-sync-specs` workflow inline (agent-driven intelligent merge) for each change, passing the delta spec analysis, and wait for it to finish. + a. **Sync included delta specs**: + - Run the `openspec-sync-specs` workflow inline (agent-driven intelligent merge) only for changes with entries in `includedDeltas`, passing only the included delta paths and explicitly instructing it to ignore that change's `excludedDeltas`. Wait for it to finish. - For conflicts, apply in resolved order. - Do not delegate to a background task — step 8c would move `changeRoot` out from under a sync that is still reading it. + - If a change has no included delta specs, do not run the sync workflow for it. - b. **Verify main specs before moving changeRoot**: - - Re-run the comparison against every capability that has a delta spec in `artifactPaths.specs.existingOutputPaths` against main spec at `/openspec/specs//spec.md` (use the store-aware `planningHome.root` from step 3 status JSON, not a hardcoded repo path). + b. **Verify included delta specs before moving changeRoot**: + - Re-run the comparison only for delta specs in `includedDeltas` against main spec at `/openspec/specs//spec.md` (use the store-aware `planningHome.root` from step 3 status JSON, not a hardcoded repo path). - Verify that main specs are updated: - ADDED requirements present - MODIFIED requirements carrying scenario and description changes named in the delta, with their other scenarios intact - REMOVED requirements gone - RENAMED requirements present under the new name and absent under the old one + - Do not verify delta specs in `excludedDeltas`; they are intentionally left unsynced. - If sync failed or any capability does not match verification, report what differs and fail/skip moving that change's `changeRoot` — do not archive that change. `changeRoot` remains intact. c. **Perform the archive**: @@ -156,6 +164,7 @@ This skill allows you to batch-archive changes, handling spec conflicts intellig - Success: archived successfully - Failed: error during archive or spec verification (record error) - Skipped: user chose not to archive (if applicable) + - Sync skipped: for every delta in `excludedDeltas`, report `sync skipped` with the change, capability, and recorded reason. This is distinct from skipping the archive. 9. **Display summary** @@ -174,7 +183,8 @@ This skill allows you to batch-archive changes, handling spec conflicts intellig Spec sync summary: - 4 delta specs synced to main specs - - 1 conflict resolved (auth: applied both in chronological order) + - 1 delta spec sync skipped (add-jwt/auth: implementation not found) + - 1 conflict resolved (auth: synced add-oauth, skipped add-jwt) ``` If any failures: @@ -266,5 +276,7 @@ No active changes found. Create a new change to get started. - Preserve .openspec.yaml when moving to archive - Archive directory target uses current date: YYYY-MM-DD-; a name that already starts with a `YYYY-MM-DD-` prefix is used as-is (never stack a second date) - If archive target exists, fail that change but continue with others -- If sync is requested, run the `openspec-sync-specs` workflow inline (agent-driven) for each change +- If sync is requested, run the `openspec-sync-specs` workflow inline (agent-driven) for each change with included delta specs +- Carry the per-delta `includedDeltas` and `excludedDeltas` decisions into execution; sync and verify only included deltas +- Report every excluded delta as `sync skipped` without treating the archive itself as skipped - Never archive a change while a spec sync is still in flight — run the sync inline and verify main specs at `/openspec/specs//spec.md` before moving `changeRoot` diff --git a/src/core/templates/workflows/bulk-archive-change.ts b/src/core/templates/workflows/bulk-archive-change.ts index 205d61d6f0..1fb73c20e4 100644 --- a/src/core/templates/workflows/bulk-archive-change.ts +++ b/src/core/templates/workflows/bulk-archive-change.ts @@ -79,8 +79,9 @@ ${STORE_SELECTION_GUIDANCE} - If neither implemented -> skip spec sync, warn user d. **Record resolution** for each conflict: - - Which change's specs to apply - - In what order (if both) + - An inclusion or exclusion decision for every delta spec, keyed by change and capability + - Which included delta specs to apply and in what order + - Which delta specs to exclude from sync because their implementation is missing - Rationale (what was found in codebase) 6. **Show consolidated status table** @@ -129,20 +130,27 @@ ${STORE_SELECTION_GUIDANCE} 8. **Execute archive for each confirmed change** + Before processing, carry the recorded decisions from step 5 (after any step 7 re-derivation) into two per-delta sets: + - \`includedDeltas\`: all non-conflicting delta specs from confirmed changes plus conflict deltas selected for sync + - \`excludedDeltas\`: conflict deltas from confirmed changes excluded because their implementation is missing + - A single change can have both included and excluded delta specs. Keep the decision per delta; do not collapse it into a per-change sync flag. + Process changes in the determined order (respecting conflict resolution): - a. **Sync specs** if delta specs exist: - - Run the \`openspec-sync-specs\` workflow inline (agent-driven intelligent merge) for each change, passing the delta spec analysis, and wait for it to finish. + a. **Sync included delta specs**: + - Run the \`openspec-sync-specs\` workflow inline (agent-driven intelligent merge) only for changes with entries in \`includedDeltas\`, passing only the included delta paths and explicitly instructing it to ignore that change's \`excludedDeltas\`. Wait for it to finish. - For conflicts, apply in resolved order. - Do not delegate to a background task — step 8c would move \`changeRoot\` out from under a sync that is still reading it. + - If a change has no included delta specs, do not run the sync workflow for it. - b. **Verify main specs before moving changeRoot**: - - Re-run the comparison against every capability that has a delta spec in \`artifactPaths.specs.existingOutputPaths\` against main spec at \`/openspec/specs//spec.md\` (use the store-aware \`planningHome.root\` from step 3 status JSON, not a hardcoded repo path). + b. **Verify included delta specs before moving changeRoot**: + - Re-run the comparison only for delta specs in \`includedDeltas\` against main spec at \`/openspec/specs//spec.md\` (use the store-aware \`planningHome.root\` from step 3 status JSON, not a hardcoded repo path). - Verify that main specs are updated: - ADDED requirements present - MODIFIED requirements carrying scenario and description changes named in the delta, with their other scenarios intact - REMOVED requirements gone - RENAMED requirements present under the new name and absent under the old one + - Do not verify delta specs in \`excludedDeltas\`; they are intentionally left unsynced. - If sync failed or any capability does not match verification, report what differs and fail/skip moving that change's \`changeRoot\` — do not archive that change. \`changeRoot\` remains intact. c. **Perform the archive**: @@ -158,6 +166,7 @@ ${STORE_SELECTION_GUIDANCE} - Success: archived successfully - Failed: error during archive or spec verification (record error) - Skipped: user chose not to archive (if applicable) + - Sync skipped: for every delta in \`excludedDeltas\`, report \`sync skipped\` with the change, capability, and recorded reason. This is distinct from skipping the archive. 9. **Display summary** @@ -176,7 +185,8 @@ ${STORE_SELECTION_GUIDANCE} Spec sync summary: - 4 delta specs synced to main specs - - 1 conflict resolved (auth: applied both in chronological order) + - 1 delta spec sync skipped (add-jwt/auth: implementation not found) + - 1 conflict resolved (auth: synced add-oauth, skipped add-jwt) \`\`\` If any failures: @@ -268,7 +278,9 @@ No active changes found. Create a new change to get started. - Preserve .openspec.yaml when moving to archive - Archive directory target uses current date: YYYY-MM-DD-; a name that already starts with a \`YYYY-MM-DD-\` prefix is used as-is (never stack a second date) - If archive target exists, fail that change but continue with others -- If sync is requested, run the \`openspec-sync-specs\` workflow inline (agent-driven) for each change +- If sync is requested, run the \`openspec-sync-specs\` workflow inline (agent-driven) for each change with included delta specs +- Carry the per-delta \`includedDeltas\` and \`excludedDeltas\` decisions into execution; sync and verify only included deltas +- Report every excluded delta as \`sync skipped\` without treating the archive itself as skipped - Never archive a change while a spec sync is still in flight — run the sync inline and verify main specs at \`/openspec/specs//spec.md\` before moving \`changeRoot\``, license: 'MIT', compatibility: 'Requires openspec CLI.', @@ -350,8 +362,9 @@ ${STORE_SELECTION_GUIDANCE} - If neither implemented -> skip spec sync, warn user d. **Record resolution** for each conflict: - - Which change's specs to apply - - In what order (if both) + - An inclusion or exclusion decision for every delta spec, keyed by change and capability + - Which included delta specs to apply and in what order + - Which delta specs to exclude from sync because their implementation is missing - Rationale (what was found in codebase) 6. **Show consolidated status table** @@ -400,20 +413,27 @@ ${STORE_SELECTION_GUIDANCE} 8. **Execute archive for each confirmed change** + Before processing, carry the recorded decisions from step 5 (after any step 7 re-derivation) into two per-delta sets: + - \`includedDeltas\`: all non-conflicting delta specs from confirmed changes plus conflict deltas selected for sync + - \`excludedDeltas\`: conflict deltas from confirmed changes excluded because their implementation is missing + - A single change can have both included and excluded delta specs. Keep the decision per delta; do not collapse it into a per-change sync flag. + Process changes in the determined order (respecting conflict resolution): - a. **Sync specs** if delta specs exist: - - Run the \`/opsx:sync\` workflow inline (agent-driven intelligent merge) for each change, passing the delta spec analysis, and wait for it to finish. + a. **Sync included delta specs**: + - Run the \`/opsx:sync\` workflow inline (agent-driven intelligent merge) only for changes with entries in \`includedDeltas\`, passing only the included delta paths and explicitly instructing it to ignore that change's \`excludedDeltas\`. Wait for it to finish. - For conflicts, apply in resolved order. - Do not delegate to a background task — step 8c would move \`changeRoot\` out from under a sync that is still reading it. + - If a change has no included delta specs, do not run the sync workflow for it. - b. **Verify main specs before moving changeRoot**: - - Re-run the comparison against every capability that has a delta spec in \`artifactPaths.specs.existingOutputPaths\` against main spec at \`/openspec/specs//spec.md\` (use the store-aware \`planningHome.root\` from step 3 status JSON, not a hardcoded repo path). + b. **Verify included delta specs before moving changeRoot**: + - Re-run the comparison only for delta specs in \`includedDeltas\` against main spec at \`/openspec/specs//spec.md\` (use the store-aware \`planningHome.root\` from step 3 status JSON, not a hardcoded repo path). - Verify that main specs are updated: - ADDED requirements present - MODIFIED requirements carrying scenario and description changes named in the delta, with their other scenarios intact - REMOVED requirements gone - RENAMED requirements present under the new name and absent under the old one + - Do not verify delta specs in \`excludedDeltas\`; they are intentionally left unsynced. - If sync failed or any capability does not match verification, report what differs and fail/skip moving that change's \`changeRoot\` — do not archive that change. \`changeRoot\` remains intact. c. **Perform the archive**: @@ -429,6 +449,7 @@ ${STORE_SELECTION_GUIDANCE} - Success: archived successfully - Failed: error during archive or spec verification (record error) - Skipped: user chose not to archive (if applicable) + - Sync skipped: for every delta in \`excludedDeltas\`, report \`sync skipped\` with the change, capability, and recorded reason. This is distinct from skipping the archive. 9. **Display summary** @@ -447,7 +468,8 @@ ${STORE_SELECTION_GUIDANCE} Spec sync summary: - 4 delta specs synced to main specs - - 1 conflict resolved (auth: applied both in chronological order) + - 1 delta spec sync skipped (add-jwt/auth: implementation not found) + - 1 conflict resolved (auth: synced add-oauth, skipped add-jwt) \`\`\` If any failures: @@ -539,7 +561,9 @@ No active changes found. Create a new change to get started. - Preserve .openspec.yaml when moving to archive - Archive directory target uses current date: YYYY-MM-DD-; a name that already starts with a \`YYYY-MM-DD-\` prefix is used as-is (never stack a second date) - If archive target exists, fail that change but continue with others -- If sync is requested, run the \`/opsx:sync\` workflow inline (agent-driven) for each change +- If sync is requested, run the \`/opsx:sync\` workflow inline (agent-driven) for each change with included delta specs +- Carry the per-delta \`includedDeltas\` and \`excludedDeltas\` decisions into execution; sync and verify only included deltas +- Report every excluded delta as \`sync skipped\` without treating the archive itself as skipped - Never archive a change while a spec sync is still in flight — run the sync inline and verify main specs at \`/openspec/specs//spec.md\` before moving \`changeRoot\`` }; } diff --git a/test/core/templates/skill-templates-parity.test.ts b/test/core/templates/skill-templates-parity.test.ts index fb3c43c5a8..79ad6636a3 100644 --- a/test/core/templates/skill-templates-parity.test.ts +++ b/test/core/templates/skill-templates-parity.test.ts @@ -50,12 +50,12 @@ const EXPECTED_FUNCTION_HASHES: Record = { getOpsxApplyCommandTemplate: '147408d7085b468981a400cc725804252c3fd84e519c57c5f6f83562e32606ee', getOpsxFfCommandTemplate: '264b514cc4849f91fb4414f639484c4181f1e5850d0d788ef276c851efa92859', getArchiveChangeSkillTemplate: '206a22b6778e97c30da9145ef51fdad449b8c995538f6fc25752ef551a37b675', - getBulkArchiveChangeSkillTemplate: 'e8f127adbbc39fabac09a389688126d043f4a88acb97c8d8191a47f2440fa05d', + getBulkArchiveChangeSkillTemplate: '7e6ce18ab3c70e38bb73d507d043d23e4d2e274f4759925cccc92a2d10dc36e6', getOpsxSyncCommandTemplate: 'df0240a79f7b4943a54c7413ab088ee48f5bf5fe19f9347c170d695c8ec777a4', getVerifyChangeSkillTemplate: 'cab4db01b5d2b1243d63d90c53747d8b39e488c60f76eba3fe8b994467f69267', getOpsxArchiveCommandTemplate: '2f337c6dfb5f988cf994e5637433c5a90d5d3b66ec20ecc1a2b723050a872efd', getOpsxOnboardCommandTemplate: '16a68b8c9819e2a7bab013c3b49a3e49ea258b68c4e7f47f0d598e30815e0a80', - getOpsxBulkArchiveCommandTemplate: 'f26699de74499c5699dfafa5a3678f05afc9284c9a9891220f30e0a2c3e2fcc6', + getOpsxBulkArchiveCommandTemplate: '75cce2b09ad484f75862f0e96afb8beedab7121f03a1b86a59d852dedd43d00b', getOpsxVerifyCommandTemplate: 'f01c0c0cef53be0956de52363d955d4ace131b1b2d77adf902f35fead9a1486d', getOpsxProposeSkillTemplate: '57fb556a060e2eb246b500922837af7573a6e100a6ed7dfaa7bd4ce0f5daffd3', getOpsxProposeCommandTemplate: '434cae3ee20835725bb1d2ccb9698310a850c5b95ed669ea15fc7a0125371c59', @@ -72,7 +72,7 @@ const EXPECTED_GENERATED_SKILL_CONTENT_HASHES: Record = { 'openspec-ff-change': 'ff3bd3eac427a1e50071ad7c70f73b556cffa3db43e90da2726e96849c3fc886', 'openspec-sync-specs': '74de778dd8a8fd4987a09621147358cc32505bb58110492ab2b4ffe7f35aa48f', 'openspec-archive-change': '64b1611dd7aee04ca268820d1b193e8bf0a39ff3672ec6ba21fb0a1bcb1786c2', - 'openspec-bulk-archive-change': '8a660127e6b889a039eca702ffa8c0358ad97c61a18a8381c3ed2f212b94147f', + 'openspec-bulk-archive-change': 'f97606104b079e7bb925fef8530b81ee9d1907db73e736aa61f80b13db43531a', 'openspec-verify-change': '57693d22940f06080c6cf8d590ac2f48240d4a5e9ce7074dacd0f8d3c9945afa', 'openspec-onboard': '1d581c12d4928d751eb79de099e275dabe9c99fc15dc1f502abebd99ad7cb7d2', 'openspec-propose': '4638400113946f4f1ee9f0bd0e965aafb200bd89b64ec7f5406ef5e948e8e218', @@ -266,7 +266,7 @@ describe('skill templates split parity', () => { for (const [variant, content] of variants) { expect(content, variant).toContain('Do not delegate to a background task'); expect(content, variant).toContain('Never archive a change while a spec sync is still in flight'); - expect(content, variant).toContain('Verify main specs before moving changeRoot'); + expect(content, variant).toContain('Verify included delta specs before moving changeRoot'); // Verification must follow delta semantics. expect(content, variant).toContain('MODIFIED requirements carrying scenario and description changes'); @@ -278,6 +278,41 @@ describe('skill templates split parity', () => { } }); + it('carries mixed included and excluded bulk-archive deltas through both generated variants', () => { + const variants: Array<[string, string]> = [ + [ + 'bulk skill', + generateSkillContent(getBulkArchiveChangeSkillTemplate(), 'PARITY-BASELINE'), + ], + ['bulk opsx command', getOpsxBulkArchiveCommandTemplate().content], + ]; + + for (const [variant, content] of variants) { + expect(content, variant).toContain( + 'An inclusion or exclusion decision for every delta spec' + ); + expect(content, variant).toContain( + 'A single change can have both included and excluded delta specs' + ); + expect(content, variant).toContain( + 'passing only the included delta paths and explicitly instructing it to ignore' + ); + expect(content, variant).not.toContain( + 'for each change, passing the delta spec analysis' + ); + expect(content, variant).toContain( + 'Re-run the comparison only for delta specs in `includedDeltas`' + ); + expect(content, variant).toContain( + 'Do not verify delta specs in `excludedDeltas`' + ); + expect(content, variant).toContain('report `sync skipped`'); + expect(content, variant).toContain( + '`sync skipped` without treating the archive itself as skipped' + ); + } + }); + // The archive instructions must mirror `openspec archive`'s date-prefix // rule (#1316): a change already named with a `YYYY-MM-DD-` prefix keeps // its name, so archived names never stack dates. Guard the caveat, the From 62bb846de93e57d735212cff68df32daefc2c2f0 Mon Sep 17 00:00:00 2001 From: Clay Good Date: Mon, 27 Jul 2026 21:01:03 -0500 Subject: [PATCH 5/6] fix(adapters): honor caller delta subsets, escape control characters, close test gaps MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adversarial review of the merged branch turned up four gaps: - Bulk archive tells the sync workflow to ignore `excludedDeltas`, but main's sync-specs calls `existingOutputPaths` the "complete list" of delta specs. An agent following both would sync the delta the caller withheld, step 8b would not catch it (it verifies only included deltas), and the run would still report `sync skipped`. Sync now honors a caller-supplied subset, mirroring the inline rule-snapshot handoff main already added. - escapeYamlValue left C0/DEL/C1 control characters raw. The repo's own parser accepts them, so tests passed while stricter parsers used by other tools reject the document. Emit them as \xHH. - The adapter matrix was a hand-maintained list driving only `description`, so raw interpolation in lingma's name/category/tags — and any newly registered adapter — passed green. It now derives from the registry and drives every string field. - Four bulk-archive template lines were guarded only by golden hashes, which this repo regenerates as routine. Also corrects the escapeYamlValue docstring, which still described the pre-PR conditional-quoting behavior, and adds the missing changeset. Co-Authored-By: Claude Opus 5 (1M context) --- .changeset/command-adapter-yaml-escaping.md | 9 ++ skills/openspec-sync-specs/SKILL.md | 11 +- src/core/command-generation/yaml.ts | 26 +++- src/core/templates/workflows/sync-specs.ts | 22 ++- test/core/command-generation/adapters.test.ts | 128 +++++++++++++----- test/core/command-generation/yaml.test.ts | 48 +++++++ .../templates/skill-templates-parity.test.ts | 56 +++++++- 7 files changed, 251 insertions(+), 49 deletions(-) create mode 100644 .changeset/command-adapter-yaml-escaping.md diff --git a/.changeset/command-adapter-yaml-escaping.md b/.changeset/command-adapter-yaml-escaping.md new file mode 100644 index 0000000000..b025426dab --- /dev/null +++ b/.changeset/command-adapter-yaml-escaping.md @@ -0,0 +1,9 @@ +--- +'@fission-ai/openspec': patch +--- + +Generated tool command files now carry valid YAML frontmatter for every supported tool. Command names ship as `OPSX: Explore`, and the unquoted `name: OPSX: Explore` that adapters emitted is not parseable YAML — strict parsers rejected the whole file, so the command failed to load. Several adapters also re-implemented their own escaping, and a few interpolated descriptions in raw. + +Escaping now lives in one place (`escapeYamlValue` / `formatTagsArray`) and every adapter uses it. String frontmatter values are always double-quoted, which also keeps values like `true`, `null` and `123` from round-tripping as booleans, nulls and numbers. Non-string fields such as `allowed-tools` and `invokable` are unchanged. Expect the first `openspec update` after upgrading to rewrite the frontmatter lines of your generated command files. + +Archive workflow guidance also gets two corrections: bulk archive now carries its per-delta include/exclude decisions into execution, so a delta whose implementation was not found is reported as `sync skipped` instead of being synced anyway, and both archive workflows verify the main specs before moving the change directory. diff --git a/skills/openspec-sync-specs/SKILL.md b/skills/openspec-sync-specs/SKILL.md index ec5579e2a4..bc0a475f95 100644 --- a/skills/openspec-sync-specs/SKILL.md +++ b/skills/openspec-sync-specs/SKILL.md @@ -39,11 +39,19 @@ This is an **agent-driven** operation - you will read delta specs and directly e 3. **Find delta specs** Use `artifactPaths.specs.existingOutputPaths` from the status JSON as the - complete list of delta spec files. If the `specs` entry is missing or + only source of delta spec paths. If the `specs` entry is missing or `existingOutputPaths` is empty, report that there are no delta specs to sync, do not infer them from other artifacts, and stop without requesting artifact instructions or writing a main spec. + Sync every path in `existingOutputPaths` unless a caller narrowed the set: + if archive invoked this workflow inline and supplied an explicit subset of + `existingOutputPaths` to sync, sync only that subset and leave the remaining + delta specs untouched — bulk archive excludes a delta whose implementation it + could not find, and syncing it anyway would write a main spec the caller + deliberately withheld. Never sync a path outside `existingOutputPaths`, and + never widen a supplied subset back to the full list. + Each delta spec file contains sections like: - `## ADDED Requirements` - New requirements to add - `## MODIFIED Requirements` - Changes to existing requirements @@ -201,6 +209,7 @@ Main specs are now updated. The change remains active - archive when implementat - Show what you're changing as you go - The operation should be idempotent - running twice should give same result - Use only `artifactPaths.specs.existingOutputPaths`; never infer delta specs from unrelated artifacts +- Honor a caller-supplied subset of `existingOutputPaths`; never widen it back to the full list - Fetch specs instructions once for direct sync, or reuse the archive-supplied snapshot inline - Stop before every main-spec write on a non-zero or invalid JSON specs-instruction response - Artifact rules constrain only the specs being written and are never copied into output files diff --git a/src/core/command-generation/yaml.ts b/src/core/command-generation/yaml.ts index cb1d44e86e..766faa993d 100644 --- a/src/core/command-generation/yaml.ts +++ b/src/core/command-generation/yaml.ts @@ -10,21 +10,33 @@ /** * Escapes a string value for safe YAML output. * - * Quotes the value with double quotes when it contains characters that - * carry special meaning in YAML (or leading/trailing whitespace), and - * escapes the characters that are not representable verbatim inside a - * double-quoted scalar: backslash, double quote, line feed and carriage - * return. Values without special characters are returned unquoted. + * Always emits a double-quoted scalar. Quoting unconditionally keeps the + * value a string no matter what it holds: an unquoted `true`, `null` or + * `123` would round-trip as a boolean, null or number, and an unquoted + * value opening with a block indicator (`|`, `>`) or containing `: ` + * is not valid YAML at all. + * + * Inside the quotes it escapes everything that cannot appear verbatim in a + * double-quoted scalar: backslash, double quote, line feed, carriage + * return, and the non-printable characters YAML's `c-printable` production + * excludes (C0 controls, DEL and C1 controls). Lenient parsers accept a raw + * control byte, but strict ones reject the document outright, so escaping + * them here keeps the generated file portable across every tool's parser. * * @param value - The raw string to embed in YAML frontmatter. - * @returns The value, double-quoted and escaped when necessary. + * @returns The value as an escaped, double-quoted YAML scalar. */ export function escapeYamlValue(value: string): string { const escaped = value .replace(/\\/g, '\\\\') .replace(/"/g, '\\"') .replace(/\n/g, '\\n') - .replace(/\r/g, '\\r'); + .replace(/\r/g, '\\r') + // Remaining non-printables have no dedicated escape; emit them as \xHH. + .replace( + /[\x00-\x08\x0b\x0c\x0e-\x1f\x7f-\x9f]/g, + (char) => `\\x${char.charCodeAt(0).toString(16).padStart(2, '0')}` + ); return `"${escaped}"`; } diff --git a/src/core/templates/workflows/sync-specs.ts b/src/core/templates/workflows/sync-specs.ts index 6bcd4c5f4c..de2e2cd12d 100644 --- a/src/core/templates/workflows/sync-specs.ts +++ b/src/core/templates/workflows/sync-specs.ts @@ -41,11 +41,19 @@ ${STORE_SELECTION_GUIDANCE} 3. **Find delta specs** Use \`artifactPaths.specs.existingOutputPaths\` from the status JSON as the - complete list of delta spec files. If the \`specs\` entry is missing or + only source of delta spec paths. If the \`specs\` entry is missing or \`existingOutputPaths\` is empty, report that there are no delta specs to sync, do not infer them from other artifacts, and stop without requesting artifact instructions or writing a main spec. + Sync every path in \`existingOutputPaths\` unless a caller narrowed the set: + if archive invoked this workflow inline and supplied an explicit subset of + \`existingOutputPaths\` to sync, sync only that subset and leave the remaining + delta specs untouched — bulk archive excludes a delta whose implementation it + could not find, and syncing it anyway would write a main spec the caller + deliberately withheld. Never sync a path outside \`existingOutputPaths\`, and + never widen a supplied subset back to the full list. + Each delta spec file contains sections like: - \`## ADDED Requirements\` - New requirements to add - \`## MODIFIED Requirements\` - Changes to existing requirements @@ -203,6 +211,7 @@ Main specs are now updated. The change remains active - archive when implementat - Show what you're changing as you go - The operation should be idempotent - running twice should give same result - Use only \`artifactPaths.specs.existingOutputPaths\`; never infer delta specs from unrelated artifacts +- Honor a caller-supplied subset of \`existingOutputPaths\`; never widen it back to the full list - Fetch specs instructions once for direct sync, or reuse the archive-supplied snapshot inline - Stop before every main-spec write on a non-zero or invalid JSON specs-instruction response - Artifact rules constrain only the specs being written and are never copied into output files`, @@ -248,11 +257,19 @@ ${STORE_SELECTION_GUIDANCE} 3. **Find delta specs** Use \`artifactPaths.specs.existingOutputPaths\` from the status JSON as the - complete list of delta spec files. If the \`specs\` entry is missing or + only source of delta spec paths. If the \`specs\` entry is missing or \`existingOutputPaths\` is empty, report that there are no delta specs to sync, do not infer them from other artifacts, and stop without requesting artifact instructions or writing a main spec. + Sync every path in \`existingOutputPaths\` unless a caller narrowed the set: + if archive invoked this workflow inline and supplied an explicit subset of + \`existingOutputPaths\` to sync, sync only that subset and leave the remaining + delta specs untouched — bulk archive excludes a delta whose implementation it + could not find, and syncing it anyway would write a main spec the caller + deliberately withheld. Never sync a path outside \`existingOutputPaths\`, and + never widen a supplied subset back to the full list. + Each delta spec file contains sections like: - \`## ADDED Requirements\` - New requirements to add - \`## MODIFIED Requirements\` - Changes to existing requirements @@ -410,6 +427,7 @@ Main specs are now updated. The change remains active - archive when implementat - Show what you're changing as you go - The operation should be idempotent - running twice should give same result - Use only \`artifactPaths.specs.existingOutputPaths\`; never infer delta specs from unrelated artifacts +- Honor a caller-supplied subset of \`existingOutputPaths\`; never widen it back to the full list - Fetch specs instructions once for direct sync, or reuse the archive-supplied snapshot inline - Stop before every main-spec write on a non-zero or invalid JSON specs-instruction response - Artifact rules constrain only the specs being written and are never copied into output files` diff --git a/test/core/command-generation/adapters.test.ts b/test/core/command-generation/adapters.test.ts index ec87ad5f72..d11618577b 100644 --- a/test/core/command-generation/adapters.test.ts +++ b/test/core/command-generation/adapters.test.ts @@ -28,7 +28,12 @@ import { roocodeAdapter } from '../../../src/core/command-generation/adapters/ro import { traeAdapter } from '../../../src/core/command-generation/adapters/trae.js'; import { windsurfAdapter } from '../../../src/core/command-generation/adapters/windsurf.js'; import { zcodeAdapter } from '../../../src/core/command-generation/adapters/zcode.js'; -import type { CommandContent } from '../../../src/core/command-generation/types.js'; +import type { + CommandContent, + ToolCommandAdapter, +} from '../../../src/core/command-generation/types.js'; +import { CommandAdapterRegistry } from '../../../src/core/command-generation/registry.js'; +import { parse as parseYaml } from 'yaml'; describe('command-generation/adapters', () => { const sampleContent: CommandContent = { @@ -950,32 +955,58 @@ describe('command-generation/adapters', () => { }); describe('YAML frontmatter escaping across adapters', () => { - const yamlAdapters = [ - amazonQAdapter, - antigravityAdapter, - auggieAdapter, - bobAdapter, - claudeAdapter, - codebuddyAdapter, - continueAdapter, - costrictAdapter, - crushAdapter, - cursorAdapter, - factoryAdapter, - githubCopilotAdapter, - iflowAdapter, - junieAdapter, - kiroAdapter, - lingmaAdapter, - ohMyPiAdapter, - opencodeAdapter, - piAdapter, - qoderAdapter, - qwenAdapter, - traeAdapter, - windsurfAdapter, - zcodeAdapter, - ]; + // Derived from the registry, not hand-listed: a newly registered adapter + // must be covered by default. Adding one that emits no YAML frontmatter is + // then a deliberate act of adding it here. + const NON_YAML_ADAPTERS = ['cline', 'kilocode', 'roocode', 'gemini']; + const yamlAdapters = CommandAdapterRegistry.getAll().filter( + (adapter) => !NON_YAML_ADAPTERS.includes(adapter.toolId) + ); + + /** + * Builds a CommandContent whose every string field carries `marker`. + */ + function contentWith(marker: string): CommandContent { + return { + id: 'explore', + name: marker, + description: marker, + category: marker, + tags: [marker, 'explore'], + body: 'Body text', + }; + } + + /** + * Returns the frontmatter fields this adapter fills from CommandContent, + * found by rendering two different markers and keeping the fields that + * change. Fields derived from the command id (Cursor's `name`/`id`) or + * emitted as constants stay put and are excluded. + */ + function contentDerivedFields(adapter: ToolCommandAdapter): string[] { + const render = (marker: string): Record => { + const match = adapter.formatFile(contentWith(marker)).match(/^---\n([\s\S]*?)\n---/); + return (parseYaml(match![1]) ?? {}) as Record; + }; + const left = render('AAA-marker'); + const right = render('BBB-marker'); + return Object.keys(left).filter( + (key) => JSON.stringify(left[key]) !== JSON.stringify(right[key]) + ); + } + + it('covers every registered YAML adapter', () => { + const baseline = contentWith('Baseline'); + expect(yamlAdapters.length).toBeGreaterThan(0); + for (const adapter of yamlAdapters) { + expect(adapter.formatFile(baseline), adapter.toolId).toMatch(/^---\n/); + } + for (const toolId of NON_YAML_ADAPTERS) { + const adapter = CommandAdapterRegistry.get(toolId); + expect(adapter, `${toolId} is excluded but not registered`).toBeDefined(); + expect(adapter!.formatFile(baseline), toolId).not.toMatch(/^---\n/); + } + }); const roundTripCases: Array<[string, string]> = [ ['plain text', 'Enter explore mode for thinking'], @@ -1016,28 +1047,53 @@ describe('command-generation/adapters', () => { for (const adapter of yamlAdapters) { describe(`${adapter.toolId} adapter table-driven round-trip`, () => { for (const [label, testVal] of roundTripCases) { - it(`preserves description value and string type for ${label}`, () => { + it(`preserves every string field and its type for ${label}`, () => { + // Every string field carries the hostile value, not just + // description: a field an adapter forgot to escape is only caught + // if the matrix actually drives that field. const content: CommandContent = { id: 'explore', - name: 'OpenSpec Explore', + name: testVal, description: testVal, - category: 'Workflow', - tags: ['workflow', 'explore'], + category: testVal, + tags: [testVal, 'explore'], body: 'Body text', }; const fileContent = adapter.formatFile(content); const frontmatterMatch = fileContent.match(/^---\n([\s\S]*?)\n---/); expect(frontmatterMatch).not.toBeNull(); + const frontmatter = frontmatterMatch![1]; + + // A raw CR survives the parser but corrupts the file for anything + // that splits on lines, so round-tripping alone would not catch it. + expect(frontmatter, 'raw carriage return in frontmatter').not.toContain('\r'); - const { parse: parseYaml } = require('yaml'); - let parsed: any; + let parsed: Record | undefined; expect(() => { - parsed = parseYaml(frontmatterMatch![1]); + parsed = parseYaml(frontmatter); }).not.toThrow(); - expect(parsed.description).toBe(testVal); - expect(typeof parsed.description).toBe('string'); + // Adapters emit different field subsets, and some derive name/id + // from the command id rather than from the content. Identify the + // content-derived fields by rendering a second time with a + // different value and seeing which outputs move — a field that is + // constant across both renders never carried our input, so it has + // nothing to round-trip. This must not be softened into "skip the + // field if it doesn't look like our value": a broken escape mangles + // the value, and skipping on mismatch would skip the very bug. + const contentFields = contentDerivedFields(adapter); + expect(contentFields.length, `${adapter.toolId} emits no content fields`) + .toBeGreaterThan(0); + + for (const field of contentFields) { + if (field === 'tags') { + expect(parsed!.tags, `${adapter.toolId}.tags`).toEqual([testVal, 'explore']); + continue; + } + expect(parsed![field], `${adapter.toolId}.${field}`).toBe(testVal); + expect(typeof parsed![field], `${adapter.toolId}.${field} type`).toBe('string'); + } }); } }); diff --git a/test/core/command-generation/yaml.test.ts b/test/core/command-generation/yaml.test.ts index 21b2944e4a..a19be2946d 100644 --- a/test/core/command-generation/yaml.test.ts +++ b/test/core/command-generation/yaml.test.ts @@ -63,6 +63,13 @@ describe('command-generation/yaml escapeYamlValue', () => { ['carriage return', 'Line 1\rLine 2'], ['crlf', 'Line 1\r\nLine 2'], ['mixed special', 'a: "b"\r\n#c\\d'], + ['tab', 'column\tseparated'], + ['escape', 'ansi\x1b[0m reset'], + ['vertical tab', 'a\x0bb'], + ['form feed', 'a\x0cb'], + ['nul', 'a\x00b'], + ['delete', 'a\x7fb'], + ['next line', 'a\x85b'], ]; for (const [label, value] of cases) { @@ -71,4 +78,45 @@ describe('command-generation/yaml escapeYamlValue', () => { }); } }); + + // YAML's c-printable production excludes the C0 controls, DEL and the C1 + // range. A raw control byte inside a double-quoted scalar is accepted by + // lenient parsers (including the one this suite uses) but rejected outright + // by stricter ones, so the generated file has to carry them as \xHH escapes + // to load in every tool. + describe('escapes non-printable characters rather than emitting them raw', () => { + const cases: Array<[string, string, string]> = [ + ['nul', '\x00', '"\\x00"'], + ['backspace', '\x08', '"\\x08"'], + ['vertical tab', '\x0b', '"\\x0b"'], + ['form feed', '\x0c', '"\\x0c"'], + ['escape', '\x1b', '"\\x1b"'], + ['delete', '\x7f', '"\\x7f"'], + ['next line', '\x85', '"\\x85"'], + ]; + + for (const [label, value, expected] of cases) { + it(`escapes ${label}`, () => { + expect(escapeYamlValue(value)).toBe(expected); + }); + } + + it('leaves tab, line feed and carriage return on their own escapes', () => { + expect(escapeYamlValue('\t')).toBe('"\t"'); + expect(escapeYamlValue('\n')).toBe('"\\n"'); + expect(escapeYamlValue('\r')).toBe('"\\r"'); + }); + + it('emits no raw control byte for any code point below U+00A0', () => { + for (let code = 0; code < 0xa0; code += 1) { + const emitted = escapeYamlValue(String.fromCharCode(code)); + // Tab is the one non-printable YAML allows verbatim. + if (code === 0x09) continue; + expect( + /[\x00-\x08\x0a-\x1f\x7f-\x9f]/.test(emitted), + `code point ${code} emitted raw` + ).toBe(false); + } + }); + }); }); diff --git a/test/core/templates/skill-templates-parity.test.ts b/test/core/templates/skill-templates-parity.test.ts index 04a54cc30f..348930b12f 100644 --- a/test/core/templates/skill-templates-parity.test.ts +++ b/test/core/templates/skill-templates-parity.test.ts @@ -42,7 +42,7 @@ const EXPECTED_FUNCTION_HASHES: Record = { getContinueChangeSkillTemplate: 'bb3e6440eeae417a8f7efd1c064024ab2fcf824ff2adbf37cfc2607a2c8c6249', getApplyChangeSkillTemplate: '031cf8f8ffc2937fc4051651bd5e1fc6159bfd225605d8e4c3181054a4e52b38', getFfChangeSkillTemplate: '225a8eaf1b3769ac5d43e079297c5fa9cc20fc2e34fec9bb0d887c8c1fb0ea71', - getSyncSpecsSkillTemplate: '125672d288cb990759679c2aa2976fccb9c13cceb2af43a89f99dd1aae9bc397', + getSyncSpecsSkillTemplate: '2e672cc4c60df64062cd654109554744c312d175e06abc245123b6bddad84ff9', getOnboardSkillTemplate: '31dffc7c3b8d75ffbd59ed751d6a1550b885b20ef90e12d236262127ee4021e9', getOpsxExploreCommandTemplate: 'e9674ddace813e685b0e9fe37149140a3d33d48aa20b9ba2b0963a7c49c9aea7', getOpsxNewCommandTemplate: '652adc870f16bb260d54436356132b6ee051a9ed7cc0464603fb31f4db259762', @@ -51,7 +51,7 @@ const EXPECTED_FUNCTION_HASHES: Record = { getOpsxFfCommandTemplate: '678375642a21d255444f0ba717e659abb2cc2b7474981d52eae900a0793e3e4d', getArchiveChangeSkillTemplate: 'd325f65b26dccba084ace510874cf92b73cecdc430d93b2d73dd0066b95619a3', getBulkArchiveChangeSkillTemplate: 'de198c7b7c1472773b013b9af917de27773fd613083309f0e8e607c005c92d3d', - getOpsxSyncCommandTemplate: 'a1404217de12a9ca31b2abe66c352ce47e5f362fb016e3650655cc599b94430a', + getOpsxSyncCommandTemplate: '5432a035bdc6799bdf7fe23451988182582dbcfdccc9dea95ae97390991ebda5', getVerifyChangeSkillTemplate: '4af69762ff061c1a76dad21725827d87b168dca8bd0c4cea133152e37cacc2ce', getOpsxArchiveCommandTemplate: '88f8b83973b2803975c89117027d2172c3376b066276e3b0025d3b8e0e8ec597', getOpsxOnboardCommandTemplate: 'e69a5aa37749727290c05b687981dd69f3b17a55514a118d088c4124c5fd8505', @@ -70,7 +70,7 @@ const EXPECTED_GENERATED_SKILL_CONTENT_HASHES: Record = { 'openspec-continue-change': '0d3fe07961b061a9bac0d18f98891038ffd89f70c4f3d987fc997379a9e6e9f4', 'openspec-apply-change': '49fc5772404e3033085384ee214c44488c93880a596a9a05dcad42f9ce86cf83', 'openspec-ff-change': '4228d75e3571097164f2360e2ad3063a5b88d44750078c3601b23a89e74c1de6', - 'openspec-sync-specs': '7eae5d8a46b8b81bd6acad9b78f5dae25e3f848052bebb291a494ed0c0f9ea67', + 'openspec-sync-specs': 'da7a6d5194cc6165c3f0f02306336c10297ca94c53fb90d8f761d365c6b97241', 'openspec-archive-change': 'bd30f9c1f5979c4b469796dc231c5ad3be3c9ede54c8eb92c5b5f96b35241265', 'openspec-bulk-archive-change': '5ac320e2004e453c78541233f48e5f6e246cc674a44f1e427cecb7b2e9587f9b', 'openspec-verify-change': '0b087d5428df63145f4853a3b136eca522e3a9cbe88047fb30e5f774d873adf4', @@ -310,6 +310,56 @@ describe('skill templates split parity', () => { expect(content, variant).toContain( '`sync skipped` without treating the archive itself as skipped' ); + + // These three carried no assertion, so deleting any of them from a + // single variant was caught only by the golden hash — and this repo + // regenerates hashes as a matter of routine, which makes that no + // protection at all. + expect(content, variant).toContain( + '`includedDeltas`: all non-conflicting delta specs from confirmed changes plus conflict deltas selected for sync' + ); + expect(content, variant).toContain( + '`excludedDeltas`: conflict deltas from confirmed changes excluded because their implementation is missing' + ); + expect(content, variant).toContain( + 'Carry the per-delta `includedDeltas` and `excludedDeltas` decisions into execution' + ); + // The worked example must show the skip, or the agent has no model of + // what a partially-synced batch report looks like. + expect(content, variant).toContain( + '1 delta spec sync skipped (add-jwt/auth: implementation not found)' + ); + } + }); + + it('lets the sync workflow honor the delta subset bulk archive hands it', () => { + // Bulk archive tells sync to ignore excludedDeltas, but sync treats + // existingOutputPaths as its own source of truth. Without an explicit + // carve-out the callee re-syncs the delta the caller withheld, step 8b + // never checks it (it verifies only includedDeltas), and the run still + // reports `sync skipped` for a spec that was in fact written. + const variants: Array<[string, string]> = [ + ['sync skill', getSyncSpecsSkillTemplate().instructions], + ['sync command', getOpsxSyncCommandTemplate().content], + ]; + + for (const [variant, content] of variants) { + expect(content, variant).toContain( + 'supplied an explicit subset of' + ); + expect(content, variant).toContain( + 'sync only that subset and leave the remaining' + ); + expect(content, variant).toContain( + 'never widen a supplied subset back to the full list' + ); + expect(content, variant).toContain( + 'Honor a caller-supplied subset of `existingOutputPaths`' + ); + + // The pre-merge wording called existingOutputPaths the "complete list", + // which reads as an instruction to override the caller's exclusions. + expect(content, variant).not.toContain('complete list of delta spec files'); } }); From 5bfd5749572bbe145d620bd32e6346b0123c94a9 Mon Sep 17 00:00:00 2001 From: Clay Good Date: Mon, 27 Jul 2026 21:22:39 -0500 Subject: [PATCH 6/6] fix(templates): iterate the selected delta subset, not the full CLI list MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A second adversarial pass found the previous fix incomplete. Narrowing step 3 ("Find delta specs") left step 4 — the loop that actually applies the changes — still reading "for each capability delta spec path returned by the CLI". An agent treating step 3 as descriptive and step 4 as operative re-widens to the full list and syncs the delta bulk archive withheld: the original defect, one step further down the template. Step 4 now iterates the step-3 selection, and the parity test pins both the new wording and the absence of the old. Also from that pass: - Generalize the carve-out beyond archive. It was conditioned on "archive invoked this workflow inline", so a user asking /opsx:sync to sync one delta read as an instruction to ignore them. - Define the two undefined edges: a named path outside existingOutputPaths, and an empty named list. Both stop and report rather than proceeding on a guess. - Drive control characters through the adapter matrix. It drove none, so the escaping this suite exists to prove had no adapter-level coverage and the raw-CR assertion could never fail. Verified live by mutation. - Give contentDerivedFields two markers that differ in length and shape. Same-shaped markers render identically for a length- or slice-derived field, which would drop it from every assertion silently. Drops the `not.toContain('complete list of delta spec files')` assertion: it banned one exact synonym while any reword of the same conflicting instruction passed, so it read as coverage without being it. Co-Authored-By: Claude Opus 5 (1M context) --- skills/openspec-sync-specs/SKILL.md | 20 ++++++---- src/core/templates/workflows/sync-specs.ts | 40 +++++++++++-------- test/core/command-generation/adapters.test.ts | 16 +++++++- .../templates/skill-templates-parity.test.ts | 34 +++++++++++----- 4 files changed, 75 insertions(+), 35 deletions(-) diff --git a/skills/openspec-sync-specs/SKILL.md b/skills/openspec-sync-specs/SKILL.md index bc0a475f95..b50f84b44c 100644 --- a/skills/openspec-sync-specs/SKILL.md +++ b/skills/openspec-sync-specs/SKILL.md @@ -44,13 +44,17 @@ This is an **agent-driven** operation - you will read delta specs and directly e do not infer them from other artifacts, and stop without requesting artifact instructions or writing a main spec. - Sync every path in `existingOutputPaths` unless a caller narrowed the set: - if archive invoked this workflow inline and supplied an explicit subset of - `existingOutputPaths` to sync, sync only that subset and leave the remaining - delta specs untouched — bulk archive excludes a delta whose implementation it - could not find, and syncing it anyway would write a main spec the caller - deliberately withheld. Never sync a path outside `existingOutputPaths`, and - never widen a supplied subset back to the full list. + Sync every path in `existingOutputPaths` unless the caller narrowed the set. + A caller narrows it by naming an explicit list of delta spec paths to sync — + archive does this inline, and a user can too ("only sync the billing delta"). + Then sync only the named paths and leave the remaining delta specs untouched: + bulk archive excludes a delta whose implementation it could not find, and + syncing it anyway would write a main spec the caller deliberately withheld. + Carry that narrowed selection through step 4; never widen it back to the full + list. If a named path is not in `existingOutputPaths`, do not sync it — + report it and stop, rather than dropping it silently. If the named list is + empty, report that there is nothing to sync and stop without writing a main + spec. Each delta spec file contains sections like: - `## ADDED Requirements` - New requirements to add @@ -78,7 +82,7 @@ This is an **agent-driven** operation - you will read delta specs and directly e selected roots, delta paths, CLI checks, or workflow steps. Use their text as constraints without copying it verbatim into a main spec or summary. - For each capability delta spec path returned by the CLI (these may belong to a selected store, not the repo): + For each capability delta spec path selected in step 3 — the full `existingOutputPaths` list, or the narrowed subset when a caller supplied one (these may belong to a selected store, not the repo): a. **Read the delta spec** to understand the intended changes diff --git a/src/core/templates/workflows/sync-specs.ts b/src/core/templates/workflows/sync-specs.ts index de2e2cd12d..bb3b4b32bf 100644 --- a/src/core/templates/workflows/sync-specs.ts +++ b/src/core/templates/workflows/sync-specs.ts @@ -46,13 +46,17 @@ ${STORE_SELECTION_GUIDANCE} do not infer them from other artifacts, and stop without requesting artifact instructions or writing a main spec. - Sync every path in \`existingOutputPaths\` unless a caller narrowed the set: - if archive invoked this workflow inline and supplied an explicit subset of - \`existingOutputPaths\` to sync, sync only that subset and leave the remaining - delta specs untouched — bulk archive excludes a delta whose implementation it - could not find, and syncing it anyway would write a main spec the caller - deliberately withheld. Never sync a path outside \`existingOutputPaths\`, and - never widen a supplied subset back to the full list. + Sync every path in \`existingOutputPaths\` unless the caller narrowed the set. + A caller narrows it by naming an explicit list of delta spec paths to sync — + archive does this inline, and a user can too ("only sync the billing delta"). + Then sync only the named paths and leave the remaining delta specs untouched: + bulk archive excludes a delta whose implementation it could not find, and + syncing it anyway would write a main spec the caller deliberately withheld. + Carry that narrowed selection through step 4; never widen it back to the full + list. If a named path is not in \`existingOutputPaths\`, do not sync it — + report it and stop, rather than dropping it silently. If the named list is + empty, report that there is nothing to sync and stop without writing a main + spec. Each delta spec file contains sections like: - \`## ADDED Requirements\` - New requirements to add @@ -80,7 +84,7 @@ ${STORE_SELECTION_GUIDANCE} selected roots, delta paths, CLI checks, or workflow steps. Use their text as constraints without copying it verbatim into a main spec or summary. - For each capability delta spec path returned by the CLI (these may belong to a selected store, not the repo): + For each capability delta spec path selected in step 3 — the full \`existingOutputPaths\` list, or the narrowed subset when a caller supplied one (these may belong to a selected store, not the repo): a. **Read the delta spec** to understand the intended changes @@ -262,13 +266,17 @@ ${STORE_SELECTION_GUIDANCE} do not infer them from other artifacts, and stop without requesting artifact instructions or writing a main spec. - Sync every path in \`existingOutputPaths\` unless a caller narrowed the set: - if archive invoked this workflow inline and supplied an explicit subset of - \`existingOutputPaths\` to sync, sync only that subset and leave the remaining - delta specs untouched — bulk archive excludes a delta whose implementation it - could not find, and syncing it anyway would write a main spec the caller - deliberately withheld. Never sync a path outside \`existingOutputPaths\`, and - never widen a supplied subset back to the full list. + Sync every path in \`existingOutputPaths\` unless the caller narrowed the set. + A caller narrows it by naming an explicit list of delta spec paths to sync — + archive does this inline, and a user can too ("only sync the billing delta"). + Then sync only the named paths and leave the remaining delta specs untouched: + bulk archive excludes a delta whose implementation it could not find, and + syncing it anyway would write a main spec the caller deliberately withheld. + Carry that narrowed selection through step 4; never widen it back to the full + list. If a named path is not in \`existingOutputPaths\`, do not sync it — + report it and stop, rather than dropping it silently. If the named list is + empty, report that there is nothing to sync and stop without writing a main + spec. Each delta spec file contains sections like: - \`## ADDED Requirements\` - New requirements to add @@ -296,7 +304,7 @@ ${STORE_SELECTION_GUIDANCE} selected roots, delta paths, CLI checks, or workflow steps. Use their text as constraints without copying it verbatim into a main spec or summary. - For each capability delta spec path returned by the CLI (these may belong to a selected store, not the repo): + For each capability delta spec path selected in step 3 — the full \`existingOutputPaths\` list, or the narrowed subset when a caller supplied one (these may belong to a selected store, not the repo): a. **Read the delta spec** to understand the intended changes diff --git a/test/core/command-generation/adapters.test.ts b/test/core/command-generation/adapters.test.ts index d11618577b..2e813914ac 100644 --- a/test/core/command-generation/adapters.test.ts +++ b/test/core/command-generation/adapters.test.ts @@ -988,8 +988,11 @@ describe('command-generation/adapters', () => { const match = adapter.formatFile(contentWith(marker)).match(/^---\n([\s\S]*?)\n---/); return (parseYaml(match![1]) ?? {}) as Record; }; - const left = render('AAA-marker'); - const right = render('BBB-marker'); + // Deliberately different in length and shape. Two same-shaped markers + // would render identically for a field derived via length or a slice, + // and such a field would then be silently dropped from every assertion. + const left = render('AAA'); + const right = render('zz-BBB-9-longer'); return Object.keys(left).filter( (key) => JSON.stringify(left[key]) !== JSON.stringify(right[key]) ); @@ -1042,6 +1045,15 @@ describe('command-generation/adapters', () => { ['leading space', ' leading'], ['trailing space', 'trailing '], ['multiple spaces', ' '], + // Without these the matrix drives no control character at all, so the + // escaping this suite exists to prove gets no adapter-level coverage — + // and the raw-CR assertion below can never fail. + ['carriage return', 'line 1\rline 2'], + ['line feed', 'line 1\nline 2'], + ['nul', 'a\x00b'], + ['escape', 'ansi\x1b[0m'], + ['delete', 'a\x7fb'], + ['next line', 'a\x85b'], ]; for (const adapter of yamlAdapters) { diff --git a/test/core/templates/skill-templates-parity.test.ts b/test/core/templates/skill-templates-parity.test.ts index 348930b12f..2d2c86e7cd 100644 --- a/test/core/templates/skill-templates-parity.test.ts +++ b/test/core/templates/skill-templates-parity.test.ts @@ -42,7 +42,7 @@ const EXPECTED_FUNCTION_HASHES: Record = { getContinueChangeSkillTemplate: 'bb3e6440eeae417a8f7efd1c064024ab2fcf824ff2adbf37cfc2607a2c8c6249', getApplyChangeSkillTemplate: '031cf8f8ffc2937fc4051651bd5e1fc6159bfd225605d8e4c3181054a4e52b38', getFfChangeSkillTemplate: '225a8eaf1b3769ac5d43e079297c5fa9cc20fc2e34fec9bb0d887c8c1fb0ea71', - getSyncSpecsSkillTemplate: '2e672cc4c60df64062cd654109554744c312d175e06abc245123b6bddad84ff9', + getSyncSpecsSkillTemplate: '2ab06e1cd331debc3056fe992c1e495a26f253214070c254a0bb51657350bd11', getOnboardSkillTemplate: '31dffc7c3b8d75ffbd59ed751d6a1550b885b20ef90e12d236262127ee4021e9', getOpsxExploreCommandTemplate: 'e9674ddace813e685b0e9fe37149140a3d33d48aa20b9ba2b0963a7c49c9aea7', getOpsxNewCommandTemplate: '652adc870f16bb260d54436356132b6ee051a9ed7cc0464603fb31f4db259762', @@ -51,7 +51,7 @@ const EXPECTED_FUNCTION_HASHES: Record = { getOpsxFfCommandTemplate: '678375642a21d255444f0ba717e659abb2cc2b7474981d52eae900a0793e3e4d', getArchiveChangeSkillTemplate: 'd325f65b26dccba084ace510874cf92b73cecdc430d93b2d73dd0066b95619a3', getBulkArchiveChangeSkillTemplate: 'de198c7b7c1472773b013b9af917de27773fd613083309f0e8e607c005c92d3d', - getOpsxSyncCommandTemplate: '5432a035bdc6799bdf7fe23451988182582dbcfdccc9dea95ae97390991ebda5', + getOpsxSyncCommandTemplate: 'de0e4a25d7bbe4f655bdc58bf162def60ce1c26f17238c49b01a0b454202e863', getVerifyChangeSkillTemplate: '4af69762ff061c1a76dad21725827d87b168dca8bd0c4cea133152e37cacc2ce', getOpsxArchiveCommandTemplate: '88f8b83973b2803975c89117027d2172c3376b066276e3b0025d3b8e0e8ec597', getOpsxOnboardCommandTemplate: 'e69a5aa37749727290c05b687981dd69f3b17a55514a118d088c4124c5fd8505', @@ -70,7 +70,7 @@ const EXPECTED_GENERATED_SKILL_CONTENT_HASHES: Record = { 'openspec-continue-change': '0d3fe07961b061a9bac0d18f98891038ffd89f70c4f3d987fc997379a9e6e9f4', 'openspec-apply-change': '49fc5772404e3033085384ee214c44488c93880a596a9a05dcad42f9ce86cf83', 'openspec-ff-change': '4228d75e3571097164f2360e2ad3063a5b88d44750078c3601b23a89e74c1de6', - 'openspec-sync-specs': 'da7a6d5194cc6165c3f0f02306336c10297ca94c53fb90d8f761d365c6b97241', + 'openspec-sync-specs': '097a104e87623c6e26131ad5e6789763dec05863f6a93b9201430b30b455a1df', 'openspec-archive-change': 'bd30f9c1f5979c4b469796dc231c5ad3be3c9ede54c8eb92c5b5f96b35241265', 'openspec-bulk-archive-change': '5ac320e2004e453c78541233f48e5f6e246cc674a44f1e427cecb7b2e9587f9b', 'openspec-verify-change': '0b087d5428df63145f4853a3b136eca522e3a9cbe88047fb30e5f774d873adf4', @@ -345,21 +345,37 @@ describe('skill templates split parity', () => { for (const [variant, content] of variants) { expect(content, variant).toContain( - 'supplied an explicit subset of' + 'A caller narrows it by naming an explicit list of delta spec paths to sync' ); expect(content, variant).toContain( - 'sync only that subset and leave the remaining' + 'sync only the named paths and leave the remaining delta specs untouched' ); expect(content, variant).toContain( - 'never widen a supplied subset back to the full list' + 'never widen it back to the full\n list' ); expect(content, variant).toContain( 'Honor a caller-supplied subset of `existingOutputPaths`' ); - // The pre-merge wording called existingOutputPaths the "complete list", - // which reads as an instruction to override the caller's exclusions. - expect(content, variant).not.toContain('complete list of delta spec files'); + // Step 4 is the operative loop. Narrowing step 3 alone left the loop + // still iterating "each path returned by the CLI", which re-widens the + // set and re-syncs the delta the caller withheld — the original bug, + // one step further down the template. + expect(content, variant).toContain( + 'For each capability delta spec path selected in step 3' + ); + expect(content, variant).not.toContain( + 'For each capability delta spec path returned by the CLI' + ); + + // The undefined edges: a named path outside existingOutputPaths, and an + // empty named list. Both must stop rather than proceed on a guess. + expect(content, variant).toContain( + 'If a named path is not in `existingOutputPaths`, do not sync it' + ); + expect(content, variant).toContain( + 'If the named list is\n empty, report that there is nothing to sync and stop' + ); } });