Skip to content

fix(memory): address Codex PR#33 P1 — cross-user identity + prefetch scoping - #36

Closed
dizhaky wants to merge 17 commits into
mainfrom
fix/codex-p1-memory-scoping-followup
Closed

fix(memory): address Codex PR#33 P1 — cross-user identity + prefetch scoping#36
dizhaky wants to merge 17 commits into
mainfrom
fix/codex-p1-memory-scoping-followup

Conversation

@dizhaky

@dizhaky dizhaky commented Jun 27, 2026

Copy link
Copy Markdown
Owner

Summary

Follow-up to Codex PR #33 review — implements 3 P1 findings that were left unresolved when PR #33 was merged.

Previous attempt: Claude Code posted patches in a PR #33 comment on 2026-06-27 but couldn't push due to egress policy. This PR applies those patches.


P1 — Cached agent identity not refreshed on gateway reuse (gateway/run.py)

_init_cached_agent_for_turn only reset _last_activity_ts and _api_call_count. In shared-thread sessions where thread_sessions_per_user=False, a reused AIAgent retained its prior _user_id, _user_name, _chat_id, etc. User B's turn was therefore synced to memory under User A's identity.

Fix: After _init_cached_agent_for_turn, overwrite all caller-identity fields from the current source object.


P1 — Prefetch result not scoped by user (plugins/memory/memgw/__init__.py + agent/memory_manager.py + agent/conversation_loop.py)

queue_prefetch correctly scoped the gateway request per-user via _user_scope(user_id), but stored the result in a single _prefetch_result with no ownership tracking. prefetch() returned that result to whoever called next — potentially a different user in shared-thread sessions.

Fix:

  • Add _prefetch_result_user field to track ownership alongside _prefetch_result
  • prefetch() accepts user_id='' and discards stale results when user mismatches
  • memory_manager.prefetch_all() accepts and threads user_id to providers
  • conversation_loop.py passes agent._user_id to prefetch_all()

P1 — Mem0 sync_turn ignores per-turn user_id (plugins/memory/mem0/__init__.py)

sync_turn accepted user_id as a keyword argument but the inner _sync() closure called client.add(messages, **self._write_filters()) which always reads self._user_id set at initialize() time. In shared gateway sessions, every turn was written to the first user's Mem0 namespace.

Fix: Capture effective_user_id = user_id or self._user_id in the outer scope and pass it directly to client.add().


Files changed

File Change
gateway/run.py +6 lines: refresh identity fields after _init_cached_agent_for_turn
plugins/memory/memgw/__init__.py Add _prefetch_result_user; scope prefetch() by user; store user in _run()
plugins/memory/mem0/__init__.py Use effective_user_id = user_id or self._user_id in sync_turn
agent/memory_manager.py Add user_id to prefetch_all() signature
agent/conversation_loop.py Pass agent._user_id to prefetch_all()

Test plan

  • Run existing memory provider tests: pytest tests/ -k memory -v
  • Manual test: gateway shared-thread session with two users — verify each user's memory is isolated
  • Manual test: prefetch with user switch — verify stale result is discarded

🤖 Generated with Claude Code


Generated by Claude Code


Note

Medium Risk
Touches gateway session identity and memory read/write paths where wrong user scoping could leak or corrupt stored context; changes are narrow and defensive.

Overview
Fixes cross-user memory identity leaks when the gateway reuses a cached AIAgent in shared-thread sessions (thread_sessions_per_user=False).

On cache hit, gateway/run.py now overwrites caller identity on the agent (_user_id, _user_name, chat/thread fields) from the current source after _init_cached_agent_for_turn, so a later user’s turn is not attributed to the first user.

Memory prefetch is threaded with user_id: memory_manager.prefetch_all() forwards it to providers; conversation_loop.py passes agent._user_id. The memgw provider tracks _prefetch_result_user with the cached prefetch blob and discards the result when prefetch() is called for a different user; ownership is cleared on session switch.

Mem0 sync_turn now uses effective_user_id = user_id or self._user_id and passes it to client.add() instead of relying only on init-time self._user_id via _write_filters().

Reviewed by Cursor Bugbot for commit 792dc62. Configure here.

dizhaky added 5 commits June 27, 2026 12:14
sync_turn accepted user_id but _write_filters() always read self._user_id set
at provider initialize() time. In shared gateway sessions, User B's turn was
synced under User A's Mem0 namespace.
queue_prefetch scoped the gateway request per-user but stored the result in a
single _prefetch_result. In shared-thread sessions, User B could consume User A's
prefetch context. Now track _prefetch_result_user and discard on mismatch.
Thread user_id from queue_prefetch_all (which already scoped requests) through
to prefetch() so providers can validate result ownership.
…R#33 P1

Thread the per-turn user identity to prefetch_all so providers can validate
which user's prefetch result to return.
…#33 P1

_init_cached_agent_for_turn only reset timing/API counters. In shared-thread
sessions (thread_sessions_per_user=False), a reused AIAgent kept its prior
_user_id/_user_name/_chat_id — so User B's turn synced memory under User A's
identity. Refresh all caller-identity fields from source after init.
@github-actions

github-actions Bot commented Jun 27, 2026

Copy link
Copy Markdown

🔎 Lint report: fix/codex-p1-memory-scoping-followup vs origin/main

ruff

Total: 0 on HEAD, 0 on base (➖ 0)

🆕 New issues: none

✅ Fixed issues: none

Unchanged: 0 pre-existing issues carried over.

ty (type checker)

Total: 8649 on HEAD, 8649 on base (➖ 0)

🆕 New issues: none

✅ Fixed issues: none

Unchanged: 4570 pre-existing issues carried over.

Diagnostics are surfaced as warnings — this check never fails the build.

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.

Fix All in Cursor

Bugbot Autofix prepared a fix for the issue found in the latest run.

  • ✅ Fixed: Prefetch user_id breaks providers
    • Added signature inspection in prefetch_all so user_id is only forwarded to providers whose prefetch method accepts it, restoring prefetch for mem0, honcho, and other legacy providers.

Create PR

Or push these changes by commenting:

@cursor push 9177ca9475
Preview (9177ca9475)
diff --git a/agent/memory_manager.py b/agent/memory_manager.py
--- a/agent/memory_manager.py
+++ b/agent/memory_manager.py
@@ -336,6 +336,18 @@
 
     # -- Prefetch / recall ---------------------------------------------------
 
+    @staticmethod
+    def _prefetch_accepts_user_id(provider: MemoryProvider) -> bool:
+        """Return True if provider.prefetch accepts a user_id keyword."""
+        try:
+            signature = inspect.signature(provider.prefetch)
+        except (TypeError, ValueError):
+            return False
+        params = signature.parameters
+        if any(p.kind == inspect.Parameter.VAR_KEYWORD for p in params.values()):
+            return True
+        return "user_id" in params
+
     def prefetch_all(self, query: str, *, session_id: str = "", user_id: str = "") -> str:
         """Collect prefetch context from all providers.
 
@@ -345,7 +357,10 @@
         parts = []
         for provider in self._providers:
             try:
-                result = provider.prefetch(query, session_id=session_id, user_id=user_id)
+                prefetch_kwargs = {"session_id": session_id}
+                if self._prefetch_accepts_user_id(provider):
+                    prefetch_kwargs["user_id"] = user_id
+                result = provider.prefetch(query, **prefetch_kwargs)
                 if result and result.strip():
                     parts.append(result)
             except Exception as e:

You can send follow-ups to the cloud agent here.

Comment @cursor review or bugbot run to trigger another review on this PR

Reviewed by Cursor Bugbot for commit 792dc62. Configure here.

Comment thread agent/memory_manager.py

dizhaky commented Jun 27, 2026

Copy link
Copy Markdown
Owner Author

Codex Review Follow-up Analysis

Reviewing the active bot findings on this PR:

🔴 cursor[bot] bug (High) + Codex P2 re-raised: prefetch_all passes user_id to incompatible providers

What it found:
prefetch_all now calls provider.prefetch(..., user_id=user_id), but the base MemoryProvider.prefetch() signature doesn't accept user_id. Providers like mem0 and honcho will raise TypeError, which is swallowed — silently dropping their prefetch context every turn. The lint report confirms: agent/memory_manager.py:348: [unknown-argument] Argument 'user_id' does not match any known parameter of bound method 'MemoryProvider.prefetch'.

This is the same risk Codex flagged as P2 in PR #33 ("Preserve compatibility with external memory providers") — now confirmed as a real TypeError in the type checker.

Suggested fix (two options):

Option A – add user_id to the base MemoryProvider interface (breaking, clean):

# agent/memory_providers.py – base class
def prefetch(self, ..., user_id: str | None = None) -> None: ...

Then each provider that cares about it overrides appropriately. External providers break on next update but get a clear path.

Option B – try/except or inspect at the call site (non-breaking, expedient):

# agent/memory_manager.py
import inspect

for provider in self._providers:
    try:
        sig = inspect.signature(provider.prefetch)
        if "user_id" in sig.parameters:
            provider.prefetch(..., user_id=user_id)
        else:
            provider.prefetch(...)
    except Exception:
        ...

Option A is cleaner long-term but requires updating every provider. Option B unblocks merging this PR quickly without risking regressions in mem0/honcho.

Recommend: land Option B as a targeted fix in this branch so the P1 memory-scoping fixes ship, then track Option A as a follow-up interface cleanup.


✅ Codex P1 from PR #33 — addressed by this PR

  • "Refresh cached agent identity before memory sync" → addressed (per PR description)
  • "Scope prefetched memory by current user" → addressed (per PR description)
  • "Apply per-turn user_id inside Mem0" → addressed (per PR description)

The user_id kwarg compatibility issue above is the one remaining blocker before this can merge.


Generated by Claude Code

dizhaky commented Jun 27, 2026

Copy link
Copy Markdown
Owner Author

Fix applied: user_id added to prefetch() signature across all providers

Addressing the High Severity finding from cursor[bot] and the P2 Codex re-raise flagged in the previous review:

Root cause: prefetch_all (added by this PR) calls provider.prefetch(..., user_id=user_id), but the base MemoryProvider.prefetch() and all provider overrides only accepted session_id — not user_id. This caused a TypeError for every non-memgw provider, silently dropped via except Exception: logger.debug(...), meaning mem0, honcho, hindsight, etc. returned no prefetch context on every turn.

Fix applied (Option A — consistent interface): Added user_id: str = "" to the prefetch() signature in:

  • agent/memory_provider.py (base class)
  • plugins/memory/mem0/__init__.py
  • plugins/memory/honcho/__init__.py
  • plugins/memory/hindsight/__init__.py
  • plugins/memory/holographic/__init__.py
  • plugins/memory/byterover/__init__.py
  • plugins/memory/openviking/__init__.py
  • plugins/memory/supermemory/__init__.py
  • plugins/memory/retaindb/__init__.py

memgw already had the param — no change needed there.

This makes prefetch() consistent with queue_prefetch() and sync_turn(), which already accepted user_id. Providers that don't use per-user scoping in their prefetch simply receive and ignore the new param.

The type-checker unknown-argument error and cursor[bot] High Severity finding should both be resolved.


Generated by Claude Code

dizhaky commented Jun 27, 2026

Copy link
Copy Markdown
Owner Author

ty fix: add user_id to mock prefetch() signatures

The lint bot flagged 2 new invalid-method-override errors introduced by this PR:

tests/agent/test_memory_provider.py:48  — FakeMemoryProvider.prefetch()
tests/agent/test_memory_user_id.py:43   — FakeMemoryProvider.prefetch()

Both mock classes had def prefetch(self, query, *, session_id=""): but MemoryProvider.prefetch() was updated in this PR to include user_id: str = "". Applied the fix in two commits:

  • d8c9594test_memory_provider.py
  • 0ded639test_memory_user_id.py

One-line change per file: session_id=""session_id="", user_id="". The rest of the PR is clean — ruff has 0 new issues and the semantic fix for the user_id scoping bug looks correct.


Generated by Claude Code

dizhaky commented Jun 27, 2026

Copy link
Copy Markdown
Owner Author

Claude triage — cursor[bot] concern + Codex P1 status

PR #36 is open and addressing 3 P1 Codex findings from PR #33 (cross-user memory scoping in shared gateway sessions).

cursor[bot] flag: prefetch_all now passes user_id to all providers. Providers that haven't adopted the kwarg (e.g. mem0, honcho) will throw TypeError, silently dropped at debug level, meaning prefetch context is lost every turn for those providers.

Assessment: This is real. The P2 compatibility comment from Codex on PR #33 (preserve external provider interface) aligns with this. Options:

  1. Use **kwargs passthrough or check provider signature before passing user_id
  2. Version the memory provider protocol and only pass user_id to updated providers

Remaining P1 items from Codex PR #33 not yet verified as fixed in this PR:

  • Refresh _user_id on cached agents before memory sync (run_agent.py) — cross-user identity leak
  • Scope prefetch by user and store per-user (not single _prefetch_result) in memgw provider
  • Apply per-turn user_id inside Mem0 write filters (self._user_id vs. kwarg)

Recommend resolving the cursor[bot] TypeError before merging.


Generated by Claude Code

dizhaky commented Jun 28, 2026

Copy link
Copy Markdown
Owner Author

Codex P2 status update (automated triage — 2026-06-28)

All 15 Codex P2 findings from PR #37 (gateway/platforms/email.py) are now addressed.

PR #38 (fix(email): add <hr> to HTML body detection) covers the last outstanding item.

Summary of what was in each finding and its resolution

Finding Resolution
Preserve literal HTML snippets in plain replies _text_to_html() escapes < > & — non-HTML bodies are safe
Recognize <img>, <pre> standalone tags Fixed: commit 69871ee
Recognize <hr> standalone tag Fixed: PR #38
Preserve whitespace in generated HTML alternatives Fixed: white-space: pre-wrap; word-wrap: break-word
Strip HTML for the plain alternative Fixed: _strip_html() applied to HTML bodies
Avoid treating compact comparisons as anchor tags (x<a and a>0) Fixed: anchor requires href/name attribute
Avoid matching p, code, div comparisons as HTML Fixed: regex requires >, />, or attr= suffix
Preserve link targets in plain fallbacks Fixed: <a href="url">label</a>label (url)
Keep block boundaries in plain fallbacks Fixed: </p>, </li>, </tr>, etc. → newlines
Separate table cells in plain fallbacks (td/th) Fixed: </td><td> → tab
Remove style/script blocks from plain fallbacks Fixed: lines 172–179 strip <style> and <script> content
Keep image references in plain fallbacks Fixed: <img>[image: alt (url)]
Allow generated HTML to wrap long lines Fixed: pre-wrap; word-wrap: break-word

Codex usage limits

chatgpt-codex-connector[bot] has reached its usage limits as of PR #28/#19 — no new Codex reviews are being posted to any open PRs (#36, #35, NousResearch#259 in kb-daemon, #177 in mcp-servers). No unaddressed Codex inline comments exist on any currently open PR.


Generated by Claude Code

@dizhaky
dizhaky marked this pull request as ready for review June 28, 2026 02:31
@chatgpt-codex-connector

Copy link
Copy Markdown

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

@dizhaky
dizhaky enabled auto-merge (squash) June 28, 2026 02:32
@dizhaky

dizhaky commented Jun 28, 2026

Copy link
Copy Markdown
Owner Author

Closing in favor of a rebased branch on latest main. All the same changes, rebased onto bae095c (#37 merge).

@dizhaky dizhaky closed this Jun 28, 2026
auto-merge was automatically disabled June 28, 2026 02:41

Pull request was closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant