feat(tools): trigger-phrase nudges in longterm_memory description#571
Open
qhkm wants to merge 3 commits into
Open
feat(tools): trigger-phrase nudges in longterm_memory description#571qhkm wants to merge 3 commits into
qhkm wants to merge 3 commits into
Conversation
Rewrite the longterm_memory tool description to enumerate concrete "Use when" / "Do NOT use when" triggers, mirroring the pattern used by Hermes Agent's memory_tool.py. The model re-reads tool descriptions on every turn, so naming the triggers turns memory persistence into a reliable per-turn check rather than something the model forgets to do. Adds a doc-test that asserts the trigger-phrase block is present so a future edit cannot silently strip it. Closes #569 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe LongTermMemoryTool description was expanded into a multi-line spec with explicit "Use when:" and "Do NOT use when:" trigger phrases, plus a tip to check categories; unit tests and two docs (AGENTS.md, CLAUDE.md) were updated to assert and document the new guidance. Long-Term Memory Tool Description Enhancement
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 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 docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 the current code and only fix it if needed.
Inline comments:
In `@src/tools/longterm_memory.rs`:
- Around line 409-428: The test test_tool_description_has_trigger_phrases is
using logical OR (||) for multi-phrase checks so removal of one canonical phrase
can pass silently; update the assertions that currently use "desc.contains(...)
|| desc.contains(...)" to require both phrases (use &&) or split them into two
separate assert! calls so each canonical trigger phrase is enforced individually
(refer to test_tool_description_has_trigger_phrases, temp_tool, and desc to
locate the checks).
🪄 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
Run ID: 48e99be8-d1a9-4550-9ea6-7d6dcf5f92ca
📒 Files selected for processing (1)
src/tools/longterm_memory.rs
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/tools/longterm_memory.rs (1)
420-427:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
||assertions still allow silent phrase removal — unresolved from the previous review.Lines 420-423 and 424-427 both use
||, so dropping all-but-one phrase per block still passes the test. The new three-way||on line 425 (AGENTS.md || CLAUDE.md || README) is actually weaker than the two-way form flagged before: any two of the three doc references can now be silently deleted without failing the guard.The stated PR goal is "future edits cannot remove it silently", which the current assertions do not achieve.
🛡️ Proposed fix: one assertion per canonical phrase
- assert!( - desc.contains("remember this") || desc.contains("don't do that again"), - "description should reference the canonical user-correction phrases", - ); - assert!( - desc.contains("AGENTS.md") || desc.contains("CLAUDE.md") || desc.contains("README"), - "description should warn against duplicating repo-level docs", - ); + assert!( + desc.contains("remember this"), + "description must contain 'remember this' trigger phrase", + ); + assert!( + desc.contains("don't do that again"), + "description must contain \"don't do that again\" trigger phrase", + ); + assert!( + desc.contains("AGENTS.md"), + "description must warn against duplicating AGENTS.md", + ); + assert!( + desc.contains("CLAUDE.md"), + "description must warn against duplicating CLAUDE.md", + ); + assert!( + desc.contains("README"), + "description must warn against duplicating README", + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools/longterm_memory.rs` around lines 420 - 427, The current compound assertions in longterm_memory.rs (the assert! calls that check desc.contains(...) for the correction phrases and for repo docs) allow silent removal of all-but-one phrase because they use ||; replace those compound checks with separate assert! statements for each canonical phrase so each required string is independently verified: assert that desc contains "remember this", assert that desc contains "don't do that again", and separately assert that desc contains "AGENTS.md", assert that desc contains "CLAUDE.md", and assert that desc contains "README" (reference the existing assert! usage and the local variable desc to locate where to change).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/tools/longterm_memory.rs`:
- Around line 420-427: The current compound assertions in longterm_memory.rs
(the assert! calls that check desc.contains(...) for the correction phrases and
for repo docs) allow silent removal of all-but-one phrase because they use ||;
replace those compound checks with separate assert! statements for each
canonical phrase so each required string is independently verified: assert that
desc contains "remember this", assert that desc contains "don't do that again",
and separately assert that desc contains "AGENTS.md", assert that desc contains
"CLAUDE.md", and assert that desc contains "README" (reference the existing
assert! usage and the local variable desc to locate where to change).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 48b45a9b-c42f-483c-8483-f8bb36ec98b5
📒 Files selected for processing (3)
AGENTS.mdCLAUDE.mdsrc/tools/longterm_memory.rs
✅ Files skipped from review due to trivial changes (2)
- AGENTS.md
- CLAUDE.md
…erts CodeRabbit caught that `||` chains in `test_tool_description_has_trigger_phrases` let any all-but-one canonical phrase be silently deleted while the test still passed. The whole point of the guard is to prevent silent regression of the trigger block, so each phrase now lives in its own `assert!` with a specific failure message: - "remember this" - "don't do that again" - "AGENTS.md" - "CLAUDE.md" - "README" No production code change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This was referenced May 9, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
longterm_memorytool'sdescription()to enumerate concrete "Use when" / "Do NOT use when" triggers, mirroring the pattern used by Hermes Agent'smemory_tool.py.test_tool_description_has_trigger_phrases) that guards the trigger block so future edits cannot silently strip it.This is Phase 1.5 of adopting Hermes Agent's "self-improving loop" pattern — the actual nudge mechanism in Hermes is not a background agent, it's strong trigger-phrase language in tool descriptions. The model re-reads tool descriptions on every turn, so naming the triggers turns memory persistence into a reliable per-turn check rather than something the model forgets to do.
Why
Today the description is functional but only tells the model how to call the tool, not when. The model only persists memories when explicitly told to. Adding concrete trigger phrases ("don't do that again", "remember this", "I always want X") aligns the description with the situations a user actually wants persisted, and the counter-triggers ("already in CLAUDE.md", "conversation-scoped") prevent over-persistence.
Test plan
cargo fmt -- --checkpassescargo clippy -- -D warningscleancargo nextest run --lib— 3501 tests passcargo test --doc— 128 doc tests passtest_tool_description_has_trigger_phrasesasserts the canonical phrases are presenttest_tool_descriptionstill passesReference
tools/memory_tool.pyline ~518 (research summarised in chat 2026-05-03)Closes #569
🤖 Generated with Claude Code
Summary by CodeRabbit