refactor: multi-provider memory adapters with scan-based builtin#227
Conversation
18f8904 to
4799252
Compare
…ifest.json dependency - Rename internal/memory/provider to internal/memory/adapters with per-provider subdirectories (builtin, mem0, openviking) - Replace manifest.json-based delete/update with scan-based index from daily files - Add mem0 and openviking provider adapters with HTTP client, chat hooks, MCP tools, and CRUD - Wire provider lifecycle into registry (auto-instantiate on create, evict on update/delete) - Split docker-compose into base stack + optional overlays (qdrant, browser, mem0, openviking) - Update admin UI to support dynamic provider config schema rendering
4799252 to
31ba897
Compare
d7562b4 to
74f1232
Compare
There was a problem hiding this comment.
Pull request overview
This PR refactors the memory provider system into a multi-adapter architecture, replaces manifest-based filesystem indexing with scan-based indexing, and adds new remote memory provider integrations (Mem0 and OpenViking) alongside registry-driven lifecycle management.
Changes:
- Introduces
internal/memory/adapterswith builtin/mem0/openviking provider implementations and shared helper/types. - Replaces
manifest.jsonupdate/delete flow with scan-based indexing over daily memory markdown files. - Splits Docker Compose into a minimal base stack plus optional overlays (qdrant/browser/mem0/openviking) and updates the admin UI to render provider configs dynamically.
Reviewed changes
Copilot reviewed 41 out of 42 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/settings/service.go | Adds gosec suppression for bounded int32 conversion |
| internal/schedule/service.go | Adds gosec suppressions for bounded int32 conversions |
| internal/memory/storefs/service.go | Removes manifest; adds scan-based index & delete/update flow |
| internal/memory/adapters/types.go | Adds provider types + config schema; renames rebuild count field |
| internal/memory/adapters/service.go | Provider CRUD + registry auto instantiate/evict/remove |
| internal/memory/adapters/registry.go | Package rename to adapters (registry core unchanged) |
| internal/memory/adapters/provider.go | Package rename + comment update for OpenViking |
| internal/memory/adapters/openviking/openviking.go | New OpenViking provider (hooks/tools/CRUD) |
| internal/memory/adapters/openviking/client.go | New OpenViking HTTP client |
| internal/memory/adapters/mem0/mem0.go | New Mem0 provider (hooks/tools/CRUD) |
| internal/memory/adapters/mem0/client.go | New Mem0 HTTP client |
| internal/memory/adapters/helpers.go | Adds shared helpers (truncate/dedupe/config access) |
| internal/memory/adapters/builtin/builtin_test.go | Updates tests to use shared TruncateSnippet helper |
| internal/memory/adapters/builtin/builtin.go | Ports builtin provider to adapters types + shared helpers |
| internal/mcp/providers/memory/provider_test.go | Updates import path to adapters |
| internal/mcp/providers/memory/provider.go | Updates import path to adapters |
| internal/mcp/providers/email/provider.go | Adds gosec suppression for bounded uint32 conversion |
| internal/mcp/providers/container/provider.go | Adds gosec suppression for bounded int32 conversion |
| internal/inbox/service.go | Adds gosec suppressions + caps ListUnread limit |
| internal/heartbeat/service.go | Adds gosec suppression for bounded int32 conversion |
| internal/handlers/memory_providers.go | Updates handler to adapters service/types |
| internal/handlers/memory.go | Updates adapters import + rebuild response field rename |
| internal/email/adapters/gmail/adapter.go | Adds gosec suppression + enforces TLS >= 1.2 |
| internal/email/adapters/generic/adapter.go | Enforces TLS >= 1.2 + gosec suppression |
| internal/conversation/flow/resolver.go | Updates import path to adapters |
| internal/channel/inbound/channel.go | Removes stray blank line |
| go.sum | Updates module checksums |
| go.mod | Promotes go-sasl/websocket deps to direct requirements |
| docker/docker-compose.yml | Removes browser service from docker/ compose file |
| docker/docker-compose.qdrant.yml | New Qdrant overlay compose |
| docker/docker-compose.openviking.yml | New OpenViking overlay compose |
| docker/docker-compose.mem0.yml | New Mem0 overlay compose |
| docker/docker-compose.browser.yml | New Browser overlay compose |
| docker-compose.yml | Base compose becomes minimal; removes qdrant/browser + volume |
| cmd/memoh/serve.go | Registers adapter factories + hooks registry into provider service |
| cmd/mcp/server.go | Adds gosec suppression for bounded int32 conversion |
| cmd/agent/main.go | Registers adapter factories + hooks registry into provider service |
| apps/web/src/pages/memory-providers/components/provider-setting.vue | Dynamic provider config form + meta fetch + secret inputs |
| apps/web/src/pages/memory-providers/components/add-memory-provider.vue | Adds mem0/openviking options to provider select |
| apps/web/src/i18n/locales/zh.json | Adds provider name translations |
| apps/web/src/i18n/locales/en.json | Adds provider name translations |
| DEPLOYMENT.md | Documents overlay-based Docker Compose startup patterns |
Comments suppressed due to low confidence (5)
internal/memory/adapters/service.go:244
- Provider configs are unmarshaled and returned verbatim in ProviderGetResponse. For mem0/openviking this will expose
api_keyvalues via the list/get endpoints. If the newProviderFieldSchema.Secretflag is meant to mark sensitive fields, consider redacting secret fields from responses (and adjusting Update semantics so omitted secrets preserve the stored value rather than clearing it).
cmd/memoh/serve.go:263 - On startup only the default memory provider is instantiated. Any non-default providers referenced by bot settings won't be present in the registry after a restart (registry.Get will fail and the handler falls back to the builtin default). Consider bootstrapping all configured providers from the DB into the registry at startup, or changing the lookup path to lazily instantiate from persisted config when a provider ID is requested.
func startMemoryProviderBootstrap(lc fx.Lifecycle, log *slog.Logger, mpService *memprovider.Service, registry *memprovider.Registry) {
mpService.SetRegistry(registry)
lc.Append(fx.Hook{
OnStart: func(ctx context.Context) error {
resp, err := mpService.EnsureDefault(ctx)
if err != nil {
log.Warn("failed to ensure default memory provider", slog.Any("error", err))
return nil
}
if _, regErr := registry.Instantiate(resp.ID, resp.Provider, resp.Config); regErr != nil {
log.Warn("failed to instantiate default memory provider", slog.Any("error", regErr))
} else {
log.Info("default memory provider ready", slog.String("id", resp.ID), slog.String("provider", resp.Provider))
}
return nil
cmd/agent/main.go:655
- On startup only the default memory provider is instantiated. Any non-default providers referenced by bot settings won't be present in the registry after a restart (registry.Get will fail and the handler falls back to the builtin default). Consider bootstrapping all configured providers from the DB into the registry at startup, or changing the lookup path to lazily instantiate from persisted config when a provider ID is requested.
func startMemoryProviderBootstrap(lc fx.Lifecycle, log *slog.Logger, mpService *memprovider.Service, registry *memprovider.Registry) {
mpService.SetRegistry(registry)
lc.Append(fx.Hook{
OnStart: func(ctx context.Context) error {
resp, err := mpService.EnsureDefault(ctx)
if err != nil {
log.Warn("failed to ensure default memory provider", slog.Any("error", err))
return nil
}
if _, regErr := registry.Instantiate(resp.ID, resp.Provider, resp.Config); regErr != nil {
log.Warn("failed to instantiate default memory provider", slog.Any("error", regErr))
} else {
log.Info("default memory provider ready", slog.String("id", resp.ID), slog.String("provider", resp.Provider))
}
return nil
internal/memory/adapters/types.go:220
- This change renames
qdrant_count->storage_countand expands ProviderType values (mem0/openviking). Any generated OpenAPI/spec + TypeScript SDK types that still reference the old field/enum will become stale and can break clients at build time. Consider including the spec/SDK regeneration in this PR (or at least gating the rename behind regenerated artifacts) so server/client stay in sync.
internal/memory/adapters/service.go:263 - tryEvictAndReinstantiate removes the existing provider instance before attempting to instantiate the new config. If instantiation fails, the registry is left without any instance for that ID, potentially breaking bots until restart or another update. Consider instantiating the new provider first and only swapping/evicting on success (or keep the old instance as a fallback).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| type openVikingClient struct { | ||
| baseURL string | ||
| apiKey string | ||
| memoryRoot string | ||
| httpClient *http.Client | ||
| } | ||
|
|
||
| func newOpenVikingClient(config map[string]any) (*openVikingClient, error) { | ||
| baseURL := adapters.StringFromConfig(config, "base_url") | ||
| if baseURL == "" { | ||
| return nil, errors.New("openviking: base_url is required") | ||
| } | ||
| baseURL = strings.TrimRight(baseURL, "/") | ||
| memoryRoot := adapters.StringFromConfig(config, "memory_root") | ||
| if memoryRoot == "" { | ||
| memoryRoot = "viking://agent/" | ||
| } | ||
| return &openVikingClient{ | ||
| baseURL: baseURL, | ||
| apiKey: adapters.StringFromConfig(config, "api_key"), | ||
| memoryRoot: strings.TrimRight(memoryRoot, "/"), | ||
| httpClient: &http.Client{ | ||
| Timeout: 30 * time.Second, | ||
| }, | ||
| }, nil |
There was a problem hiding this comment.
memory_root is read into openVikingClient.memoryRoot but never used in any request construction. This makes the exposed config field ineffective and can lead to unexpected scoping behavior. Either wire memoryRoot into the OpenViking API calls (e.g., prefix/derive agent IDs or pass the root explicitly if the API supports it) or remove it from the config schema/client struct.
|
|
||
| return c.JSON(http.StatusOK, memprovider.RebuildResult{ | ||
| FsCount: len(fsItems), | ||
| QdrantCount: len(fsItems), | ||
| StorageCount: len(fsItems), | ||
| MissingCount: 0, | ||
| RestoredCount: 0, | ||
| }) |
There was a problem hiding this comment.
Swagger annotations in this handler still reference provider.* types, but the memory types have moved to the adapters package (internal/memory/adapters). Even though this JSON response now uses adapters.RebuildResult fields, OpenAPI generation will fail until the annotation package names are updated to match the new Go package.
| "github.com/labstack/echo/v4" | ||
|
|
||
| memprovider "github.com/memohai/memoh/internal/memory/provider" | ||
| memprovider "github.com/memohai/memoh/internal/memory/adapters" | ||
| ) | ||
|
|
There was a problem hiding this comment.
The swagger annotations in this handler still reference provider.* types (e.g. provider.ProviderMeta / provider.ProviderGetResponse), but the package was renamed to adapters. OpenAPI generation will break until those annotation package names are updated to the new Go package.
| entryPath := path.Join(memoryDirPath(), entry.GetPath()) | ||
| content, readErr := s.readFile(ctx, botID, entryPath) | ||
| if readErr != nil { | ||
| continue | ||
| } | ||
| parsed, parseErr := parseMemoryDayMD(content) | ||
| if parseErr != nil { | ||
| legacy, legacyErr := parseLegacyMemoryMD(content) | ||
| if legacyErr != nil { | ||
| continue | ||
| } |
There was a problem hiding this comment.
buildScanIndex silently skips files when readFile/parse fails (it just continues). That can leave the index incomplete, causing PersistMemories/RemoveMemories/SyncOverview to miss entries without any error surfaced. Consider returning an error (or at least accumulating and returning/logging parse/read errors) so callers can detect corrupted/unreadable memory files instead of proceeding with partial state.
| func (c *openVikingClient) GetAll(ctx context.Context, agentID string, limit int) ([]ovMemory, error) { | ||
| url := "/memories?agent_id=" + agentID | ||
| if limit > 0 { | ||
| url += fmt.Sprintf("&limit=%d", limit) | ||
| } | ||
| var results []ovMemory | ||
| if err := c.doJSON(ctx, http.MethodGet, url, nil, &results); err != nil { | ||
| return nil, fmt.Errorf("openviking get all: %w", err) | ||
| } | ||
| return results, nil | ||
| } | ||
|
|
||
| func (c *openVikingClient) Update(ctx context.Context, memoryID, content string) (*ovMemory, error) { | ||
| var result ovMemory | ||
| if err := c.doJSON(ctx, http.MethodPut, "/memories/"+memoryID, ovUpdateRequest{Content: content}, &result); err != nil { | ||
| return nil, fmt.Errorf("openviking update: %w", err) | ||
| } | ||
| return &result, nil | ||
| } | ||
|
|
||
| func (c *openVikingClient) Delete(ctx context.Context, memoryID string) error { | ||
| if err := c.doJSON(ctx, http.MethodDelete, "/memories/"+memoryID, nil, nil); err != nil { | ||
| return fmt.Errorf("openviking delete: %w", err) | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| func (c *openVikingClient) DeleteAll(ctx context.Context, agentID string) error { | ||
| url := "/memories?agent_id=" + agentID | ||
| if err := c.doJSON(ctx, http.MethodDelete, url, nil, nil); err != nil { |
There was a problem hiding this comment.
GetAll/DeleteAll build URLs by concatenating agentID directly into the query string (e.g. "/memories?agent_id=" + agentID). Since bot IDs are taken from request params and not URL-escaped here, values containing &, ?, or spaces will break the request (and can lead to surprising server-side parsing). Consider using url.Values / QueryEscape for query params.
| func (c *mem0Client) GetAll(ctx context.Context, agentID string, limit int) ([]mem0Memory, error) { | ||
| url := "/memories?agent_id=" + agentID | ||
| if limit > 0 { | ||
| url += fmt.Sprintf("&limit=%d", limit) | ||
| } | ||
| var results []mem0Memory | ||
| if err := c.doJSON(ctx, http.MethodGet, url, nil, &results); err != nil { | ||
| return nil, fmt.Errorf("mem0 get all: %w", err) | ||
| } | ||
| return results, nil | ||
| } | ||
|
|
||
| func (c *mem0Client) Update(ctx context.Context, memoryID string, text string) (*mem0Memory, error) { | ||
| var result mem0Memory | ||
| if err := c.doJSON(ctx, http.MethodPut, "/memories/"+memoryID, mem0UpdateRequest{Text: text}, &result); err != nil { | ||
| return nil, fmt.Errorf("mem0 update: %w", err) | ||
| } | ||
| return &result, nil | ||
| } | ||
|
|
||
| func (c *mem0Client) Delete(ctx context.Context, memoryID string) error { | ||
| if err := c.doJSON(ctx, http.MethodDelete, "/memories/"+memoryID, nil, nil); err != nil { | ||
| return fmt.Errorf("mem0 delete: %w", err) | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| func (c *mem0Client) DeleteAll(ctx context.Context, agentID string) error { | ||
| url := "/memories?agent_id=" + agentID | ||
| if err := c.doJSON(ctx, http.MethodDelete, url, nil, nil); err != nil { |
There was a problem hiding this comment.
GetAll/DeleteAll build URLs by concatenating agentID directly into the query string (e.g. "/memories?agent_id=" + agentID). Since bot IDs are taken from request params and not URL-escaped here, values containing &, ?, or spaces will break the request (and can lead to surprising server-side parsing). Consider using url.Values / QueryEscape for query params.
| for _, item := range parsed { | ||
| id := strings.TrimSpace(item.ID) | ||
| if id == "" { | ||
| continue | ||
| } | ||
| if _, ok := index[id]; !ok { | ||
| index[id] = scanEntry{FilePath: entryPath} | ||
| } | ||
| } |
There was a problem hiding this comment.
buildScanIndex stores only the first file path seen for a given memory ID. If the same ID exists in multiple daily files (e.g., from prior bugs/manual edits), PersistMemories will only remove it from one old file and can leave duplicates behind. Consider indexing id -> set/list of file paths and removing the ID from all non-target files (or at least detecting duplicates and preferring the newest).
| limit := req.Limit | ||
| if limit <= 0 { | ||
| limit = ovDefaultLimit | ||
| } | ||
| memories, err := p.client.Search(ctx, botID, req.Query, limit) |
There was a problem hiding this comment.
Search uses req.Limit directly (only defaulting when <=0) but never caps it to ovMaxLimit. Since the HTTP handler passes payload.Limit through unbounded, this can trigger very large remote queries/responses. Consider applying the same max cap used by the MCP tool path (ovMaxLimit).
| limit := req.Limit | ||
| if limit <= 0 { | ||
| limit = mem0DefaultLimit | ||
| } | ||
| memories, err := p.client.Search(ctx, mem0SearchRequest{ | ||
| Query: req.Query, | ||
| AgentID: botID, | ||
| RunID: req.RunID, | ||
| Limit: limit, | ||
| }) |
There was a problem hiding this comment.
Search uses req.Limit directly (only defaulting when <=0) but never caps it to mem0MaxLimit. Since the HTTP handler passes payload.Limit through unbounded, this can trigger very large remote queries/responses. Consider applying the same max cap used by the MCP tool path (mem0MaxLimit).
bb12fd3 to
c4cb15e
Compare
18150db to
9c00885
Compare
9c00885 to
ad77258
Compare
a3fd143 to
b090a09
Compare
1032e9e to
394a29b
Compare
394a29b to
52d3603
Compare
Summary
internal/memory/providerintointernal/memory/adapterswith per-provider subdirectories (builtin/,mem0/,openviking/)manifest.json-based delete/update with scan-based index built from daily memory filesmem0andopenvikingprovider adapters (HTTP client, chat hooks, MCP tools, CRUD)docker-compose.ymlinto minimal base stack + optional overlays (qdrant,browser,mem0,openviking)Follow-up work
MEMORY.md/memory/*.mdfile references in system prompt only when builtin provider is active; remote providers should not encourage the agent to write filesystem memory fileshttp://127.0.0.1:6334which may cause connection errors on startupOnAfterChatcurrently stores raw message concatenation; consider adding LLM-based fact extraction (theLLMinterface andExtractRequest/DecideRequesttypes already exist but are unwired)base_urlis reachable)internal/healthcheck)mise run swagger-generate && mise run sdk-generateto update OpenAPI spec and TypeScript SDK with new provider types andStorageCountfield renamememoh.shone-click installer to use overlay pattern (qdrant+browseras default overlays)devenv/docker-compose.ymlto match the new overlay pattern for local developmentTest plan
go build ./...passesgo test ./internal/memory/...passesdocker compose -f docker-compose.yml configvalidatesdocker compose -f docker-compose.yml -f docker/docker-compose.qdrant.yml -f docker/docker-compose.browser.yml configvalidatesMade with Cursor