fix(store): refuse to remove a store that contains another store (#1880)

* fix(store): refuse to remove a store that contains another store

store remove deleted the target folder recursively after checking only
the target's own metadata. Another registered store living inside that
folder, such as a shared store vendored as a git submodule (a layout
store register accepts), was deleted with it, uncommitted work
included, and its registry entry was left pointing at a missing path.

Refuse with store_remove_contains_registered_store when another
registration's canonical root is inside the folder. The check runs in
the beforeCommit hook, under the registry lock that commits the
removal, which now receives the registrations that will remain.

* docs(store): move nested-store remove refusal to docs-lab

Document store_remove_contains_registered_store on the canonical docs-lab CLI reference instead of legacy docs/cli.md. docs/agent-contract.md keeps the error-code entry.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

---------

Co-authored-by: Clay Good <hi@claygood.com>
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>

.changeset/store-remove-keeps-nested-stores.md

@@ -0,0 +1,5 @@
+---
+'@fission-ai/openspec': patch
+---
+
+Stop `openspec store remove` deleting a store the user did not name. Remove deletes the target's folder recursively, but it checked only the target's own metadata, so any other registered store living inside that folder was deleted with it, uncommitted planning work included, while its registry entry was left pointing at a path that no longer existed. The natural way to get there is a shared store vendored into another as a git submodule, a layout `store register` accepts. Remove now refuses when another registration points inside the folder, checked under the same registry lock that commits the removal, and the error names each nested store with the `openspec store unregister` command to run first. Removing a store whose other registrations are siblings is unchanged, and `store register` still accepts nested checkouts.

docs-lab/reference/cli.md

@@ -1719,6 +1719,8 @@ Error: Pass --yes to delete store files non-interactively.
 Fix: openspec store remove design-system --yes
 ```
 
+Remove exits 1 and deletes nothing when the folder lacks matching store metadata, or when it contains another registered store (for example a store vendored as a Git submodule). In that case the error is `store_remove_contains_registered_store`: run `openspec store unregister <nested-id>` first, or `openspec store unregister <id>` to forget the store without deleting files.
+
 **Options**
 
 | Flag | Effect |

docs/agent-contract.md

@@ -112,7 +112,7 @@ setup/register: `{ "store": {id, root, metadata_path?}, "registry": {path, regis
 `invalid_store_id`, `invalid_store_registry`, `invalid_store_metadata`, `store_registry_busy`, `store_not_found`, `no_store_registry`, `store_registry_changed`, `store_metadata_missing`, `store_metadata_id_mismatch`, `store_metadata_invalid`, `store_id_conflict`, `store_path_conflict`, `store_already_registered` (info).
 
 ### Store setup/register/remove
-`store_setup_id_required`, `store_setup_path_required`, `store_setup_path_not_directory`, `store_setup_inside_git_repo`, `store_setup_non_empty_directory`, `store_setup_cancelled`, `store_path_required`, `store_path_missing`, `store_path_not_directory`, `store_root_pointer_declared`, `store_register_root_unhealthy`, `store_register_identity_confirmation_required`, `store_register_cancelled`, `store_remote_empty`, `store_remote_requires_hand_edit`, `store_remove_confirmation_required`, `store_remove_cancelled`, `store_remove_path_not_directory`, `store_remove_metadata_missing`, `store_root_missing` (warning in remove, error in doctor), `store_root_not_directory`.
+`store_setup_id_required`, `store_setup_path_required`, `store_setup_path_not_directory`, `store_setup_inside_git_repo`, `store_setup_non_empty_directory`, `store_setup_cancelled`, `store_path_required`, `store_path_missing`, `store_path_not_directory`, `store_root_pointer_declared`, `store_register_root_unhealthy`, `store_register_identity_confirmation_required`, `store_register_cancelled`, `store_remote_empty`, `store_remote_requires_hand_edit`, `store_remove_confirmation_required`, `store_remove_cancelled`, `store_remove_path_not_directory`, `store_remove_metadata_missing`, `store_remove_contains_registered_store`, `store_root_missing` (warning in remove, error in doctor), `store_root_not_directory`.
 
 ### Store git
 `store_git_init_failed`, `store_git_identity_missing`, `store_git_commit_failed`, `store_git_no_commits` (warning), `store_clone_fragile_directories` (warning), `store_remote_divergence` (info, doctor), `store_checkout_drift` (info, doctor).

src/core/store/operations.ts

@@ -949,6 +949,40 @@ async function assertSafeToDeleteStoreRoot(storeRoot: string, id: string): Promi
   return { exists: true };
 }
 
+/**
+ * Deleting a store root takes everything under it, including any other
+ * store registered inside it (a shared store vendored as a submodule, for
+ * example). `store remove <id>` never asked for that store to go.
+ */
+function assertNoRegisteredStoreInside(
+  storeRoot: string,
+  id: string,
+  others: Array<{ id: string; storeRoot: string }>
+): void {
+  const root = normalizeRegistryPathForComparison(storeRoot);
+  const nested = others.filter((other) => {
+    const relative = path.relative(root, normalizeRegistryPathForComparison(other.storeRoot));
+    return (
+      relative.length > 0 &&
+      relative !== '..' &&
+      !relative.startsWith(`..${path.sep}`) &&
+      !path.isAbsolute(relative)
+    );
+  });
+  if (nested.length === 0) return;
+
+  const listed = nested.map((other) => `'${other.id}' (${other.storeRoot})`).join(', ');
+  const unregister = nested.map((other) => `openspec store unregister ${other.id}`).join(', then ');
+  throw new StoreError(
+    `Store remove refuses to delete ${storeRoot}: it contains ${nested.length === 1 ? 'another registered store' : 'other registered stores'}: ${listed}.`,
+    'store_remove_contains_registered_store',
+    {
+      target: 'store.root',
+      fix: `Unregister or remove ${nested.length === 1 ? 'that store' : 'those stores'} first (${unregister}), or run "openspec store unregister ${id}" to forget '${id}' without deleting files.`,
+    }
+  );
+}
+
 export async function removeStore(
   target: PreparedStoreCleanup
 ): Promise<StoreCleanupResult> {
@@ -964,9 +998,12 @@ export async function removeStore(
     id,
     expectedBackend: target.backend,
     globalDataDir: target.globalDataDir,
-    beforeCommit: async (entry) => {
+    beforeCommit: async (entry, remaining) => {
       const safeTarget = await assertSafeToDeleteStoreRoot(entry.storeRoot, id);
       rootMissing = !safeTarget.exists;
+      if (safeTarget.exists) {
+        assertNoRegisteredStoreInside(entry.storeRoot, id, remaining);
+      }
     },
   });
 

src/core/store/registry.ts

@@ -39,7 +39,11 @@ export interface GetRegisteredStoreInput extends ResolveRegisteredStoreInput {
 export interface UnregisterStoreInput extends StorePathOptions {
   id: string;
   expectedBackend?: StoreGitBackendConfig;
-  beforeCommit?: (entry: RegisteredStoreEntry) => Promise<void>;
+  /** Runs under the registry lock, with the registrations that will remain. */
+  beforeCommit?: (
+    entry: RegisteredStoreEntry,
+    remaining: RegisteredStoreEntry[]
+  ) => Promise<void>;
 }
 
 export type ListRegisteredStoresOptions = StorePathOptions;
@@ -414,7 +418,11 @@ export async function unregisterStoreRegistration(
         ...result.removed,
         storeRoot: getStoreRootForBackend(result.removed.backend),
       };
-      await input.beforeCommit?.(removedEntry);
+      const remaining = listStoreRegistryEntries(result.next).map((entry) => ({
+        ...entry,
+        storeRoot: getStoreRootForBackend(entry.backend),
+      }));
+      await input.beforeCommit?.(removedEntry, remaining);
       removed = result.removed;
       return result.next;
     },

test/commands/store-remove-nested.test.ts

@@ -0,0 +1,270 @@
+import { afterEach, beforeEach, describe, expect, it } from 'vitest';
+import { execFileSync } from 'node:child_process';
+import * as fs from 'node:fs';
+import * as os from 'node:os';
+import * as path from 'node:path';
+
+import {
+  getGlobalDataDir,
+  getStoreMetadataPath,
+  readStoreRegistryState,
+  writeStoreMetadataState,
+  writeStoreRegistryState,
+} from '../../src/core/index.js';
+import { runCLI, type RunCLIResult } from '../helpers/run-cli.js';
+import { createHealthyOpenSpecRoot, isolatedGitEnv } from '../helpers/store-git.js';
+
+/**
+ * `store remove` deletes the store folder recursively. Another registered
+ * store can live inside that folder, most naturally a shared store vendored as
+ * a git submodule, and `store register` accepts that layout. Removing the
+ * outer store must never delete the inner one or the uncommitted work in it,
+ * and must never leave the registry pointing into a deleted folder.
+ */
+describe('store remove with another registered store inside the target', () => {
+  let tempDir: string;
+  let globalDataDir: string;
+  let env: NodeJS.ProcessEnv;
+
+  beforeEach(() => {
+    tempDir = fs.realpathSync.native(
+      fs.mkdtempSync(path.join(os.tmpdir(), 'openspec-store-remove-nested-'))
+    );
+    env = {
+      XDG_DATA_HOME: path.join(tempDir, 'data'),
+      XDG_CONFIG_HOME: path.join(tempDir, 'config'),
+      OPEN_SPEC_INTERACTIVE: '0',
+      OPENSPEC_TELEMETRY: '0',
+    };
+    globalDataDir = getGlobalDataDir({ env });
+  });
+
+  afterEach(() => {
+    fs.rmSync(tempDir, { recursive: true, force: true, maxRetries: 5, retryDelay: 100 });
+  });
+
+  async function makeStore(relativePath: string, id: string): Promise<string> {
+    const root = path.join(tempDir, relativePath);
+    createHealthyOpenSpecRoot(root);
+    await writeStoreMetadataState(root, { version: 1, id });
+    return fs.realpathSync.native(root);
+  }
+
+  async function register(stores: Record<string, string>): Promise<void> {
+    await writeStoreRegistryState(
+      {
+        version: 1,
+        stores: Object.fromEntries(
+          Object.entries(stores).map(([id, localPath]) => [
+            id,
+            { backend: { type: 'git' as const, local_path: localPath } },
+          ])
+        ),
+      },
+      { globalDataDir }
+    );
+  }
+
+  /** Planning work that exists only on disk, never committed anywhere. */
+  function writeDraft(storeRoot: string): string {
+    const draft = path.join(storeRoot, 'openspec', 'changes', 'draft-idea', 'proposal.md');
+    fs.mkdirSync(path.dirname(draft), { recursive: true });
+    fs.writeFileSync(draft, '# Draft (uncommitted work)\n');
+    return draft;
+  }
+
+  /** `team-plans` with `plat` registered inside it, as a vendored store would be. */
+  async function nestedLayout(): Promise<{ teamPlans: string; plat: string; draft: string }> {
+    const teamPlans = await makeStore(path.join('openspec', 'team-plans'), 'team-plans');
+    const plat = await makeStore(path.join('openspec', 'team-plans', 'vendor', 'plat'), 'plat');
+    await register({ plat, 'team-plans': teamPlans });
+    return { teamPlans, plat, draft: writeDraft(plat) };
+  }
+
+  function remove(id: string, options: { json?: boolean } = { json: true }): Promise<RunCLIResult> {
+    return runCLI(
+      ['store', 'remove', id, '--yes', ...(options.json ? ['--json'] : [])],
+      { cwd: tempDir, env }
+    );
+  }
+
+  function parseJson(result: RunCLIResult): any {
+    try {
+      return JSON.parse(result.stdout);
+    } catch (error) {
+      throw new Error(
+        `Could not parse JSON.\nCommand: ${result.command}\nstdout:\n${result.stdout}\nstderr:\n${result.stderr}\n${String(error)}`
+      );
+    }
+  }
+
+  async function registeredIds(): Promise<string[]> {
+    const registry = await readStoreRegistryState({ globalDataDir });
+    return Object.keys(registry?.stores ?? {}).sort();
+  }
+
+  it('control: removes the store when the other registered store is a sibling', async () => {
+    const teamPlans = await makeStore(path.join('openspec', 'team-plans'), 'team-plans');
+    const plat = await makeStore(path.join('work', 'plat'), 'plat');
+    await register({ plat, 'team-plans': teamPlans });
+    const draft = writeDraft(plat);
+
+    const result = await remove('team-plans');
+
+    expect(result.exitCode).toBe(0);
+    expect(parseJson(result).files).toEqual(
+      expect.objectContaining({ deleted: true, deleted_path: teamPlans })
+    );
+    expect(fs.existsSync(teamPlans)).toBe(false);
+    expect(fs.existsSync(draft)).toBe(true);
+    expect(await registeredIds()).toEqual(['plat']);
+  }, 30_000);
+
+  it('refuses to delete a folder that contains another registered store', async () => {
+    const { teamPlans, draft } = await nestedLayout();
+
+    const result = await remove('team-plans');
+
+    expect(result.exitCode).toBe(1);
+    expect(fs.existsSync(draft)).toBe(true);
+    expect(fs.existsSync(getStoreMetadataPath(teamPlans))).toBe(true);
+    expect(await registeredIds()).toEqual(['plat', 'team-plans']);
+  }, 30_000);
+
+  it('names the nested store and the way out in the JSON refusal', async () => {
+    const { plat } = await nestedLayout();
+
+    const payload = parseJson(await remove('team-plans'));
+
+    expect(payload.store).toBeNull();
+    expect(payload.files).toBeNull();
+    expect(payload.status).toHaveLength(1);
+    const [diagnostic] = payload.status;
+    expect(diagnostic).toEqual(
+      expect.objectContaining({
+        severity: 'error',
+        code: 'store_remove_contains_registered_store',
+        target: 'store.root',
+      })
+    );
+    expect(diagnostic.message).toContain("'plat'");
+    expect(diagnostic.message).toContain(plat);
+    expect(diagnostic.fix).toContain('openspec store unregister plat');
+  }, 30_000);
+
+  it('prints the refusal in human mode and deletes nothing', async () => {
+    const { draft } = await nestedLayout();
+
+    const result = await remove('team-plans', { json: false });
+
+    expect(result.exitCode).toBe(1);
+    expect(result.stderr).toContain("'plat'");
+    expect(result.stderr).toContain('openspec store unregister plat');
+    expect(fs.existsSync(draft)).toBe(true);
+  }, 30_000);
+
+  it('lists every nested store in the refusal', async () => {
+    const teamPlans = await makeStore(path.join('openspec', 'team-plans'), 'team-plans');
+    const plat = await makeStore(path.join('openspec', 'team-plans', 'vendor', 'plat'), 'plat');
+    const docs = await makeStore(path.join('openspec', 'team-plans', 'vendor', 'docs'), 'docs');
+    await register({ docs, plat, 'team-plans': teamPlans });
+
+    const [diagnostic] = parseJson(await remove('team-plans')).status;
+
+    expect(diagnostic.code).toBe('store_remove_contains_registered_store');
+    expect(diagnostic.message).toContain("'docs'");
+    expect(diagnostic.message).toContain("'plat'");
+    expect(await registeredIds()).toEqual(['docs', 'plat', 'team-plans']);
+  }, 30_000);
+
+  it('does not treat a sibling that shares the name prefix as nested', async () => {
+    const teamPlans = await makeStore(path.join('stores', 'team-plans'), 'team-plans');
+    const archive = await makeStore(path.join('stores', 'team-plans-archive'), 'team-plans-archive');
+    await register({ 'team-plans': teamPlans, 'team-plans-archive': archive });
+
+    const result = await remove('team-plans');
+
+    expect(result.exitCode).toBe(0);
+    expect(fs.existsSync(teamPlans)).toBe(false);
+    expect(fs.existsSync(getStoreMetadataPath(archive))).toBe(true);
+  }, 30_000);
+
+  it('refuses while a stale registration still points inside the folder', async () => {
+    const teamPlans = await makeStore(path.join('openspec', 'team-plans'), 'team-plans');
+    await register({
+      plat: path.join(teamPlans, 'vendor', 'plat'),
+      'team-plans': teamPlans,
+    });
+
+    const result = await remove('team-plans');
+
+    expect(result.exitCode).toBe(1);
+    expect(parseJson(result).status[0].code).toBe('store_remove_contains_registered_store');
+    expect(fs.existsSync(teamPlans)).toBe(true);
+  }, 30_000);
+
+  it('removes the outer store once the nested store is unregistered', async () => {
+    const { teamPlans, draft } = await nestedLayout();
+    const unregister = await runCLI(['store', 'unregister', 'plat', '--json'], { cwd: tempDir, env });
+    expect(unregister.exitCode).toBe(0);
+    // Unregister forgets the registration and leaves the files alone.
+    expect(fs.existsSync(draft)).toBe(true);
+
+    const result = await remove('team-plans');
+
+    expect(result.exitCode).toBe(0);
+    expect(fs.existsSync(teamPlans)).toBe(false);
+    expect(await registeredIds()).toEqual([]);
+  }, 30_000);
+
+  // Creating a directory symlink needs elevated rights on Windows.
+  it.skipIf(process.platform === 'win32')(
+    'finds a nested store registered through a symlinked path',
+    async () => {
+      const teamPlans = await makeStore(path.join('openspec', 'team-plans'), 'team-plans');
+      await makeStore(path.join('openspec', 'team-plans', 'vendor', 'plat'), 'plat');
+      const link = path.join(tempDir, 'team-plans-link');
+      fs.symlinkSync(teamPlans, link, 'dir');
+      await register({
+        plat: path.join(link, 'vendor', 'plat'),
+        'team-plans': teamPlans,
+      });
+
+      const result = await remove('team-plans');
+
+      expect(result.exitCode).toBe(1);
+      expect(parseJson(result).status[0].code).toBe('store_remove_contains_registered_store');
+      expect(fs.existsSync(path.join(teamPlans, 'vendor', 'plat'))).toBe(true);
+    },
+    30_000
+  );
+
+  it('refuses a store vendored as a git submodule and keeps its uncommitted work', async () => {
+    const gitEnv = { ...process.env, ...isolatedGitEnv(tempDir) };
+    const git = (cwd: string, args: string[]) =>
+      execFileSync('git', args, { cwd, env: gitEnv, stdio: 'pipe' });
+
+    const upstream = await makeStore(path.join('upstream', 'plat'), 'plat');
+    git(upstream, ['init', '-q']);
+    git(upstream, ['add', '-A']);
+    git(upstream, ['commit', '-qm', 'plat store']);
+
+    const teamPlans = await makeStore(path.join('openspec', 'team-plans'), 'team-plans');
+    git(teamPlans, ['init', '-q']);
+    git(teamPlans, ['add', '-A']);
+    git(teamPlans, ['commit', '-qm', 'team-plans store']);
+    git(teamPlans, ['-c', 'protocol.file.allow=always', 'submodule', 'add', '-q', upstream, 'vendor/plat']);
+    git(teamPlans, ['commit', '-qm', 'vendor plat']);
+
+    const plat = fs.realpathSync.native(path.join(teamPlans, 'vendor', 'plat'));
+    await register({ plat, 'team-plans': teamPlans });
+    const draft = writeDraft(plat);
+
+    const result = await remove('team-plans');
+
+    expect(result.exitCode).toBe(1);
+    expect(parseJson(result).status[0].code).toBe('store_remove_contains_registered_store');
+    expect(fs.existsSync(draft)).toBe(true);
+    expect(await registeredIds()).toEqual(['plat', 'team-plans']);
+  }, 30_000);
+});