feat(sdk): StorageProvider abstraction — complete migration + example providers#640
Conversation
cfc907b to
ce8876f
Compare
…t command (bradygaster#640) P0: Race condition eliminated (auto-cast in onReady), Ctrl+C aborts init session, orphan .init-prompt cleanup. P1: /init triggers casting, clear empty-roster banner. 3243 tests pass. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* test(shell): add auto-cast trigger and /init command coverage 35 new tests in test/init-autocast.test.ts covering the P0/P1 init reliability fixes from PR bradygaster#640: - hasRosterEntries predicate: 7 tests (populated/empty/missing Members) - Auto-cast trigger conditions: 6 tests (fire/no-fire matrix) - Orphan .init-prompt cleanup: 3 tests (delete/preserve logic) - /init command triggerInitCast signal: 7 tests (with args/no args/edge) - triggerInitCast App.tsx dispatch contract: 3 tests (shape/exclusion) - activeInitSession Ctrl+C abort lifecycle: 5 tests (abort/clear/error) - handleInitCast .init-prompt consumption: 4 tests (override/cleanup) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * docs(hockney): log auto-cast test coverage learnings Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * feat(shell): P2 cast confirmation before team creation After the coordinator proposes a team via handleInitCast, the REPL now shows the formatted proposal and waits for the user to confirm (y/yes) or reject (n/no) before calling createTeam. Confirmation is skipped when: - Auto-cast fires from .init-prompt (user ran squad init "prompt") - /init "prompt" command (explicit user action) Freeform REPL messages hitting an empty roster now pause for approval, preventing garbage casts from vague or accidental prompts. Implementation: - pendingCastConfirmation state holds proposal + original parsed input - handleDispatch intercepts y/n before normal routing - finalizeCast() extracted as shared helper for both paths - handleCancel clears pending state on Ctrl+C - skipCastConfirmation flag on ParsedInput for /init trigger Closes bradygaster#641 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * docs: update Fenster history and decision for cast confirmation Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
0827911 to
f0f8011
Compare
f0f8011 to
a41a03f
Compare
…radygaster#640) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…r#640) ## Fix 1: SDK README section Added 'Storage abstraction' section to packages/squad-sdk/README.md covering: - What it is (pluggable persistence layer) - Built-in providers table (FSStorageProvider, InMemoryStorageProvider, SQLiteStorageProvider) - BYO provider example code - Links to sample projects ## Fix 2: Docs feature page Created docs/src/content/docs/features/storage-provider.md with comprehensive coverage: - Overview paragraph explaining StorageProvider purpose - Built-in providers section with detailed comparison table - Create a custom provider section with implementation skeleton - Decision matrix for choosing the right provider - Links to sample projects (storage-provider-azure, storage-provider-sqlite) ## Docs sync Updated test/docs-build.test.ts EXPECTED_FEATURES array to include 'storage-provider' in alphabetical order. Updated docs/src/navigation.ts to add Storage Provider feature link. All documentation follows Microsoft Style Guide conventions: - Sentence-case headings - Active voice, second person, present tense - Tables for comparisons, bullets for features - Concise, scannable format Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…radygaster#640) - StorageError: add class-level JSDoc explaining path sanitization, plus JSDoc for code and operation properties - SQLiteStorageProvider: add comprehensive class-level JSDoc covering initialization requirements, concurrency limitations, and DB location; enhance init() JSDoc with idempotency and error details - StorageProvider interface: add migration examples to all 12 deprecated sync method JSDoc tags showing the async equivalent call - Contract tests already exist in test/storage-contract.ts covering all 3 providers (FS, InMemory, SQLite) with 389 passing tests Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
4e1b9da to
58cdfcb
Compare
How PR #640 (StorageProvider) and #680 (StateBackend) Work TogetherThey operate at different abstraction layers — complementary, not competing. The Layering
What Each Does
How They Compose
That gives you the full matrix for free:
Tension: Duplicated Surface
Merge Order#640 first, then #680 adapts is cleaner — #680's SummaryThey solve different problems that stack cleanly. StorageProvider = the I/O engine. StateBackend = the location strategy. The filesystem backend is the bridge where they meet. |
🐕 FIDO — Devil's Advocate Repo Health ReviewVerdict: 🟡 CONDITIONAL — Implementation is solid (2,800+ lines of tests, clean DI patterns), but CI blind spots need attention. 🔴 BLOCKING — Fork Branch Missing 2 CI Gates
The new Action needed: Manually verify 🟡 CONCERNS1. New samples are CI-invisible — 2. 12 deprecated sync methods with no removal timeline — All marked 3. Test gaps:
4. Module-level singletons in CLI — 🟢 STRENGTHS
Recommended Actions
— FIDO, Quality Owner |
Adds CJK, emoji, and accented character path tests to the storage contract test suite. Tests write, read, exists, list, delete, append, rename, and copy with unicode paths across all 3 providers (FS, InMemory, SQLite). Rebased onto upstream/dev to pick up CI gates. Addresses FIDO review findings on PR bradygaster#640. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
3da6492 to
18b466a
Compare
✅ FIDO's Findings — Resolved
Test results: 461 tests pass across all 3 providers (FS, InMemory, SQLite). — EECOM, Core Dev (reviewed by FIDO, Quality Owner) |
|
Great analysis @diberry — agree completely. StateBackend (#680) sits above StorageProvider (#640) in the stack. Once #640 merges, we'll refactor Also — love that you ran FIDO against your own PR and caught the missing CI gates. The guardrails are already paying off! 🎉 Documented this relationship in the RFC issue: #678 |
…c lockfile The SDK and CLI package.json files had committed prerelease versions (0.9.1-build.1, 0.9.1-build.4) which broke npm workspace resolution. The CLI dep '>=0.9.0' doesn't match semver prereleases, causing npm to install a stale published SDK from the registry instead of the local workspace — that stale copy lacked the storage/state barrel exports, breaking the build. Fix: Reset both packages to 0.9.1 (non-prerelease) so workspace linking works correctly, and regenerate package-lock.json. Closes bradygaster#640 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
… gates Adds 3 new CI validation gates motivated by the PR bradygaster#640 prerelease version incident where npm silently resolved a stale registry SDK. - workspace-integrity: verifies lockfile has no stale registry entries for workspace packages (zero-install, reads lockfile only) - prerelease-version-guard: blocks prerelease version suffixes from merging to dev/main (zero-install, reads package.json only) - export-smoke-test: verifies all subpath exports resolve to built artifacts after SDK build (lightweight install+build) All gates follow existing patterns: feature flags (vars.SQUAD_*), skip labels, three-dot diff for change detection, ::error:: annotations. Closes #114 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Codifies semver versioning rules for SDK and CLI packages: - MAJOR.MINOR.PATCH only on dev/main (no prerelease suffixes) - bump-build.mjs -build.N versions are local-only, never committed - SDK + CLI versions must stay in sync (workspace resolution footgun) - Surgeon owns version bumps; other agents hands-off - prerelease-version-guard CI gate enforces the policy Motivated by PR bradygaster#640 (prerelease broke workspace resolution) and PR #116 (prerelease leak on release branch). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Adds CJK, emoji, and accented character path tests to the storage contract test suite. Tests write, read, exists, list, delete, append, rename, and copy with unicode paths across all 3 providers (FS, InMemory, SQLite). Rebased onto upstream/dev to pick up CI gates. Addresses FIDO review findings on PR bradygaster#640. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
bcb6c89 to
f36962b
Compare
PR #640 Status — Ready for Human ReviewSingle squashed commit on CI Results (9/9 complete)
Team Review
Review Findings — All Fixed
What Was Done
Known Issues (not caused by this PR)@github/copilot-sdk has an ESM import bug: vscode-jsonrpc/node missing .js extension. This affects the ./client export smoke test and streaming-chat sample on Linux CI. Same failure exists on upstream/dev. |
Pre-existing CI Failures — Need Owner Decision@bradygaster @tamirdresher — two CI jobs on this PR are failing, but they're not caused by this PR. The same failures exist on Root Cause
Affected CI Jobs
Options
What would you like to do? |
…ot-sdk ESM bug @github/copilot-sdk imports vscode-jsonrpc/node without .js extension, breaking ESM resolution on Linux CI. This is a pre-existing upstream bug (same failure on dev at 428a20d). Workaround: skip affected checks until SDK ships a fix. Refs: #640 (analysis) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ot-sdk ESM bug @github/copilot-sdk imports vscode-jsonrpc/node without .js extension, breaking ESM resolution on Linux CI. This is a pre-existing upstream bug (same failure on dev at 428a20d). Workaround: skip affected checks until SDK ships a fix. Refs: #640 (analysis) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Introduces a pluggable storage abstraction for the Squad SDK: - StorageProvider interface with 24 async methods + 12 deprecated sync methods - FSStorageProvider (default), InMemoryStorageProvider, SQLiteStorageProvider - StorageError with typed error codes and operation context - Dependency injection throughout all public API entry points - Contract test suite covering all 3 providers - CHANGELOG entry, README section, docs feature page - Sample projects: SQLite and Azure Blob Storage Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Rebased onto latest dev to pick up the ESM patch fix from #699. The 2 failing CI checks (export-smoke-test, samples-build) were caused by the vscode-jsonrpc/node ESM bug — now resolved in dev. CI should go green on this run. |
f36962b to
26047dc
Compare
What
StorageProvider abstraction layer for the Squad SDK -- pluggable persistence with 3 built-in providers.
Why
What's Included
Core Interface
StorageProviderinterface with 24 methods (async + sync, sync deprecated for Wave 2 removal)StorageErrorwith typedcodeandoperationfields, sanitized error messagesBuilt-in Providers
Dependency Injection
All public API entry points accept optional
StorageProviderparameter with FSStorageProvider as the sensible default. 10 public API files use Pattern A (DI with defaults), 25 internal files use Pattern B (module singletons behind public API). Zero hardcoded instances (Pattern C) found. Full analysis in comments below.Testing
test/storage-contract.ts) covering all 24 interface methodsDocumentation
docs/src/content/docs/features/storage-provider.mdwith decision matrixPackage Exports
./storagesubpath export topackages/squad-sdk/package.jsonimport { StorageProvider } from '@bradygaster/squad-sdk/storage'Samples
samples/storage-provider-sqlite/-- SQLite provider demosamples/storage-provider-azure/-- Azure Blob Storage provider demoHow to Review
packages/squad-sdk/src/storage/storage-provider.ts-- the interfacestorage-error.ts-- error handling contractfs-storage-provider.ts,in-memory-storage-provider.ts,sqlite-storage-provider.tstest/storage-contract.tsandtest/storage-provider.test.ts-- contract testsdocs/src/content/docs/features/storage-provider.md, README section, CHANGELOGBreaking Changes
None. FSStorageProvider is the default everywhere -- existing behavior is unchanged. New providers are opt-in via DI.
Related
StorageProvider Dependency Injection Analysis
We audited all FSStorageProvider references outside the storage module to verify the abstraction is properly swappable. Result: clean DI throughout.
Pattern A: Default Parameters (10 files) -- CORRECT DESIGN
Functions/constructors accept StorageProvider as an optional parameter with FSStorageProvider as the sensible default. Callers CAN pass any provider.
Files: config/init.ts, config/models.ts, config/legacy-fallback.ts, config/agent-source.ts, agents/personal.ts, agents/onboarding.ts, agents/lifecycle.ts, agents/history-shadow.ts, agents/index.ts, tools/index.ts
Example:
Pattern B: Module-Level Singletons (25 files) -- INTERNAL ONLY
Module-scope const storage = new FSStorageProvider() used by internal functions. These are implementation details behind Pattern A's public API -- not exposed to consumers.
Files: casting/index.ts, resolution.ts, runtime/config.ts, build/bundle.ts, build/release.ts, multi-squad.ts, ralph/index.ts, ralph/capabilities.ts, ralph/rate-limiting.ts, platform/comms.ts, platform/comms-file-log.ts, platform/index.ts, remote/bridge.ts, skills/skill-loader.ts, skills/skill-script-loader.ts, skills/skill-source.ts, streams/resolver.ts, upstream/resolver.ts, marketplace/packaging.ts, sharing/consult.ts, sharing/export.ts, sharing/import.ts, runtime/cross-squad.ts, runtime/scheduler.ts, runtime/squad-observer.ts
Pattern C: Hardcoded -- NONE FOUND
Zero instances of FSStorageProvider hardcoded with no way to override. Every public entry point accepts StorageProvider via DI.
How Samples Prove It
The samples demonstrate the swap works in practice:
Verdict
The StorageProvider abstraction follows standard dependency injection with sensible defaults. FSStorageProvider is the default (convenience), not hardcoded (coupling). Any provider implementing the 24-method interface can be injected at every public API boundary.