Skip to content

fix: resolve working memory key via search index when user_id/namespace omitted#239

Open
bsbodden wants to merge 1 commit intomainfrom
bsb/fix-235
Open

fix: resolve working memory key via search index when user_id/namespace omitted#239
bsbodden wants to merge 1 commit intomainfrom
bsb/fix-235

Conversation

@bsbodden
Copy link
Collaborator

Closes #235

Summary

  • PUT /v1/working-memory/{session_id} accepts user_id and namespace in the request body, embedding them into the Redis key (e.g. working_memory:demo:alice:session-1)
  • GET and DELETE take those values as query params, which default to None when omitted, producing a different key (working_memory:session-1) — resulting in a 404
  • This adds an index-based fallback: when a direct key lookup returns nothing, the existing working memory search index is queried by session_id to resolve the actual Redis key
  • Multi-tenant isolation is preserved (namespace/user_id remain in keys); the fallback only fires when the direct lookup misses

Changes

  • working_memory.py: Add _resolve_working_memory_key_via_index() helper; wire fallback into get_working_memory() and delete_working_memory(); return stored namespace/user_id from the JSON document instead of caller-supplied params
  • mcp.py: Add missing user_id and namespace parameters to the MCP get_working_memory tool
  • tests/test_issue_235.py: 9 tests covering key resolution, API round-trips, delete without params, multi-tenant isolation, and non-existent session handling

Test plan

  • test_get_resolves_via_index_when_params_omitted — PUT with scoping params, GET without → 200
  • test_get_with_partial_params_resolves — GET with namespace only still resolves
  • test_get_with_correct_params_still_uses_fast_path — direct lookup still works when params match
  • test_delete_resolves_via_index_when_params_omitted — DELETE without params removes the correct session
  • test_api_put_body_params_get_without_query_params — full HTTP round-trip
  • test_api_delete_without_query_params — full HTTP DELETE round-trip
  • test_multi_tenant_isolation_preserved — same session_id in different namespaces stays separate
  • test_nonexistent_session_still_returns_none — no false positives
  • Full test suite: 781 passed, 105 skipped, 0 failures

Copilot AI review requested due to automatic review settings March 21, 2026 00:16
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Review completed. Found one potential issue with multi-session ambiguity handling.


🤖 Automated review complete. Please react with 👍 or 👎 on the individual review comments to provide feedback on their usefulness.

@jit-ci
Copy link

jit-ci bot commented Mar 21, 2026

🛡️ Jit Security Scan Results

CRITICAL HIGH MEDIUM

✅ No security findings were detected in this PR


Security scan by Jit

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Fixes issue #235 where GET/DELETE /v1/working-memory/{session_id} could 404 after a successful scoped PUT because user_id/namespace were provided in the PUT body but omitted from GET/DELETE query params. The PR adds an index-based fallback to resolve the correct Redis key by session_id.

Changes:

  • Add _resolve_working_memory_key_via_index() and wire it into get_working_memory() and delete_working_memory() as a fallback when the direct key lookup misses.
  • Return stored namespace/user_id from the persisted JSON document (instead of echoing caller-supplied params).
  • Extend the MCP get_working_memory tool to accept optional user_id/namespace, and add regression tests for the bug and related scenarios.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File Description
agent_memory_server/working_memory.py Adds index-based key resolution fallback for GET/DELETE when scoping params are omitted.
agent_memory_server/mcp.py Adds optional user_id and namespace parameters to the MCP get_working_memory tool.
tests/test_issue_235.py Introduces a dedicated regression test suite for key resolution behavior and API round-trips.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

…ce omitted (#235)

PUT /v1/working-memory/{session_id} stores user_id and namespace from
the request body into the Redis key, but GET/DELETE construct the key
from query params. When callers omit those params, a different key is
produced and the session appears missing (404).

Add an index-based fallback: when a direct key lookup returns nothing,
query the working memory search index by session_id to resolve the
actual Redis key. This preserves multi-tenant isolation (namespace +
user_id remain in keys) while making GET/DELETE work without requiring
callers to repeat scoping parameters.

Changes:
- Add _resolve_working_memory_key_via_index() helper in working_memory.py
- Use fallback in get_working_memory() and delete_working_memory()
- Return stored namespace/user_id from JSON data instead of caller params
- Add user_id/namespace params to MCP get_working_memory tool
- Add comprehensive tests covering the fix and multi-tenant isolation
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.

GET /v1/working-memory/{session_id} returns 404 after successful PUT when user_id/namespace are in body

2 participants