feat(sdk): git-notes + orphan-branch state backends (#807)#810
feat(sdk): git-notes + orphan-branch state backends (#807)#810tamirdresher wants to merge 4 commits intodevfrom
Conversation
Adds StateBackend interface with three implementations: - WorktreeBackend: default, reads/writes .squad/ on disk - GitNotesBackend: stores state in refs/notes/squad - OrphanBranchBackend: stores state on squad-state orphan branch Also adds: - resolveStateBackend() config resolution with fallback - --state-backend CLI flag for init and watch commands - stateBackend field in SquadDirConfig and WatchConfig - Unit tests covering all backends and config resolution Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
🛫 PR Readiness Check
|
| Status | Check | Details |
|---|---|---|
| ❌ | Single commit | 4 commits — consider squashing before review |
| ✅ | Not in draft | Ready for review |
| ✅ | Branch up to date | Up to date with dev |
| ❌ | Copilot review | No Copilot review yet — it may still be processing |
| ✅ | Changeset present | Changeset file found |
| ✅ | Scope clean | No .squad/ or docs/proposals/ files |
| ✅ | No merge conflicts | No merge conflicts |
| ❌ | Copilot threads resolved | 8 unresolved Copilot thread(s) — fix and resolve before merging |
| ❌ | CI passing | 15 check(s) still running |
This check runs automatically on every push. Fix any ❌ items and push again.
See CONTRIBUTING.md and PR Requirements for details.
There was a problem hiding this comment.
Pull request overview
Adds git-native state storage options for Squad’s .squad/ state to avoid worktree churn and PR pollution, wiring new SDK backends into config resolution and exposing a CLI flag for init and watch.
Changes:
- Introduces
StateBackendabstraction plusGitNotesBackendandOrphanBranchBackendimplementations in the SDK. - Adds
stateBackendplumbing to SDK/CLI config loading and exposes--state-backendin the CLI. - Adds unit tests for the new backends and a changeset for SDK/CLI minor release.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
test/state-backend.test.ts |
Adds unit tests covering backend read/write/list semantics and backend resolution behavior. |
packages/squad-sdk/src/state-backend.ts |
Implements Worktree/Git Notes/Orphan Branch backends and resolveStateBackend() factory. |
packages/squad-sdk/src/resolution.ts |
Extends .squad/config.json schema model to include stateBackend. |
packages/squad-sdk/src/index.ts |
Exports new backend types/classes and resolver from the SDK barrel. |
packages/squad-cli/src/cli/commands/watch/config.ts |
Adds stateBackend to watch config merge/normalization. |
packages/squad-cli/src/cli-entry.ts |
Adds --state-backend flag handling for init (writes config) and watch (passes override). |
.changeset/git-state-backends.md |
Declares minor bumps and documents the new state backend options. |
| function gitExec(args: string, cwd: string): string | null { | ||
| try { | ||
| return execSync(`git ${args}`, { cwd, encoding: 'utf-8', stdio: ['pipe', 'pipe', 'pipe'] }).trim(); | ||
| } catch { return null; } |
There was a problem hiding this comment.
gitExec() builds a shell command string (execSync("git ${args}")). Because args can include user-influenced values (e.g., branch name and relative paths), this is vulnerable to shell-injection and quoting bugs (especially on Windows). Use execFileSync('git', [...])/spawnSync with an argument array (no shell) and pass path/ref values as separate args.
| export class WorktreeBackend implements StateBackend { | ||
| readonly name = 'worktree'; | ||
| private readonly root: string; | ||
| constructor(squadDir: string) { this.root = squadDir; } | ||
| read(relativePath: string): string | undefined { | ||
| return storage.readSync(path.join(this.root, relativePath)) ?? undefined; | ||
| } | ||
| write(relativePath: string, content: string): void { | ||
| storage.writeSync(path.join(this.root, relativePath), content); | ||
| } | ||
| exists(relativePath: string): boolean { | ||
| return storage.existsSync(path.join(this.root, relativePath)); | ||
| } |
There was a problem hiding this comment.
WorktreeBackend joins this.root with relativePath, but uses an FSStorageProvider constructed without a rootDir, so ../ segments can escape the squad directory and read/write arbitrary files. Instantiate FSStorageProvider with rootDir: squadDir (per-backend) or explicitly reject traversal segments before calling into storage.
| list(relativeDir: string): string[] { | ||
| const blob = this.loadBlob(); | ||
| const dirPrefix = normalizeKey(relativeDir) + '/'; | ||
| const entries = new Set<string>(); | ||
| for (const key of Object.keys(blob)) { | ||
| if (key.startsWith(dirPrefix)) { | ||
| const rest = key.slice(dirPrefix.length); | ||
| const slash = rest.indexOf('/'); | ||
| entries.add(slash === -1 ? rest : rest.slice(0, slash)); | ||
| } |
There was a problem hiding this comment.
GitNotesBackend.list('') currently returns an empty array because dirPrefix becomes '/', which will never match normalized keys like team.md. To keep list() behavior consistent with the other backends, handle the empty-dir case by using an empty prefix and returning top-level entries.
| private loadBlob(): Record<string, string> { | ||
| const raw = gitExec(`notes --ref=${this.ref} show HEAD`, this.cwd); | ||
| if (!raw) return {}; | ||
| try { | ||
| const parsed: unknown = JSON.parse(raw); | ||
| if (parsed !== null && typeof parsed === 'object' && !Array.isArray(parsed)) { | ||
| return parsed as Record<string, string>; | ||
| } | ||
| return {}; | ||
| } catch { return {}; } | ||
| } | ||
|
|
||
| private saveBlob(blob: Record<string, string>): void { | ||
| const json = JSON.stringify(blob, null, 2); | ||
| try { | ||
| execSync(`git notes --ref=${this.ref} add -f --file - HEAD`, { | ||
| cwd: this.cwd, input: json, encoding: 'utf-8', stdio: ['pipe', 'pipe', 'pipe'], | ||
| }); | ||
| } catch { throw new Error('git-notes backend: failed to write note on HEAD'); } | ||
| } | ||
|
|
||
| read(relativePath: string): string | undefined { | ||
| const blob = this.loadBlob(); | ||
| return blob[normalizeKey(relativePath)]; | ||
| } | ||
| write(relativePath: string, content: string): void { | ||
| const blob = this.loadBlob(); | ||
| blob[normalizeKey(relativePath)] = content; | ||
| this.saveBlob(blob); | ||
| } | ||
| exists(relativePath: string): boolean { | ||
| return normalizeKey(relativePath) in this.loadBlob(); | ||
| } |
There was a problem hiding this comment.
exists() uses the in operator on the parsed JSON blob. A note containing __proto__/prototype keys can make in behave unexpectedly and opens the door to prototype-pollution style issues. Prefer creating a null-prototype map when loading, validate that all values are strings, and use Object.prototype.hasOwnProperty.call(blob, key) for existence checks.
| consult?: boolean; | ||
| /** True when extraction is disabled for consult sessions (read-only consultation) */ | ||
| extractionDisabled?: boolean; | ||
| /** State storage backend: worktree | external | git-notes | orphan */ | ||
| stateBackend?: string; | ||
| } |
There was a problem hiding this comment.
SquadDirConfig.stateBackend is typed as string, which loses type-safety and allows invalid values to propagate. Consider importing StateBackendType and typing this as StateBackendType, or at least narrowing/validating to the supported set when loading config.json.
| export interface WatchConfig { | ||
| interval: number; | ||
| execute: boolean; | ||
| maxConcurrent: number; | ||
| timeout: number; | ||
| copilotFlags?: string; | ||
| /** Hidden — fully override the agent command. */ | ||
| agentCmd?: string; | ||
| /** State storage backend: worktree | external | git-notes | orphan */ | ||
| stateBackend?: string; | ||
| /** Per-capability config: `true` / `false` / object with sub-options. */ |
There was a problem hiding this comment.
WatchConfig.stateBackend is treated as a free-form string. Since downstream code will likely assume one of worktree | external | git-notes | orphan, validate/narrow the value at load time (or type it as StateBackendType) so an invalid config doesn't silently propagate to runtime behavior.
| const stateBackendIdx = args.indexOf('--state-backend'); | ||
| const stateBackendVal = (stateBackendIdx !== -1 && args[stateBackendIdx + 1]) | ||
| ? args[stateBackendIdx + 1] | ||
| : undefined; | ||
|
|
There was a problem hiding this comment.
--state-backend parsing accepts whatever token follows, including another flag when the value is omitted (e.g. --state-backend --sdk). Validate that the next arg exists, does not start with -, and is one of the supported backend values before using it.
| const stateBackendIdx = args.indexOf('--state-backend'); | |
| const stateBackendVal = (stateBackendIdx !== -1 && args[stateBackendIdx + 1]) | |
| ? args[stateBackendIdx + 1] | |
| : undefined; | |
| const supportedStateBackends = new Set(['fs', 'sqlite']); | |
| const stateBackendIdx = args.indexOf('--state-backend'); | |
| let stateBackendVal: string | undefined; | |
| if (stateBackendIdx !== -1) { | |
| const candidate = args[stateBackendIdx + 1]; | |
| if (!candidate || candidate.startsWith('-') || !supportedStateBackends.has(candidate)) { | |
| fatal(`Invalid value for --state-backend. Supported values: ${Array.from(supportedStateBackends).join(', ')}`); | |
| } | |
| stateBackendVal = candidate; | |
| } |
| runInit(dest, { includeWorkflows: !noWorkflows && !hasGlobal, sdk, roles, isGlobal: hasGlobal }).then(async () => { | ||
| if (stateBackendVal) { | ||
| const { join } = await import('node:path'); | ||
| const { existsSync, readFileSync, writeFileSync, mkdirSync } = await import('node:fs'); | ||
| const squadDir = join(dest, '.squad'); | ||
| if (!existsSync(squadDir)) mkdirSync(squadDir, { recursive: true }); | ||
| const configPath = join(squadDir, 'config.json'); | ||
| let config: Record<string, unknown> = {}; | ||
| try { | ||
| if (existsSync(configPath)) { | ||
| config = JSON.parse(readFileSync(configPath, 'utf-8')); | ||
| } | ||
| } catch { /* fresh config */ } | ||
| config['stateBackend'] = stateBackendVal; | ||
| writeFileSync(configPath, JSON.stringify(config, null, 2) + '\n'); | ||
| console.log(`✓ State backend set to '${stateBackendVal}' in .squad/config.json`); | ||
| } | ||
| }).catch(err => { | ||
| fatal(err.message); | ||
| }); |
There was a problem hiding this comment.
When .squad/config.json is missing or malformed, this code overwrites it with { stateBackend: ... }, which can discard existing settings and also creates a config file that fails loadDirConfig()'s schema check (missing version/teamRoot). Prefer: (1) refuse to overwrite malformed JSON, and (2) when creating a new config file, write a schema-valid object (or store stateBackend under an existing schema such as watch/another dedicated section).
| runInit(dest, { includeWorkflows: !noWorkflows && !hasGlobal, sdk, roles, isGlobal: hasGlobal }).then(async () => { | |
| if (stateBackendVal) { | |
| const { join } = await import('node:path'); | |
| const { existsSync, readFileSync, writeFileSync, mkdirSync } = await import('node:fs'); | |
| const squadDir = join(dest, '.squad'); | |
| if (!existsSync(squadDir)) mkdirSync(squadDir, { recursive: true }); | |
| const configPath = join(squadDir, 'config.json'); | |
| let config: Record<string, unknown> = {}; | |
| try { | |
| if (existsSync(configPath)) { | |
| config = JSON.parse(readFileSync(configPath, 'utf-8')); | |
| } | |
| } catch { /* fresh config */ } | |
| config['stateBackend'] = stateBackendVal; | |
| writeFileSync(configPath, JSON.stringify(config, null, 2) + '\n'); | |
| console.log(`✓ State backend set to '${stateBackendVal}' in .squad/config.json`); | |
| } | |
| }).catch(err => { | |
| fatal(err.message); | |
| }); | |
| try { | |
| await runInit(dest, { includeWorkflows: !noWorkflows && !hasGlobal, sdk, roles, isGlobal: hasGlobal }); | |
| if (stateBackendVal) { | |
| const { join } = await import('node:path'); | |
| const { existsSync, readFileSync, writeFileSync, mkdirSync } = await import('node:fs'); | |
| const squadDir = join(dest, '.squad'); | |
| if (!existsSync(squadDir)) mkdirSync(squadDir, { recursive: true }); | |
| const configPath = join(squadDir, 'config.json'); | |
| if (!existsSync(configPath)) { | |
| fatal('Failed to set state backend: .squad/config.json was not created by init.'); | |
| } | |
| let parsedConfig: unknown; | |
| try { | |
| parsedConfig = JSON.parse(readFileSync(configPath, 'utf-8')); | |
| } catch { | |
| fatal('Failed to set state backend: existing .squad/config.json contains malformed JSON. Please fix it manually and retry.'); | |
| } | |
| if (!parsedConfig || typeof parsedConfig !== 'object' || Array.isArray(parsedConfig)) { | |
| fatal('Failed to set state backend: .squad/config.json must contain a JSON object.'); | |
| } | |
| const config = parsedConfig as Record<string, unknown>; | |
| if (!('version' in config) || !('teamRoot' in config)) { | |
| fatal('Failed to set state backend: .squad/config.json is missing required fields (version, teamRoot).'); | |
| } | |
| config['stateBackend'] = stateBackendVal; | |
| writeFileSync(configPath, JSON.stringify(config, null, 2) + '\n'); | |
| console.log(`✓ State backend set to '${stateBackendVal}' in .squad/config.json`); | |
| } | |
| } catch (err) { | |
| fatal(err instanceof Error ? err.message : String(err)); | |
| } |
| const result = gitExec(`show ${this.branch}:${normalizeKey(relativePath)}`, this.cwd); | ||
| return result ?? undefined; |
There was a problem hiding this comment.
gitExec() always .trim()s stdout. OrphanBranchBackend.read() uses it to fetch file contents via git show, which will drop leading/trailing whitespace and trailing newlines, making reads lossy compared to WorktreeBackend. Avoid trimming for commands that return file content (or add a separate helper for raw output).
| const result = gitExec(`show ${this.branch}:${normalizeKey(relativePath)}`, this.cwd); | |
| return result ?? undefined; | |
| try { | |
| return execSync(`git show ${this.branch}:${normalizeKey(relativePath)}`, { | |
| cwd: this.cwd, | |
| encoding: 'utf-8', | |
| stdio: ['pipe', 'pipe', 'pipe'], | |
| }); | |
| } catch { | |
| return undefined; | |
| } |
🏗️ Architectural Review
Automated architectural review — informational only. |
- Add restart message after --self upgrade (stale code warning) - Fix permission handling: only suggest sudo for npm, not pnpm/yarn - Warn when --insider used without --self - Add test verifying selfUpgradeCli is called with correct args - Export selfUpgradeCli, call it from cli-entry before runUpgrade (exit early) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Fix shell injection in gitExec: use execFileSync with args array
- Add path traversal validation in WorktreeBackend (reject .. in paths)
- Fix GitNotes list('') to return root entries (empty prefix match)
- Fix prototype pollution: use Object.hasOwn() instead of in operator
- Validate --state-backend value and reject flag-as-value
- Add gitExecContent with trimEnd() for content reads
- Add comment documenting config merge-not-overwrite pattern
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Superseded by #830 — combined into a single watch next-gen PR for easier review. |
|
Closing — superseded by #830 (watch-next-gen combined PR) |
What
Implements Options A (git-notes) and B (orphan-branch) from issue #807, complementing PR #797 (external directory).
Why
.squad/state in the working tree gets destroyed on branch switch and pollutes diffs/PRs. Git-native backends solve this properly.How
StateBackendinterface in squad-sdkstate-backend.tsGitNotesBackend: usesrefs/notes/squadref — state as JSON blob on HEAD noteOrphanBranchBackend: uses dedicated orphan branch with git plumbing (no worktree checkout)resolveStateBackend()config resolution with priority: CLI > config.json > worktree default--state-backendCLI flag oninitandwatchcommandsstateBackendfield added toSquadDirConfigandWatchConfigTesting
^escape handling in git plumbingExports
StateBackendinterface +StateBackendType+StateBackendConfigtypesWorktreeBackend,GitNotesBackend,OrphanBranchBackendclassesresolveStateBackend()functionBreaking changes
None — new opt-in feature, worktree remains default