From 133f585fad15f3c120f870b1be8bb81e43d248f8 Mon Sep 17 00:00:00 2001 From: Clay Good Date: Wed, 5 Aug 2026 16:48:56 -0500 Subject: [PATCH 1/4] fix(update): don't hijack the agents target on legacy Codex upgrade Codex and the vendor-neutral `agents` target share `.agents/skills`. In upgradeLegacyTools, a Codex install inferred only from global ~/.codex/prompts wrote Codex skills into `.agents` and flipped the ownership marker agents -> codex, silently rewriting an existing agents-owned tree. The main generation path reconciles shared-target ownership first; this legacy-upgrade path did not. Add sharedSkillRootOwnedByOther() and skip generation when a different tool already owns the shared root (marker or existing tree), while still allowing a genuine first-time Codex upgrade with no `.agents` yet. Co-Authored-By: Claude Opus 4.8 --- .../fix-legacy-upgrade-agents-ownership.md | 7 ++ src/core/shared-skill-target.ts | 24 +++++++ src/core/update.ts | 16 ++++- test/core/shared-skill-target.test.ts | 67 +++++++++++++++++++ 4 files changed, 113 insertions(+), 1 deletion(-) create mode 100644 .changeset/fix-legacy-upgrade-agents-ownership.md create mode 100644 test/core/shared-skill-target.test.ts diff --git a/.changeset/fix-legacy-upgrade-agents-ownership.md b/.changeset/fix-legacy-upgrade-agents-ownership.md new file mode 100644 index 0000000000..749306cac3 --- /dev/null +++ b/.changeset/fix-legacy-upgrade-agents-ownership.md @@ -0,0 +1,7 @@ +--- +"@fission-ai/openspec": patch +--- + +### Bug Fixes + +- **Don't let a legacy Codex upgrade hijack the vendor-neutral `agents` target** — `openspec update` no longer overwrites an existing `.agents` skills tree (and its ownership marker) when Codex is detected only from leftover global `~/.codex/prompts`. Because Codex and the vendor-neutral `agents` target share `.agents/skills`, a project that used the `agents` target could have its generic skills silently rewritten with Codex-specific syntax and its target flipped to Codex on the next `update --force`. The legacy-upgrade path now respects the established owner of a shared skills directory, matching the one-writer rule `openspec init` already applies. A genuine first-time Codex upgrade (no `.agents` tree yet) is unaffected. diff --git a/src/core/shared-skill-target.ts b/src/core/shared-skill-target.ts index e3f214442e..f769b9b41a 100644 --- a/src/core/shared-skill-target.ts +++ b/src/core/shared-skill-target.ts @@ -158,6 +158,30 @@ export function isSharedSkillTargetActive(projectPath: string, toolId: string): .some((candidate) => candidate.value === toolId); } +/** + * Whether generating `toolId` into its shared skills root would clobber a tree + * a DIFFERENT tool already owns. True only when the root already carries an + * ownership signal (a marker or generated skills) AND reconciliation resolves + * it to another tool. An empty or unclaimed root returns false, so a genuine + * first-time legacy upgrade — e.g. a Codex-only user with no `.agents` yet — + * is never blocked from creating it. + */ +export function sharedSkillRootOwnedByOther(projectPath: string, toolId: string): boolean { + const tool = AI_TOOLS.find((candidate) => candidate.value === toolId); + if (!tool?.skillsDir) return false; + const sharingRoot = AI_TOOLS.filter((candidate) => candidate.skillsDir === tool.skillsDir); + if (sharingRoot.length < 2) return false; + + const hasOwnerSignal = + readSharedSkillTarget(projectPath, tool.skillsDir) !== undefined || + hasCurrentSkills(projectPath, tool.skillsDir); + if (!hasOwnerSignal) return false; + + return !reconcileSharedSkillTargets(projectPath, sharingRoot).some( + (candidate) => candidate.value === toolId + ); +} + export function writeSharedSkillTarget(projectPath: string, toolId: string): void { const tool = AI_TOOLS.find((candidate) => candidate.value === toolId); if (!tool?.skillsDir) return; diff --git a/src/core/update.ts b/src/core/update.ts index 3ac02feb44..d4cfcc7c1a 100644 --- a/src/core/update.ts +++ b/src/core/update.ts @@ -70,7 +70,7 @@ import { shouldReconcileCommandFilesForTool, shouldRemoveSkillsForTool, } from './command-surface.js'; -import { writeSharedSkillTarget } from './shared-skill-target.js'; +import { writeSharedSkillTarget, sharedSkillRootOwnedByOther } from './shared-skill-target.js'; import { includesGitHubCopilot, writeCopilotCloudFiles, removeCopilotCloudFiles, isCopilotCloudEnabled, readCopilotCloudOptIn, findUnmanagedCloudFiles } from './github-copilot/cloud-agent.js'; const require = createRequire(import.meta.url); @@ -1107,6 +1107,20 @@ export class UpdateCommand { const skillTemplates = getSkillTemplates(toolWorkflows); const commandContents = getCommandContents(toolWorkflows); + // A shared skills root (e.g. `.agents`) already owned by another tool + // must not be overwritten by a tool inferred from legacy artifacts: a + // Codex install detected only from global `~/.codex/prompts` would + // otherwise rewrite an existing vendor-neutral `agents` tree with + // Codex-specific syntax and flip its ownership marker `agents → codex`. + // Leave the established owner in place. (init applies the same + // one-writer rule up front when both targets are selected.) + if (shouldGenerateSkills && sharedSkillRootOwnedByOther(projectPath, tool.value)) { + spinner.info( + `Skipped ${tool.name}: ${tool.skillsDir}/skills is already managed by another tool.` + ); + continue; + } + // Create skill files when delivery includes skills if (shouldGenerateSkills) { for (const { template, dirName } of skillTemplates) { diff --git a/test/core/shared-skill-target.test.ts b/test/core/shared-skill-target.test.ts new file mode 100644 index 0000000000..f4593d6566 --- /dev/null +++ b/test/core/shared-skill-target.test.ts @@ -0,0 +1,67 @@ +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import { promises as fs } from 'fs'; +import os from 'os'; +import path from 'path'; +import { sharedSkillRootOwnedByOther } from '../../src/core/shared-skill-target.js'; + +/** + * `.agents` is shared by the vendor-neutral `agents` target and Codex. When a + * legacy Codex install is detected only from global `~/.codex/prompts`, the + * update path must not rewrite an existing `agents`-owned `.agents` tree. This + * guards the predicate that decides that. + */ +describe('sharedSkillRootOwnedByOther', () => { + let projectPath: string; + + const writeAgentsSkill = async (marker?: string) => { + const skillsRoot = path.join(projectPath, '.agents', 'skills'); + const skillDir = path.join(skillsRoot, 'openspec-propose'); + await fs.mkdir(skillDir, { recursive: true }); + // Generic invocation syntax => inferred owner is `agents` (not `$openspec-`). + await fs.writeFile( + path.join(skillDir, 'SKILL.md'), + '# openspec-propose\n\nRun /openspec-propose to start.\n' + ); + if (marker !== undefined) { + await fs.writeFile(path.join(skillsRoot, '.openspec-target'), `${marker}\n`); + } + }; + + beforeEach(async () => { + projectPath = await fs.mkdtemp(path.join(os.tmpdir(), 'openspec-shared-target-')); + }); + + afterEach(async () => { + await fs.rm(projectPath, { recursive: true, force: true }); + }); + + it('reports the .agents root as owned by another tool when agents holds it (marker + generic tree)', async () => { + await writeAgentsSkill('agents'); + // Codex, inferred only from global prompts, must not clobber this tree. + expect(sharedSkillRootOwnedByOther(projectPath, 'codex')).toBe(true); + // The owner itself is never "owned by another". + expect(sharedSkillRootOwnedByOther(projectPath, 'agents')).toBe(false); + }); + + it('infers agents ownership from a generic tree even without a marker', async () => { + await writeAgentsSkill(); // no marker; content is generic `/openspec-` + expect(sharedSkillRootOwnedByOther(projectPath, 'codex')).toBe(true); + }); + + it('does NOT block Codex when the marker names Codex', async () => { + await writeAgentsSkill('codex'); + expect(sharedSkillRootOwnedByOther(projectPath, 'codex')).toBe(false); + }); + + it('does NOT block a first-time legacy upgrade with no .agents tree yet', async () => { + // Codex-only user with global prompts and no `.agents`: nothing to clobber. + expect(sharedSkillRootOwnedByOther(projectPath, 'codex')).toBe(false); + expect(sharedSkillRootOwnedByOther(projectPath, 'agents')).toBe(false); + }); + + it('returns false for a tool that does not share its skills root', async () => { + await writeAgentsSkill('agents'); + // Claude writes to its own `.claude` root, never `.agents`. + expect(sharedSkillRootOwnedByOther(projectPath, 'claude')).toBe(false); + }); +}); From 2bf0eafd86b64b52c139b05b631f1a6cce425324 Mon Sep 17 00:00:00 2001 From: Clay Good Date: Thu, 6 Aug 2026 09:00:43 -0500 Subject: [PATCH 2/4] test(update): cover the hijack guard end-to-end; name the owner on skip Harden the agents-target ownership fix after a multi-agent review: - Add an integration test that runs the real update flow for the bug scenario (agents-owned .agents + a legacy global Codex prompt) and asserts the marker stays `agents` and skills keep generic `/openspec-` syntax. A unit test of the predicate can't catch a future refactor that stops calling it; this can. - Name the owning tool in the skip message ("...managed by another tool (Shared .agents skills)") via a new sharedSkillRootOwner() helper that sharedSkillRootOwnedByOther now delegates to. - Add a unit case for the ambiguous-tree branch (existing skills, no marker, no inferable syntax) and one asserting sharedSkillRootOwner names agents. - Document the known, harmless re-offer tradeoff (a skipped tool isn't recorded as configured, so a persistent legacy prompt re-offers it). Co-Authored-By: Claude Opus 4.8 --- src/core/shared-skill-target.ts | 32 +++++++++++++++++---------- src/core/update.ts | 16 +++++++++++--- test/core/shared-skill-target.test.ts | 22 +++++++++++++++++- test/core/update.test.ts | 25 +++++++++++++++++++++ 4 files changed, 79 insertions(+), 16 deletions(-) diff --git a/src/core/shared-skill-target.ts b/src/core/shared-skill-target.ts index f769b9b41a..a17c862447 100644 --- a/src/core/shared-skill-target.ts +++ b/src/core/shared-skill-target.ts @@ -159,27 +159,35 @@ export function isSharedSkillTargetActive(projectPath: string, toolId: string): } /** - * Whether generating `toolId` into its shared skills root would clobber a tree - * a DIFFERENT tool already owns. True only when the root already carries an + * The tool that already owns `toolId`'s shared skills root, when a DIFFERENT + * one does. Returns the owner's tool id only when the root already carries an * ownership signal (a marker or generated skills) AND reconciliation resolves - * it to another tool. An empty or unclaimed root returns false, so a genuine - * first-time legacy upgrade — e.g. a Codex-only user with no `.agents` yet — - * is never blocked from creating it. + * it to another tool. An empty or unclaimed root returns undefined, so a + * genuine first-time legacy upgrade — e.g. a Codex-only user with no `.agents` + * yet — is never reported as owned. */ -export function sharedSkillRootOwnedByOther(projectPath: string, toolId: string): boolean { +export function sharedSkillRootOwner(projectPath: string, toolId: string): string | undefined { const tool = AI_TOOLS.find((candidate) => candidate.value === toolId); - if (!tool?.skillsDir) return false; + if (!tool?.skillsDir) return undefined; const sharingRoot = AI_TOOLS.filter((candidate) => candidate.skillsDir === tool.skillsDir); - if (sharingRoot.length < 2) return false; + if (sharingRoot.length < 2) return undefined; const hasOwnerSignal = readSharedSkillTarget(projectPath, tool.skillsDir) !== undefined || hasCurrentSkills(projectPath, tool.skillsDir); - if (!hasOwnerSignal) return false; + if (!hasOwnerSignal) return undefined; + + const owner = reconcileSharedSkillTargets(projectPath, sharingRoot)[0]?.value; + return owner && owner !== toolId ? owner : undefined; +} - return !reconcileSharedSkillTargets(projectPath, sharingRoot).some( - (candidate) => candidate.value === toolId - ); +/** + * Whether generating `toolId` into its shared skills root would clobber a tree + * a DIFFERENT tool already owns — the guard the legacy-upgrade path uses before + * writing skills. See {@link sharedSkillRootOwner} for the ownership rules. + */ +export function sharedSkillRootOwnedByOther(projectPath: string, toolId: string): boolean { + return sharedSkillRootOwner(projectPath, toolId) !== undefined; } export function writeSharedSkillTarget(projectPath: string, toolId: string): void { diff --git a/src/core/update.ts b/src/core/update.ts index d4cfcc7c1a..7075f6ef70 100644 --- a/src/core/update.ts +++ b/src/core/update.ts @@ -70,7 +70,7 @@ import { shouldReconcileCommandFilesForTool, shouldRemoveSkillsForTool, } from './command-surface.js'; -import { writeSharedSkillTarget, sharedSkillRootOwnedByOther } from './shared-skill-target.js'; +import { writeSharedSkillTarget, sharedSkillRootOwner } from './shared-skill-target.js'; import { includesGitHubCopilot, writeCopilotCloudFiles, removeCopilotCloudFiles, isCopilotCloudEnabled, readCopilotCloudOptIn, findUnmanagedCloudFiles } from './github-copilot/cloud-agent.js'; const require = createRequire(import.meta.url); @@ -1114,9 +1114,19 @@ export class UpdateCommand { // Codex-specific syntax and flip its ownership marker `agents → codex`. // Leave the established owner in place. (init applies the same // one-writer rule up front when both targets are selected.) - if (shouldGenerateSkills && sharedSkillRootOwnedByOther(projectPath, tool.value)) { + // + // Skipping here means the tool is never recorded as configured, so a + // persistent legacy signal (global `~/.codex/prompts`, which update does + // not auto-remove) re-offers it on later runs. That repeat is idempotent + // and harmless — the alternative is the silent hijack this prevents. + const sharedOwner = shouldGenerateSkills + ? sharedSkillRootOwner(projectPath, tool.value) + : undefined; + if (sharedOwner) { + const ownerName = + AI_TOOLS.find((candidate) => candidate.value === sharedOwner)?.name ?? sharedOwner; spinner.info( - `Skipped ${tool.name}: ${tool.skillsDir}/skills is already managed by another tool.` + `Skipped ${tool.name}: ${tool.skillsDir}/skills is already managed by another tool (${ownerName}).` ); continue; } diff --git a/test/core/shared-skill-target.test.ts b/test/core/shared-skill-target.test.ts index f4593d6566..86faf3b7cc 100644 --- a/test/core/shared-skill-target.test.ts +++ b/test/core/shared-skill-target.test.ts @@ -2,7 +2,10 @@ import { describe, it, expect, beforeEach, afterEach } from 'vitest'; import { promises as fs } from 'fs'; import os from 'os'; import path from 'path'; -import { sharedSkillRootOwnedByOther } from '../../src/core/shared-skill-target.js'; +import { + sharedSkillRootOwnedByOther, + sharedSkillRootOwner, +} from '../../src/core/shared-skill-target.js'; /** * `.agents` is shared by the vendor-neutral `agents` target and Codex. When a @@ -64,4 +67,21 @@ describe('sharedSkillRootOwnedByOther', () => { // Claude writes to its own `.claude` root, never `.agents`. expect(sharedSkillRootOwnedByOther(projectPath, 'claude')).toBe(false); }); + + it('treats an existing tree with no marker and no inferable syntax as agents-owned', async () => { + // Neither `$openspec-` nor `/openspec-` in the content and no marker: + // ownership can't be inferred, so reconciliation keeps the established + // `agents` target rather than letting Codex claim the existing tree. + const skillDir = path.join(projectPath, '.agents', 'skills', 'openspec-propose'); + await fs.mkdir(skillDir, { recursive: true }); + await fs.writeFile(path.join(skillDir, 'SKILL.md'), '# openspec-propose\n\nNo invocation syntax here.\n'); + expect(sharedSkillRootOwnedByOther(projectPath, 'codex')).toBe(true); + }); + + it('names the owning tool via sharedSkillRootOwner', async () => { + await writeAgentsSkill('agents'); + expect(sharedSkillRootOwner(projectPath, 'codex')).toBe('agents'); + // The owner is never "owned by another"; an unclaimed root has no owner. + expect(sharedSkillRootOwner(projectPath, 'agents')).toBeUndefined(); + }); }); diff --git a/test/core/update.test.ts b/test/core/update.test.ts index 3a40b7e97c..406a2eb4a4 100644 --- a/test/core/update.test.ts +++ b/test/core/update.test.ts @@ -613,6 +613,31 @@ metadata: ).toContain('User edit'); }); + it('does not let a legacy Codex global prompt hijack an established agents target', async () => { + // Regression for the hijack this PR fixes: the guard must actually be + // invoked by the update flow, not merely be correct in isolation. + setMockConfig({ featureFlags: {}, profile: 'core', delivery: 'skills' }); + // The vendor-neutral `agents` target owns `.agents` (marker + generic skills). + await new InitCommand({ tools: 'agents', force: true }).execute(testDir); + // A leftover global Codex install, detected only from `~/.codex/prompts`. + const promptDir = path.join(process.env.CODEX_HOME!, 'prompts'); + await fs.mkdir(promptDir, { recursive: true }); + await fs.writeFile(path.join(promptDir, 'opsx-explore.md'), 'legacy explore prompt'); + + await new UpdateCommand({ force: true }).execute(testDir); + + const skillsDir = path.join(testDir, '.agents', 'skills'); + // Ownership marker is not flipped to codex... + expect(await fs.readFile(path.join(skillsDir, '.openspec-target'), 'utf-8')).toBe('agents\n'); + // ...and the tree keeps generic `/openspec-` syntax, never Codex `$openspec-`. + const propose = await fs.readFile( + path.join(skillsDir, 'openspec-propose', 'SKILL.md'), + 'utf-8' + ); + expect(propose).not.toContain('$openspec-'); + expect(propose).toContain('/openspec-'); + }); + it('should let an explicit Codex init take ownership of an agents tree', async () => { await new InitCommand({ tools: 'agents', force: true }).execute(testDir); From f8ea958ec6edef7c04d6eea4c943fbe5c3d4b8e3 Mon Sep 17 00:00:00 2001 From: Clay Good Date: Thu, 6 Aug 2026 13:44:32 -0500 Subject: [PATCH 3/4] fix(update): preserve skipped tool's legacy files; add upgrade-path tests Adversarial review of the shared-root ownership guard surfaced one real integration defect and the review asks from alfred/CodeRabbit. Defect: when the guard skips a legacy Codex upgrade because the `.agents` root is owned by another tool, the caller's immediate legacy cleanup still deleted Codex's repo-local `.codex/prompts/openspec-*.md`. That violates the cleanup contract (remove X only because replacement Y was written): no replacement is written for a skipped tool, so its legacy files must stay. `upgradeLegacyTools` now reports `skippedSharedSkillTools`, and `performImmediateLegacyCleanup` exempts those tools' repo-local artifacts via a new `omitToolLegacyArtifacts` helper. Refactored the per-artifact tool matching out of `getToolsFromLegacyArtifacts` so both share one matcher. Tests (addressing the review + the defect): - update.test.ts: hijack test now asserts Codex is absent from the persisted configured-tool set and that the skip names the established owner. - update.test.ts: inverse no-root case proves a first-time Codex upgrade still writes the `codex` marker via the real UpdateCommand path. - update.test.ts: a skipped tool's repo-local `.codex/prompts` is preserved. - legacy-cleanup.test.ts: unit coverage for omitToolLegacyArtifacts. - shared-skill-target.test.ts: assert sharedSkillRootOwner resolves 'agents'. Docs + changeset updated to describe the preserve-on-skip behavior. Co-Authored-By: Claude Opus 4.8 --- .../fix-legacy-upgrade-agents-ownership.md | 2 +- docs/supported-tools.md | 7 ++ src/core/legacy-cleanup.ts | 77 +++++++++++++------ src/core/update.ts | 42 ++++++++-- test/core/legacy-cleanup.test.ts | 39 ++++++++++ test/core/shared-skill-target.test.ts | 2 + test/core/update.test.ts | 66 ++++++++++++++++ 7 files changed, 201 insertions(+), 34 deletions(-) diff --git a/.changeset/fix-legacy-upgrade-agents-ownership.md b/.changeset/fix-legacy-upgrade-agents-ownership.md index 749306cac3..085a4a4ebf 100644 --- a/.changeset/fix-legacy-upgrade-agents-ownership.md +++ b/.changeset/fix-legacy-upgrade-agents-ownership.md @@ -4,4 +4,4 @@ ### Bug Fixes -- **Don't let a legacy Codex upgrade hijack the vendor-neutral `agents` target** — `openspec update` no longer overwrites an existing `.agents` skills tree (and its ownership marker) when Codex is detected only from leftover global `~/.codex/prompts`. Because Codex and the vendor-neutral `agents` target share `.agents/skills`, a project that used the `agents` target could have its generic skills silently rewritten with Codex-specific syntax and its target flipped to Codex on the next `update --force`. The legacy-upgrade path now respects the established owner of a shared skills directory, matching the one-writer rule `openspec init` already applies. A genuine first-time Codex upgrade (no `.agents` tree yet) is unaffected. +- **Don't let a legacy Codex upgrade hijack the vendor-neutral `agents` target** — `openspec update` no longer overwrites an existing `.agents` skills tree (and its ownership marker) when Codex is detected only from leftover global `~/.codex/prompts`. Because Codex and the vendor-neutral `agents` target share `.agents/skills`, a project that used the `agents` target could have its generic skills silently rewritten with Codex-specific syntax and its target flipped to Codex on the next `update --force`. The legacy-upgrade path now respects the established owner of a shared skills directory, matching the one-writer rule `openspec init` already applies. When an upgrade is skipped this way, that tool's repo-local legacy files (e.g. `.codex/prompts/openspec-*.md`) are also preserved rather than cleaned up, since no replacement was written to take their place. A genuine first-time Codex upgrade (no `.agents` tree yet) is unaffected. diff --git a/docs/supported-tools.md b/docs/supported-tools.md index 756a80e878..37d583b71d 100644 --- a/docs/supported-tools.md +++ b/docs/supported-tools.md @@ -180,6 +180,13 @@ For pre-marker projects, OpenSpec infers ownership from managed skill references generic canonical tree alongside legacy `.codex/skills` is treated as an older dual-target install and consolidated into the compatible shared tree. +`openspec update` honors this ownership too. If a project owns `.agents` as the +vendor-neutral target and a leftover Codex install is detected only from stray +prompt files, the update leaves the established `agents` tree in place instead of +rewriting it with Codex syntax, and preserves those legacy prompt files rather +than deleting them. To hand the shared tree to Codex, run `openspec init --tools +codex` explicitly. + ## Non-Interactive Setup For CI/CD or scripted setup, use `--tools` (and optionally `--profile`): diff --git a/src/core/legacy-cleanup.ts b/src/core/legacy-cleanup.ts index ccc47df160..3db9651f28 100644 --- a/src/core/legacy-cleanup.ts +++ b/src/core/legacy-cleanup.ts @@ -801,35 +801,14 @@ export function formatDeferredGlobalPromptSummary(detection: LegacyDetectionResu export function getToolsFromLegacyArtifacts(detection: LegacyDetectionResult): string[] { const tools = new Set(); - // Match directories to tool IDs for (const dir of detection.slashCommandDirs) { - for (const [toolId, pattern] of Object.entries(LEGACY_SLASH_COMMAND_PATHS)) { - if (pattern.type === 'directory' && pattern.path === dir) { - tools.add(toolId); - break; - } - } + const toolId = legacyToolIdForDir(dir); + if (toolId) tools.add(toolId); } - // Match files to tool IDs using glob patterns for (const file of detection.slashCommandFiles) { - // Normalize file path to use forward slashes for consistent matching (Windows compatibility) - const normalizedFile = normalizePathForMatch(file); - for (const [toolId, pattern] of Object.entries(LEGACY_SLASH_COMMAND_PATHS)) { - if (pattern.type === 'files' && pattern.pattern) { - const patterns = Array.isArray(pattern.pattern) ? pattern.pattern : [pattern.pattern]; - let matched = false; - for (const p of patterns) { - const regex = globToRegex(p); - if (regex.test(normalizedFile)) { - tools.add(toolId); - matched = true; - break; - } - } - if (matched) break; - } - } + const toolId = legacyToolIdForFile(file); + if (toolId) tools.add(toolId); } for (const prompt of getLegacyGlobalPromptMatches(detection)) { @@ -839,6 +818,26 @@ export function getToolsFromLegacyArtifacts(detection: LegacyDetectionResult): s return Array.from(tools); } +/** The tool that owns a repo-local legacy slash-command directory, if any. */ +function legacyToolIdForDir(dir: string): string | undefined { + for (const [toolId, pattern] of Object.entries(LEGACY_SLASH_COMMAND_PATHS)) { + if (pattern.type === 'directory' && pattern.path === dir) return toolId; + } + return undefined; +} + +/** The tool that owns a repo-local legacy slash-command file, if any. */ +function legacyToolIdForFile(file: string): string | undefined { + // Normalize to forward slashes so the glob patterns match on Windows too. + const normalizedFile = normalizePathForMatch(file); + for (const [toolId, pattern] of Object.entries(LEGACY_SLASH_COMMAND_PATHS)) { + if (pattern.type !== 'files' || !pattern.pattern) continue; + const patterns = Array.isArray(pattern.pattern) ? pattern.pattern : [pattern.pattern]; + if (patterns.some((p) => globToRegex(p).test(normalizedFile))) return toolId; + } + return undefined; +} + /** * Normalizes global Codex prompt matches so callers can rely on workflow-aware * metadata even when older detection results only carry file paths. @@ -902,6 +901,34 @@ export function omitGlobalLegacyPromptFiles(detection: LegacyDetectionResult): L return nextDetection; } +/** + * Returns a detection snapshot with the repo-local slash-command artifacts of + * the given tools removed. The legacy-upgrade path uses this to skip cleaning a + * tool's legacy files when its replacement was deliberately NOT written — e.g. a + * Codex upgrade suppressed because the shared `.agents` root is already owned by + * another tool. Deleting the legacy prompt without writing its replacement would + * violate the cleanup contract ("remove X because replacement Y now exists") and + * strip the tool's only OpenSpec integration. + */ +export function omitToolLegacyArtifacts( + detection: LegacyDetectionResult, + toolIds: readonly string[] +): LegacyDetectionResult { + if (toolIds.length === 0) return detection; + const skip = new Set(toolIds); + const nextDetection: LegacyDetectionResult = { + ...detection, + slashCommandDirs: detection.slashCommandDirs.filter( + (dir) => !skip.has(legacyToolIdForDir(dir) ?? '') + ), + slashCommandFiles: detection.slashCommandFiles.filter( + (file) => !skip.has(legacyToolIdForFile(file) ?? '') + ), + }; + nextDetection.hasLegacyArtifacts = hasLegacyArtifacts(nextDetection); + return nextDetection; +} + /** * Builds a detection snapshot containing only the selected global Codex prompt * matches for replacement-gated cleanup. diff --git a/src/core/update.ts b/src/core/update.ts index 7075f6ef70..69fa4bafe4 100644 --- a/src/core/update.ts +++ b/src/core/update.ts @@ -38,6 +38,7 @@ import { getLegacyWorkflowIdsForTool, getToolsFromLegacyArtifacts, omitGlobalLegacyPromptFiles, + omitToolLegacyArtifacts, pickGlobalLegacyPromptFiles, type LegacyDetectionResult, } from './legacy-cleanup.js'; @@ -84,6 +85,12 @@ type LegacyUpgradeResult = { newlyConfiguredTools: string[]; workflowOverrides: Partial>; deferredGlobalCleanup?: LegacyDetectionResult; + /** + * Tools whose skill generation was skipped because another tool already owns + * their shared skills root. Their repo-local legacy artifacts must be exempt + * from immediate cleanup — no replacement was written to justify deleting them. + */ + skippedSharedSkillTools?: string[]; }; /** @@ -886,7 +893,11 @@ export class UpdateCommand { desiredWorkflows, delivery ); - await this.performImmediateLegacyCleanup(projectPath, detection); + await this.performImmediateLegacyCleanup( + projectPath, + detection, + legacyUpgrade.skippedSharedSkillTools + ); return { ...legacyUpgrade, deferredGlobalCleanup: pickGlobalLegacyPromptFiles( @@ -919,7 +930,11 @@ export class UpdateCommand { desiredWorkflows, delivery ); - await this.performImmediateLegacyCleanup(projectPath, detection); + await this.performImmediateLegacyCleanup( + projectPath, + detection, + legacyUpgrade.skippedSharedSkillTools + ); return { ...legacyUpgrade, deferredGlobalCleanup: pickGlobalLegacyPromptFiles( @@ -939,9 +954,15 @@ export class UpdateCommand { */ private async performImmediateLegacyCleanup( projectPath: string, - detection: LegacyDetectionResult + detection: LegacyDetectionResult, + skippedSharedSkillTools: readonly string[] = [] ): Promise { - const immediateDetection = omitGlobalLegacyPromptFiles(detection); + // Tools whose upgrade was skipped (shared root owned by another) had no + // replacement written, so their repo-local legacy files must be preserved. + const immediateDetection = omitToolLegacyArtifacts( + omitGlobalLegacyPromptFiles(detection), + skippedSharedSkillTools + ); if (immediateDetection.hasLegacyArtifacts) { await this.performLegacyCleanup(projectPath, immediateDetection); } @@ -1083,6 +1104,7 @@ export class UpdateCommand { // Create skills/commands for selected tools using effective profile+delivery. const newlyConfigured: string[] = []; + const skippedSharedSkillTools: string[] = []; const workflowOverrides: LegacyUpgradeResult['workflowOverrides'] = {}; for (const toolId of selectedTools) { @@ -1116,9 +1138,12 @@ export class UpdateCommand { // one-writer rule up front when both targets are selected.) // // Skipping here means the tool is never recorded as configured, so a - // persistent legacy signal (global `~/.codex/prompts`, which update does - // not auto-remove) re-offers it on later runs. That repeat is idempotent - // and harmless — the alternative is the silent hijack this prevents. + // persistent legacy signal re-offers it on later runs. Because no + // replacement is written, this tool is also exempted from immediate + // legacy cleanup (see skippedSharedSkillTools) — otherwise a repo-local + // `.codex/prompts` would be deleted with nothing put in its place. That + // repeat is idempotent and harmless — the alternative is the silent + // hijack this prevents. const sharedOwner = shouldGenerateSkills ? sharedSkillRootOwner(projectPath, tool.value) : undefined; @@ -1128,6 +1153,7 @@ export class UpdateCommand { spinner.info( `Skipped ${tool.name}: ${tool.skillsDir}/skills is already managed by another tool (${ownerName}).` ); + skippedSharedSkillTools.push(tool.value); continue; } @@ -1188,6 +1214,6 @@ export class UpdateCommand { console.log(); } - return { newlyConfiguredTools: newlyConfigured, workflowOverrides }; + return { newlyConfiguredTools: newlyConfigured, workflowOverrides, skippedSharedSkillTools }; } } diff --git a/test/core/legacy-cleanup.test.ts b/test/core/legacy-cleanup.test.ts index 55c72d5f85..d607ab615c 100644 --- a/test/core/legacy-cleanup.test.ts +++ b/test/core/legacy-cleanup.test.ts @@ -17,6 +17,7 @@ import { formatDetectionSummary, formatProjectMdMigrationHint, getToolsFromLegacyArtifacts, + omitToolLegacyArtifacts, LEGACY_CONFIG_FILES, LEGACY_GLOBAL_SLASH_COMMAND_PATHS, LEGACY_SLASH_COMMAND_PATHS, @@ -1479,4 +1480,42 @@ ${OPENSPEC_MARKERS.end}`); expect(tools).toHaveLength(0); }); }); + + describe('omitToolLegacyArtifacts', () => { + const baseDetection = () => ({ + configFiles: [], + configFilesToUpdate: [], + slashCommandDirs: ['.claude/commands/openspec'], + slashCommandFiles: ['.codex/prompts/openspec-explore.md', '.cursor/commands/openspec-apply.md'], + globalSlashCommandFiles: [], + hasOpenspecAgents: false, + hasProjectMd: false, + hasRootAgentsWithMarkers: false, + hasLegacyArtifacts: true, + }); + + it('removes only the named tool\'s repo-local artifacts', () => { + const result = omitToolLegacyArtifacts(baseDetection(), ['codex']); + expect(result.slashCommandFiles).toEqual(['.cursor/commands/openspec-apply.md']); + // Other tools' files and directories are untouched. + expect(result.slashCommandDirs).toEqual(['.claude/commands/openspec']); + expect(result.hasLegacyArtifacts).toBe(true); + }); + + it('recomputes hasLegacyArtifacts to false when nothing is left', () => { + const detection = { + ...baseDetection(), + slashCommandDirs: [], + slashCommandFiles: ['.codex/prompts/openspec-explore.md'], + }; + const result = omitToolLegacyArtifacts(detection, ['codex']); + expect(result.slashCommandFiles).toEqual([]); + expect(result.hasLegacyArtifacts).toBe(false); + }); + + it('returns the detection unchanged when no tools are skipped', () => { + const detection = baseDetection(); + expect(omitToolLegacyArtifacts(detection, [])).toBe(detection); + }); + }); }); diff --git a/test/core/shared-skill-target.test.ts b/test/core/shared-skill-target.test.ts index 86faf3b7cc..37e7fd90ad 100644 --- a/test/core/shared-skill-target.test.ts +++ b/test/core/shared-skill-target.test.ts @@ -76,6 +76,8 @@ describe('sharedSkillRootOwnedByOther', () => { await fs.mkdir(skillDir, { recursive: true }); await fs.writeFile(path.join(skillDir, 'SKILL.md'), '# openspec-propose\n\nNo invocation syntax here.\n'); expect(sharedSkillRootOwnedByOther(projectPath, 'codex')).toBe(true); + // The established `agents` target is the resolved owner of the ambiguous tree. + expect(sharedSkillRootOwner(projectPath, 'codex')).toBe('agents'); }); it('names the owning tool via sharedSkillRootOwner', async () => { diff --git a/test/core/update.test.ts b/test/core/update.test.ts index 406a2eb4a4..9bf84259a2 100644 --- a/test/core/update.test.ts +++ b/test/core/update.test.ts @@ -1,6 +1,7 @@ import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; import { UpdateCommand, scanInstalledWorkflows } from '../../src/core/update.js'; import { InitCommand } from '../../src/core/init.js'; +import { getConfiguredToolsForProfileSync } from '../../src/core/profile-sync-drift.js'; import { FileSystemUtils } from '../../src/utils/file-system.js'; import { OPENSPEC_MARKERS } from '../../src/core/config.js'; import type { GlobalConfig } from '../../src/core/global-config.js'; @@ -624,7 +625,18 @@ metadata: await fs.mkdir(promptDir, { recursive: true }); await fs.writeFile(path.join(promptDir, 'opsx-explore.md'), 'legacy explore prompt'); + // The skip message is emitted via an ora spinner, which writes to the + // process streams rather than through console.log. + let streamOutput = ''; + const capture = (chunk: unknown) => { + streamOutput += String(chunk); + return true; + }; + const stdoutSpy = vi.spyOn(process.stdout, 'write').mockImplementation(capture as never); + const stderrSpy = vi.spyOn(process.stderr, 'write').mockImplementation(capture as never); await new UpdateCommand({ force: true }).execute(testDir); + stdoutSpy.mockRestore(); + stderrSpy.mockRestore(); const skillsDir = path.join(testDir, '.agents', 'skills'); // Ownership marker is not flipped to codex... @@ -636,6 +648,60 @@ metadata: ); expect(propose).not.toContain('$openspec-'); expect(propose).toContain('/openspec-'); + // Generation AND configuration are skipped: Codex is never recorded as a + // configured tool, so a stray global prompt cannot flip ownership later. + const configured = getConfiguredToolsForProfileSync(testDir); + expect(configured).toContain('agents'); + expect(configured).not.toContain('codex'); + // The skip names the established owner so the user understands why. + expect(streamOutput).toMatch(/Skipped Codex/); + expect(streamOutput).toMatch(/managed by another tool \(Shared \.agents skills\)/); + }); + + it('lets a first-time legacy Codex upgrade claim an unowned agents root', async () => { + // Inverse of the hijack guard: with no `.agents` tree yet, nothing is + // owned, so the real update path must still generate Codex skills and + // stamp the `codex` marker — proving the guard is not over-broad. + setMockConfig({ featureFlags: {}, profile: 'core', delivery: 'skills' }); + const promptDir = path.join(process.env.CODEX_HOME!, 'prompts'); + await fs.mkdir(promptDir, { recursive: true }); + await fs.writeFile(path.join(promptDir, 'opsx-explore.md'), 'legacy explore prompt'); + + await new UpdateCommand({ force: true }).execute(testDir); + + const skillsDir = path.join(testDir, '.agents', 'skills'); + // The codex marker is written (writeSharedSkillTarget on the non-owned path). + expect(await fs.readFile(path.join(skillsDir, '.openspec-target'), 'utf-8')).toBe('codex\n'); + // A single opsx-explore prompt infers only the `explore` workflow, and the + // generated skill carries Codex `$openspec-` syntax. + const explore = await fs.readFile( + path.join(skillsDir, 'openspec-explore', 'SKILL.md'), + 'utf-8' + ); + expect(explore).toContain('$openspec-'); + // Codex is now recorded as configured (mirrors the negative check above). + expect(getConfiguredToolsForProfileSync(testDir)).toContain('codex'); + }); + + it('preserves a skipped tool\'s repo-local legacy prompts instead of deleting them', async () => { + // When the guard skips Codex (agents owns `.agents`), no replacement skill + // is written — so Codex's repo-local `.codex/prompts` must NOT be cleaned + // up. Deleting them would strip the legacy signal with nothing in its place. + setMockConfig({ featureFlags: {}, profile: 'core', delivery: 'skills' }); + await new InitCommand({ tools: 'agents', force: true }).execute(testDir); + const legacyPrompts = path.join(testDir, '.codex', 'prompts'); + await fs.mkdir(legacyPrompts, { recursive: true }); + await fs.writeFile(path.join(legacyPrompts, 'openspec-explore.md'), 'legacy repo-local prompt'); + + await new UpdateCommand({ force: true }).execute(testDir); + + // agents tree preserved, and the repo-local legacy prompt survives. + expect( + await fs.readFile(path.join(testDir, '.agents', 'skills', '.openspec-target'), 'utf-8') + ).toBe('agents\n'); + expect( + await FileSystemUtils.fileExists(path.join(legacyPrompts, 'openspec-explore.md')) + ).toBe(true); }); it('should let an explicit Codex init take ownership of an agents tree', async () => { From 3bc0cc17f95245e89b06d736551cf5179cc44816 Mon Sep 17 00:00:00 2001 From: Clay Good Date: Fri, 7 Aug 2026 08:49:56 -0500 Subject: [PATCH 4/4] test(update): lock in legacy-prompt preservation on skipped Codex upgrade MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address the outstanding CodeRabbit review notes on #1522. The fix itself is confirmed correct by three independent adversarial reviews — these are test-only hardening that locks in the guarantees the fix promises: - Assert the global ~/.codex/prompts survives (byte-for-byte) in the hijack scenario. Previously the test set the prompt up but never checked it was preserved; on unfixed code Codex would be generated, its 'explore' workflow would read as installed, and the deferred global cleanup would delete the prompt — so this assertion fails without the fix. - Assert the repo-local .codex/prompts is preserved by content, not mere existence (distinguishes 'left untouched' from 'deleted+rewritten'). - Restore the stdout/stderr spies in a finally so a throw can't swallow output for the rest of the suite. - Cover backslash-delimited (Windows) paths in omitToolLegacyArtifacts. Co-Authored-By: Claude Opus 4.8 --- test/core/legacy-cleanup.test.ts | 13 +++++++++++++ test/core/update.test.ts | 31 ++++++++++++++++++++++--------- 2 files changed, 35 insertions(+), 9 deletions(-) diff --git a/test/core/legacy-cleanup.test.ts b/test/core/legacy-cleanup.test.ts index d607ab615c..f357052649 100644 --- a/test/core/legacy-cleanup.test.ts +++ b/test/core/legacy-cleanup.test.ts @@ -1517,5 +1517,18 @@ ${OPENSPEC_MARKERS.end}`); const detection = baseDetection(); expect(omitToolLegacyArtifacts(detection, [])).toBe(detection); }); + + it('omits backslash-delimited paths for the skipped tool (Windows)', () => { + // Defensive: `legacyToolIdForFile` normalizes separators, so a + // Windows-style path must map to `codex` and be filtered too. + const detection = { + ...baseDetection(), + slashCommandDirs: [], + slashCommandFiles: ['.codex\\prompts\\openspec-explore.md'], + }; + const result = omitToolLegacyArtifacts(detection, ['codex']); + expect(result.slashCommandFiles).toEqual([]); + expect(result.hasLegacyArtifacts).toBe(false); + }); }); }); diff --git a/test/core/update.test.ts b/test/core/update.test.ts index 9bf84259a2..c5751da7ce 100644 --- a/test/core/update.test.ts +++ b/test/core/update.test.ts @@ -622,11 +622,13 @@ metadata: await new InitCommand({ tools: 'agents', force: true }).execute(testDir); // A leftover global Codex install, detected only from `~/.codex/prompts`. const promptDir = path.join(process.env.CODEX_HOME!, 'prompts'); + const globalPrompt = path.join(promptDir, 'opsx-explore.md'); await fs.mkdir(promptDir, { recursive: true }); - await fs.writeFile(path.join(promptDir, 'opsx-explore.md'), 'legacy explore prompt'); + await fs.writeFile(globalPrompt, 'legacy explore prompt'); // The skip message is emitted via an ora spinner, which writes to the - // process streams rather than through console.log. + // process streams rather than through console.log. Restore the spies in a + // finally so a throw can never swallow stdout for the rest of the suite. let streamOutput = ''; const capture = (chunk: unknown) => { streamOutput += String(chunk); @@ -634,9 +636,12 @@ metadata: }; const stdoutSpy = vi.spyOn(process.stdout, 'write').mockImplementation(capture as never); const stderrSpy = vi.spyOn(process.stderr, 'write').mockImplementation(capture as never); - await new UpdateCommand({ force: true }).execute(testDir); - stdoutSpy.mockRestore(); - stderrSpy.mockRestore(); + try { + await new UpdateCommand({ force: true }).execute(testDir); + } finally { + stdoutSpy.mockRestore(); + stderrSpy.mockRestore(); + } const skillsDir = path.join(testDir, '.agents', 'skills'); // Ownership marker is not flipped to codex... @@ -656,6 +661,12 @@ metadata: // The skip names the established owner so the user understands why. expect(streamOutput).toMatch(/Skipped Codex/); expect(streamOutput).toMatch(/managed by another tool \(Shared \.agents skills\)/); + // The legacy signal must survive: because Codex was skipped, no + // replacement skill exists, so the deferred global-prompt cleanup must + // preserve `~/.codex/prompts` untouched (byte-for-byte) rather than + // delete it — otherwise the skip could never re-offer Codex later. + expect(await FileSystemUtils.fileExists(globalPrompt)).toBe(true); + expect(await fs.readFile(globalPrompt, 'utf-8')).toBe('legacy explore prompt'); }); it('lets a first-time legacy Codex upgrade claim an unowned agents root', async () => { @@ -695,13 +706,15 @@ metadata: await new UpdateCommand({ force: true }).execute(testDir); - // agents tree preserved, and the repo-local legacy prompt survives. + // agents tree preserved, and the repo-local legacy prompt survives + // byte-for-byte — asserting content, not mere existence, distinguishes + // "left untouched" from "deleted then rewritten". expect( await fs.readFile(path.join(testDir, '.agents', 'skills', '.openspec-target'), 'utf-8') ).toBe('agents\n'); - expect( - await FileSystemUtils.fileExists(path.join(legacyPrompts, 'openspec-explore.md')) - ).toBe(true); + const preservedPrompt = path.join(legacyPrompts, 'openspec-explore.md'); + expect(await FileSystemUtils.fileExists(preservedPrompt)).toBe(true); + expect(await fs.readFile(preservedPrompt, 'utf-8')).toBe('legacy repo-local prompt'); }); it('should let an explicit Codex init take ownership of an agents tree', async () => {