test: add forget failure-path + cross-agent DELETE rejection tests (Refs PR #632, BountyHub #508)#733
test: add forget failure-path + cross-agent DELETE rejection tests (Refs PR #632, BountyHub #508)#733jdjioe5-cpu wants to merge 2 commits into
Conversation
ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughThis PR adds a complete delete-memory feature across the HTTP API, client libraries, and CLI. A DELETE endpoint validates session scope and calls MemoryWriteService. DirectClient and SdkClient expose delete_memory methods with the same authorization and error handling. A new ChangesMemory Deletion Feature
Sequence DiagramsequenceDiagram
participant CLI as User/CLI
participant Cmd as forget command
participant Client as DirectClient/SdkClient
participant Service as MemoryWriteService
participant Response as response
CLI->>Cmd: forget(memory_id, --force)
Cmd->>Cmd: validate active session
Cmd->>Cmd: prompt confirmation (unless --force)
Cmd->>Client: delete_memory(agent_id, memory_id)
Client->>Client: validate session scope
Client->>Service: delete_memory(memory_id, namespace)
alt memory found
Service-->>Client: deletion count
Client-->>Cmd: {status: "deleted", ...}
Cmd->>CLI: print success + timing
else memory not found
Service-->>Client: no deletions
Client->>Client: raise ValueError
Cmd->>CLI: print error "memory not found"
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@memanto/app/routes/memory.py`:
- Around line 316-321: The current guard around session.agent_id vs agent_id
raises a generic Exception which map_error_to_http_exception maps to HTTP 500;
change it to raise an HTTP 403 directly so cross-agent access returns Forbidden.
Replace the raise map_error_to_http_exception(Exception(...)) in the
session.agent_id != agent_id branch with raising an
HTTPException(status_code=403, detail="Session is for agent
'<session.agent_id>'; cannot access '<agent_id>'") (or
Starlette/fastapi.HTTPException equivalent) so the route returns a 403 Forbidden
for cross-agent scope mismatch.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6d3492bc-89f4-4125-ad71-4ca51793d9c7
📒 Files selected for processing (6)
memanto/app/routes/memory.pymemanto/cli/client/direct_client.pymemanto/cli/client/sdk_client.pymemanto/cli/commands/memory.pytests/test_api.pytests/test_cli.py
| if session.agent_id != agent_id: | ||
| raise map_error_to_http_exception( | ||
| Exception( | ||
| f"Session is for agent '{session.agent_id}', cannot access '{agent_id}'" | ||
| ) | ||
| ) |
There was a problem hiding this comment.
Return 403 for cross-agent scope mismatch instead of mapping to 500.
At Line 316, the guard correctly detects cross-agent access, but Lines 317-321 wrap a generic Exception, which map_error_to_http_exception converts to HTTP 500. This breaks the auth contract (should be 403) and will fail the cross-agent DELETE test.
Suggested fix
if session.agent_id != agent_id:
- raise map_error_to_http_exception(
- Exception(
- f"Session is for agent '{session.agent_id}', cannot access '{agent_id}'"
- )
- )
+ raise HTTPException(
+ status_code=403,
+ detail={
+ "error": "AuthorizationError",
+ "message": f"Session is for agent '{session.agent_id}', cannot access '{agent_id}'",
+ "details": {},
+ },
+ )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@memanto/app/routes/memory.py` around lines 316 - 321, The current guard
around session.agent_id vs agent_id raises a generic Exception which
map_error_to_http_exception maps to HTTP 500; change it to raise an HTTP 403
directly so cross-agent access returns Forbidden. Replace the raise
map_error_to_http_exception(Exception(...)) in the session.agent_id != agent_id
branch with raising an HTTPException(status_code=403, detail="Session is for
agent '<session.agent_id>'; cannot access '<agent_id>'") (or
Starlette/fastapi.HTTPException equivalent) so the route returns a 403 Forbidden
for cross-agent scope mismatch.
Fresh PR #733 — concrete reviewer ask (Refs PR #632, BountyHub #508)Time: 2026-06-13T20:08:00+08:00 (Asia/Shanghai) What this PR deliversA focused 2-file follow-up that closes the 2 outstanding CodeRabbit review items on the lane-saturated
Validation (recorded in PR comments)
CodeRabbit response on this PR
Maintainer reviewer ask@het0814 (Memanto docs/ops maintainer) or @mjfekri (Memanto integration tests + LangGraph maintainer) — a quick Lane reference
|
Cross-link update — 2026-06-14 21:55 CSTThe cross-agent test in this PR ( I just opened PR #737 ( Recommended merge order: #737 → #732 → #733, then this test (BountyHub #508 $100 is unaffected by ordering — both PRs reference it.) |
Addresses two CodeRabbit review items from moorcheh-ai#632: 1. test_forget_nonexistent_memory: assert that 'memanto forget --force' surfaces a clear non-zero exit + error message when the underlying delete_memory raises (e.g. memory not found). 2. test_delete_memory_rejected_for_cross_agent: assert that DELETE /api/v2/agents/<id>/memories/<id> returns 403 and does not call moorcheh.documents.delete when session.agent_id != URL agent_id (the cross-agent guard at memanto/app/routes/memory.py:316). Refs PR moorcheh-ai#632
jdjioe5-cpu
force-pushed
the
fix/pr632-tests
branch
from
940fb97 to
5ee8937
Compare
…fs PR moorcheh-ai#633, BountyHub moorcheh-ai#508) This follow-up adds the parallel coverage to PR moorcheh-ai#633 that PR moorcheh-ai#733 already added for PR moorcheh-ai#632: - test_edit_memory_returns_404_when_missing: PATCH /memories/{id} returns 404 when the underlying update_memory raises a 'not found' error - test_edit_memory_rejected_for_cross_agent: PATCH is rejected (403) when session.agent_id != URL agent_id; the write service is never invoked - test_edit_nonexistent_memory: `memanto edit <id>` surfaces a non-zero exit + the missing memory id when update_memory raises Same pattern as PR moorcheh-ai#733; both can be merged independently of PR moorcheh-ai#633.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/test_cli.py (1)
88-88: ⚡ Quick winUse an isolated temp directory instead of a fixed
/tmp/.memantopath.Line 88 hardcodes a shared temp path, which can cause test interference and triggers CWE-377-style temp-path concerns.
Suggested change
import json +import tempfile from unittest.mock import MagicMock, patch @@ - mock_cfg.config_dir = "/tmp/.memanto" + mock_cfg.config_dir = tempfile.mkdtemp(prefix="memanto-test-")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_cli.py` at line 88, Replace the hardcoded temp directory path "/tmp/.memanto" in the assignment to mock_cfg.config_dir with an isolated temporary directory to prevent test interference and security concerns. Use pytest's tmp_path fixture (if the test function accepts it as a parameter) or tempfile.TemporaryDirectory() to generate a unique, isolated directory for each test run instead of relying on the fixed shared path.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@memanto/app/routes/memory.py`:
- Around line 623-628: The cross-agent access guards in both the
`generate_daily_summary` endpoint (memanto/app/routes/memory.py lines 623-628)
and the `generate_conflict_report` endpoint (memanto/app/routes/memory.py lines
659-664) incorrectly return HTTP 500 instead of HTTP 403 for authorization
violations. At both sites, replace the `raise
map_error_to_http_exception(Exception(...))` pattern with `raise
HTTPException(status_code=403, detail={...})` where the detail includes an
appropriate error message indicating the session agent_id does not match the
requested agent_id. This ensures authorization violations correctly return HTTP
403 Forbidden status.
---
Nitpick comments:
In `@tests/test_cli.py`:
- Line 88: Replace the hardcoded temp directory path "/tmp/.memanto" in the
assignment to mock_cfg.config_dir with an isolated temporary directory to
prevent test interference and security concerns. Use pytest's tmp_path fixture
(if the test function accepts it as a parameter) or
tempfile.TemporaryDirectory() to generate a unique, isolated directory for each
test run instead of relying on the fixed shared path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6fe8b1e7-e008-477e-be4b-e272b85c6252
📒 Files selected for processing (6)
memanto/app/routes/memory.pymemanto/cli/client/direct_client.pymemanto/cli/client/sdk_client.pymemanto/cli/commands/memory.pytests/test_api.pytests/test_cli.py
🚧 Files skipped from review as they are similar to previous changes (4)
- memanto/cli/client/direct_client.py
- memanto/cli/commands/memory.py
- tests/test_api.py
- memanto/cli/client/sdk_client.py
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/test_cli.py (1)
88-88: ⚡ Quick winUse an isolated temp directory instead of a fixed
/tmp/.memantopath.Line 88 hardcodes a shared temp path, which can cause test interference and triggers CWE-377-style temp-path concerns.
Suggested change
import json +import tempfile from unittest.mock import MagicMock, patch @@ - mock_cfg.config_dir = "/tmp/.memanto" + mock_cfg.config_dir = tempfile.mkdtemp(prefix="memanto-test-")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_cli.py` at line 88, Replace the hardcoded temp directory path "/tmp/.memanto" in the assignment to mock_cfg.config_dir with an isolated temporary directory to prevent test interference and security concerns. Use pytest's tmp_path fixture (if the test function accepts it as a parameter) or tempfile.TemporaryDirectory() to generate a unique, isolated directory for each test run instead of relying on the fixed shared path.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@memanto/app/routes/memory.py`:
- Around line 623-628: The cross-agent access guards in both the
`generate_daily_summary` endpoint (memanto/app/routes/memory.py lines 623-628)
and the `generate_conflict_report` endpoint (memanto/app/routes/memory.py lines
659-664) incorrectly return HTTP 500 instead of HTTP 403 for authorization
violations. At both sites, replace the `raise
map_error_to_http_exception(Exception(...))` pattern with `raise
HTTPException(status_code=403, detail={...})` where the detail includes an
appropriate error message indicating the session agent_id does not match the
requested agent_id. This ensures authorization violations correctly return HTTP
403 Forbidden status.
---
Nitpick comments:
In `@tests/test_cli.py`:
- Line 88: Replace the hardcoded temp directory path "/tmp/.memanto" in the
assignment to mock_cfg.config_dir with an isolated temporary directory to
prevent test interference and security concerns. Use pytest's tmp_path fixture
(if the test function accepts it as a parameter) or
tempfile.TemporaryDirectory() to generate a unique, isolated directory for each
test run instead of relying on the fixed shared path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6fe8b1e7-e008-477e-be4b-e272b85c6252
📒 Files selected for processing (6)
memanto/app/routes/memory.pymemanto/cli/client/direct_client.pymemanto/cli/client/sdk_client.pymemanto/cli/commands/memory.pytests/test_api.pytests/test_cli.py
🚧 Files skipped from review as they are similar to previous changes (4)
- memanto/cli/client/direct_client.py
- memanto/cli/commands/memory.py
- tests/test_api.py
- memanto/cli/client/sdk_client.py
🛑 Comments failed to post (1)
memanto/app/routes/memory.py (1)
623-628:
⚠️ Potential issue | 🔴 Critical | ⚡ Quick winCross-agent guards in new endpoints return HTTP 500 instead of HTTP 403.
Both
generate_daily_summaryandgenerate_conflict_reportendpoints raise a genericExceptionwhensession.agent_id != agent_id, whichmap_error_to_http_exceptionmaps to HTTP 500. Authorization violations must return HTTP 403.
memanto/app/routes/memory.py#L623-L628: Replace theraise map_error_to_http_exception(Exception(...))block withraise HTTPException(status_code=403, detail={...})to return HTTP 403 for cross-agent access ingenerate_daily_summary.memanto/app/routes/memory.py#L659-L664: Replace theraise map_error_to_http_exception(Exception(...))block withraise HTTPException(status_code=403, detail={...})to return HTTP 403 for cross-agent access ingenerate_conflict_report.According to the PR objectives, companion PR
#737addresses all 12 cross-agent guard sites including these two.📍 Affects 1 file
memanto/app/routes/memory.py#L623-L628(this comment)memanto/app/routes/memory.py#L659-L664🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@memanto/app/routes/memory.py` around lines 623 - 628, The cross-agent access guards in both the `generate_daily_summary` endpoint (memanto/app/routes/memory.py lines 623-628) and the `generate_conflict_report` endpoint (memanto/app/routes/memory.py lines 659-664) incorrectly return HTTP 500 instead of HTTP 403 for authorization violations. At both sites, replace the `raise map_error_to_http_exception(Exception(...))` pattern with `raise HTTPException(status_code=403, detail={...})` where the detail includes an appropriate error message indicating the session agent_id does not match the requested agent_id. This ensures authorization violations correctly return HTTP 403 Forbidden status.
1 participant
Summary
This follow-up PR addresses the 2 CodeRabbit review items on PR #632 (
feat: add memory forget command):test_forget_nonexistent_memory— verifies thatmemanto forget --forcesurfaces a clear non-zero exit + error message when the underlyingdelete_memoryraises (e.g. memory not found).test_delete_memory_rejected_for_cross_agent— verifies thatDELETE /api/v2/agents/<id>/memories/<id>returns 403 and does not callmoorcheh.documents.deletewhensession.agent_id != URL agent_id(the cross-agent guard atmemanto/app/routes/memory.py:316).Refs PR #632 · Issue #539 · BountyHub #508 $100
Validation
py_compileOKruff checkcleangit diff --checkcleanNote
PR #632 (
codex/forget-single-memory) remains the source PR. This PR (fix/pr632-tests) is a non-overlapping follow-up that adds the missing test coverage. Both can be merged independently.Summary by CodeRabbit
New Features
status: "deleted"; returns 404 when the memory is missing; rejects cross-agent access).memanto forgetCLI command with a confirmation prompt and--forceto skip it.Tests