feat(memory): #1755 route seedInitialMemories through v2 plugin #1759

Merged

hongming merged 2 commits from chore/issue-1755-seed-initial-memories-v2 into main 2026-05-24 05:20:56 +00:00

Conversation 16 Commits 2 Files Changed 6

+369 -138

2 Commits

Author	SHA1	Message	Date
hongming	0eceddcb96	review(memory): #1755 add log-capture + partial-failure tests (#1759 review N1) ... Three review findings from the #1759 hostile re-review: 1. `TestSeedInitialMemories_PluginNotWired_LogsAndSkips` only asserted "doesn't panic, doesn't error" — didn't capture log output. A future refactor that drops the operator warning would regress silently. Added a captureSeedLogs helper (mirrors the captureLogs pattern in internal/memory/wiring/wiring_test.go) and three substring assertions pinning the operator-facing message: - "v2 memory plugin not wired" (operator-actionable phrasing) - "MEMORY_PLUGIN_URL" (the env var to set) - The workspace ID (so the log line is diagnosable per-workspace) 2. `TestSeedInitialMemories_PluginCommitError_ContinuesLoop` asserted the attempt count but not that each failure produced an individual log line. Added captureSeedLogs around the call and asserted exactly 3 "plugin commit failed" lines plus the underlying error message text ("plugin down"). Without this, a "swallow errors" regression in the loop would pass the count assertion silently. 3. New `TestSeedInitialMemories_PartialFailure_CounterIsAccurate` — the original test set fails ALL plugin calls; the successful tests succeed all of them. There was no coverage for the mixed case (some succeed, some fail), so the `seeded %d/%d` summary log line could regress from "tracks actual successes" to "tracks attempts" undetected. New test uses an alternatingFailingPlugin helper to alternate success/failure across 4 seeds, then asserts "seeded 2/4 memories" appears in the captured output. Refs: #1759 review findings N1 (log-capture, partial-success coverage), all non-blocking but worth tightening. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>	2026-05-23 21:14:18 -07:00
hongming	f8a0afc452	feat(memory): #1755 route seedInitialMemories through v2 plugin ... Closes #1755. Gates Phase A3 (drop agent_memories) per the #1733 sequencing — A3 cannot land until this PR is in production, or workspace creation with seeded initial_memories breaks. ## Problem After PR #1747 (Phase A1) lands, the MCP memory tools (commit_memory, recall_memory) read and write exclusively through the v2 plugin. But seedInitialMemories was still INSERTing directly into the legacy agent_memories table at workspace-create time. Result: a workspace provisioned with `initial_memories` from a template had rows in agent_memories that were invisible to recall_memory — every fresh workspace silently had no seeded memory. Flagged in the #1747 hostile review (finding C3) and deliberately deferred to keep that PR small. This PR is that deferral. ## Change - New `seedMemoryPluginAPI` interface on WorkspaceHandler — the slice of `*client.Client` (memory/client) that seedInitialMemories needs (CommitMemory only). Parallel to MCPHandler's memoryPluginAPI. - New `WithSeedMemoryPlugin` setter — main.go wires this alongside WithNamespaceCleanup when MEMORY_PLUGIN_URL is set (memBundle.Plugin). - seedInitialMemories converted from package-level function to method on *WorkspaceHandler. Three call sites updated: - workspace.go:606 — Create handler - org.go:807 — org-template global memories - org_import.go:261 — per-workspace initial_memories - Routes through h.seedMemoryPlugin.CommitMemory(ctx, "workspace:<id>", contract.MemoryWrite{Content, Kind=fact, Source=runtime}). - Source=runtime distinguishes platform-seeded memories from agent-written (Source=agent) and canvas-written (Source=user). - All seeds land in the workspace's own `workspace:<id>` namespace. Pre-A1 callers wrote TEAM/GLOBAL-scoped seeds with namespace=workspace:<id> in agent_memories anyway, so behaviour- preserving for LOCAL and a no-op-shrink for TEAM/GLOBAL (agent can promote later via commit_memory_v2). - When the plugin is nil (operator hasn't set MEMORY_PLUGIN_URL), seedInitialMemories logs a loud warning with operator-actionable hint and SKIPS the seed batch — there is no SQL fallback after A1. Self-hosted operators without v2 wired get a clear signal that seeded memories from templates aren't persisted on their deployment. - Truncation + redaction order preserved exactly: 1. Validate scope (LOCAL/TEAM/GLOBAL). 2. Skip empty content. 3. Truncate to maxMemoryContentLength (100 KiB, CWE-400 cap). 4. Apply redactSecrets. 5. Plugin commit. - Plugin commit errors are non-fatal: loop continues for the next seed, mirroring the original "each insert is attempted independently" contract. ## Tests 8 existing seedInitialMemories tests rewritten + 2 new ones: | Test | Coverage | |---|---| | TestSeedInitialMemories_TruncatesOversizedContent | Parameterized truncation contract at the 100k boundary; asserts on plugin.calls[0].Body.Content length | | TestSeedInitialMemories_RedactsSecrets | Redaction fires before plugin commit | | TestSeedInitialMemories_InvalidScopeSkipped | Invalid scope = no plugin call | | TestSeedInitialMemories_EmptyMemoriesNil | Nil slice = no plugin call | | TestSeedInitialMemories_Truncation | Plain truncation, plugin sees 100k | | TestSeedInitialMemories_ContentUnderLimit | Under-limit content passes through unchanged | | TestSeedInitialMemories_ExactlyAtLimit | Boundary case (== limit) — no truncation | | TestSeedInitialMemories_EmptyContent | Empty content skipped | | TestSeedInitialMemories_OversizedWithSecrets | Truncation BEFORE redaction; raw secret literal never reaches plugin | | TestSeedInitialMemories_PluginNotWired_LogsAndSkips | **NEW** — fail-soft when MEMORY_PLUGIN_URL unset | | TestSeedInitialMemories_PluginCommitError_ContinuesLoop | **NEW** — each plugin call is independent; one failure doesn't abort the batch | Test infrastructure: new `stubSeedPlugin` captures every plugin call for assertion (parallel to `stubMemoryPlugin` in mcp_tools_memory_v2_ test.go). ## Stage A - `go vet ./...` clean. - `go test -short -count=1 ./...` green across every package (workspace-server unit + integration suites). - Stage B / C deferred to staging deploy after PR #1742 (A0 schema isolation) and PR #1747 (A1 v1-fallback removal) land. ## Sequencing - Branched on top of PR #1737 (awareness backend removal — it does the seedInitialMemories signature pre-work this PR needs). - Land order: #1737 → #1742 → #1747 → this. When #1737 merges, this branch rebases cleanly onto main with the seed-migration as the only delta. When #1747 merges, the v1 fallback is gone and this PR's "skip when plugin not wired" semantics become the only behaviour. ## Gating Phase A3 Phase A3 (drop agent_memories table) requires this to land first. After this ships: - New workspaces with initial_memories → plugin. - Pre-A1 workspaces with rows in agent_memories → migrated by the one-shot Phase A2 backfill (cmd/memory-backfill, separate ops task). - A3 can then DROP TABLE agent_memories with no live writers. Refs: closes #1755, follow-up to #1747 review finding C3, gates Phase A3 of #1733. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>	2026-05-23 21:14:18 -07:00