fix(sdk): recursive deleteDir + rename 'external' stub to 'external-stub'#1487
Conversation
🟠 Impact Analysis — PR #1487Risk tier: 🟠 HIGH 📊 Summary
🎯 Risk Factors
📦 Modules Affecteddocs (1 file)
root (1 file)
squad-cli (6 files)
squad-sdk (2 files)
tests (1 file)
This report is generated automatically for every PR. See #733 for details. |
🛫 PR Readiness Check
PR Scope: 📦🔧 Mixed (product + infrastructure)
|
| Status | Check | Details |
|---|---|---|
| ✅ | Single commit | 1 commit — clean history |
| ✅ | 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 | No Copilot review threads |
| ❌ | CI passing | 7 check(s) still running |
Files Changed (11 files, +143 −29)
| File | +/− |
|---|---|
.changeset/fix-1211-deletedir-external-stub.md |
+6 −0 |
docs/src/content/docs/features/state-backends.md |
+1 −1 |
packages/squad-cli/src/cli-entry.ts |
+2 −1 |
packages/squad-cli/src/cli/commands/doctor.ts |
+2 −1 |
packages/squad-cli/src/cli/commands/install-hooks.ts |
+1 −1 |
packages/squad-cli/src/cli/commands/sync.ts |
+1 −1 |
packages/squad-cli/src/cli/commands/watch/config.ts |
+2 −2 |
packages/squad-cli/src/cli/core/init.ts |
+6 −3 |
packages/squad-sdk/src/config/init.ts |
+1 −1 |
packages/squad-sdk/src/state-backend.ts |
+46 −13 |
test/state-backend.test.ts |
+75 −5 |
Total: +143 −29
This check runs automatically on every push. Fix any ❌ items and push again.
See CONTRIBUTING.md and PR Requirements for details.
🏗️ Architectural Review
Automated architectural review — informational only. |
…tub'
StateBackendStorageAdapter.deleteDir did a single-level list+delete
pass, so nested keys survived: on git-notes the flat blob has no
deletable key for a directory segment, meaning deleteDir('agents/x')
silently left agents/x/history/2026/log.md behind. deleteDir now
deletes each child, then recurses into whatever the backend still
lists beneath it — one-commit subtree deletes on tree-based backends,
full flat-key cleanup on git-notes. WorktreeBackend.delete learned to
route directory keys to a recursive remove so the same walk is safe
when the adapter wraps the local backend.
The stateBackend: 'external' stub is renamed to 'external-stub'. The
collision bradygaster#1211 predicted can't happen as written (the real feature
shipped as stateLocation: 'external', a different key), but the stub
name now reads like the externalize feature while actually falling
back to 'local'. Legacy 'external' is still accepted everywhere and
normalized with a one-shot deprecation warning, following the
git-notes -> two-layer migration pattern; squad init and watch config
write/normalize the canonical name.
Part of bradygaster#1211
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
ebcfaf1 to
e185ae4
Compare
|
CI caught three call sites my local typecheck missed (stale sdk dist locally — CI builds fresh): doctor's configuredStateBackend now maps legacy 'external' to 'external-stub' like its other aliases, and install-hooks/sync treat 'external-stub' as a no-hooks backend alongside 'external'. rebuilt both packages locally and re-ran: doctor 36/36, install-hooks + sync green, cli tsc clean against the fresh sdk build. |
tamirdresher
left a comment
tamirdresher
left a comment
There was a problem hiding this comment.
Approving. Head verified at e185ae4c25b91d92f0ec7477e839ed936102df62; CI is green; the 11-file diff cleanly addresses concerns E and F only (not the whole of #1211). Not merging — keeping #1211 open because A/I already landed on dev (tryUpdateRef CAS + StateBackendConcurrencyError + the 5-attempt regression test are present in packages/squad-sdk/src/state-backend.ts and test/state-backend.test.ts) but the C-residual and D-residual items still need follow-up work.
2 participants
Part of #1211 — the deleteDir + stub-rename slice (concerns E and F). Concerns A and I already landed with the CAS work (tryUpdateRef, StateBackendConcurrencyError, retry/injector tests), so this is the remainder.
E — deleteDir left nested keys behind
StateBackendStorageAdapter.deleteDir was a single-level list+delete pass. On git-notes the flat blob has no deletable key for a directory segment, so
deleteDir('agents/x')deleted nothing underagents/x/history/2026/— the nested keys silently survived. deleteDir now deletes each child, then recurses into whatever the backend still lists beneath it: tree-based backends (orphan, two-layer) drop whole subtrees in one commit, git-notes gets the full flat-key walk, and the dual case where a git-notes key is both a leaf and a directory prefix is covered.WorktreeBackend.delete routes directory keys to a recursive remove so the walk is also safe when the adapter wraps the local backend — this matches the orphan backend, where deleting a tree entry already dropped everything beneath it.
F — stub rename, adjusted to reality
The issue predicted a config collision with a planned
stateLocation: 'external'. That feature has since shipped under a different key, so the parse collision can't happen as written — but the stub name got worse, not better:stateBackend: 'external'now reads like the externalize feature while actually warning and falling back to'local'. Renamed the stub to'external-stub', following the existing git-notes → two-layer migration pattern: legacy'external'is still accepted everywhere (config.json,--state-backend, watch config,squad init) and normalized with a one-shot deprecation warning that points atsquad externalize.squad initand watch config write/normalize the canonical name so configs don't trip the warning forever. One stale line in docs/state-backends.md updated to match.The issue also asked for an upgrade migration that rewrites user configs on disk — the shipped git-notes precedent does read-time normalization instead of disk rewrites, so I followed that. If a disk rewrite is wanted it can ride on migrate-backend later.
Tests
a/b/c,a/b/d,a/b/e/f,a/b/e/g/h,a/other→deleteDir('a/b')→ onlya/othersurvives), a deleteDirSync variant, the leaf+prefix dual-key case, WorktreeBackend directory delete, external-stub resolution, legacy-alias migration, and one-shot deprecation warningNote on line endings: the touched region of cli-entry.ts is CRLF-committed (the file is mixed-EOL at HEAD), so the two added lines keep CRLF to match their neighbors — same situation as #1464.
git diff -wmatchesgit diffapart from those endings and one real indentation change inside WorktreeBackend.delete.🤖 Generated with Claude Code