From 5c8134e9c1562703e751d0241fa97bc1cee5fe1b Mon Sep 17 00:00:00 2001 From: Dwin Gharibi Date: Sun, 6 Sep 2026 10:23:02 +0330 Subject: [PATCH 1/3] fix(update): refresh generated files that drifted --- src/core/shared/tool-detection.ts | 33 +++++- .../tool-detection-command-drift.test.ts | 112 ++++++++++++++++++ 2 files changed, 144 insertions(+), 1 deletion(-) create mode 100644 test/core/shared/tool-detection-command-drift.test.ts diff --git a/src/core/shared/tool-detection.ts b/src/core/shared/tool-detection.ts index 74481c4670..81e6d6cbb0 100644 --- a/src/core/shared/tool-detection.ts +++ b/src/core/shared/tool-detection.ts @@ -361,7 +361,38 @@ export function getToolVersionStatus( } } - const needsUpdate = configured && (generatedByVersion === null || generatedByVersion !== currentVersion); + // 3. A version marker in a skill file only proves the SKILL files came from + // this CLI. It says nothing about the command files written beside them, + // which a user may have hand-edited or a partial write may have truncated. + // Without this, `update` answered "all tools up to date" while a damaged + // command file sat on disk, repairable only by knowing to pass --force. + // The content comparison already exists; it was simply never consulted + // once a skill file supplied a version. + // + // Scoped to tools that have BOTH, so the commands-only path above keeps + // its exact behaviour, and skipped when the delivery mode generates no + // commands for this tool - there would be nothing to compare against, and + // `areCommandFilesUpToDate` reports an empty command set as "not current". + let commandsDrifted = false; + if (skillConfigured && commandConfigured) { + let generatesCommands = true; + try { + generatesCommands = shouldGenerateCommandsForTool( + toolId, + getGlobalConfig().delivery ?? 'both' + ); + } catch { + generatesCommands = true; + } + commandsDrifted = + generatesCommands && !areCommandFilesUpToDate(projectRoot, toolId, options); + } + + const needsUpdate = + configured && + (generatedByVersion === null || + generatedByVersion !== currentVersion || + commandsDrifted); return { toolId, diff --git a/test/core/shared/tool-detection-command-drift.test.ts b/test/core/shared/tool-detection-command-drift.test.ts new file mode 100644 index 0000000000..62f4d6f067 --- /dev/null +++ b/test/core/shared/tool-detection-command-drift.test.ts @@ -0,0 +1,112 @@ +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import { promises as fs } from 'fs'; +import path from 'path'; +import os from 'os'; +import { getToolVersionStatus } from '../../../src/core/shared/tool-detection.js'; + +/** + * `openspec update` decided a tool was current from the `generatedBy` version + * marker in its skill files alone. That marker says nothing about the command + * files written beside them, so a hand-edited or truncated command file left + * `update` reporting "All tool(s) up to date" and repairing nothing - the file + * could only be restored by knowing to pass `--force`. A deleted command file + * was already detected; a damaged one was not. + * + * Every fixture here writes the CURRENT version into the skill marker, so the + * version check is always satisfied and command-file drift is the only thing + * that can move `needsUpdate`. + */ +describe('getToolVersionStatus (command file drift)', () => { + let projectRoot: string; + + const CURRENT = '9.9.9'; + const SKILL_NAMES = [ + 'openspec-propose', + 'openspec-apply-change', + 'openspec-archive-change', + ]; + const COMMAND_DIR = '.claude/commands/opsx'; + + beforeEach(async () => { + projectRoot = await fs.mkdtemp(path.join(os.tmpdir(), 'openspec-drift-')); + }); + afterEach(async () => { + await fs.rm(projectRoot, { recursive: true, force: true }); + }); + + async function writeSkills(version: string) { + for (const name of SKILL_NAMES) { + const dir = path.join(projectRoot, '.claude/skills', name); + await fs.mkdir(dir, { recursive: true }); + await fs.writeFile( + path.join(dir, 'SKILL.md'), + `---\nname: ${name}\ngeneratedBy: "${version}"\n---\n\nbody\n` + ); + } + } + + async function writeCommands(contents: Record) { + const dir = path.join(projectRoot, COMMAND_DIR); + await fs.mkdir(dir, { recursive: true }); + for (const [name, body] of Object.entries(contents)) { + await fs.writeFile(path.join(dir, name), body); + } + } + + it('reads the current version marker back, so the fixture is sound', async () => { + await writeSkills(CURRENT); + + const status = getToolVersionStatus(projectRoot, 'claude', CURRENT); + + expect(status.configured).toBe(true); + expect(status.generatedByVersion).toBe(CURRENT); + }); + + it('leaves a tool with a current marker and no command files alone', async () => { + // No command dir at all: nothing to compare, behaviour unchanged. + await writeSkills(CURRENT); + + const status = getToolVersionStatus(projectRoot, 'claude', CURRENT); + + expect(status.needsUpdate).toBe(false); + }); + + it('flags a tool whose command files no longer match what would be generated', async () => { + await writeSkills(CURRENT); + await writeCommands({ 'propose.md': 'CORRUPTED\n', 'apply.md': 'CORRUPTED\n' }); + + const status = getToolVersionStatus(projectRoot, 'claude', CURRENT); + + expect(status.configured).toBe(true); + // The version marker is current, so only command drift can set this. + expect(status.generatedByVersion).toBe(CURRENT); + expect(status.needsUpdate).toBe(true); + }); + + it('flags a tool whose command file was truncated to nothing', async () => { + await writeSkills(CURRENT); + await writeCommands({ 'propose.md': '' }); + + const status = getToolVersionStatus(projectRoot, 'claude', CURRENT); + + expect(status.generatedByVersion).toBe(CURRENT); + expect(status.needsUpdate).toBe(true); + }); + + it('still flags a stale version marker regardless of command content', async () => { + await writeSkills('0.0.1'); + await writeCommands({ 'propose.md': 'anything\n' }); + + const status = getToolVersionStatus(projectRoot, 'claude', CURRENT); + + expect(status.generatedByVersion).toBe('0.0.1'); + expect(status.needsUpdate).toBe(true); + }); + + it('reports an unconfigured project as needing no update', async () => { + const status = getToolVersionStatus(projectRoot, 'claude', CURRENT); + + expect(status.configured).toBe(false); + expect(status.needsUpdate).toBe(false); + }); +}); From 95b680a2693dbdb70f6b3cea926a0cf46ef96f79 Mon Sep 17 00:00:00 2001 From: Dwin Gharibi Date: Sun, 6 Sep 2026 14:05:46 +0330 Subject: [PATCH 2/3] test(update): isolate command drift from missing files and host config --- .../tool-detection-command-drift.test.ts | 171 +++++++++++++----- 1 file changed, 130 insertions(+), 41 deletions(-) diff --git a/test/core/shared/tool-detection-command-drift.test.ts b/test/core/shared/tool-detection-command-drift.test.ts index 62f4d6f067..ea0cbc7d07 100644 --- a/test/core/shared/tool-detection-command-drift.test.ts +++ b/test/core/shared/tool-detection-command-drift.test.ts @@ -2,38 +2,70 @@ import { describe, it, expect, beforeEach, afterEach } from 'vitest'; import { promises as fs } from 'fs'; import path from 'path'; import os from 'os'; -import { getToolVersionStatus } from '../../../src/core/shared/tool-detection.js'; +import { + getToolVersionStatus, + SKILL_NAMES, +} from '../../../src/core/shared/tool-detection.js'; +import { getCommandContents } from '../../../src/core/shared/skill-generation.js'; +import { + CommandAdapterRegistry, + generateCommands, +} from '../../../src/core/command-generation/index.js'; +import { getProfileWorkflows } from '../../../src/core/profiles.js'; /** * `openspec update` decided a tool was current from the `generatedBy` version - * marker in its skill files alone. That marker says nothing about the command - * files written beside them, so a hand-edited or truncated command file left - * `update` reporting "All tool(s) up to date" and repairing nothing - the file - * could only be restored by knowing to pass `--force`. A deleted command file - * was already detected; a damaged one was not. + * marker in its skill files alone. That marker only proves the SKILL files came + * from this CLI - it says nothing about the command files written beside them, + * which a user may have hand-edited or a partial write may have truncated. So a + * damaged command file left `update` reporting "All tool(s) up to date" and + * repairing nothing, recoverable only by knowing to pass `--force`. * - * Every fixture here writes the CURRENT version into the skill marker, so the - * version check is always satisfied and command-file drift is the only thing - * that can move `needsUpdate`. + * These tests are built so that command-file CONTENT is the only variable: + * + * - The skill marker always carries the version passed to `getToolVersionStatus`, + * so the version check is satisfied and cannot be what moves `needsUpdate`. + * - Every fixture writes the COMPLETE generated command set first and asserts the + * install reads as clean. `areCommandFilesUpToDate` returns false on the first + * MISSING file, before comparing any content, so a partial fixture would pass + * these tests without ever reaching the content comparison they exist to check. + * - Global config is redirected to a temp `XDG_CONFIG_HOME` pinned to + * profile `core` / delivery `both`. The host's own config would otherwise + * decide which commands are expected (a custom profile changes the set) and + * whether commands are compared at all (`delivery: skills` skips them). */ describe('getToolVersionStatus (command file drift)', () => { let projectRoot: string; + let configHome: string; + let previousXdgConfigHome: string | undefined; const CURRENT = '9.9.9'; - const SKILL_NAMES = [ - 'openspec-propose', - 'openspec-apply-change', - 'openspec-archive-change', - ]; - const COMMAND_DIR = '.claude/commands/opsx'; + const TOOL_ID = 'claude'; beforeEach(async () => { projectRoot = await fs.mkdtemp(path.join(os.tmpdir(), 'openspec-drift-')); + configHome = await fs.mkdtemp(path.join(os.tmpdir(), 'openspec-drift-cfg-')); + + previousXdgConfigHome = process.env.XDG_CONFIG_HOME; + process.env.XDG_CONFIG_HOME = configHome; + await fs.mkdir(path.join(configHome, 'openspec'), { recursive: true }); + await fs.writeFile( + path.join(configHome, 'openspec', 'config.json'), + JSON.stringify({ profile: 'core', delivery: 'both' }) + ); }); + afterEach(async () => { + if (previousXdgConfigHome === undefined) { + delete process.env.XDG_CONFIG_HOME; + } else { + process.env.XDG_CONFIG_HOME = previousXdgConfigHome; + } await fs.rm(projectRoot, { recursive: true, force: true }); + await fs.rm(configHome, { recursive: true, force: true }); }); + /** Skill files carrying `version` in their `generatedBy` marker. */ async function writeSkills(version: string) { for (const name of SKILL_NAMES) { const dir = path.join(projectRoot, '.claude/skills', name); @@ -45,66 +77,123 @@ describe('getToolVersionStatus (command file drift)', () => { } } - async function writeCommands(contents: Record) { - const dir = path.join(projectRoot, COMMAND_DIR); - await fs.mkdir(dir, { recursive: true }); - for (const [name, body] of Object.entries(contents)) { - await fs.writeFile(path.join(dir, name), body); + /** + * The exact command files this CLI would generate for the pinned profile, + * written to disk. Returns their absolute paths so a test can damage one. + */ + async function writeGeneratedCommands(): Promise { + const adapter = CommandAdapterRegistry.get(TOOL_ID); + if (!adapter) throw new Error(`no command adapter for ${TOOL_ID}`); + const commands = generateCommands( + getCommandContents(getProfileWorkflows('core')), + adapter + ); + expect(commands.length).toBeGreaterThan(0); + + const written: string[] = []; + for (const command of commands) { + const target = path.isAbsolute(command.path) + ? command.path + : path.join(projectRoot, command.path); + await fs.mkdir(path.dirname(target), { recursive: true }); + await fs.writeFile(target, command.fileContent); + written.push(target); } + return written; } - it('reads the current version marker back, so the fixture is sound', async () => { + /** A complete, current install: current marker plus every generated command. */ + async function writeCleanInstall(): Promise { await writeSkills(CURRENT); + const commands = await writeGeneratedCommands(); + // Guard: the fixture itself must read as clean, otherwise the drift tests + // below could pass on a missing file rather than on changed content. + expect(getToolVersionStatus(projectRoot, TOOL_ID, CURRENT).needsUpdate).toBe(false); + return commands; + } - const status = getToolVersionStatus(projectRoot, 'claude', CURRENT); + it('reads a complete, current install as needing no update', async () => { + await writeCleanInstall(); + + const status = getToolVersionStatus(projectRoot, TOOL_ID, CURRENT); expect(status.configured).toBe(true); expect(status.generatedByVersion).toBe(CURRENT); + expect(status.needsUpdate).toBe(false); }); - it('leaves a tool with a current marker and no command files alone', async () => { - // No command dir at all: nothing to compare, behaviour unchanged. - await writeSkills(CURRENT); + it('flags an otherwise-current install whose command file content was edited', async () => { + const commands = await writeCleanInstall(); + await fs.writeFile(commands[0], 'CORRUPTED\n'); - const status = getToolVersionStatus(projectRoot, 'claude', CURRENT); + const status = getToolVersionStatus(projectRoot, TOOL_ID, CURRENT); - expect(status.needsUpdate).toBe(false); + // Marker still current, every command file still present: only the changed + // content can be what moved this. + expect(status.generatedByVersion).toBe(CURRENT); + expect(status.needsUpdate).toBe(true); }); - it('flags a tool whose command files no longer match what would be generated', async () => { - await writeSkills(CURRENT); - await writeCommands({ 'propose.md': 'CORRUPTED\n', 'apply.md': 'CORRUPTED\n' }); + it('flags an otherwise-current install whose command file was truncated', async () => { + const commands = await writeCleanInstall(); + await fs.writeFile(commands[0], ''); - const status = getToolVersionStatus(projectRoot, 'claude', CURRENT); + const status = getToolVersionStatus(projectRoot, TOOL_ID, CURRENT); - expect(status.configured).toBe(true); - // The version marker is current, so only command drift can set this. expect(status.generatedByVersion).toBe(CURRENT); expect(status.needsUpdate).toBe(true); }); - it('flags a tool whose command file was truncated to nothing', async () => { + it('flags an otherwise-current install whose command file was appended to', async () => { + const commands = await writeCleanInstall(); + const original = await fs.readFile(commands[0], 'utf-8'); + await fs.writeFile(commands[0], `${original}\nMY CUSTOM NOTE\n`); + + const status = getToolVersionStatus(projectRoot, TOOL_ID, CURRENT); + + expect(status.needsUpdate).toBe(true); + }); + + it('also flags a deleted command file, which this status check used to miss', async () => { + const commands = await writeCleanInstall(); + await fs.rm(commands[0]); + + const status = getToolVersionStatus(projectRoot, TOOL_ID, CURRENT); + + expect(status.needsUpdate).toBe(true); + }); + + // `openspec update` already rewrote a DELETED command file before this change, + // but not via this function: `getToolsNeedingProfileSync` catches a missing + // file independently, and its result is unioned with `needsUpdate` in + // update.ts. So deletion is not a behaviour change at the CLI level - it is + // simply now caught here too, which is why the test above says "also". + // Content drift was caught by neither, and that is what this change fixes. + + it('leaves a skills-only install driven by the version marker', async () => { + // No command files at all, so there is nothing to compare: this exercises + // the unchanged marker-only path, not the new comparison. await writeSkills(CURRENT); - await writeCommands({ 'propose.md': '' }); - const status = getToolVersionStatus(projectRoot, 'claude', CURRENT); + const status = getToolVersionStatus(projectRoot, TOOL_ID, CURRENT); + expect(status.configured).toBe(true); expect(status.generatedByVersion).toBe(CURRENT); - expect(status.needsUpdate).toBe(true); + expect(status.needsUpdate).toBe(false); }); - it('still flags a stale version marker regardless of command content', async () => { + it('still flags a stale version marker even when every command file is current', async () => { await writeSkills('0.0.1'); - await writeCommands({ 'propose.md': 'anything\n' }); + await writeGeneratedCommands(); - const status = getToolVersionStatus(projectRoot, 'claude', CURRENT); + const status = getToolVersionStatus(projectRoot, TOOL_ID, CURRENT); expect(status.generatedByVersion).toBe('0.0.1'); expect(status.needsUpdate).toBe(true); }); it('reports an unconfigured project as needing no update', async () => { - const status = getToolVersionStatus(projectRoot, 'claude', CURRENT); + const status = getToolVersionStatus(projectRoot, TOOL_ID, CURRENT); expect(status.configured).toBe(false); expect(status.needsUpdate).toBe(false); From df1497d355701ec54399fe4a5215b29aaabea75c Mon Sep 17 00:00:00 2001 From: Clay Good Date: Wed, 9 Sep 2026 10:49:04 -0500 Subject: [PATCH 3/3] chore(changeset): track the command-drift fix `openspec update` now reports and repairs tools it previously called up to date, so users will see a behavior change. That belongs in the changelog. Co-Authored-By: Claude Opus 5 --- .changeset/update-refreshes-drifted-commands.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/update-refreshes-drifted-commands.md diff --git a/.changeset/update-refreshes-drifted-commands.md b/.changeset/update-refreshes-drifted-commands.md new file mode 100644 index 0000000000..2652435f33 --- /dev/null +++ b/.changeset/update-refreshes-drifted-commands.md @@ -0,0 +1,5 @@ +--- +'@fission-ai/openspec': patch +--- + +Stop `openspec update` reporting a tool up to date while a damaged command file sits on disk. The check read the `generatedBy` version marker in a tool's skill files alone, which proves only that the skill files came from this CLI and says nothing about the command files written beside them, so a hand-edited or truncated command file left update printing "All 1 tool(s) up to date" and repairing nothing; the file could be restored only by knowing to pass `--force`. A deleted command file was already detected, so the claim was false only for a damaged one. Update now also compares command-file content, using the comparison that already existed and was simply never consulted once a skill file supplied a version. Scoped to tools configured for both skills and commands, so the commands-only path is unchanged, and skipped when the delivery mode generates no commands for the tool. Fixes #1807.