feat(memory-plugin): #1733 A0 — isolate v2 plugin tables under memory_plugin schema #1742

Merged

hongming merged 3 commits from chore/issue-1733-memory-plugin-schema-isolation into main 2026-05-24 04:12:59 +00:00

Conversation 16 Commits 3 Files Changed 4

+105 -3

3 Commits

Author	SHA1	Message	Date
hongming	ea01341a9f	ci: refire stale lint workflows on #1742 (post-persona-ack)	2026-05-23 19:42:00 -07:00
hongming	e03b3fb243	review(memory-plugin): #1742 — search_path fallback to public + per-file embed test ... Addresses two findings from the #1742 review (PR-comment context: the extension-resolution finding was flagged as a production blocker for any tenant that previously installed pgvector in `public`): 1. entrypoint-tenant.sh — search_path now `memory_plugin,public` (was `memory_plugin`). Pgvector is schema-bound, not relocated by search_path. On tenants where pgvector was already installed into `public` by a prior boot or operator action, the previous single-schema search_path made `vector(1536)` references in 001_memory_v2.up.sql unresolvable — boot would die inside the 30s health gate. The `,public` fallback resolves `vector` via the tail schema while still landing every new CREATE TABLE in `memory_plugin` (SSOT preserved — search_path picks the FIRST writable schema for new objects). Verified locally with the exact failure case from the review: - Fresh tenant: CREATE EXTENSION vector → extension lands in `memory_plugin` (SSOT intent). - Pre-existing tenant (pgvector pre-installed in `public`): CREATE EXTENSION IF NOT EXISTS is a no-op, extension stays in `public`, vector(1536) resolves via fallback, plugin boots, memory tables still land in `memory_plugin`. 2. migrations_embed_test.go — replaced the global "at least one file has CREATE TABLE somewhere" assertion with explicit per-file pins: - 000_schema_bootstrap.up.sql MUST contain `CREATE SCHEMA`. - 001_memory_v2.up.sql MUST contain `CREATE TABLE` AND mention both `memory_records` and `memory_namespaces`. The global check let a future rewrite of 001 silently regress to schema-only as long as ANY embedded file declared a table. 3. Updated 000_schema_bootstrap.up.sql comment block to document the new search_path shape + the pgvector resolution rule, so a future reader doesn't have to re-derive why the fallback exists. Stage A: - Plugin tests still green (`go test -count=1 ./cmd/memory-plugin-postgres/...`). - Local Postgres-in-Docker round-trip exercising BOTH the pre-existing-public and fresh-tenant paths; logs + schema verification in the commit-discussion thread. Refs: #1742 (this PR), review-finding C1 (extension resolution) + C2 (test invariant weakened). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>	2026-05-23 16:53:43 -07:00
hongming	354bb5a4d4	feat(memory-plugin): #1733 A0 — isolate v2 plugin tables under memory_plugin schema ... The Memory v2 plugin sidecar already auto-starts on tenant boot (per entrypoint-tenant.sh, since 2026-05-14) when `MEMORY_V2_CUTOVER=true`, which CP sets on every tenant since 2026-05-05 (controlplane ec2.go:2232). Plugin tables today land in the tenant Postgres's `public` schema, sharing the namespace with platform-tenant tables — a soft SSOT violation flagged in #1733. CTO decision 2026-05-23: same Postgres instance, separate schema. Changes: - New `cmd/memory-plugin-postgres/migrations/000_schema_bootstrap.up.sql` + matching `.down.sql`. Migration filenames sort alphabetically (per `main.go:runMigrationsFromEmbed → sort.Strings`), so 000 runs before the existing 001_memory_v2 which assumes the schema exists in search_path. - `entrypoint-tenant.sh`: when defaulting `MEMORY_PLUGIN_DATABASE_URL` from the tenant's `DATABASE_URL`, append `search_path=memory_plugin` (handles both `?` and `&` URL forms). When the operator explicitly sets `MEMORY_PLUGIN_DATABASE_URL` (separate DB entirely), they keep full control — no rewriting. - `migrations_embed_test.go`: relax the per-file assertion (was: every *.up.sql must contain `CREATE TABLE`; now: every file must contain a recognized DDL keyword AND at least one file declares CREATE TABLE). Required because the new 000 file is schema-only. Stage A — verified locally against `pgvector/pgvector:pg16` (the same image CP launches on tenant EC2): - Plugin binary boots, embedded migrations apply in order `000_schema_bootstrap → 001_memory_v2`. - Tables land in `memory_plugin` schema; `public` stays empty. - End-to-end PUT namespace / POST memory / POST search round-trips through the new schema. - The entrypoint URL-mutation logic exercised on 3 cases: DATABASE_URL with `?`, without `?`, and operator-override (no rewrite). - `go test -short -count=1 ./...` green across all packages (workspace-server + plugin). Operational follow-ups (deliberately out of scope for this PR): - Tenants currently running with rows in `public.memory_*` need a one-time migration to `memory_plugin.memory_*`. Handled in #1733 Phase A2 (per-tenant backfill via `cmd/memory-backfill`); not gating this PR because no production tenant has rows in v2 yet (every tenant_workspace_server has been silently falling back to v1 SQL on `agent_memories` since 2026-05-05). - `CREATE EXTENSION vector` (in 001_memory_v2) implicitly lands in the first writable search_path schema — i.e. `memory_plugin` now. Dropping the schema cascade-drops the extension. If a future operator drops the schema intentionally, they must re-`CREATE EXTENSION`. Acceptable for a defensive operator action; flagged in `000_schema_bootstrap.down.sql`. Refs: #1733 (memory SSOT — Phase A0 of revised plan). Followed by A1 (workspace-server: kill v1 fallback, fail-fast on missing plugin) in a separate core PR. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>	2026-05-23 14:27:59 -07:00