fix(memory): register built-in memory embedding providers before manager init (#72875)#72937
Conversation
Greptile SummaryFixes the Confidence Score: 4/5Safe to merge — fix is correct and well-scoped; only P2 style finding present. The root-cause analysis is accurate, the registration call is placed at the right chokepoint (loadProviderResult), and idempotency is guaranteed by filterUnregisteredMemoryEmbeddingProviderAdapters. The only finding is a P2 code-duplication suggestion. No files require special attention. Prompt To Fix All With AIThis is a comment left during a code review.
Path: extensions/memory-core/src/memory/provider-adapters.ts
Line: 139-146
Comment:
**Duplicated logic — delegate to existing function**
`ensureBuiltInMemoryEmbeddingProvidersRegistered` is identical to the body of `registerBuiltInMemoryEmbeddingProviders` that already lives in the same file. Now that `registerMemoryEmbeddingProvider` is imported directly, the new function can simply delegate, eliminating the duplication and keeping the guarding comment in one place.
```suggestion
export function ensureBuiltInMemoryEmbeddingProvidersRegistered(): void {
registerBuiltInMemoryEmbeddingProviders({ registerMemoryEmbeddingProvider });
}
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix(memory): register built-in memory em..." | Re-trigger Greptile |
|
Thanks for the context here. I did a careful shell check against current Close because current main already resolves the So I’m closing this as already implemented rather than keeping a duplicate issue open. Review detailsBest possible solution: Keep the shipped manifest-backed provider contract and close this stale branch instead of rebasing manager-side registration into the current SDK layout. Do we have a high-confidence way to reproduce the issue? No for current main: the historical failure has strong report and fix evidence, but current source resolves Is this the best way to solve the issue? Yes for current main: manifest-backed provider ownership is the narrower maintainable fix because it preserves plugin boundaries and covers standalone CLI cold lookup without adding manager-specific eager registration. Security review: Security review cleared: The PR only changes in-process memory provider registration and TypeScript exports, with no dependency, workflow, permission, secret-handling, download, or artifact-execution changes. What I checked:
Likely related people:
Codex review notes: model gpt-5.5, reasoning high; reviewed against ba625e2cff29; fix evidence: release v2026.5.7, commit e069d03945ef. |
Fixes #72875
Summary
Unknown memory embedding provider: localerror, breaking memory sync and status checks.registerBuiltInMemoryEmbeddingProviders) beforeMemoryIndexManagerinitializes the provider, aligning it with the capability CLI path.Change Type (select all)
Scope (select all touched areas)
Linked Issue/PR
memorySearch provider: "local"fails with "Unknown memory embedding provider: local" but capability embedding path works #72875Root Cause (if applicable)
MemoryIndexManagercalledcreateEmbeddingProviderwithout ensuring built-in providers were registered in the registry, unlike the CLI path.Regression Test Plan (if applicable)
pnpm check:changedtypecheck/lint) confirms type safety and existing unit tests cover the provider registration mechanisms.User-visible / Behavior Changes
None. (The memory feature simply works as originally intended instead of crashing).
Diagram (if applicable)
N/A
Security Impact (required)
No)No)No)No)No)Yes, explain risk + mitigation: N/ARepro + Verification
Environment
Steps
Expected
Memory operations complete successfully without throwing
Unknown memory embedding provider: local.Actual
(Before fix) Throws
Unknown memory embedding provider: local.(After fix) Executes successfully.
Evidence
Human Verification (required)
pnpm check:changedlocally — all typechecks (core, extensions, tests) passed, linting passed with 0 warnings/errors, no import cycles detected.Review Conversations
Compatibility / Migration
Yes)No)No)Risks and Mitigations
None.