feat(memory): memgw provider — Memory Gateway hybrid recall as default - #30
Conversation
New plugins/memory/memgw/ MemoryProvider that connects Hermes to the self-hosted Memory Gateway over Streamable-HTTP MCP: - memgw_recall / memgw_retain / memgw_reflect tools - background prefetch (recall|reflect), non-blocking sync_turn - on_delegation -> Experience capture, on_session_end summary - circuit breaker + threaded event loop (mirrors mem0/hindsight patterns) - cloud (Bearer) or local (keyless) mode - set as Hermes default memory.provider (degrades to built-in if unavailable) - 13 tests passing, ruff clean; provider discovered + loads via plugin system Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
🔎 Lint report:
|
| Rule | Count |
|---|---|
unresolved-import |
3 |
invalid-assignment |
1 |
First entries
tests/plugins/memory/test_memgw_provider.py:6: [unresolved-import] unresolved-import: Cannot resolve imported module `pytest`
tests/plugins/memory/test_memgw_provider.py:52: [invalid-assignment] invalid-assignment: Object of type `FakeClient` is not assignable to attribute `_client` of type `None | MemGatewayClient`
plugins/memory/memgw/client.py:63: [unresolved-import] unresolved-import: Cannot resolve imported module `mcp`
plugins/memory/memgw/client.py:64: [unresolved-import] unresolved-import: Cannot resolve imported module `mcp.client.streamable_http`
✅ Fixed issues: none
Unchanged: 4565 pre-existing issues carried over.
Diagnostics are surfaced as warnings — this check never fails the build.
![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: 01f182c28c
ℹ️ 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".
Codex review follow-up — 9 of 10 findings addressedI've implemented fixes for all P2 findings and the two actionable P1 findings in commit 8485df1513e084e7af9198f9feecb20511aeac21. Here's what was applied and what still needs a decision: Applied fixes
Still needs a decision: P1 — "Keep memgw out of bundled providers"Codex flagged that
This PR adds
I haven't removed the directory because doing so would delete the substance of this PR — that call should be yours. |
- is_available() now checks for the mcp package before returning True, preventing ModuleNotFoundError on activation when credentials are set but the extra was never installed (P2) - pin pip dep to mcp>=1.26.0,<2 in plugin.yaml, matching the audited version in pyproject.toml (P2) - clear DEFAULT_CONFIG memory.provider so fresh installs don't default to an external provider the user hasn't configured (P1) - add on_session_switch() to clear _prefetch_result on session rotation so stale prefetch from a prior session can't bleed into a new one (P2) - pass user_id in all gateway calls (recall/write/reflect) when running in a multi-user gateway context, enabling server-side user scoping (P1) - remove the blocking join on the previous sync thread from sync_turn() so the turn path is never delayed by a slow/hung prior sync (P2) - track delegation threads in _delegation_threads list and join them in shutdown() so on_delegation writes aren't silently killed on exit (P2) - raise RuntimeError in _unwrap() when isError=True so circuit-breaker records the failure and tool calls surface the error to the model (P2) - guard _ensure_loop() with _loop_lock to prevent concurrent first-calls from racing and leaking a loop thread (P2)
8485df1 to
2784469
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2784469504
ℹ️ 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".
|
Codex review: 3 P1 architectural issues need resolution before merge Codex raised 3 P1 findings that conflict with constraints documented in AGENTS.md. These aren't implementation bugs — they're structural decisions that need a policy call or architectural change before this PR can safely land. P1-A: AGENTS.md states the P1-B: AGENTS.md says plugins must not modify core files. Defaulting P1-C: Multi-user isolation is absent
P2 findings (agree with all, non-blocking discussion) Codex also found 9 P2s in the plugin implementation itself. Brief assessment:
These are all worth fixing in follow-up commits once the P1s are resolved. |
…P errors Addresses the 6 live findings on PR #30: - multi-user scoping: recall/write/reflect payloads now include user_id (_user_scope) so gateway sessions don't share one namespace - MCP client: _unwrap raises on CallToolResult.isError so tool-level failures don't reset the circuit breaker as successes - is_available checks the optional mcp dependency is installed - prefetch: monotonic generation guard so a slow older worker can't overwrite a newer result; on_session_switch clears + invalidates in-flight prefetch - sync writers tracked in a list; shutdown joins all (not just the last) so an in-flight retain isn't dropped - delegation threads tracked + reaped - doc: README explains the deliberate bundled-plugin exception (personal default for this fork; degrades to built-in when mcp/key absent) - +4 tests (scoping, session-switch invalidation, stale-prefetch guard) 17 memgw tests pass, ruff clean Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b4c78d4845
ℹ️ 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".
| def _user_scope(self) -> dict: | ||
| """Return user scoping metadata for multi-user gateway sessions.""" | ||
| if self._user_id: | ||
| return {'user_id': self._user_id} |
There was a problem hiding this comment.
Fresh evidence for the remaining scoping problem is that gateway thread sessions are shared across participants by default (thread_sessions_per_user=False) and the gateway reuses the cached AIAgent by that shared session_key; this provider stores user_id only during initialize(), so in a shared thread/group session the second participant's later recall/write calls still send the first participant's user_id. That corrupts Memory Gateway data and can surface one user's memories to another unless the scope is refreshed per turn/tool call or includes the shared chat/session identity instead of a stale constructor value.
Useful? React with 👍 / 👎.
| def _run_sync(self, coro: Any) -> Any: | ||
| loop = self._ensure_loop() | ||
| future = asyncio.run_coroutine_threadsafe(coro, loop) | ||
| return future.result(timeout=self._timeout + 5.0) |
There was a problem hiding this comment.
When the MCP endpoint hangs longer than this wait, future.result(...) raises in the caller but the coroutine remains scheduled on the shared loop because the future is never cancelled. In a gateway outage or stalled local server, repeated prefetch/sync/tool calls can leave pending HTTP sessions running in the background even after the provider records failures and opens the breaker; cancel the future on timeout before re-raising.
Useful? React with 👍 / 👎.
| url = cfg.get('api_url', '') | ||
| if cfg.get('api_key'): | ||
| return True | ||
| return 'localhost' in url or '127.0.0.1' in url |
There was a problem hiding this comment.
When MEMGW_API_URL has no key and merely contains the substring localhost or 127.0.0.1 in a remote hostname/userinfo/path (for example https://localhost@example.com/mcp), this treats it as trusted local mode and activates the provider without authentication. That can make Hermes start syncing conversation memory to a non-local endpoint the local-mode gate was supposed to reject; parse the URL and require the hostname to be exactly loopback/localhost.
Useful? React with 👍 / 👎.
… — Codex PR #30 review (#31) Two chatgpt-codex-connector[bot] review comments on PR #30 that still applied to current main (the other 13 were already addressed in the merged PR): 1. client.py #13 — Cancel timed-out MCP calls: _run_sync now cancels the concurrent.futures.Future on timeout before re-raising, so a stalled MCP endpoint doesn't leave a pending HTTP session running on the shared background loop after the caller has given up / opened the breaker. 2. __init__.py #15 — Parse the host before allowing keyless local mode: is_available() now urlparse's the URL and requires an exact loopback host (localhost/127.0.0.1/::1) instead of a substring match, so a URL like 'https://localhost@example.com/mcp' (host=example.com) is no longer trusted as local keyless mode. Adds regression tests: test_memgw_client_timeout.py (cancel-on-timeout fails against pre-fix client) + TestKeylessLocalModeHostParsing (3/4 fail pre-fix). Deferred (already fixed in merged PR #30): MCP dep import check, MCP dep pin, loop lock, isError handling, stale-prefetch generation, sync/delegation thread tracking + shutdown join, session-switch prefetch invalidation, no join on turn path, default provider unset. Deferred (complex, needs interface change): #3/#4 per-turn user_id refresh for shared gateway sessions — sync_turn/prefetch only receive session_id, not user_id, so threading identity through requires a MemoryProvider interface change; #1 in-tree placement is an architectural call (AGENTS.md says existing in-tree providers stay).
…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
|
Codex review follow-up (automated triage): Reviewed the P1 findings on this PR. Agree with all four:
These four P1s need follow-up issues/PRs. Items 3 & 4 are the most urgent for production safety if the gateway is deployed multi-user. Generated by Claude Code |
…odex P1) (#33) * fix(memgw): pass user_id per-call to prevent cross-user memory scoping 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 * fix(ty): add user_id kwarg to queue_prefetch/sync_turn overrides 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. * chore: regenerate uv.lock after pytest-timeout upper-bound pin 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. * fix(ty): add user_id keyword arg to retaindb overrides 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. * fix(ty): add user_id keyword arg to mem0 overrides 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. * fix(ty): add user_id keyword arg to agent overrides 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. --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
…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
2 participants
New
plugins/memory/memgw/MemoryProvider connecting Hermes to the self-hosted Memory Gateway (Neo4j+Qdrant+Notion) over its Streamable-HTTP MCP endpoint, set as the defaultmemory.provider(replacing the bundled hindsight provider as default; hindsight remains as a fallback option).Tools exposed to the model
memgw_recall— hybrid recall (semantic + keyword + graph fusion via RRF)memgw_retain— store a durable memorymemgw_reflect— synthesized beliefs (mental models)Auto behaviour
prefetch(recall|reflect) injected before each turnsync_turnon_delegation→ records subagent task+result as anexperienceon_session_end→ session summaryHardening (mirrors mem0/hindsight patterns)
is_available()returns False without a key in cloud mode → Hermes degrades to built-in memory, no hard dependencyVerification
tests/plugins/memory/test_memgw_provider.py), ruff clean🤖 Generated with Claude Code