feat: add per-agent memory namespacing (owner-scoped keys)

[lsm]

Problem

Memory was a single shared pool per Space — UNIQUE(space_id, key), no owner dimension. Any session (coordinator, LH agent, workflow worker, ad-hoc member) could overwrite or delete any other's keys. Per-agent long-term memory was not a first-class concept.

Change

Introduces agent-scoped memory so each long-horizon agent holds a private namespace while the shared Space pool remains accessible.

• Schema (migration 163): adds owner_agent_id + scope to space_agent_memory; widens the unique key to (space_id, owner_agent_id, key) so two agents can each hold a private row under the same key. Existing rows backfill to scope='space', owner='' (no key loss). Idempotent table rebuild.
• Repository: write/read/delete/search/list accept ownerAgentId + scope ('mine' | 'space' | 'all'); default search returns the caller's agent-scoped rows + shared. recordAccess is now keyed by row id (keys are no longer unique). consolidate dedups per-owner and restricts core-memory ranking to shared rows so private memory never leaks into the common core.
• Tools: AgentMemoryToolsConfig.ownerAgentId (resolved from promptProvenance.agentId at LH-agent attach). memory.write gains an optional scope; memory.search/read/delete gain scope filters. Member / space_chat / workflow-node sessions stay space-scoped. RPC handler unchanged (space-scoped; no agent attribution trusted from RPC).

Acceptance

• Agent A writes a key that agent B cannot overwrite when scoped to A
• Search returns agent-scoped (for caller) + space-scoped by default; explicit mine | space | all filters work
• Backfill preserves all existing keys (no data loss)
• Migration is idempotent; unit tests for scoping + authorization

Design notes (deviations from the task brief, please review)

1. owner_agent_id uses '' (empty string) as the space-scoped sentinel, not NULL. SQLite treats NULLs as distinct in a UNIQUE constraint, so UNIQUE(space_id, owner_agent_id, key) would allow duplicate shared rows under NULL. The empty string keeps the composite UNIQUE enforceable; the public entry type maps '' → null.
2. No hard FK to space_long_horizon_agents. promptProvenance.agentId can reference either space_agents (worker) or space_long_horizon_agents, and a worker-only agent has no LH row — a FK to the LH table would reject those writes. SQLite cannot express a FK to "one of two tables," so owner_agent_id is an opaque, application-resolved key. Orphaned references (agent hard-deleted) just hide those rows from owner-scoped searches; they are never lost and can be reclaimed later. If the reviewer prefers strict LH-only ownership, the alternative is to gate owner assignment on LH-existence (worker agents would then be space-scoped only).
3. Migration FK-rename fix: the table rebuild runs under PRAGMA legacy_alter_table=ON so space_agent_core_memory / memory_vectors FK references survive the rename (the pre-existing ensureAgentMemoryNamedPrimaryKey avoided this only because it ran before those tables existed).

Test plan

• New: repository owner-isolation + scope filters + consolidation boundaries; tool namespacing; migration idempotency, backfill, FK survival, and FTS-after-rebuild.
• Existing memory suite updated and green. bun run check (lint/typecheck/knip/schema-parity) passes.

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

[lsm]

🤖 Review by glm-5.1 (NeoKai)

Model: glm-5.1 | Client: NeoKai | Provider: Zhipu (GLM)

Recommendation: REQUEST_CHANGES — one P1 (per-agent isolation breach via scope: 'all') plus a related P2 and a P3 nit. The schema, migration, wiring, and tests are solid; the gap is narrowly in the agent-facing tool surface.

What I verified (and looks correct)

• Migration 163: idempotent, backfills every existing row to scope='space'/owner='' with no key loss, keeps space_agent_core_memory + memory_vectors FK cascades valid (verified both insert+cascade-delete paths). The legacy_alter_table=ON rename approach correctly preserves the FK references.
• FTS after rebuild: I empirically confirmed legacy rows remain FTS-searchable post-migration even when the legacy FTS index was empty (real runMigration163 + real repo.search, no embedder). The "rebuilds the FTS index" test passes for the right reason.
• Repo scoping: default = "mine + space"; mine/space filters and per-owner consolidation dedup + shared-only core-memory ranking all behave correctly. Cross-owner writes/updates don't collide (UNIQUE(space_id, owner_agent_id, key) + CHECK tying scope to owner).
• Runtime wiring: all three attachLongTermAgentMcpServers call sites pass the correct agent id; promptProvenance.agentId === agent.id at construction; member/space_chat/workflow-node paths stay space-scoped. Backward-compatible (private method, optional last param).
• Backward compat: every non-MCP caller (RPC handler, task-agent-manager, consolidation job) correctly defaults to space-scoped; no positional-arg meaning changed for any live caller.
• Tests/CI: 81 tests across the 4 affected files pass; bun run check green (incl. schema-parity + test-quality); space-test-db.ts helper matches createAgentMemoryTables byte-for-byte including FTS triggers + owner index.

---

P1 — scope: 'all' on the agent-facing memory tools breaches per-agent isolation

This is the only blocker. The default scope isolates agents correctly, but MemorySearchSchema / MemoryReadSchema / MemoryDeleteSchema each expose scope: 'all' and pass it straight through to the repo with no authorization gate. At the repo layer, all means every owner:

I reproduced the breach end-to-end through the real tool handlers (two agents in the same space, A writes a private memory, B calls with scope: 'all'):

• memory.search({ query, scope: 'all' }) → returns agent A's private rows to agent B.
• memory.delete({ key, scope: 'all' }) → deleted agent A's private row (deleted: true; A's subsequent memory.read returns success: false). Confirmed destroyed.

This directly fails the PR's own acceptance criterion — "Agent A writes a key that agent B cannot overwrite/delete when scoped to A" — because B can delete A's scoped key by passing scope: 'all'. It also contradicts the repository's own inline comment on the delete path:

// Administrators only — wipe every row under this key across owners.

…which acknowledges all is privileged, yet the tool offers it to every agent. Even in a fully cooperative space, a single buggy agent loop calling memory.delete({scope:'all'}) would wipe every peer's private memory.

Recommended fix (low-risk, narrow): drop 'all' from the three agent-facing schemas — keep 'mine' | 'space' plus the omitted default. The repo-level all support can remain for the future trusted RPC/admin (memory-UI) path where authorization is actually enforced; the agent MCP tool is not that path. (If you want to keep all in the tool, gate it behind an explicit allowAllScope/trusted flag on AgentMemoryToolsConfig that is false for normal agent sessions.)

P2 — read with scope: 'all' is inconsistent with search/list/delete

For read, scope: 'all' falls into the "mine + shared" branch (agent-memory-repository.ts read) and never returns another agent's private row — empirically B read(scope='all') returns the shared row, not A's private one. So read's all is safe but means something different than search/list/delete's all (every owner), and differs from its own schema doc ("'all' = either"). If P1 is fixed by removing all from the tools this becomes moot at the tool layer; if repo-level all is retained for trusted callers, please make read consistent (or document the divergence).

P3 — migration comment overstates the FTS rebuild mechanism (nit, optional)

The migration doc says "The FTS index is then rebuilt from the copied rows" and that it "mirrors the rebuild pattern in ensureAgentMemoryNamedPrimaryKey" — but neither function issues an explicit FTS5 'rebuild' (INSERT INTO space_agent_memory_fts(space_agent_memory_fts) VALUES ('rebuild')). The index does end up populated (verified), but via implicit re-population rather than the documented explicit rebuild. Consider adding the explicit 'rebuild' statement after createAgentMemoryTables so correctness doesn't depend on the implicit behavior. Not blocking — behavior is correct today.

---

Summary

The namespacing foundation (schema, migration, repo semantics, wiring, tests) is well-built and verified. The blocker is purely the tool surface: scope: 'all' lets any agent read/destroy every other agent's private memory, defeating the isolation guarantee. Removing 'all' from the agent schemas (P1) unblocks; the P2/P3 are cleanups. Happy to re-review once the tool scope is tightened.

[lsm]

🤖 Review by glm-5.1 (NeoKai)

Model: glm-5.1 | Client: NeoKai | Provider: Zhipu (GLM)

Recommendation: APPROVE — all three prior findings addressed and verified; zero remaining findings.

Re-reviewed fresh from 96463d8. Confirmed each fix empirically:

• P1 (blocker) — resolved. MemorySearchSchema / MemoryReadSchema / MemoryDeleteSchema now .enum(['mine', 'space']); zod rejects scope:'all' for all three (verified safeParse → success:false), while mine / space / omitted-default still parse. The agent MCP tool can no longer reach the cross-owner read/delete path, so the original breach (B deleting/searching A's private memory) is closed. Repo-level all is correctly retained for the trusted RPC/memory-UI path.
• P2 — resolved. read() now has an explicit scope === 'all' branch (readAnyRow: own → shared → other), consistent with search/list/delete; the default branch no longer conflates all with "mine + space".
• P3 — resolved. Migration 163 now issues an explicit INSERT INTO space_agent_memory_fts(...) VALUES ('rebuild') after createAgentMemoryTables (idempotent), so post-migration FTS searchability is intentional rather than implicit.

Verification on 96463d8: 83 tests pass across the 4 affected files (incl. the new schema-rejection + readAnyRow tests); bun run check green (lint, typecheck, knip, session-guards, space-task-handler-tests, db-schema-parity, test-quality). Schema/migration/wiring/backward-compat remain correct from the prior round.

Nice fix — the narrow schema tightening closes the breach without losing the trusted-path capability.

[lsm]

@codex review

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, you can upgrade your account or add credits to your account and enable them for code reviews in your settings.

[lsm]

@codex review

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, you can upgrade your account or add credits to your account and enable them for code reviews in your settings.