fix(memgw): pass user_id per-call to fix cross-user memory scoping (Codex P1) - #33
Conversation
…g in shared gateway sessions Closes the remaining open P1 Codex finding from the PR #30 review chain. In shared gateway sessions (thread_sessions_per_user=False), multiple users share a cached AIAgent instance. _user_id was stored once at initialize() time and reused by _user_scope() for all subsequent sync_turn/queue_prefetch calls, routing User B's memories into User A's gateway namespace. Fix: propagate user_id as an optional keyword argument through the full call chain: run_agent._sync_external_memory_for_turn -> MemoryManager.sync_all / queue_prefetch_all -> MemoryProvider.sync_turn / queue_prefetch (base interface updated) -> MemGatewayProvider._user_scope(user_id) -- per-call override wins, falls back to self._user_id for non-gateway single-user sessions _user_scope() now accepts an explicit uid that takes priority over the cached self._user_id, so every background write and prefetch is scoped to the user who actually triggered the turn, not the user who first initialized the session. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NFLEjSSx3Wr2j4D3v4HNN4
|
🔎 Lint report:
|
The base class MemoryProvider now declares user_id as a keyword-only default arg on both queue_prefetch and sync_turn. Seven providers and one test stub that override these methods were not updated in PR #33, causing ty to report invalid-method-override (17 new diagnostics). Add user_id: str = "" to each override; providers that do not use multi-user scoping can safely ignore it. Also bound pytest-timeout in [dependency-groups] per supply-chain policy: >=2.4.0,<3.
Codex Review — Automated Follow-upChanges pushed to this PR (commit
|
| File | Methods updated |
|---|---|
plugins/memory/byterover/__init__.py |
queue_prefetch, sync_turn |
plugins/memory/openviking/__init__.py |
queue_prefetch, sync_turn |
plugins/memory/supermemory/__init__.py |
sync_turn |
plugins/memory/hindsight/__init__.py |
queue_prefetch, sync_turn |
plugins/memory/honcho/__init__.py |
queue_prefetch, sync_turn |
plugins/memory/holographic/__init__.py |
sync_turn |
tests/agent/test_memory_provider.py |
queue_prefetch, sync_turn |
These providers do not serve multi-user sessions and can safely ignore the parameter.
2. Unbounded pytest-timeout spec — Resolved
Changed "pytest-timeout>=2.4.0" → "pytest-timeout>=2.4.0,<3" in [dependency-groups] per the repo supply-chain policy.
Overall Codex review status — all findings resolved ✅
This PR closes the last open item from the Codex review chain that started with PR #30:
| Severity | Finding | Status |
|---|---|---|
| P1 | Scope memgw calls by gateway user (_user_id stale after initialize()) |
✅ Fixed here |
| P1 | Keep memgw out of bundled providers | ✅ Deliberate exception (inert without mcp/MEMGW_API_KEY) |
| P1 | Leave default memory provider unset | ✅ Fixed in main (DEFAULT_CONFIG["memory"]["provider"] = "") |
| P2 (×9) | Various robustness issues (dep check, timeout cancel, loop init, etc.) | ✅ All fixed in main |
Automated Codex review follow-up by Claude Code.
Generated by Claude Code
The previous commit added <3 upper bound to pytest-timeout per repo supply-chain policy, but did not regenerate uv.lock. Running `uv lock` updates the lockfile to reflect the new constraint.
The base MemoryProvider.sync_turn and .queue_prefetch gained user_id: str = "" as a keyword-only default, but these overrides were missed in the previous commit. Fixes 5 remaining invalid-method-override ty warnings.
The base MemoryProvider.sync_turn and .queue_prefetch gained user_id: str = "" as a keyword-only default, but these overrides were missed in the previous commit. Fixes 5 remaining invalid-method-override ty warnings.
The base MemoryProvider.sync_turn and .queue_prefetch gained user_id: str = "" as a keyword-only default, but these overrides were missed in the previous commit. Fixes 5 remaining invalid-method-override ty warnings.
Automated CI fix —
|
| File | Methods |
|---|---|
plugins/memory/retaindb/__init__.py |
queue_prefetch, sync_turn |
plugins/memory/mem0/__init__.py |
queue_prefetch, sync_turn |
tests/agent/test_memory_user_id.py |
sync_turn |
All 5 invalid-method-override warnings should now be cleared.
Overall Codex review chain status
All findings from the PR #30 Codex review are now resolved:
| Severity | Finding | Status |
|---|---|---|
| P1 | Cross-user memory scoping (_user_id stale after initialize()) |
✅ Fixed in this PR |
| P1 | Keep memgw out of bundled providers | ✅ Deliberate exception (inert without mcp/MEMGW_API_KEY) |
| P1 | Leave default memory provider unset | ✅ Fixed in main |
| P2 ×9 | Robustness issues (dep check, timeout cancel, loop init, URL parsing, etc.) | ✅ All fixed in main (PRs #31, #32) |
PR is ready to merge pending CI green.
Automated follow-up by Claude Code.
Generated by Claude Code
Automated CI status noteThe What this means:
Recommended next steps:
Automated Codex-review follow-up audit by Claude Code. Generated by Claude Code |
Automated Codex Review Audit — Full PR #30 → #33 ChainAutomated audit of all Codex findings across PRs #30–#33 by Claude Code. SummaryAll Codex P1 and P2 findings from the PR #30 review chain are addressed in the current code. No new unresolved findings detected. P1 Findings (3 total)
P2 Findings (12 total — all resolved)
Blocker:
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5891ac1df6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| self._memory_manager.sync_all( | ||
| original_user_message, final_response, | ||
| session_id=self.session_id or "", | ||
| user_id=getattr(self, "_user_id", "") or "", |
There was a problem hiding this comment.
_user_id is set when the AIAgent is constructed, but the gateway reuses cached agents for a shared session_key and _init_cached_agent_for_turn() only resets activity/API counters (gateway/run.py:15018-15034, called at gateway/run.py:16365). In shared gateway sessions such as threads with thread_sessions_per_user=False, a message from User B can therefore reuse the agent created for User A, so this new user_id= argument still sends User A's id to sync_all/queue_prefetch_all and continues writing/prefetching B's turn in A's memory namespace.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed across two merged PRs:
- PR fix(memory): pass user_id through prefetch + scope per-user (Codex PR#33 P1) #41 (fix(memory): pass user_id through prefetch + scope per-user) —
_init_cached_agent_for_turn()now refreshes_user_idon every reused agent so the per-turn caller is always reflected before any memory op. - PR fix(memory): pass per-turn user_id through model tool calls + compat fallback #48 (fix(memory): pass per-turn user_id through model tool calls + compat fallback) —
tool_executor.pynow passesuser_id=agent._user_idinto every memoryhandle_tool_calldispatch, closing the final tool-call path.
Reviewed and confirmed by Claude Code.
Generated by Claude Code
| if self._is_breaker_open() or self._prefetch_method == 'off' or not query: | ||
| return | ||
|
|
||
| scope = self._user_scope(user_id) |
There was a problem hiding this comment.
This captures a user-specific scope for the background prefetch, but the result is still stored in the provider's single _prefetch_result and consumed by prefetch_all() at the start of whichever turn comes next, which has no user_id parameter (agent/conversation_loop.py:626 and agent/memory_manager.py:339-348). In shared thread/chat sessions, if User A's completed turn queues an A-scoped recall and User B speaks next, B's prompt can receive A's Memory Gateway context; key or invalidate the cached prefetch by user/session before returning it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in two merged PRs:
- PR fix(memory): pass user_id through prefetch + scope per-user (Codex PR#33 P1) #41 (fix(memory): pass user_id through prefetch + scope per-user) —
prefetch_allandqueue_prefetch_allnow forwarduser_idto each provider so the recall is scoped to the current caller. - PR fix(mem0): scope prefetch search and result cache by user_id #44 (fix(mem0): scope prefetch search and result cache by user_id) —
_prefetch_resultis now a(user_id, text)tuple;prefetch()discards any cached result whoseuser_iddiffers from the active turn, preventing cross-user injection.
Reviewed and confirmed by Claude Code.
Generated by Claude Code
| for provider in self._providers: | ||
| try: | ||
| provider.sync_turn(user_content, assistant_content, session_id=session_id) | ||
| provider.sync_turn(user_content, assistant_content, session_id=session_id, user_id=user_id) |
There was a problem hiding this comment.
Because Hermes supports dynamically loaded user memory providers, any installed provider written against the previous sync_turn(..., session_id="") / queue_prefetch(..., session_id="") interface will now raise TypeError when MemoryManager always supplies user_id; the exceptions are swallowed, so those providers silently stop syncing and prefetching. Please keep a fallback path for providers that do not yet accept the new kwarg, or require/validate **kwargs during plugin load.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in PR #48 (fix(memory): pass per-turn user_id through model tool calls + compat fallback): memory_manager.py now wraps prefetch_all, queue_prefetch_all, and sync_all calls in a TypeError catch block, so providers that have not yet adopted the user_id kwarg continue to operate silently instead of silently dropping all prefetch/sync.
Reviewed and confirmed by Claude Code.
Generated by Claude Code
| self._prefetch_thread.start() | ||
|
|
||
| def sync_turn(self, user_content: str, assistant_content: str, *, session_id: str = "") -> None: | ||
| def sync_turn(self, user_content: str, assistant_content: str, *, session_id: str = "", user_id: str = "") -> None: |
There was a problem hiding this comment.
This accepts the new per-turn user_id, but Mem0 still writes via _write_filters(), which reads self._user_id set once in initialize(). In a shared gateway session backed by a cached provider instance, even if the caller passes User B's id here, the turn is still added under the first user's Mem0 namespace; use the kwarg when building the add/search filters or refresh the provider's scoped user each turn.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in two merged PRs:
- PR fix(mem0): scope prefetch search and result cache by user_id #44 (fix(mem0): scope prefetch search and result cache by user_id) —
queue_prefetchnow uses the kwarguser_idfor its Mem0 search filter rather thanself._user_id. - PR fix(memory): pass per-turn user_id through model tool calls + compat fallback #48 (fix(memory): pass per-turn user_id through model tool calls + compat fallback) — all three Mem0 tool branches (
mem0_add,mem0_search,mem0_delete) now derivecall_user_id = kwargs.get("user_id", "") or self._user_idand build inline per-call filters, soself._user_idis never used alone.
Reviewed and confirmed by Claude Code.
Generated by Claude Code
|
Claude Code automated review — Codex PR#33 follow-up P1/P2 issues I reviewed the four Codex comments left on this PR and implemented fixes for all of them. The session's egress policy blocked P1 — Cached agent identity not refreshed (gateway/run.py)
Fix (after the agent._user_id = source.user_id or ""
agent._user_name = source.user_name or ""
agent._chat_id = source.chat_id or ""
agent._chat_name = source.chat_name or ""
agent._chat_type = source.chat_type or ""
agent._thread_id = source.thread_id or ""P1 — memgw prefetch result not scoped by user (plugins/memory/memgw/init.py + agent/memory_manager.py + agent/conversation_loop.py)
Fix: add # __init__.py: add to __init__
self._prefetch_result_user: str = ''
# prefetch() signature change:
def prefetch(self, query: str, *, session_id: str = '', user_id: str = '') -> str:
if self._prefetch_thread and self._prefetch_thread.is_alive():
self._prefetch_thread.join(timeout=3.0)
with self._prefetch_lock:
if user_id and self._prefetch_result_user and self._prefetch_result_user != user_id:
self._prefetch_result = ''
self._prefetch_result_user = ''
self._prefetch_gen += 1
result = self._prefetch_result
self._prefetch_result = ''
self._prefetch_result_user = ''
if not result:
return ''
return f'## Memory Gateway\n{result}'
# on_session_switch: also clear _prefetch_result_user
self._prefetch_result_user = ''
# queue_prefetch _run(): also store user_id
self._prefetch_result = text
self._prefetch_result_user = user_id# memory_manager.py prefetch_all signature:
def prefetch_all(self, query: str, *, session_id: str = "", user_id: str = "") -> str:
# ...
result = provider.prefetch(query, session_id=session_id, user_id=user_id)# conversation_loop.py:
_ext_prefetch_cache = agent._memory_manager.prefetch_all(
_query,
user_id=getattr(agent, "_user_id", "") or "",
) or ""P1 — Mem0 sync_turn uses stale self._user_id (plugins/memory/mem0/init.py)
Fix: def sync_turn(self, user_content, assistant_content, *, session_id="", user_id=""):
...
effective_user_id = user_id or self._user_id
def _sync():
...
client.add(messages, user_id=effective_user_id, agent_id=self._agent_id)P2 — External providers raise TypeError (agent/memory_manager.py)Old providers without the Fix: add try:
provider.sync_turn(..., user_id=user_id)
except TypeError:
try:
provider.sync_turn(...) # old signature, no user_id
except Exception as e:
logger.warning(...)
except Exception as e:
logger.warning(...)All five files pass Generated by Claude Code |
|
Follow-up on Codex P1: Scope prefetched memory by current user After auditing main, sync_turn() from PRs #33/#36/#41 is correct: it already uses effective_user_id and passes it directly to client.add(). ✅ However, queue_prefetch() and prefetch() still had the scoping bug:
In a shared gateway session, User A prefetch result could be injected into User B prompt context. Fix landed in PR #43: scope the search filter to per-call user_id, store result as (user_id, text) tuple, discard mismatched results in prefetch(). Remaining lower-priority note: handle_tool_call for mem0_conclude still uses _write_filters() -> self._user_id. For agent-side tool calls the agent is already scoped to one user, so this is acceptable unless the gateway ever exposes multi-user tool calls on one agent instance. Generated by Claude Code |
…fallback Addresses two remaining gaps flagged in Codex reviews on PRs #30 and #33. **P2 — model-facing tool calls used stale init-time user_id** `tool_executor.py` called `handle_tool_call` without `user_id`, so when the model invoked memgw_recall/retain/reflect or mem0_search/profile/conclude in a shared gateway session, both providers fell back to the user_id captured at `initialize()` time (i.e. the first user's id). In shared-thread sessions (`thread_sessions_per_user=False`) this meant one user's tool calls could read or write another user's memory scope. Fix: pass `user_id=agent._user_id` at the `tool_executor` call site (already refreshed per-turn by `gateway/run.py:16368`); thread it through `memory_manager.handle_tool_call` → provider, then use it in both `memgw.handle_tool_call` (via `_user_scope(call_user_id)`) and `mem0.handle_tool_call` (as per-call `read_filters` / `write_filters`). **P2 — external providers without user_id kwarg raised TypeError silently** Old third-party providers that override `sync_turn`/`prefetch`/`queue_prefetch` without the `user_id` keyword arg raised `TypeError`, which was caught by the broad `except Exception` and logged — causing silent sync/prefetch failures. Fix: add a specific `except TypeError` in `memory_manager.sync_all`, `prefetch_all`, and `queue_prefetch_all` that retries the call without the `user_id` kwarg, keeping old plugins functional while new ones get full per-user scoping. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QKeVgEJBrwJQSH2BSXA4oL
Codex review routine — daily status (2026-06-28)Codex activity on recent PRs: Codex hit its monthly usage cap on all PRs from #38 through #46 — no new review content was posted (each received only the "You have reached your Codex usage limits" message). Outstanding Codex P1 findings from this PR
Risk note: In shared-thread gateway sessions ( If the owner closed PRs #45 and #46 intentionally (architectural change planned elsewhere, or shared sessions not in active use), no action needed. If the closures were incidental, a new PR is warranted. Automated Codex review follow-up by Claude Code. Generated by Claude Code |
Codex review routine — daily status (2026-06-29)Codex activity on recent PRs: Codex remains at its monthly usage cap — all PRs from #28 through #48 received only the "You have reached your Codex usage limits" message. No new review content was posted. All Codex findings from PR #33 — now fully resolved ✅Yesterday's status noted two open items. Both are now closed:
PR #48 (merged 2026-06-29 00:23 UTC) closed the last two previously-open items. No further action needed on the Codex PR #33 review chain. Automated Codex review follow-up by Claude Code. Generated by Claude Code |
2 participants
Summary
Test plan
Codex finding addressed
Source: Codex inline comment on PR #30 at (2026-06-25)
Severity: P1 — genuine data isolation bug for multi-user gateway deployments
Generated with Claude Code
https://claude.ai/code/session_01NFLEjSSx3Wr2j4D3v4HNN4
Generated by Claude Code