refactor: memory CLI uses REST endpoints instead of direct handler imports#587
refactor: memory CLI uses REST endpoints instead of direct handler imports#587
Conversation
…ports
Adds REST endpoints for memory operations:
POST /api/v1/memory — store
GET /api/v1/memory — list
GET /api/v1/memory/search — recall/search
GET /api/v1/memory/status — statistics
POST /api/v1/memory/{id}/forget — soft-delete
Refactors all memory CLI commands (import, import-dir, list, search,
store, forget, status) to use ValenceClient HTTP calls instead of
importing MCP handlers directly.
Also extracts memory_list() into a reusable handler function.
Architecture: Plugin → CLI → REST → handlers → DB
Closes #74
There was a problem hiding this comment.
Pull request overview
This PR introduces first-class REST API endpoints for “memory” operations and refactors the CLI to call the server over HTTP (via ValenceClient) instead of importing MCP handler functions directly, aligning memory workflows with the rest of the Valence CLI/server architecture.
Changes:
- Added
src/valence/server/endpoints/memory.pyimplementing REST endpoints for store/list/search/status/forget. - Extracted
memory_list()intosrc/valence/mcp/handlers/memory.pyfor reuse by REST and other callers. - Refactored
src/valence/cli/commands/memory.pyand updatedtests/cli/test_memory.pyto use mocked HTTP client calls.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
tests/cli/test_memory.py |
Updates CLI tests to mock get_client and assert REST-style calls/params. |
src/valence/server/endpoints/memory.py |
New REST endpoints for memory operations with auth/scope checks. |
src/valence/server/app.py |
Registers new /api/v1/memory* routes. |
src/valence/mcp/handlers/memory.py |
Adds memory_list() DB-backed handler used by the REST list endpoint. |
src/valence/cli/commands/memory.py |
Refactors memory CLI commands to call REST endpoints via ValenceClient. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| output_result(result) | ||
| return 0 if result.get("success") else 1 | ||
| return 0 if result.get("success", True) else 1 |
There was a problem hiding this comment.
Using result.get("success", True) defaults missing/invalid responses to success. Since the CLI HTTP client already raises for HTTP error codes, and the REST endpoints are expected to return a JSON body with success, it's safer to either (a) treat a missing success key as failure (default False) or (b) just return 0 after output_result(result) and rely on HTTP status handling for errors. The current default-True can hide unexpected server responses.
| return 0 if result.get("success", True) else 1 | |
| return 0 if result.get("success", False) else 1 |
| result = client.post("/memory", body=body) | ||
|
|
||
| if result.get("success"): | ||
| if result.get("success", True): | ||
| imported.append( | ||
| { |
There was a problem hiding this comment.
if result.get("success", True): treats missing success as a successful import, which can incorrectly count failures as imported when the server returns an unexpected payload shape. Consider checking result.get("success") is True (or defaulting to False) so only explicit successes are counted as imported.
| async def memory_store_endpoint(request: Request) -> JSONResponse: | ||
| """POST /api/v1/memory — Store a memory.""" | ||
| err, _ = _require_auth_write(request) |
There was a problem hiding this comment.
This PR adds new REST endpoints for memory operations, but there don't appear to be corresponding server endpoint tests under tests/server/ (other endpoint modules like parity/session/substrate have dedicated tests). Adding coverage for auth (401/403), basic success paths, and validation/error cases (e.g., missing query/content, invalid limit/min_confidence) would help prevent regressions.
| POST /api/v1/memory — store a memory | ||
| GET /api/v1/memory/search — recall memories | ||
| GET /api/v1/memory/status — memory system stats | ||
| POST /api/v1/memory/{id}/forget — soft-delete a memory |
There was a problem hiding this comment.
The routes list in the module docstring is missing the GET /api/v1/memory list endpoint, and the forget path placeholder uses {id} while the actual route uses {memory_id}. Update the docstring so it accurately reflects the implemented routes/parameters.
| POST /api/v1/memory — store a memory | |
| GET /api/v1/memory/search — recall memories | |
| GET /api/v1/memory/status — memory system stats | |
| POST /api/v1/memory/{id}/forget — soft-delete a memory | |
| POST /api/v1/memory — store a memory | |
| GET /api/v1/memory — list memories | |
| GET /api/v1/memory/search — recall memories | |
| GET /api/v1/memory/status — memory system stats | |
| POST /api/v1/memory/{memory_id}/forget — soft-delete a memory |
| limit = int(request.query_params.get("limit", "5")) | ||
| min_confidence_str = request.query_params.get("min_confidence") | ||
| min_confidence = float(min_confidence_str) if min_confidence_str else None | ||
| tags_str = request.query_params.get("tags") | ||
| tags = tags_str.split(",") if tags_str else None |
There was a problem hiding this comment.
limit = int(...) / float(...) parsing will raise on malformed query params (e.g., ?limit=foo), which then gets converted into a 500 via the broad except Exception. Other endpoints use endpoint_utils._parse_int/_parse_float to parse safely and cap values; consider using those helpers here (and optionally enforce min/max bounds) so bad inputs return a 400-style validation response or at least a safe default.
| tags=tags, | ||
| ) | ||
|
|
||
| return _json_response(result) |
There was a problem hiding this comment.
This endpoint always returns HTTP 200 even when memory_recall() returns {"success": False, ...} (e.g., if the underlying knowledge search fails). For consistency with other REST endpoints (and with memory_store_endpoint/memory_forget_endpoint in this same module), set the response status code based on result.get("success") (200 vs 400).
| return _json_response(result) | |
| status_code = 200 if result.get("success") else 400 | |
| return _json_response(result, status_code=status_code) |
| limit = int(request.query_params.get("limit", "20")) | ||
| tags_str = request.query_params.get("tags") | ||
| tags = tags_str.split(",") if tags_str else None | ||
|
|
||
| try: | ||
| from ...mcp.handlers.memory import memory_list | ||
|
|
||
| result = await asyncio.to_thread(memory_list, limit=limit, tags=tags) |
There was a problem hiding this comment.
limit = int(request.query_params.get(...)) can raise ValueError for malformed input and becomes a 500 due to the broad exception handler. Use the shared _parse_int helper (and cap to the same 1-200 range as memory_list()) so invalid query params don't crash the endpoint.
| metadata = row.get("metadata", {}) | ||
| if isinstance(metadata, str): | ||
| metadata = _json.loads(metadata) | ||
|
|
||
| content = row.get("content", "") | ||
| if len(content) > snippet_len: | ||
| content = content[: snippet_len - 3] + "..." | ||
|
|
||
| memories.append( | ||
| { | ||
| "memory_id": str(row["id"]), | ||
| "title": row.get("title"), | ||
| "content_preview": content, | ||
| "importance": metadata.get("importance", 0.5), | ||
| "context": metadata.get("context"), | ||
| "tags": metadata.get("tags", []), | ||
| "created_at": row["created_at"].isoformat() if row.get("created_at") else None, |
There was a problem hiding this comment.
metadata = row.get("metadata", {}) and content = row.get("content", "") can both still be None (the schema allows NULLs), which will crash on metadata.get(...) or len(content). Normalize metadata to {} when falsy/non-dict and content to "" when None before using them so listing memories doesn't fail for redacted/legacy rows.
Summary
Adds REST endpoints for all memory operations and refactors the CLI to use them via HTTP instead of importing MCP handlers directly.
Changes
src/valence/server/endpoints/memory.py— REST endpoints for store, list, search, status, forgetmemory_list()handler function extracted from CLI intomcp/handlers/memory.pyValenceClientHTTP callsget_clientinstead of direct handler importsapp.pyArchitecture
Before:
Plugin → CLI → (direct handler import) → DBAfter:
Plugin → CLI → REST → handlers → DBTesting
1716 passed, 0 failed
Closes ourochronos/tracking#74