diff --git a/.changeset/fix-1211-deletedir-external-stub.md b/.changeset/fix-1211-deletedir-external-stub.md new file mode 100644 index 000000000..fe18c8de2 --- /dev/null +++ b/.changeset/fix-1211-deletedir-external-stub.md @@ -0,0 +1,6 @@ +--- +"@bradygaster/squad-sdk": patch +"@bradygaster/squad-cli": patch +--- + +Fix StateBackendStorageAdapter.deleteDir leaving nested keys behind: a single-level list+delete pass missed deeper subtrees (e.g. `agents/x/history/2026/log.md` survived a delete of `agents/x`), silently leaking state on git-notes and orphan backends. deleteDir now walks the full subtree on every backend. Also rename the unimplemented `stateBackend: 'external'` placeholder to `'external-stub'` so it can no longer be confused with the real external-state feature (`squad externalize` / `stateLocation: 'external'`); the legacy name is still accepted and normalized with a one-time deprecation warning. diff --git a/docs/src/content/docs/features/state-backends.md b/docs/src/content/docs/features/state-backends.md index 6d8a5ed87..ee621ed38 100644 --- a/docs/src/content/docs/features/state-backends.md +++ b/docs/src/content/docs/features/state-backends.md @@ -277,7 +277,7 @@ git-native backends (orphan, two-layer) operate in the correct repository contex - State backends are **opt-in** — the default is `local` (no behavior change) - All backends implement the same interface — agents don't know or care which backend is active - Empty directories are automatically pruned after the last file is deleted (orphan backend) -- The `external` backend type exists as a stub for future external storage (see [External State](./external-state)) +- The `external-stub` backend type is an unimplemented placeholder that falls back to `local` (the legacy name `external` is still accepted with a deprecation warning); for real external storage see [External State](./external-state) - State backends are available in the **insider** release channel (`@bradygaster/squad-cli@insider`) - 63 unit tests + 46 E2E tests cover all backends including security hardening, content fidelity, and directory pruning diff --git a/packages/squad-cli/src/cli-entry.ts b/packages/squad-cli/src/cli-entry.ts index b92d95dd2..664184c33 100644 --- a/packages/squad-cli/src/cli-entry.ts +++ b/packages/squad-cli/src/cli-entry.ts @@ -646,11 +646,12 @@ async function main(): Promise { const rawStateBackend = (stateBackendIdx !== -1 && args[stateBackendIdx + 1]) ? args[stateBackendIdx + 1] : undefined; - const validBackends = ['local', 'orphan', 'two-layer', 'external'] as const; + const validBackends = ['local', 'orphan', 'two-layer', 'external', 'external-stub'] as const; if (rawStateBackend && !(validBackends as readonly string[]).includes(rawStateBackend)) { console.error(`\u26a0\ufe0f Invalid --state-backend "${rawStateBackend}". Valid: ${validBackends.join(', ')}.`); process.exit(1); } + // Legacy 'external' is normalized (with a deprecation warning) inside resolveStateBackend. const mappedBackend = rawStateBackend as StateBackendType | undefined; // Resolve the full state context (paths + backend) once at entry. diff --git a/packages/squad-cli/src/cli/commands/doctor.ts b/packages/squad-cli/src/cli/commands/doctor.ts index cede58a91..44916d142 100644 --- a/packages/squad-cli/src/cli/commands/doctor.ts +++ b/packages/squad-cli/src/cli/commands/doctor.ts @@ -216,7 +216,8 @@ function configuredStateBackend(squadDir: string): StateBackendType | undefined const backend = config?.['stateBackend']; if (backend === 'worktree') return 'local'; if (backend === 'git-notes') return 'two-layer'; - if (backend === 'local' || backend === 'external' || backend === 'orphan' || backend === 'two-layer') { + if (backend === 'external') return 'external-stub'; + if (backend === 'local' || backend === 'external-stub' || backend === 'orphan' || backend === 'two-layer') { return backend; } return undefined; diff --git a/packages/squad-cli/src/cli/commands/install-hooks.ts b/packages/squad-cli/src/cli/commands/install-hooks.ts index 759e2cc0c..591f2545c 100644 --- a/packages/squad-cli/src/cli/commands/install-hooks.ts +++ b/packages/squad-cli/src/cli/commands/install-hooks.ts @@ -229,7 +229,7 @@ export function installGitHooks(cwd: string, options: InstallHooksOptions = {}): } } catch { /* proceed anyway */ } - if (backend === 'local' || backend === 'external' || backend === null) { + if (backend === 'local' || backend === 'external' || backend === 'external-stub' || backend === null) { console.log(`${DIM}squad install-hooks: backend is '${backend || 'local'}' — hooks not needed (state syncs with normal git operations).${RESET}`); return; } diff --git a/packages/squad-cli/src/cli/commands/sync.ts b/packages/squad-cli/src/cli/commands/sync.ts index eaeeacb8a..1ff75648c 100644 --- a/packages/squad-cli/src/cli/commands/sync.ts +++ b/packages/squad-cli/src/cli/commands/sync.ts @@ -245,7 +245,7 @@ export async function runSync(options: SyncOptions): Promise { const quiet = options.quiet ?? false; // Skip sync for backends that don't need it - if (backend === 'local' || backend === 'external' || backend === null) { + if (backend === 'local' || backend === 'external' || backend === 'external-stub' || backend === null) { if (!quiet) console.log(`squad sync: backend is '${backend || 'local'}' — no remote sync needed.`); return; } diff --git a/packages/squad-cli/src/cli/commands/watch/config.ts b/packages/squad-cli/src/cli/commands/watch/config.ts index 78e9f092a..b4d4756d0 100644 --- a/packages/squad-cli/src/cli/commands/watch/config.ts +++ b/packages/squad-cli/src/cli/commands/watch/config.ts @@ -144,9 +144,9 @@ function normalizeFileConfig(raw: Record): Partial if (typeof raw['sentinelFile'] === 'string') result.sentinelFile = raw['sentinelFile']; if (typeof raw['stateBackend'] === 'string') { const backend = raw['stateBackend']; - const validBackends = ['local', 'orphan', 'two-layer', 'external'] as const; + const validBackends = ['local', 'orphan', 'two-layer', 'external', 'external-stub'] as const; if ((validBackends as readonly string[]).includes(backend)) { - result.stateBackend = backend as StateBackendType; + result.stateBackend = (backend === 'external' ? 'external-stub' : backend) as StateBackendType; } } diff --git a/packages/squad-cli/src/cli/core/init.ts b/packages/squad-cli/src/cli/core/init.ts index c226e3c8e..c3c66fa41 100644 --- a/packages/squad-cli/src/cli/core/init.ts +++ b/packages/squad-cli/src/cli/core/init.ts @@ -320,17 +320,20 @@ export async function runInit(dest: string, options: RunInitOptions = {}): Promi // Configure state backend if specified at init time if (options.stateBackend) { - const validBackends = ['local', 'orphan', 'two-layer', 'external']; + const validBackends = ['local', 'orphan', 'two-layer', 'external', 'external-stub']; if (validBackends.includes(options.stateBackend)) { + // 'external' is the legacy name for the 'external-stub' placeholder — + // write the canonical name so configs don't trip the deprecation warning. + const backendValue = options.stateBackend === 'external' ? 'external-stub' : options.stateBackend; const configPath = path.join(squadDir, 'config.json'); let config: Record = {}; try { const raw = storage.readSync(configPath); if (raw) config = JSON.parse(raw); } catch { /* start fresh */ } - config['stateBackend'] = options.stateBackend; + config['stateBackend'] = backendValue; storage.writeSync(configPath, JSON.stringify(config, null, 2) + '\n'); - success(`state backend: ${options.stateBackend}`); + success(`state backend: ${backendValue}`); // Auto-create orphan branch for orphan/two-layer backends // Uses git plumbing (mktree + commit-tree + update-ref) so the working tree is never touched. diff --git a/packages/squad-sdk/src/config/init.ts b/packages/squad-sdk/src/config/init.ts index 03fe76f0f..184da690f 100644 --- a/packages/squad-sdk/src/config/init.ts +++ b/packages/squad-sdk/src/config/init.ts @@ -174,7 +174,7 @@ export interface InitOptions { * staged into the working-tree commit graph. * When 'local' (or undefined), removes the marker block if present. */ - stateBackend?: 'local' | 'orphan' | 'two-layer' | 'external' | string; + stateBackend?: 'local' | 'orphan' | 'two-layer' | 'external-stub' | string; } /** diff --git a/packages/squad-sdk/src/state-backend.ts b/packages/squad-sdk/src/state-backend.ts index c3b022eb8..09eda75e6 100644 --- a/packages/squad-sdk/src/state-backend.ts +++ b/packages/squad-sdk/src/state-backend.ts @@ -117,7 +117,7 @@ function isExpectedMissing(err: unknown): boolean { return GIT_EXPECTED_MISSING_RE.test(stderr) || GIT_EXPECTED_MISSING_RE.test(msg); } -export type StateBackendType = 'local' | 'external' | 'orphan' | 'two-layer'; +export type StateBackendType = 'local' | 'external-stub' | 'orphan' | 'two-layer'; export interface StateBackend { read(relativePath: string): string | undefined; @@ -338,7 +338,13 @@ export class WorktreeBackend implements StateBackend { const key = normalizeKey(relativePath); const full = path.join(this.root, key); if (!storage.existsSync(full)) return false; - storage.deleteSync(full); + // Directory keys are removed with their subtree — matching the orphan + // backend, where deleting a tree entry drops everything beneath it. + if (storage.isDirectorySync(full)) { + storage.deleteDirSync(full); + } else { + storage.deleteSync(full); + } return true; } append(relativePath: string, content: string): void { @@ -811,9 +817,7 @@ export class StateBackendStorageAdapter implements StorageProvider { this.backend.delete(this.toRelative(filePath)); } async deleteDir(dirPath: string): Promise { - const rel = this.toRelative(dirPath); - const entries = this.backend.list(rel); - for (const entry of entries) { this.backend.delete(rel ? rel + '/' + entry : entry); } + this.deleteDirRecursive(this.toRelative(dirPath)); } async isDirectory(targetPath: string): Promise { return this.backend.list(this.toRelative(targetPath)).length > 0; @@ -841,13 +845,25 @@ export class StateBackendStorageAdapter implements StorageProvider { listSync(dirPath: string): string[] { return this.backend.list(this.toRelative(dirPath)); } deleteSync(filePath: string): void { this.backend.delete(this.toRelative(filePath)); } deleteDirSync(dirPath: string): void { - const rel = this.toRelative(dirPath); - const entries = this.backend.list(rel); - for (const entry of entries) { this.backend.delete(rel ? rel + '/' + entry : entry); } + this.deleteDirRecursive(this.toRelative(dirPath)); } isDirectorySync(targetPath: string): boolean { return this.backend.list(this.toRelative(targetPath)).length > 0; } + /** + * Delete every key under `rel`, including nested subtrees. A single-level + * list+delete pass is not enough: git-notes stores flat keys whose "directory" + * segments are not deletable keys themselves, so `agents/x/history/2026/log.md` + * would survive a delete of `agents/x`. delete() first (removes leaf keys, and + * whole subtrees on tree-based backends), then recurse into whatever remains. + */ + private deleteDirRecursive(rel: string): void { + for (const entry of this.backend.list(rel)) { + const child = rel ? rel + '/' + entry : entry; + this.backend.delete(child); + if (this.backend.list(child).length > 0) this.deleteDirRecursive(child); + } + } mkdirSync(_dirPath: string, _options?: { recursive?: boolean }): void { /* no-op */ } renameSync(oldPath: string, newPath: string): void { const content = this.backend.read(this.toRelative(oldPath)); @@ -1143,17 +1159,29 @@ export function verifyStateBackend(backend: StateBackend): { ok: boolean; error? } function isValidBackendType(value: string): value is StateBackendType { - return ['local', 'worktree', 'external', 'git-notes', 'orphan', 'two-layer'].includes(value); + return ['local', 'worktree', 'external', 'external-stub', 'git-notes', 'orphan', 'two-layer'].includes(value); } -// Note: 'worktree' and 'git-notes' are accepted for backward compatibility but normalized away +// Note: 'worktree', 'git-notes', and 'external' are accepted for backward compatibility but normalized away -// One-shot flag: warn once per process so repeated resolveStateBackend() calls +// One-shot flags: warn once per process so repeated resolveStateBackend() calls // (e.g. multiple agent startups in the same process) don't spam the console. let _warnedGitNotesMigration = false; +let _warnedExternalStubMigration = false; /** Normalize legacy aliases to canonical backend type names. */ function normalizeBackendType(type: string): StateBackendType { if (type === 'worktree') return 'local'; + if (type === 'external') { + if (!_warnedExternalStubMigration) { + _warnedExternalStubMigration = true; + console.warn( + "[deprecation] stateBackend: 'external' is renamed to 'external-stub'; please update .squad/config.json. " + + "Note: 'external-stub' is an unimplemented placeholder that falls back to 'local'. " + + "To store state outside the working tree, use `squad externalize` instead." + ); + } + return 'external-stub'; + } if (type === 'git-notes') { if (!_warnedGitNotesMigration) { _warnedGitNotesMigration = true; @@ -1180,8 +1208,8 @@ function createBackend(type: StateBackendType, squadDir: string, repoRoot: strin case 'two-layer': requireGitRepository(repoRoot); return new TwoLayerBackend(repoRoot); - case 'external': { - console.warn(`⚠️ State backend 'external' is a stub (PR #797). Using 'local' backend.`); + case 'external-stub': { + console.warn(`⚠️ State backend 'external-stub' is a stub (PR #797). Using 'local' backend.`); return new WorktreeBackend(squadDir); } default: throw new Error(`Unknown state backend type: ${type}`); @@ -1195,4 +1223,9 @@ function requireGitRepository(repoRoot: string): void { /** @internal Reset the one-shot git-notes migration warn flag. Only for use in tests. */ export function _resetGitNotesMigrationWarnForTesting(): void { _warnedGitNotesMigration = false; +} + +/** @internal Reset the one-shot external-stub migration warn flag. Only for use in tests. */ +export function _resetExternalStubMigrationWarnForTesting(): void { + _warnedExternalStubMigration = false; } \ No newline at end of file diff --git a/test/state-backend.test.ts b/test/state-backend.test.ts index 0fba2f078..ea7bdb350 100644 --- a/test/state-backend.test.ts +++ b/test/state-backend.test.ts @@ -4,8 +4,8 @@ import { join } from 'node:path'; import { execSync, execFileSync } from 'node:child_process'; import { randomBytes } from 'node:crypto'; import { tmpdir } from 'node:os'; -import { WorktreeBackend, GitNotesBackend, OrphanBranchBackend, TwoLayerBackend, CircuitBreaker, GitExecError, resolveStateBackend, validateStateKey, StateBackendStorageAdapter, verifyStateBackend, _resetGitNotesMigrationWarnForTesting } from '../packages/squad-sdk/src/state-backend.js'; -import type { StateBackendType } from '../packages/squad-sdk/src/state-backend.js'; +import { WorktreeBackend, GitNotesBackend, OrphanBranchBackend, TwoLayerBackend, CircuitBreaker, GitExecError, resolveStateBackend, validateStateKey, StateBackendStorageAdapter, verifyStateBackend, _resetGitNotesMigrationWarnForTesting, _resetExternalStubMigrationWarnForTesting } from '../packages/squad-sdk/src/state-backend.js'; +import type { StateBackend, StateBackendType } from '../packages/squad-sdk/src/state-backend.js'; import { resolveSquadState, clearResolveSquadCache } from '../packages/squad-sdk/src/resolution.js'; import { ToolRegistry } from '../packages/squad-sdk/src/tools/index.js'; @@ -35,6 +35,14 @@ describe('WorktreeBackend', () => { }); it('list returns empty for non-existent directory', () => { expect(new WorktreeBackend(squadDir()).list('nonexistent')).toEqual([]); }); it('name is local', () => { expect(new WorktreeBackend(squadDir()).name).toBe('local'); }); + it('delete on a directory key removes the whole subtree', () => { + const b = new WorktreeBackend(squadDir()); + b.write('agents/x/history/log.md', 'entry'); + b.write('agents/other.md', 'keep'); + expect(b.delete('agents/x')).toBe(true); + expect(b.exists('agents/x/history/log.md')).toBe(false); + expect(b.read('agents/other.md')).toBe('keep'); + }); }); describe('GitNotesBackend', () => { @@ -104,7 +112,7 @@ describe('OrphanBranchBackend', () => { describe('resolveStateBackend()', () => { const squadDir = () => join(TMP, '.squad'); - beforeEach(() => { clearResolveSquadCache(); _resetGitNotesMigrationWarnForTesting(); if (existsSync(TMP)) rmSync(TMP, { recursive: true, force: true }); initRepo(); mkdirSync(squadDir(), { recursive: true }); }); + beforeEach(() => { clearResolveSquadCache(); _resetGitNotesMigrationWarnForTesting(); _resetExternalStubMigrationWarnForTesting(); if (existsSync(TMP)) rmSync(TMP, { recursive: true, force: true }); initRepo(); mkdirSync(squadDir(), { recursive: true }); }); afterEach(() => { clearResolveSquadCache(); if (existsSync(TMP)) rmSync(TMP, { recursive: true, force: true }); }); it('defaults to local', () => { expect(resolveStateBackend(squadDir(), TMP).name).toBe('local'); }); it('reads stateBackend from config.json (git-notes migrates to two-layer)', () => { @@ -120,10 +128,11 @@ describe('resolveStateBackend()', () => { expect(resolveStateBackend(squadDir(), TMP).name).toBe('local'); }); it('falls back on malformed JSON', () => { writeFileSync(join(squadDir(), 'config.json'), 'bad'); expect(resolveStateBackend(squadDir(), TMP).name).toBe('local'); }); - it('external returns local stub', () => { expect(resolveStateBackend(squadDir(), TMP, 'external').name).toBe('local'); }); + it('external-stub returns local stub', () => { expect(resolveStateBackend(squadDir(), TMP, 'external-stub').name).toBe('local'); }); + it('legacy external alias migrates to external-stub (local)', () => { expect(resolveStateBackend(squadDir(), TMP, 'external' as any).name).toBe('local'); }); it('legacy worktree alias accepted', () => { expect(resolveStateBackend(squadDir(), TMP, 'worktree' as any).name).toBe('local'); }); it('all valid types accepted', () => { - for (const t of ['local', 'external', 'orphan', 'two-layer'] as const) expect(resolveStateBackend(squadDir(), TMP, t)).toBeDefined(); + for (const t of ['local', 'external-stub', 'orphan', 'two-layer'] as const) expect(resolveStateBackend(squadDir(), TMP, t)).toBeDefined(); }); it('legacy git-notes migrates to two-layer', () => { expect(resolveStateBackend(squadDir(), TMP, 'git-notes' as any).name).toBe('two-layer'); @@ -141,6 +150,20 @@ describe('resolveStateBackend()', () => { warnSpy.mockRestore(); } }); + it('external deprecation warning fires exactly once per process across repeated calls', () => { + const warnSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}); + try { + resolveStateBackend(squadDir(), TMP, 'external' as any); + resolveStateBackend(squadDir(), TMP, 'external' as any); + resolveStateBackend(squadDir(), TMP, 'external' as any); + // The deprecation warning fires on the FIRST call only; the per-call + // "is a stub" warning from createBackend still fires every time. + const deprecations = warnSpy.mock.calls.filter((c) => String(c[0]).includes("renamed to 'external-stub'")); + expect(deprecations).toHaveLength(1); + } finally { + warnSpy.mockRestore(); + } + }); it('soft-falls-back to local when an explicit git-native backend is unavailable', () => { const nonGitRoot = join(tmpdir(), `.squad-state-non-git-${randomBytes(4).toString('hex')}`); const nonGitSquad = join(nonGitRoot, '.squad'); @@ -562,6 +585,53 @@ describe('StateBackendStorageAdapter', () => { const adapter = new StateBackendStorageAdapter(backend, squadDir()); expect(await adapter.stat('nope.md')).toBeUndefined(); }); + + // Recursive deleteDir regression (#1211 concern E): a single-level + // list+delete pass left nested keys behind — deleteDir must walk the + // full subtree on every backend. + const deleteDirBackends: Array<[string, () => StateBackend]> = [ + ['worktree', () => new WorktreeBackend(squadDir())], + ['git-notes', () => new GitNotesBackend(TMP)], + ['orphan', () => new OrphanBranchBackend(TMP)], + ['two-layer', () => new TwoLayerBackend(TMP)], + ]; + for (const [name, makeBackend] of deleteDirBackends) { + it(`deleteDir removes nested subtrees (${name})`, { timeout: 30_000 }, async () => { + const adapter = new StateBackendStorageAdapter(makeBackend(), squadDir()); + for (const key of ['a/b/c', 'a/b/d', 'a/b/e/f', 'a/b/e/g/h', 'a/other']) { + adapter.writeSync(key, `content of ${key}`); + } + await adapter.deleteDir('a/b'); + expect(adapter.existsSync('a/b/c')).toBe(false); + expect(adapter.existsSync('a/b/d')).toBe(false); + expect(adapter.existsSync('a/b/e/f')).toBe(false); + expect(adapter.existsSync('a/b/e/g/h')).toBe(false); + expect(adapter.listSync('a/b')).toEqual([]); + expect(adapter.readSync('a/other')).toBe('content of a/other'); + }); + } + + it('deleteDirSync removes nested subtrees (git-notes)', () => { + const adapter = new StateBackendStorageAdapter(new GitNotesBackend(TMP), squadDir()); + adapter.writeSync('a/b/c', 'x'); + adapter.writeSync('a/b/e/g/h', 'y'); + adapter.writeSync('a/other', 'keep'); + adapter.deleteDirSync('a/b'); + expect(adapter.existsSync('a/b/c')).toBe(false); + expect(adapter.existsSync('a/b/e/g/h')).toBe(false); + expect(adapter.readSync('a/other')).toBe('keep'); + }); + + it('deleteDir removes a git-notes key that is both a file and a directory prefix', async () => { + const backend = new GitNotesBackend(TMP); + const adapter = new StateBackendStorageAdapter(backend, squadDir()); + // Flat-key stores allow 'a/b' to be a leaf AND a directory prefix at once. + backend.write('a/b', 'leaf'); + backend.write('a/b/c', 'nested'); + await adapter.deleteDir('a'); + expect(backend.exists('a/b')).toBe(false); + expect(backend.exists('a/b/c')).toBe(false); + }); }); describe('ToolRegistry state tools with git-native backend', () => {