fix(cli): replace hardcoded state lists with dynamic directory scan#908
fix(cli): replace hardcoded state lists with dynamic directory scan#908tamirdresher merged 1 commit intodevfrom
Conversation
🟡 Impact Analysis — PR #908Risk tier: 🟡 MEDIUM 📊 Summary
🎯 Risk Factors
📦 Modules Affectedroot (1 file)
squad-cli (2 files)
squad-sdk (2 files)
tests (1 file)
|
🏗️ Architectural Review
Automated architectural review — informational only. |
There was a problem hiding this comment.
Pull request overview
This PR replaces hardcoded .squad/ state artifact lists with a centralized SDK “state manifest”, then updates the CLI externalize/internalize flows to discover artifacts from that manifest and perform a copy→verify→delete migration with rollback and orphan detection.
Changes:
- Added
state-manifest.tsinsquad-sdkto declare state artifacts and provide helpers (dirs/files/exclusions + orphan detection). - Rewrote
squad externalize/squad internalizeto be manifest-driven and to use a copy-verify-delete flow with rollback and audit logging. - Added tests covering manifest shape, legacy artifact coverage, exclusions, and orphan detection.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
test/external-state.test.ts |
Adds tests for manifest helpers and detectOrphanArtifacts() alongside existing external state tests. |
packages/squad-sdk/src/state-manifest.ts |
Introduces the canonical manifest list and helper APIs (dirs/files/exclusions + orphan detection). |
packages/squad-sdk/src/index.ts |
Re-exports the manifest types/functions from the SDK barrel. |
packages/squad-cli/src/cli/commands/externalize.ts |
Switches externalize/internalize to manifest discovery with 3-phase migration, rollback, and audit output. |
.changeset/discoverable-state-manifest.md |
Adds a changeset describing the manifest-driven migration work. |
cbac697 to
bc9d88e
Compare
🛫 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 | 5 unresolved Copilot thread(s) — fix and resolve before merging |
| ❌ | CI passing | 1 check(s) failing: test |
| ✅ | Issue linked | Issue reference found |
| ✅ | Protected files | No protected bootstrap files changed |
Files Changed (6 files, +636 −104)
| File | +/− |
|---|---|
.changeset/discoverable-state-manifest.md |
+9 −0 |
packages/squad-cli/src/cli-entry.ts |
+2 −1 |
packages/squad-cli/src/cli/commands/externalize.ts |
+398 −101 |
packages/squad-sdk/src/index.ts |
+4 −0 |
packages/squad-sdk/src/state-manifest.ts |
+113 −0 |
test/external-state.test.ts |
+110 −2 |
Total: +636 −104
This check runs automatically on every push. Fix any ❌ items and push again.
See CONTRIBUTING.md and PR Requirements for details.
f44d681 to
f2404cf
Compare
🔍 Squad Review — Kaylee (Engineering)
Verdict: ✅ Ready to merge Review by Squad AI team (Kaylee — Engineering) · requested by Dina Berry |
f2404cf to
b9a3bc2
Compare
b9a3bc2 to
93c6be3
Compare
diberry
left a comment
There was a problem hiding this comment.
(submitting pending review to unblock new review)
diberry
left a comment
There was a problem hiding this comment.
suggestion: runInternalize doesn't filter KEEP_LOCAL entries when copying from external to local. Under normal operation the external dir won't contain these files, but if it ever gets contaminated (manual copy, third-party tool), internalize would silently overwrite the local config.json, manifest.json, etc. A one-liner if (KEEP_LOCAL.has(entry)) continue; at the top of the loop would make this safe.
nit: PR description says "Only config.json is kept local" but KEEP_LOCAL now has 6 entries. Worth updating so reviewers aren't misled.
nit: isDirectorySync is marked @deprecated in the StorageProvider interface. This PR adds two new call-sites. Not a blocker since the whole externalize flow is sync, but worth a comment acknowledging the tech debt.
- Dynamic listSync() scan replaces static STATE_DIRS/STATE_FILES arrays - Eliminates silent orphaning when new state artifacts are added - KEEP_LOCAL protects 5 files read from the working tree by runtime code that does not go through external-state resolution: config.json, manifest.json, workstreams.json, upstream.json, squad-registry.json - Add 6 tests for runExternalize/runInternalize covering basic ops, dynamic scan of unknown entries, config preservation, round-trip, config cleanup, and empty .squad/ edge case Closes #863 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
93c6be3 to
a01fd71
Compare
|
✅ CI validation passed on fork: diberry#141 — 7/7 checks green |
…908) - Dynamic listSync() scan replaces static STATE_DIRS/STATE_FILES arrays - Eliminates silent orphaning when new state artifacts are added - KEEP_LOCAL protects 5 files read from the working tree by runtime code that does not go through external-state resolution: config.json, manifest.json, workstreams.json, upstream.json, squad-registry.json - Add 6 tests for runExternalize/runInternalize covering basic ops, dynamic scan of unknown entries, config preservation, round-trip, config cleanup, and empty .squad/ edge case Closes #863 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Summary
Replaces the hardcoded
STATE_DIRS/STATE_FILESarrays in externalize/internalize with a dynamiclistSync()scan of.squad/. AKEEP_LOCALset of 6 entries stays in the working tree — everything else is discovered at runtime.KEEP_LOCAL entries (6):
config.json,manifest.json,workstreams.json,upstream.json,squad-registry.json,_upstream_reposThese files are read from the working tree by runtime code that does not go through external-state resolution, so they must remain local.
This prevents silent orphaning when new state artifacts are added to
.squad/(e.g. new agent directories, plugin files) without updating the hardcoded lists.Changes
externalize.ts): ~15 lines of net changeforloops with oneKEEP_LOCALset + one dynamic scan loop per functionKEEP_LOCALguard inrunInternalizeto prevent contaminated external dirs from overwriting local-only filesTesting
All 12 existing
external-state.test.tstests pass unchanged.Closes #863