fix(sdk): wire FSStorageProvider rootDir in resolveSquadState, fix teamRoot='.' sentinel - #1666
Conversation
…amRoot='.' sentinel Fixes bradygaster#1555 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
🟡 Impact Analysis — PR #1666Risk tier: 🟡 MEDIUM 📊 Summary
🎯 Risk Factors
📦 Modules Affectedroot (1 file)
squad-sdk (1 file)
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 | 8 check(s) still running |
Files Changed (3 files, +61 −3)
| File | +/− |
|---|---|
.changeset/1555-fs-storage-rootdir.md |
+5 −0 |
packages/squad-sdk/src/resolution.ts |
+10 −2 |
test/state-backend.test.ts |
+46 −1 |
Total: +61 −3
This check runs automatically on every push. Fix any ❌ items and push again.
See CONTRIBUTING.md and PR Requirements for details.
|
same CI failure as #1665 — pre-existing, unrelated: |
|
👋 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. |
|
Closing as a duplicate of #1695 — the diffs are byte-identical (same 3 files, same +61/-3 change to |
What
resolveSquadState()now constructs the local-backendFSStorageProviderwithrootDirset, andresolveSquadPaths()treatsconfig.teamRoot === '.'as local mode instead of remote mode.Why
Fixes #1555
Two independent defects in the same path, both live on
dev, both hit bysquad_decide/squad_state_*(the MCP state tools wired up instate-mcp.ts):resolveSquadState()built the local backend's storage asnew FSStorageProvider()— norootDir.FSStorageProvider's traversal guard isif (!this.rootDir) return filePath;— an unsetrootDirmakes it a no-op, so it never validated anything. Everysquad_state_*/squad_decidewrite just trusted whatever path got built upstream instead of confining it to.squad/.resolveSquadPaths()'s remote-mode check isif (config && config.teamRoot).squad externalizewritesteamRoot: '.'intoconfig.jsonas its "no separate team root" marker (externalize.ts:99) — but'.'is truthy, so this took the remote branch and computedteamDir = path.resolve(projectRoot, '.'), which is justprojectRoot— one directory above.squad/, not.squad/itself.Once (1) is fixed on its own, (2) turns into a live path-traversal rejection instead of a silent misplacement:
state-mcp.tsbuildsToolRegistry(context.paths.teamDir, ...), every tool doespath.join(this.squadRoot, key), and withteamDirone level too high, those paths land outside the now-enforcedrootDir—squad_decidestarts throwingPath traversal blocked: [team-root]\decisions\inbox\<file>.md. Fixing both together is what actually gets state writes back to.squad/decisions/inbox/on an externalized project instead of either silently misplacing them or hard-failing.How
resolution.tsresolveSquadState():new FSStorageProvider(paths.teamDir)— matches what every local-backend caller already builds its paths against (e.g.state-mcp.tspassescontext.paths.teamDirasToolRegistry'ssquadRoot).resolution.tsresolveSquadPaths():if (config && config.teamRoot && config.teamRoot !== '.')—'.'now falls through to the existing local-mode branch (teamDir: projectDir), which is what it was always meant to mean.Testing
Added a
#1555 regression: rootDir wiring + teamRoot=. sentinelblock totest/state-backend.test.ts:teamRoot: '.'(the externalize.ts marker) resolvesmode: 'local'andteamDir === .squad/, not the parent directoryteamDir(Path traversal blocked) and still round-trips a path inside it — provesrootDiris actually wired, not just presentToolRegistrybuilt the same waystate-mcp.tsbuilds it (new ToolRegistry(ctx.paths.teamDir, undefined, ctx.storage)), calling the realsquad_decidetool handler, asserting the decision file lands in.squad/decisions/inbox/and not one level up in the repo rootAlso ran the broader set of suites that touch
resolveSquadPaths/resolveSquadState(dual-root resolver, effective-squad-dir, external-state, state-mcp, ensure-squad-path-dual, rc, copilot, export-import, extract, notes-promote, cast, consult, upgrade-state-backend, watch-notes-promote, mcp-spec-init, npm-registry-fallback):All 3 failures (
copilot.test.ts,extract.test.ts,rc.test.ts, eachmodule exports run* function) are the same pre-existing cold dynamic-import 5s timeout aswatch.test.tshits on this box — confirmed by rerunning just those 3 files with--testTimeout=20000: all 58 tests pass. Not caused by this change.cd packages/squad-sdk && npm run buildthencd packages/squad-cli && npm run build(package-local, no root prebuild churn) — clean.tsc --noEmitclean on both packages.eslinton both changed files: 0 errors, resolution.ts's 43 warnings are the same pre-existingn/no-synccategories the file already had (0 new), state-backend.test.ts has 0.Diff hygiene:
git diffvsgit diff -widentical,git diff --checkclean —resolution.tshad drifted to CRLF on disk despite an LF-committed blob (same box quirk as usual), normalized back to LF and reverified againstgit show HEAD:<path>before staging..changeset/1555-fs-storage-rootdir.md(patch@bradygaster/squad-sdk)PR Readiness Checklist
Branch & Commit
dev(upstream, fresh fetch)devBuild & Test
npm run build(per-package) passesnpm test— targeted suite 147/147; broader 16-suite regression sweep 343/346 (3 pre-existing unrelated failures, proven)tsc --noEmitcleaneslint— 0 errors, pre-existing warning categories onlyChangeset
Docs
resolveSquadState/resolveSquadPathssignatures unchanged)Exports
Breaking Changes
None. Both changes make existing, documented semantics (rootDir confinement;
teamRoot: '.'meaning "no separate team root") actually hold — no signature or behavior change for any caller that wasn't already relying on the broken behavior.Waivers
None.