Repository navigation
feat: add atomic repository-scoped state creation (#1947) - #1948
bradygaster merged 11 commits into
Conversation
Add true atomic create-if-absent semantics across all state backends (local/WorktreeBackend, git-notes/GitNotesBackend, orphan-branch/ OrphanBranchBackend, two-layer/TwoLayerBackend), all StorageProvider implementations (FSStorageProvider, InMemoryStorageProvider, SQLiteStorageProvider), StateBackendStorageAdapter, ToolRegistry (squad_state_create_if_absent), state-mcp MCP bridge, and Azure Blob sample provider. Key guarantees: - Exactly one concurrent creator succeeds; all others receive StateKeyConflictError (typed, exported from squad-sdk) - Existing content is never overwritten - Repository scope is verified at operation time; invalid/ambiguous identity fails with StateBackendUncertaintyError (never silently widens) - Two-layer disagreement/failure is fail-closed: orphan succeeds but notes disagrees => StateBackendUncertaintyError, never success-shaped conflict - Local backend uses O_CREAT|O_EXCL (no process-local locking needed) - Git-native backends use compare-and-swap loops with re-check after CAS loss; retry exhaustion is uncertainty, not conflict - Repeated legitimate conflicts never trip the circuit breaker Registered in: StorageProvider interface, every implementation/adapter, ToolRegistry, state-mcp MCP_TOOL_ALIASES + tools/list, public docs, changeset. Tests: 39 createIfAbsent tests (WorktreeBackend, GitNotesBackend, OrphanBranchBackend, TwoLayerBackend, FSStorageProvider, InMemoryStorageProvider, SQLiteStorageProvider, ToolRegistry) + 6 state-mcp bridge tests including squad_state_create_if_absent MCP surface. Closes github#1947 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
🛫 PR Readiness Check
PR Scope: 📦🔧 Mixed (product + infrastructure)
|
| Status | Check | Details |
|---|---|---|
| ❌ | Single commit | 11 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 | 1 active Copilot thread(s) resolved (3 outdated skipped) |
| ❌ | CI passing | 6 check(s) still running |
Files Changed (32 files, +1480 −43)
| File | +/− |
|---|---|
.changeset/1947-create-if-absent.md |
+13 −0 |
.squad-templates/scribe-charter.md |
+8 −3 |
.squad-templates/skills/coordinator-source-of-truth/SKILL.md |
+1 −1 |
.squad-templates/spawn-reference.md |
+5 −0 |
docs/src/content/docs/features/state-backends.md |
+124 −1 |
docs/src/content/docs/features/storage-provider.md |
+16 −1 |
packages/squad-cli/src/cli/commands/state-mcp.ts |
+3 −1 |
packages/squad-cli/templates/scribe-charter.md |
+8 −3 |
packages/squad-cli/templates/skills/coordinator-source-of-truth/SKILL.md |
+1 −1 |
packages/squad-cli/templates/spawn-reference.md |
+5 −0 |
packages/squad-sdk/src/state-backend.ts |
+282 −1 |
packages/squad-sdk/src/storage/fs-storage-provider.ts |
+45 −2 |
packages/squad-sdk/src/storage/in-memory-storage-provider.ts |
+12 −0 |
packages/squad-sdk/src/storage/index.ts |
+1 −1 |
packages/squad-sdk/src/storage/sqlite-storage-provider.ts |
+23 −0 |
packages/squad-sdk/src/storage/storage-error.ts |
+43 −0 |
packages/squad-sdk/src/storage/storage-provider.ts |
+22 −0 |
packages/squad-sdk/src/tools/index.ts |
+69 −1 |
packages/squad-sdk/templates/scribe-charter.md |
+8 −3 |
packages/squad-sdk/templates/skills/coordinator-source-of-truth/SKILL.md |
+1 −1 |
packages/squad-sdk/templates/spawn-reference.md |
+5 −0 |
samples/storage-provider-azure/azure-blob-storage-provider.ts |
+32 −1 |
templates/scribe-charter.md |
+8 −3 |
templates/skills/coordinator-source-of-truth/SKILL.md |
+1 −1 |
templates/spawn-reference.md |
+5 −0 |
test/cli/state-mcp.test.ts |
+75 −9 |
test/compat-v041.test.ts |
+3 −2 |
test/helpers/typescript-source-loader.mjs |
+33 −0 |
test/state-backend-create-if-absent.test.ts |
+614 −0 |
test/tools.test.ts |
+5 −3 |
workflows/shared/builtins/scribe-charter.md |
+8 −3 |
workflows/squad-workflows.manifest.json |
+1 −1 |
Total: +1480 −43
This check runs automatically on every push. Fix any ❌ items and push again.
See CONTRIBUTING.md and PR Requirements for details.
🟠 Impact Analysis — PR #1948Risk tier: 🟠 HIGH 📊 Summary
🎯 Risk Factors
📦 Modules Affecteddocs (2 files)
root (7 files)
squad-cli (4 files)
squad-sdk (11 files)
templates (3 files)
tests (5 files)
|
🏗️ Architectural Review
Automated architectural review — informational only. |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
There are correctness/contract gaps to address (notably around atomic local writes and typed uncertainty behavior), plus at least one concurrency test that doesn’t actually exercise concurrent contention.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review tier: Lite
Findings: 1
New issues introduced by this change (4)
| Severity | Finding |
|---|---|
 |
packages/squad-sdk/src/state-backend.ts — WorktreeBackend.createIfAbsent uses fsWriteSync(fd, buf) once; writeSync can legally perform a… |
 |
test/state-backend-create-if-absent.test.ts — These "concurrent create" tests for git-backed/two-layer backends are sequential (a simple… |
 |
docs/src/content/docs/features/state-backends.md — This docs section hardcodes the first shipping version as 0.14.0, but the PR description states the… |
|
packages/squad-sdk/src/storage/sqlite-storage-provider.ts — SQLiteStorageProvider.createIfAbsent calls this.persist() without translating persistence failures… |
What changed in this PR
Adds an atomic, repository-scoped “create-if-absent” state primitive across Squad’s storage/state stack, exposing it via MCP as squad_state_create_if_absent with typed conflict vs uncertainty outcomes to prevent concurrent creators from overwriting canonical artifacts.
Changes:
- Introduces
createIfAbsentonStorageProviderand implements it for FS, in-memory, and SQLite providers; wires it throughStateBackendStorageAdapter. - Implements backend-specific atomic creation semantics for local, git-notes, orphan-branch, and two-layer state backends (including fail-closed uncertainty on two-layer disagreement).
- Registers the new MCP tool, adds tests, and updates docs/templates/samples to describe usage and failure shapes.
| File | Description |
|---|---|
| test/state-backend-create-if-absent.test.ts | Adds cross-backend tests for create-if-absent semantics, conflicts, isolation, and uncertainty. |
| test/cli/state-mcp.test.ts | Verifies state-mcp exposes squad_state_create_if_absent and exercises canonical-key creation behavior. |
| templates/spawn-reference.md | Documents when to use squad_state_create_if_absent and how to handle conflict/uncertainty. |
| templates/scribe-charter.md | Updates Scribe guidance to prefer create-if-absent for exclusive canonical artifacts. |
| samples/storage-provider-azure/azure-blob-storage-provider.ts | Adds an Azure Blob sample implementation using conditional upload for atomic create. |
| packages/squad-sdk/templates/spawn-reference.md | Syncs spawn-reference template with create-if-absent guidance. |
| packages/squad-sdk/templates/skills/coordinator-source-of-truth/SKILL.md | Updates state-tool list to include create-if-absent for mutable state. |
| packages/squad-sdk/templates/scribe-charter.md | Syncs Scribe charter template with exclusive-canonical-artifact guidance. |
| packages/squad-sdk/src/tools/index.ts | Registers squad_state_create_if_absent tool with typed failure shapes. |
| packages/squad-sdk/src/storage/storage-provider.ts | Extends StorageProvider interface with createIfAbsent and its error contract. |
| packages/squad-sdk/src/storage/storage-error.ts | Adds exported typed errors: StateKeyConflictError and StateBackendUncertaintyError. |
| packages/squad-sdk/src/storage/sqlite-storage-provider.ts | Implements createIfAbsent via INSERT OR IGNORE and conflict detection. |
| packages/squad-sdk/src/storage/index.ts | Re-exports new typed storage errors from the SDK storage entrypoint. |
| packages/squad-sdk/src/storage/in-memory-storage-provider.ts | Implements createIfAbsent for test-friendly in-memory provider. |
| packages/squad-sdk/src/storage/fs-storage-provider.ts | Implements createIfAbsent using exclusive open (wx) with typed conflict/uncertainty. |
| packages/squad-sdk/src/state-backend.ts | Adds createIfAbsent to backends + repo-identity fail-closed checks for git-native backends. |
| packages/squad-cli/templates/spawn-reference.md | Syncs CLI template spawn-reference with create-if-absent guidance. |
| packages/squad-cli/templates/skills/coordinator-source-of-truth/SKILL.md | Syncs CLI skill template to include create-if-absent in state tools. |
| packages/squad-cli/templates/scribe-charter.md | Syncs CLI template Scribe charter with exclusive-canonical-artifact guidance. |
| packages/squad-cli/src/cli/commands/state-mcp.ts | Adds alias mapping so state-mcp bridge exposes the new tool name. |
| docs/src/content/docs/features/storage-provider.md | Documents createIfAbsent as a core StorageProvider method and its semantics. |
| docs/src/content/docs/features/state-backends.md | Documents atomic create-if-absent semantics, typed errors, and MCP tool surface. |
| .squad/skills/coordinator-source-of-truth/SKILL.md | Updates canonical skill content to include create-if-absent among state tools. |
| .squad-templates/spawn-reference.md | Syncs canonical spawn template with create-if-absent guidance. |
| .squad-templates/scribe-charter.md | Syncs canonical Scribe template with exclusive-canonical-artifact guidance. |
| .changeset/1947-create-if-absent.md | Declares a minor bump for sdk/cli and summarizes the new capability. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
bradygaster
left a comment
bradygaster
left a comment
There was a problem hiding this comment.
Security & State-Safety Review — PR #1948
Verdict: REQUEST CHANGES (not approved)
I independently verified the diff, built both packages from the PR's fork commit (bdad2c5), and ran the test suites myself rather than trusting the PR body's claims. npm run build passed; vitest run test/state-backend-create-if-absent.test.ts (39/39) and test/cli/state-mcp.test.ts (6/6) passed as claimed. A full-repo vitest run shows 13 pre-existing failures, all in unrelated "externalized state" tests (#1396–#1399) that this PR's diff cannot touch (confirmed: no changes to resolution/export/rc/copilot/cast code) — not a regression from this PR.
The overall design is strong: real OS-level exclusivity (O_EXCL/wx) for FS/local, INSERT OR IGNORE + rows-modified check for SQLite, git-native CAS loops that re-read at the exact pinned ref/tree before retrying (correctly distinguishing "someone else created this key" from "someone else changed something unrelated"), a assertRepositoryIdentity check that fails closed against the git rev-parse --show-toplevel-walks-up-to-enclosing-repo confusion, and an Azure sample using If-None-Match: *. Two-layer disagreement is fail-closed (StateBackendUncertaintyError), not silently reported as success. Template sync across the 3 physical copies of coordinator-source-of-truth/SKILL.md is complete.
However, 4 Copilot review threads on this PR are still unresolved, and I confirmed by reading the source that at least one is a real, unaddressed correctness bug:
- [high] Silent truncation / false-success in
WorktreeBackend.createIfAbsent—packages/squad-sdk/src/state-backend.ts(~L422-424): after the exclusiveopenSync(..., 'wx'), the write isfsWriteSync(fd, buf)called once, with no check of the returned byte count and no retry loop.fs.writeSynccan return fewer bytes than the buffer length; if that happens here, the call does not throw,createIfAbsentreturns success, and the "canonical" artifact is silently truncated/corrupted on disk with no error surfaced to the caller. This directly contradicts the PR's own stated invariant ("never silently succeeds"). Note the asyncFSStorageProvider.createIfAbsentsibling implementation avoids this by usingfh.writeFile(...), which loops to completion — the sync path needs the same treatment (loop until all bytes are written, or usewriteFileSync(fd, buf)which handles this internally). - [medium]
SQLiteStorageProvider.createIfAbsentdoesn't map persistence failures to the typed uncertainty error —packages/squad-sdk/src/storage/sqlite-storage-provider.ts(~L153):this.persist()runs un-guarded after the in-memoryINSERT OR IGNOREsucceeds. If the on-disk write/rename fails (ENOSPC/EACCES/etc.), the row exists in memory but not on disk, and the raw error propagates as a generic failure instead ofStateBackendUncertaintyError— violating the documented typed-error contract callers are told to rely on. - [medium] "Concurrent create" tests for git-notes/orphan/two-layer backends are sequential, not concurrent —
test/state-backend-create-if-absent.test.ts(~L144 and the equivalent blocks for GitNotes/Orphan/TwoLayer): these run a plainforloop callingcreateIfAbsentone at a time on two instances, which passes even for a non-atomic implementation. The CAS-loop logic looks correct on inspection, but the "exactly one of two concurrent creators wins" claim is unverified for the git-native backends — only the FS/InMemory suites use realPromise.all/parallel contention. Needs true parallel execution (worker threads / child processes) or the "concurrent" framing should be removed from the test names. - [low] Docs hardcode a shipping version that contradicts the PR description —
docs/src/content/docs/features/state-backends.md(~L297-299) states the feature ships in0.14.0, while the PR body says the version is intentionally left to the release process. Pick one.
Per review protocol, a verified high-severity correctness gap (item 1) that undermines the PR's core "never silently succeeds" guarantee is blocking. Please fix items 1–2 (and ideally 3), then request re-review.
|
👋 Friendly nudge — this PR has had no activity for 7 days. What needs attention:
If this PR is abandoned, please close it. If it's blocked on something external, leave a comment so the team knows. |
|
👋 Friendly nudge — this PR has had no activity for 8 days. What needs attention:
If this PR is abandoned, please close it. If it's blocked on something external, leave a comment so the team knows. |
|
👋 Friendly nudge — this PR has had no activity for 7 days. What needs attention:
If this PR is abandoned, please close it. If it's blocked on something external, leave a comment so the team knows. |
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5d70ed23-b351-4d22-929e-3ff74135ba83
Closes github#1947 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5d70ed23-b351-4d22-929e-3ff74135ba83
bradygaster
left a comment
bradygaster
left a comment
There was a problem hiding this comment.
Verified remediation in 8f29358: complete local writes, typed SQLite persistence uncertainty, real child-process CAS coverage, release-derived versioning, and policy-safe Git fixtures. Focused tests, type-checks, package builds, root build, lint, and docs checks pass.
Closes github#1947 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5d70ed23-b351-4d22-929e-3ff74135ba83
Closes github#1947 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5d70ed23-b351-4d22-929e-3ff74135ba83
Closes github#1947 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5d70ed23-b351-4d22-929e-3ff74135ba83
…47-atomic-state-create
…47-atomic-state-create
Closes github#1947 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5d70ed23-b351-4d22-929e-3ff74135ba83
…47-atomic-state-create
Closes #1947
Why
State MCP currently exposes unconditional writes, so concurrent workflows cannot safely claim a canonical repository-scoped artifact without risking overwrite. This adds a true create-if-absent primitive that fails closed when identity or outcome cannot be proven.
Approach
createIfAbsenttoStorageProvider, its implementations, state backends, and the storage adapter.squad_state_create_if_absentin ToolRegistry and state MCP so it appears intools/list.writebehavior.Validation
vitest run test/state-backend-create-if-absent.test.ts- 39/39 passedvitest run test/cli/state-mcp.test.ts- 6/6 passednpm run build- passedIntegration
After the release containing this changeset, consumers should update their exact
@bradygaster/squad-clipin, addsquad_state_create_if_absentto the MCP allowlist, restart the MCP server, verify the tool appears intools/list, and use it instead ofsquad_state_writefor exclusive canonical artifact creation. The released version is intentionally determined by the release process rather than hardcoded in this PR.