diff --git a/.changeset/1483-external-state-collision.md b/.changeset/1483-external-state-collision.md new file mode 100644 index 000000000..e7d26410c --- /dev/null +++ b/.changeset/1483-external-state-collision.md @@ -0,0 +1,5 @@ +--- +'@bradygaster/squad-cli': patch +--- + +Refuse to overwrite external state owned by a different project. diff --git a/docs/src/content/docs/features/external-state.md b/docs/src/content/docs/features/external-state.md index f44f24dd7..4d5db783a 100644 --- a/docs/src/content/docs/features/external-state.md +++ b/docs/src/content/docs/features/external-state.md @@ -74,7 +74,7 @@ Reviewers see squad metadata mixed with your actual code changes. **Result:** - Squad state persists across branch switches - PRs never contain `.squad/` files -- State is isolated per repository (based on repo name) +- State is keyed per project; use `--key` when projects resolve to the same default key --- @@ -107,6 +107,14 @@ squad externalize - All squad state lives in global directory - Branch switches don't affect squad data +External state directories are keyed by the project directory name by default. Squad records the owning project in an `.owner.json` file in the global directory. If a different project resolves to a key that is already owned, `squad externalize` refuses before moving state. Choose a unique key to keep both projects' state separate: + +```bash +squad externalize --key acme-api +``` + +A populated directory from an older Squad version can be adopted when this project's `.squad/config.json` already points to that key. An occupied directory without matching project configuration is left untouched and refused rather than overwritten. + --- ### Internalize @@ -172,7 +180,7 @@ The thin marker file `.squad/config.json` tracks state location: skills/ ``` -Each repo gets its own isolated directory based on repository name. State is never shared across repos. +Each project key gets its own directory. The default key comes from the project directory name, so projects with the same name should use distinct keys with `squad externalize --key `. --- @@ -193,7 +201,7 @@ Each repo gets its own isolated directory based on repository name. State is nev ## Multi-Repo Workflows -External state is **isolated per repository** — each repo gets its own global directory. If you work on multiple repos, each maintains separate squad state: +External state is **keyed per project**. If you work on multiple repos with different directory names, each gets a separate global directory. If two project directories resolve to the same key, choose a distinct key with `--key`: ``` ~/Library/Application Support/squad/projects/ diff --git a/packages/squad-cli/src/cli/commands/externalize.ts b/packages/squad-cli/src/cli/commands/externalize.ts index 10ab9b6dd..8ba45382d 100644 --- a/packages/squad-cli/src/cli/commands/externalize.ts +++ b/packages/squad-cli/src/cli/commands/externalize.ts @@ -15,10 +15,12 @@ */ import path from 'node:path'; +import { writeFileSync } from 'node:fs'; import { FSStorageProvider, resolveExternalStateDir, deriveProjectKey } from '@bradygaster/squad-sdk'; import { fatal } from '../core/errors.js'; const storage = new FSStorageProvider(); +const EXTERNAL_OWNER_FILE = '.owner.json'; /** Entries under .squad/ that must NOT be externalized (they stay in the repo). * All of these are read from the working tree by runtime code that does not @@ -37,6 +39,7 @@ const KEEP_LOCAL = new Set([ 'upstream.json', 'squad-registry.json', '_upstream_repos', + EXTERNAL_OWNER_FILE, ]); /** @@ -53,7 +56,25 @@ export function runExternalize(projectDir: string, projectKey?: string): void { // Derive project key const key = projectKey || deriveProjectKey(projectDir); - const externalDir = resolveExternalStateDir(key, true); + const externalDir = resolveExternalStateDir(key, false); + + // Read the local config before moving anything. It can identify an existing + // external directory created by an older Squad version that had no owner file. + const configPath = path.join(squadDir, 'config.json'); + let existingConfig: Record = {}; + if (storage.existsSync(configPath)) { + try { + const raw = storage.readSync(configPath); + if (raw != null) { + const parsed = JSON.parse(raw); + if (parsed && typeof parsed === 'object' && !Array.isArray(parsed)) { + existingConfig = parsed as Record; + } + } + } catch { /* start fresh if config is malformed */ } + } + + ensureExternalStateOwnership(projectDir, key, externalDir, existingConfig); console.log(`📦 Externalizing .squad/ state to: ${externalDir}`); @@ -79,20 +100,6 @@ export function runExternalize(projectDir: string, projectKey?: string): void { } // Write thin config.json marker, preserving any existing config fields - const configPath = path.join(squadDir, 'config.json'); - let existingConfig: Record = {}; - if (storage.existsSync(configPath)) { - try { - const raw = storage.readSync(configPath); - if (raw != null) { - const parsed = JSON.parse(raw); - if (parsed && typeof parsed === 'object' && !Array.isArray(parsed)) { - existingConfig = parsed as Record; - } - } - } catch { /* start fresh if config is malformed */ } - } - const config = { ...existingConfig, version: 1, @@ -113,6 +120,71 @@ export function runExternalize(projectDir: string, projectKey?: string): void { console.log(` Run \`squad internalize\` to move state back.`); } +function ensureExternalStateOwnership( + projectDir: string, + projectKey: string, + externalDir: string, + existingConfig: Record, +): void { + const ownerPath = path.join(externalDir, EXTERNAL_OWNER_FILE); + const projectPath = path.resolve(projectDir); + const normalizedProjectPath = normalizeProjectPath(projectPath); + + const assertOwner = (): void => { + let ownerProjectPath: unknown; + try { + const raw = storage.readSync(ownerPath); + const owner = JSON.parse(raw ?? '') as { projectPath?: unknown }; + ownerProjectPath = owner?.projectPath; + } catch { + fatal(`External state directory has an invalid ownership marker: ${ownerPath}. Refusing to move state.`); + } + + if (typeof ownerProjectPath !== 'string') { + fatal(`External state directory has an invalid ownership marker: ${ownerPath}. Refusing to move state.`); + } + if (normalizeProjectPath(ownerProjectPath) !== normalizedProjectPath) { + fatal(`External state directory "${externalDir}" already belongs to "${ownerProjectPath}". Choose a different project key with --key before externalizing.`); + } + }; + + if (storage.existsSync(ownerPath)) { + assertOwner(); + return; + } + + const existingEntries = storage.existsSync(externalDir) + ? storage.listSync?.(externalDir) ?? [] + : []; + if (existingEntries.includes(EXTERNAL_OWNER_FILE)) { + assertOwner(); + return; + } + + const canAdoptLegacyDirectory = existingConfig['stateLocation'] === 'external' + && existingConfig['projectKey'] === projectKey; + if (existingEntries.length > 0 && !canAdoptLegacyDirectory) { + fatal(`External state directory "${externalDir}" already contains data but has no ownership marker. Refusing to move state. Choose a different project key with --key, or run externalize from the project whose config.json points to this key.`); + } + + storage.mkdirSync(externalDir, { recursive: true }); + try { + writeFileSync( + ownerPath, + JSON.stringify({ version: 1, projectPath }, null, 2) + '\n', + { flag: 'wx' }, + ); + } catch (error) { + if ((error as NodeJS.ErrnoException).code !== 'EEXIST') throw error; + assertOwner(); + } +} + +function normalizeProjectPath(projectPath: string): string { + const resolved = path.resolve(projectPath); + return process.platform === 'win32' ? resolved.toLowerCase() : resolved; +} + /** * Move externalized state back into the working tree. */ @@ -150,7 +222,7 @@ export function runInternalize(projectDir: string): void { // Dynamically discover everything in the external dir and copy it back. const entries = storage.listSync?.(externalDir) ?? []; for (const entry of entries) { - if (KEEP_LOCAL.has(entry)) continue; + if (KEEP_LOCAL.has(entry) || entry === EXTERNAL_OWNER_FILE) continue; const src = path.join(externalDir, entry); const dest = path.join(squadDir, entry); if (storage.isDirectorySync(src)) { diff --git a/test/external-state.test.ts b/test/external-state.test.ts index e70239463..7e5c5a4fc 100644 --- a/test/external-state.test.ts +++ b/test/external-state.test.ts @@ -183,6 +183,8 @@ describe('runExternalize / runInternalize', () => { const externalDir = resolveExternalStateDir(key, false); expect(existsSync(path.join(externalDir, 'team.md'))).toBe(true); expect(existsSync(path.join(externalDir, 'agents', 'bot.md'))).toBe(true); + const owner = JSON.parse(readFileSync(path.join(externalDir, '.owner.json'), 'utf-8')); + expect(owner.projectPath).toBe(path.resolve(projectDir)); // KEEP_LOCAL entries must NOT be externalized expect(existsSync(path.join(externalDir, 'manifest.json'))).toBe(false); expect(existsSync(path.join(externalDir, 'workstreams.json'))).toBe(false); @@ -246,6 +248,7 @@ describe('runExternalize / runInternalize', () => { expect(readFileSync(path.join(squadDir, 'team.md'), 'utf-8')).toBe('# My Team\n'); expect(readFileSync(path.join(squadDir, 'custom-data.json'), 'utf-8')).toBe('{"round":"trip"}'); expect(readFileSync(path.join(squadDir, 'log', 'session.md'), 'utf-8')).toBe('## Session 1\n'); + expect(existsSync(path.join(squadDir, '.owner.json'))).toBe(false); }); // T4b: binary files survive externalize → internalize byte-for-byte (#1489) @@ -313,4 +316,46 @@ describe('runExternalize / runInternalize', () => { const config = JSON.parse(readFileSync(path.join(squadDir, 'config.json'), 'utf-8')); expect(config.stateLocation).toBe('external'); }); + + it('refuses to overwrite state owned by another project with the same key', () => { + writeFileSync(path.join(squadDir, 'team.md'), '# First project\n'); + runExternalize(projectDir); + + const externalDir = resolveExternalStateDir(deriveProjectKey(projectDir), false); + const otherProjectDir = path.join(TEST_ROOT, 'another-parent', path.basename(projectDir)); + const otherSquadDir = path.join(otherProjectDir, '.squad'); + mkdirSync(otherSquadDir, { recursive: true }); + writeFileSync(path.join(otherSquadDir, 'team.md'), '# Second project\n'); + + expect(() => runExternalize(otherProjectDir)).toThrow(/already belongs.*--key/); + expect(readFileSync(path.join(externalDir, 'team.md'), 'utf-8')).toBe('# First project\n'); + expect(readFileSync(path.join(otherSquadDir, 'team.md'), 'utf-8')).toBe('# Second project\n'); + }); + + it('refuses an occupied legacy directory without a matching external config', () => { + const externalDir = resolveExternalStateDir(deriveProjectKey(projectDir), false); + mkdirSync(externalDir, { recursive: true }); + writeFileSync(path.join(externalDir, 'team.md'), '# Existing state\n'); + writeFileSync(path.join(squadDir, 'team.md'), '# Local state\n'); + + expect(() => runExternalize(projectDir)).toThrow(/no ownership marker/); + expect(readFileSync(path.join(externalDir, 'team.md'), 'utf-8')).toBe('# Existing state\n'); + expect(readFileSync(path.join(squadDir, 'team.md'), 'utf-8')).toBe('# Local state\n'); + }); + + it('adopts a populated legacy directory already referenced by this project', () => { + const key = deriveProjectKey(projectDir); + const externalDir = resolveExternalStateDir(key, false); + mkdirSync(externalDir, { recursive: true }); + writeFileSync(path.join(externalDir, 'team.md'), '# Existing state\n'); + writeFileSync( + path.join(squadDir, 'config.json'), + JSON.stringify({ stateLocation: 'external', projectKey: key }), + ); + + expect(() => runExternalize(projectDir)).not.toThrow(); + const owner = JSON.parse(readFileSync(path.join(externalDir, '.owner.json'), 'utf-8')); + expect(owner.projectPath).toBe(path.resolve(projectDir)); + expect(readFileSync(path.join(externalDir, 'team.md'), 'utf-8')).toBe('# Existing state\n'); + }); });