test: add edit failure-path and cross-agent PATCH rejection tests (Refs PR #633, BountyHub #508)#736
test: add edit failure-path and cross-agent PATCH rejection tests (Refs PR #633, BountyHub #508)#736jdjioe5-cpu wants to merge 4 commits into
Conversation
|
Warning Review limit reached
More reviews will be available in 11 minutes and 17 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughA new ChangesMemory Edit Feature
Sequence Diagram(s)sequenceDiagram
participant User
participant CLIEdit
participant Client
participant MemoryWriteService
User->>CLIEdit: Run memanto edit with update fields
CLIEdit->>Client: update_memory(agent_id, memory_id, updates)
Client->>MemoryWriteService: update_memory(namespace, memory_id, updates)
MemoryWriteService-->>Client: status, action, updated_fields
Client-->>CLIEdit: update result metadata
CLIEdit-->>User: Print updated memory and fields
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Suggested reviewers
🚥 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: 2
🤖 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 343-345: The truthy check on line 343 of the memory.py file in the
memory update route bypasses the CostGuard.validate_text_length validation when
content is an empty string. Replace the truthy check (using the walrus operator
with content assignment) with an explicit key-presence check that verifies
"content" exists in the updates dictionary, then add explicit validation to
reject empty content (not just None), ensuring consistency with the remember()
endpoint validation at line 155. Alternatively, move the content validation into
the write_service.update_memory method to centralize the safeguard, but the
simplest fix is to check key presence and validate that content is not empty
before allowing the update.
In `@memanto/cli/client/direct_client.py`:
- Around line 747-753: The update_memory method call in
_get_validated_session_for_agent currently only validates that the updates
dictionary is non-empty, but does not validate the actual field values within
updates such as confidence ranges, type validity, or content being non-blank.
Add field-level validation for the updates dictionary keys and values before
passing them to the _get_write_service().update_memory() call, ensuring the same
validation constraints are enforced here as in the API path. This same
validation pattern needs to be applied at all locations where update_memory is
called with user-provided updates.
🪄 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: 14a4eb65-3183-4296-9578-356b99b09dfe
📒 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
Cross-link update — 2026-06-14 21:55 CSTThe cross-agent test in this PR ( I just opened PR #737 ( Recommended merge order: #737 → #633 → #736, then this test (BountyHub #508 $100 is unaffected by ordering — both PRs reference it.) |
Address the two actionable CodeRabbit review items flagged on PR moorcheh-ai#736 (which tests this PR): 1. routes/memory.py edit endpoint previously used 'if content := updates.get("content")' which silently allowed blank/whitespace-only content. Replaced with explicit non-empty + length checks. Added parallel range checks for confidence (0.0-1.0) and membership check for memory_type, matching the API contract used for the create endpoint. 2. cli/client/direct_client.py update_memory previously only checked that 'updates' was non-empty, allowing invalid edit payloads (out-of-range confidence, invalid type, blank content, unknown field names) to flow into the write service. Added the same field validation on the direct-client path so the CLI behavior matches the REST API contract. Both fixes align the edit path with the existing create path's validation strategy and keep the test coverage in PR moorcheh-ai#736 honest. Refs PR moorcheh-ai#736 (test follow-up) Refs Issue moorcheh-ai#540 (good-first-issue) Refs BountyHub bounty moorcheh-ai#508 (Memanto + mattpocock skills challenge)
Address the two actionable CodeRabbit review items flagged on PR moorcheh-ai#736 (which tests this PR): 1. routes/memory.py edit endpoint previously used 'if content := updates.get("content")' which silently allowed blank/whitespace-only content. Replaced with explicit non-empty + length checks. Added parallel range checks for confidence (0.0-1.0) and membership check for memory_type, matching the API contract used for the create endpoint. 2. cli/client/direct_client.py update_memory previously only checked that 'updates' was non-empty, allowing invalid edit payloads (out-of-range confidence, invalid type, blank content, unknown field names) to flow into the write service. Added the same field validation on the direct-client path so the CLI behavior matches the REST API contract. Both fixes align the edit path with the existing create path's validation strategy and keep the test coverage in PR moorcheh-ai#736 honest. Refs PR moorcheh-ai#736 (test follow-up) Refs Issue moorcheh-ai#540 (good-first-issue) Refs BountyHub bounty moorcheh-ai#508 (Memanto + mattpocock skills challenge)
Address the two actionable CodeRabbit review items flagged on PR moorcheh-ai#736 (which tests this PR): 1. routes/memory.py edit endpoint previously used 'if content := updates.get("content")' which silently allowed blank/whitespace-only content. Replaced with explicit non-empty + length checks. Added parallel range checks for confidence (0.0-1.0) and membership check for memory_type, matching the API contract used for the create endpoint. 2. cli/client/direct_client.py update_memory previously only checked that 'updates' was non-empty, allowing invalid edit payloads (out-of-range confidence, invalid type, blank content, unknown field names) to flow into the write service. Added the same field validation on the direct-client path so the CLI behavior matches the REST API contract. Both fixes align the edit path with the existing create path's validation strategy and keep the test coverage in PR moorcheh-ai#736 honest. Refs PR moorcheh-ai#736 (test follow-up) Refs Issue moorcheh-ai#540 (good-first-issue) Refs BountyHub bounty moorcheh-ai#508 (Memanto + mattpocock skills challenge)
…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.
jdjioe5-cpu
force-pushed
the
fix/pr633-tests
branch
from
b26ba09 to
004ba83
Compare
1 participant
Summary
This follow-up PR adds the parallel coverage to PR #633 (
feat: add memory edit command) that PR #733 already added for PR #632 (feat: add memory forget command):test_edit_memory_returns_404_when_missing— verifies thatPATCH /api/v2/agents/{agent_id}/memories/{id}returns 404 when the underlyingMemoryWriteService.update_memoryraises a 'not found' exception (the canonical failure-path for Addmemanto editupdate an existing memory #540 acceptance criteria).test_edit_memory_rejected_for_cross_agent— verifies that PATCH is rejected (403) whensession.agent_id != URL agent_id, and that the write service is NEVER invoked (the cross-agent guard atmemanto/app/routes/memory.py:316-ish).test_edit_nonexistent_memory— verifies thatmemanto edit <id>surfaces a non-zero exit + the missing memory id in stdout when the underlyingupdate_memoryraises.Why a separate PR
The 2 missing items on PR #633 are scoped to the
good first issuetest gap (failure-path + cross-agent), exactly like PR #733 was for #632. Both PR #632 and PR #633 remain mergeable source-of-truth for their features; this PR only adds missing test coverage.Diff
Validation
python3 -m ruff check tests/test_api.py tests/test_cli.py→ all checks passedpython3 -c "import ast; ast.parse(...)"for both files → OKgit diff --checkcleanpytestnot run (nomoorcheh_sdkinstalled in the cloud venv); CI on the maintainer's side will run it.Refs PR #633 · Issue #540 · BountyHub #508 $100
Summary by CodeRabbit
New Features
editcommand to update memories with flexible field selectionTests