Add Claude Code skills Memanto memory bridge example#618
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAdds a Memanto-backed skill memory bridge (models and backend protocol), two backends (Local JSONL and memanto CLI with timeouts), orchestration (begin/record/end), a runnable demo, tests (including CLI timeout conversion), and README documentation. ChangesClaude Code Skills Memory Bridge
Sequence Diagram(s) sequenceDiagram
participant Client
participant SkillMemoryBridge
participant MemoryBackend
Client->>SkillMemoryBridge: begin_skill(project, skill, prompt, cwd, files)
SkillMemoryBridge->>MemoryBackend: recall(query, limit)
MemoryBackend-->>SkillMemoryBridge: MemoryRecord[] (recalled)
Client->>SkillMemoryBridge: record_event(event)
Client->>SkillMemoryBridge: end_skill(summary)
SkillMemoryBridge->>MemoryBackend: remember(memory) (persist classified events + artifact)
MemoryBackend-->>SkillMemoryBridge: ack
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
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)
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 `@examples/claudecode-skills-memanto/skill_memory_bridge.py`:
- Around line 157-189: The subprocess calls in MemantoCliBackend.remember and
MemantoCliBackend.recall can hang indefinitely; update both methods to pass a
bounded timeout argument to subprocess.run (e.g., timeout=10 or a configurable
constant/ENV like MEMANTO_TIMEOUT) and handle subprocess.TimeoutExpired (catch
it, log an error via the module logger/processLogger and either re-raise a
custom exception or return a safe failure value). Ensure you add the timeout
parameter to the subprocess.run call sites in remember() and recall() and add a
small try/except around them to handle TimeoutExpired cleanly.
🪄 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: 46b94dc8-447b-4d58-9b57-b90cfa4e009b
📒 Files selected for processing (4)
examples/claudecode-skills-memanto/README.mdexamples/claudecode-skills-memanto/demo.pyexamples/claudecode-skills-memanto/skill_memory_bridge.pyexamples/claudecode-skills-memanto/test_skill_memory_bridge.py
There was a problem hiding this comment.
🧹 Nitpick comments (1)
examples/claudecode-skills-memanto/test_skill_memory_bridge.py (1)
96-106: 💤 Low valueAdd a matching test for
remembertimeouts (TimeoutExpired→TimeoutError)The current test covers the timeout-to-
TimeoutErrorconversion forrecall, butMemantoCliBackend.rememberuses the samesubprocess.TimeoutExpired→TimeoutErrorwrapping (message:memanto remember timed out after {timeout:.1f}s), so adding a parallel test would lock in both behaviors.🤖 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 `@examples/claudecode-skills-memanto/test_skill_memory_bridge.py` around lines 96 - 106, Add a new test mirroring test_memanto_cli_backend_converts_timeout but targeting MemantoCliBackend.remember: monkeypatch subprocess.run to a fake_run that raises subprocess.TimeoutExpired(command, kwargs["timeout"]), instantiate MemantoCliBackend(timeout_seconds=1.0), call backend.remember(...) and assert it raises TimeoutError with a match for the remember-specific message (e.g. "memanto remember timed out after 1.0s" or an appropriate regex to match the formatted timeout); reference MemantoCliBackend.remember and the existing test_memanto_cli_backend_converts_timeout for structure.
🤖 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.
Nitpick comments:
In `@examples/claudecode-skills-memanto/test_skill_memory_bridge.py`:
- Around line 96-106: Add a new test mirroring
test_memanto_cli_backend_converts_timeout but targeting
MemantoCliBackend.remember: monkeypatch subprocess.run to a fake_run that raises
subprocess.TimeoutExpired(command, kwargs["timeout"]), instantiate
MemantoCliBackend(timeout_seconds=1.0), call backend.remember(...) and assert it
raises TimeoutError with a match for the remember-specific message (e.g.
"memanto remember timed out after 1.0s" or an appropriate regex to match the
formatted timeout); reference MemantoCliBackend.remember and the existing
test_memanto_cli_backend_converts_timeout for structure.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 70805170-9b82-43d4-94db-7ca3f146d06c
📒 Files selected for processing (2)
examples/claudecode-skills-memanto/skill_memory_bridge.pyexamples/claudecode-skills-memanto/test_skill_memory_bridge.py
🚧 Files skipped from review as they are similar to previous changes (1)
- examples/claudecode-skills-memanto/skill_memory_bridge.py
|
Addressed the CodeRabbit timeout finding in commit cb6fea4:
Validation after the change:
|
|
Added the matching remember-timeout coverage requested in the latest CodeRabbit nitpick in commit 4d700a9. Validation after the change: \python -m pytest examples\claudecode-skills-memanto -q\ (6 passed) and \python -m ruff check examples\claudecode-skills-memanto\ (all checks passed). |
|
Follow-up pushed to strengthen the review signal after the automated docstring note. The new example Python files now have docstrings on all function/class definitions in the added skill bridge/demo/tests by a local AST count (39/39, 100%). Validation run locally:
Latest head: 8d57ed3 |
|
We were blown away by the community's creativity and the sheer volume of high-quality submissions! After reviewing all the pull requests against the bounty's success matrix, we have decided to move forward with merging PR #692, which implemented a highly portable prompt-injection architecture via CLAUDE.md. We are closing this PR because it falls into one of the architectural approaches that we ultimately decided against for the ecosystem:
We deeply appreciate the time and engineering effort you put into this submission. The codebase was fantastic to review, and we hope to see you in future Moorcheh bounties! |
2 participants
Summary
Bounty
Addresses #508.
BountyHub: https://www.bountyhub.dev/bounty/view/3a63800c-7c18-41d2-870f-c62344f8a3fe
Showcase: https://gist.github.com/auruminternum/abee535665f19ea8dccf4fabddf781e4
Validation
/claim #508
Summary by CodeRabbit
New Features
Documentation
Tests