feat: Memory using mempalace#121
Conversation
|
@jrusz Added as WIP but it is fully functional in case you wanna give a try to compare. |
adrianriobo
changed the title
jrusz
left a comment
jrusz
left a comment
There was a problem hiding this comment.
Overall the integration approach is solid and follows claudio's patterns well. Renovate tracking, opt-in gating via env var, and hook placement all look good.
One thing I couldn't comment on inline: conf/.mempalace/config.json is empty (0 bytes). If mempalace expects valid JSON, this could cause parse errors — should it be {} at minimum?
jrusz
left a comment
jrusz
left a comment
There was a problem hiding this comment.
Added some mostly nitpick comments, should be fine as is though
adrianriobo
force-pushed
the
fix-46
branch
2 times, most recently
from
8f6c762 to
ebabad3
Compare
|
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 (8)
✅ Files skipped from review due to trivial changes (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughIntegrates MemPalace as an opt-in persistent memory plugin into the Claudio container image. The Containerfile installs the pinned MemPalace package, registers and configures its hooks, and fixes a pyopenssl CVE. Claude agent configuration gains MCP permissions and a memory-protocol context file. Renovate tracks the MemPalace PyPI version. README and a new CLAUDE.md document the full feature. ChangesMemPalace Memory Integration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 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: 5
🤖 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 `@CLAUDE.md`:
- Line 71: Update the version string in CLAUDE.md to match the Makefile VERSION
value; specifically change the current line containing "Current version:
`0.6.0-dev`" to reflect `0.6.1-dev` (or alternatively update the Makefile
VERSION to `0.6.0-dev` if the Makefile is wrong) so the documented version and
the VERSION variable are consistent.
- Line 16: Update the documentation entry referencing the environment variable
for starting MemPalace: replace the incorrect `MEMPAL_DIR` with the actual
variable `MEMPAL_ENABLED` (or vice-versa if you prefer to change the
implementation) so docs and code match; check `entrypoint.sh`, README.md, and
CLAUDE.md for any other occurrences of `MEMPAL_DIR` and ensure they consistently
use `MEMPAL_ENABLED` (or update the code to use `MEMPAL_DIR`) and keep the
descriptive text about starting MemPalace unchanged.
In `@conf/.claude/context.d/CLAUDE-memory.md`:
- Around line 12-14: The fenced code block containing the ToolSearch call lacks
a language identifier (MD040); update the triple-backtick fence that wraps the
line starting with ToolSearch(...) to include a language tag such as text (e.g.,
```text) so markdownlint passes; ensure the opening fence immediately preceding
the ToolSearch(...) line is changed and the closing fence remains intact.
- Around line 7-20: The startup steps are contradictory: the text says run steps
1–3 but then instructs to call “both tools in parallel” while three tool calls
are listed (ToolSearch, mempalace_status, mempalace_diary_read, and
mempalace_search). Fix by making the language consistent: replace the “both
tools in parallel” phrase with “all three tools in parallel” (or explicitly
state which two if only two are intended) and ensure ordering: run
ToolSearch(query: ...) first, then call mempalace_status and
mempalace_diary_read (agent_name: "session-hook", last_n: 5) in parallel and
call mempalace_search(query: "<topic>") as required before answering about past
work; update the documentation text to reference the exact tool names
(ToolSearch, mempalace_status, mempalace_diary_read, mempalace_search) so there
is no ambiguity.
In `@Containerfile`:
- Around line 99-105: The Dockerfile sets ENV MEMPAL_SAVE_INTERVAL to 5 but the
sed replacement in the mempal_save_hook.sh uses a different fallback
(${MEMPAL_SAVE_INTERVAL:-15}), causing inconsistent defaults; update the sed
command to use the same default (change ${MEMPAL_SAVE_INTERVAL:-15} to
${MEMPAL_SAVE_INTERVAL:-5}) or alternatively change ENV MEMPAL_SAVE_INTERVAL to
15 so both places match; modify the sed invocation that targets SAVE_INTERVAL in
${HOME}/.claude/plugins/marketplaces/mempalace/hooks/mempal_save_hook.sh so its
fallback aligns with the ENV value.
🪄 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: f16d28e5-7c33-4f17-94a5-3d279790f8f2
📒 Files selected for processing (9)
CLAUDE.mdContainerfileMakefileREADME.mdconf/.claude/context.d/CLAUDE-base.mdconf/.claude/context.d/CLAUDE-memory.mdconf/.claude/settings.jsonconf/.mempalace/config.jsonrenovate.json
| | File | Purpose | | ||
| |---|---| | ||
| | `Containerfile` | Multi-stage OCI image build (preparer → final) | | ||
| | `entrypoint.sh` | Container startup: configures git identity, SSH signing key, gcloud auth, starts MemPalace if `MEMPAL_DIR` is set | |
There was a problem hiding this comment.
Inconsistent variable name in documentation.
Line 16 mentions MEMPAL_DIR but the actual variable used throughout README.md and the implementation is MEMPAL_ENABLED. This could confuse users.
Proposed fix
-| `entrypoint.sh` | Container startup: configures git identity, SSH signing key, gcloud auth, starts MemPalace if `MEMPAL_DIR` is set |
+| `entrypoint.sh` | Container startup: configures git identity, SSH signing key, gcloud auth, starts MemPalace if `MEMPAL_ENABLED` is set |📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| | `entrypoint.sh` | Container startup: configures git identity, SSH signing key, gcloud auth, starts MemPalace if `MEMPAL_DIR` is set | | |
| | `entrypoint.sh` | Container startup: configures git identity, SSH signing key, gcloud auth, starts MemPalace if `MEMPAL_ENABLED` is set | |
🤖 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 `@CLAUDE.md` at line 16, Update the documentation entry referencing the
environment variable for starting MemPalace: replace the incorrect `MEMPAL_DIR`
with the actual variable `MEMPAL_ENABLED` (or vice-versa if you prefer to change
the implementation) so docs and code match; check `entrypoint.sh`, README.md,
and CLAUDE.md for any other occurrences of `MEMPAL_DIR` and ensure they
consistently use `MEMPAL_ENABLED` (or update the code to use `MEMPAL_DIR`) and
keep the descriptive text about starting MemPalace unchanged.
| 2. Tag the commit: `make tag` (creates `v<VERSION>` git tag) | ||
| 3. Push the tag — the `build.yml` workflow publishes to Quay and GHCR | ||
|
|
||
| Current version: `0.6.0-dev` |
There was a problem hiding this comment.
Version mismatch with Makefile.
The documented version 0.6.0-dev doesn't match the VERSION in Makefile which is 0.6.1-dev.
Proposed fix
-Current version: `0.6.0-dev`
+Current version: `0.6.1-dev`📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Current version: `0.6.0-dev` | |
| Current version: `0.6.1-dev` |
🤖 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 `@CLAUDE.md` at line 71, Update the version string in CLAUDE.md to match the
Makefile VERSION value; specifically change the current line containing "Current
version: `0.6.0-dev`" to reflect `0.6.1-dev` (or alternatively update the
Makefile VERSION to `0.6.0-dev` if the Makefile is wrong) so the documented
version and the VERSION variable are consistent.
| ``` | ||
| ToolSearch(query: "select:mcp__plugin_mempalace_mempalace__mempalace_status,mcp__plugin_mempalace_mempalace__mempalace_diary_read") | ||
| ``` |
There was a problem hiding this comment.
Add a language identifier to the fenced code block (MD040).
This currently violates markdownlint and can fail docs quality gates.
Suggested fix
-```
+```text
ToolSearch(query: "select:mcp__plugin_mempalace_mempalace__mempalace_status,mcp__plugin_mempalace_mempalace__mempalace_diary_read")</details>
<details>
<summary>🧰 Tools</summary>
<details>
<summary>🪛 markdownlint-cli2 (0.22.1)</summary>
[warning] 12-12: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
</details>
</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @conf/.claude/context.d/CLAUDE-memory.md around lines 12 - 14, The fenced
code block containing the ToolSearch call lacks a language identifier (MD040);
update the triple-backtick fence that wraps the line starting with
ToolSearch(...) to include a language tag such as text (e.g., ```text) so
markdownlint passes; ensure the opening fence immediately preceding the
ToolSearch(...) line is changed and the closing fence remains intact.
</details>
<!-- fingerprinting:phantom:triton:hawk -->
<!-- d98c2f50 -->
<!-- This is an auto-generated comment by CodeRabbit -->
| ENV MEMPAL_SAVE_INTERVAL 5 | ||
| RUN pip install --no-cache-dir MemPalace==${MEMPALACE_V}; \ | ||
| claude plugin marketplace add MemPalace/mempalace@v${MEMPALACE_V};\ | ||
| claude plugin install --scope user mempalace; \ | ||
| grep -q '^SAVE_INTERVAL=[0-9]' ${HOME}/.claude/plugins/marketplaces/mempalace/hooks/mempal_save_hook.sh || \ | ||
| { echo 'ERROR: SAVE_INTERVAL pattern not found in hook script'; exit 1; }; \ | ||
| sed -i 's/^SAVE_INTERVAL=[0-9]\+/SAVE_INTERVAL="${MEMPAL_SAVE_INTERVAL:-15}"/' ${HOME}/.claude/plugins/marketplaces/mempalace/hooks/mempal_save_hook.sh; |
There was a problem hiding this comment.
Inconsistent default values for MEMPAL_SAVE_INTERVAL.
The ENV MEMPAL_SAVE_INTERVAL 5 sets the container default to 5, but the sed replacement on line 105 uses ${MEMPAL_SAVE_INTERVAL:-15} as a fallback. While the ENV ensures the runtime default is effectively 5, the fallback of 15 in the script is confusing and inconsistent with the documented default.
Consider aligning the fallback value:
Proposed fix
- sed -i 's/^SAVE_INTERVAL=[0-9]\+/SAVE_INTERVAL="${MEMPAL_SAVE_INTERVAL:-15}"/' ${HOME}/.claude/plugins/marketplaces/mempalace/hooks/mempal_save_hook.sh;
+ sed -i 's/^SAVE_INTERVAL=[0-9]\+/SAVE_INTERVAL="${MEMPAL_SAVE_INTERVAL:-5}"/' ${HOME}/.claude/plugins/marketplaces/mempalace/hooks/mempal_save_hook.sh;🤖 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 `@Containerfile` around lines 99 - 105, The Dockerfile sets ENV
MEMPAL_SAVE_INTERVAL to 5 but the sed replacement in the mempal_save_hook.sh
uses a different fallback (${MEMPAL_SAVE_INTERVAL:-15}), causing inconsistent
defaults; update the sed command to use the same default (change
${MEMPAL_SAVE_INTERVAL:-15} to ${MEMPAL_SAVE_INTERVAL:-5}) or alternatively
change ENV MEMPAL_SAVE_INTERVAL to 15 so both places match; modify the sed
invocation that targets SAVE_INTERVAL in
${HOME}/.claude/plugins/marketplaces/mempalace/hooks/mempal_save_hook.sh so its
fallback aligns with the ENV value.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Claudio <claudio@aipcc-cicd.com>
2 participants
This commit introduces a memory framework for claude, this allow claudio to extend its usage beyond a single session
Fixes #46
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores