Repository navigation
fix(sdk): guard registered memory providers - #2333
Conversation
🛫 PR Readiness Check
PR Scope: 📦🔧 Mixed (product + infrastructure)
|
| Status | Check | Details |
|---|---|---|
| ❌ | Single commit | 2 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 | No Copilot review threads |
| ❌ | CI passing | 13 check(s) still running |
Files Changed (3 files, +208 −7)
| File | +/− |
|---|---|
.changeset/memory-provider-guards.md |
+5 −0 |
packages/squad-sdk/src/memory/index.ts |
+79 −7 |
test/memory-providers.test.ts |
+124 −0 |
Total: +208 −7
This check runs automatically on every push. Fix any ❌ items and push again.
See CONTRIBUTING.md and PR Requirements for details.
🟡 Impact Analysis — PR #2333Risk tier: 🟡 MEDIUM 📊 Summary
🎯 Risk Factors
📦 Modules Affectedroot (1 file)
squad-sdk (1 file)
tests (1 file)
|
|
Thanks Jose, another sharp catch on the memory governance boundary. I verified on dev that packages/squad-sdk/src/memory/index.ts had no timeout on provider.write (:838) or provider.search (:936), ran an unguarded Promise.all in providerStatus() (:1112), and trusted recalled provider.search() hits without re-checking supportedClasses, FORBIDDEN patterns, or loadGuidance (:937). Bounding write, search, and status with withProviderTimeout and filtering recalled hits through his.classify and I verified the merge-tree against dev is clean and approved the CI workflow runs. As you noted, line 411 of packages/squad-sdk/src/memory/index.ts (safeProviderErrorReason) touches the same signature as #2319. We are in a release freeze for 2.0.0-preview.2 right now and will merge your memory series in order (#2315, #2317, #2319, then #2333) right after the release ships, with that one-line combine on safeProviderErrorReason (provider: Pick<MemoryProvider, 'name'> and operation: 'write' | 'search' | 'status'). |
LocalMemoryStore trusted registered providers completely once governance had run on the way in: - every provider write/search/status call now has a timeout (providerTimeoutMs, default 5000 ms), audited as a provider error; - providerStatus() guards each provider, so one throwing or hung status() shows up as unavailable instead of failing the whole call; - recalled results are checked on the way back: classes the provider could not have been sent and FORBIDDEN-classified titles/snippets are dropped and audited, and an invalid loadGuidance falls back to the class default; - the replication audit records the provider id. No interface signatures change. Closes github#2332
80ca2e6 to
c8557b1
Compare
|
Rebased on |
What
LocalMemoryStorenow guards its registered memory providers:providerStatus();Why
Closes #2332
Governance ran before anything reached a provider, but nothing after that. A hung provider stalled
memory.write/memory.searchwith no limit. One throwing or hungstatus()brokeproviderStatus()for everyone. And whatever a provider returned fromsearch()went straight to the agent, including classes it was never sent and secret-looking text that would have been rejected on write.How
All in
packages/squad-sdk/src/memory/index.ts:withProviderTimeout()bounds providerwrite,searchandstatuscalls. The limit is configurable through the newLocalMemoryStoreOptions.providerTimeoutMs, with a default of 5000 ms; non-positive or non-finite values fall back to the default. A timeout throwsProviderTimeoutError, which goes through the existing safeprovider-erroraudit path. The call isn't cancelled, so a slow write may still land in the provider later; the option's doc comment says so.providerStatus(). Each provider'sstatus()is wrapped, so a throw or timeout becomes{ available: false, reason: '<name> provider status failed (<ErrorName>); raw provider error text omitted' }, and the other providers are still reported.write()routes to providers (LOCAL,DECISION,POLICY), and its title and snippet don't classify as FORBIDDEN.rejectaudit record per provider with a count, and no content.loadGuidancegoes through the samenormalizeLoadGuidancehelper local entries use, falling back to the class default.writerecord for a replicated copy now carriesprovider: <id>, the same as the error records.MemoryProviderdoc comment describes the return-path contract.No interface signatures change, and nothing in the CLI registers a provider today, so shipped Squad behaves the same. The changes take effect for SDK users who pass
registeredProviders.I left the scope,
limit,ToolRegistrywiring and delete propagation out on purpose. Those need a design decision first (listed in the issue).Heads-up: this changes the same line as #2319: the
safeProviderErrorReasonsignature, which there accepts the host adapter and here adds'status'. Whichever merges second needs a one-line rebase, and I'll do it.Testing
New tests in
test/memory-providers.test.ts:searchtimes out, local results still come back, and the timeout is audited;writetimes out without failing the local write;status()providers are reported as unavailable next to a healthy one;COPILOT_MEMORY) or a credential-like snippet are dropped and counted, and the audit doesn't contain the snippet;loadGuidancefalls back to the class default.Results:
memory/index.tschange reverted, all 6 new tests fail; the two timeout tests hang until the test timeout.test/memory-providers.test.ts,memory-governance,memory-value-benchmark,tools,cli/state-mcpandcli/state-mcp-lifecycle: 174 passed.npm run lint: clean..changeset/memory-provider-guards.md, sdk patch)PR Readiness Checklist
Branch & Commit
dev(notmain)dev(9d7dff8)Build & Test
npm run lintpassesDocs
registeredProviders; the option and the provider contract are documented in the sourceBreaking Changes
None. The interface is unchanged. A provider that returned results outside its declared classes, or FORBIDDEN-looking text, will now have those results dropped, which is the intended behavior.
Waivers
None.