Conversation
github-actions
Bot
added
scope: agent
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a significant enhancement to the agent's compaction mechanism by enabling the automatic extraction of structured memories. During the compaction process, the system now leverages an LLM to identify and extract high-confidence durable memories from archived conversation turns, subsequently appending these to MEMORY.md. This feature aims to improve the agent's long-term memory and understanding by systematically capturing key information, while also ensuring efficiency through deduplication and best-effort processing. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces an excellent new feature for automatically extracting structured memories during context compaction. The implementation is well-structured, covering the LLM interaction for extraction, deduplication against existing memories, and updating the workspace. The addition of tests for the new functionality is also great. I've identified a few areas for improvement: a bug in the memory deduplication logic that could allow duplicates, an opportunity to enhance debuggability by logging JSON parsing errors from the LLM, and a small refactoring to make one of the compaction functions more concise.
zmanian
left a comment
zmanian
left a comment
There was a problem hiding this comment.
Review: Structured Memory Extraction at Compaction
Overall this is a clean, well-scoped first pass. The best-effort pattern is solid, the LLM prompt is well-constrained, and the test coverage is good. A few items worth addressing before merge:
Dedup logic (Gemini flag) -- addressed
The second commit correctly fixes the dedup to extract content from - **kind**: content lines before comparison. The current version using split_once("**: ") is correct.
Issues to address
1. extract_json_object is fragile with nested braces
The extract_json_object function uses text.find('{') / text.rfind('}') which works for the simple schema here, but will silently produce garbage if the LLM wraps the response in markdown fences containing an explanation with braces before/after the JSON. Since the LLM is asked to return "ONLY JSON", this is low risk, but a more robust approach would be to find the outermost balanced braces, or at minimum add a serde_json::from_str validation inside extract_json_object before returning. Not a blocker, but worth a comment or a follow-up.
2. Dedup reads the entire MEMORY.md on every candidate memory
structured_memory_exists calls workspace.memory().await for each candidate memory in the loop (up to 5 times). This re-reads MEMORY.md from the database on every call. Should read once before the loop and pass the content in. Something like:
let memory_content = workspace.memory().await.map(|d| d.content).unwrap_or_default();
for memory in parsed.memories.into_iter().take(MEMORY_EXTRACTION_MAX_CANDIDATES) {
// ...
if memory_already_exists(&memory_content, &memory.content) {
continue;
}
// ...
}This is a real performance issue -- 5 DB reads when 1 suffices.
3. kind field is not validated
The kind field from the LLM is used directly as a string in the formatted output (- **{kind}**: ...). If the LLM hallucinates a kind like "personal_secret" or returns garbage, it gets written verbatim. Consider validating against the known enum variants (preference | identity | project | plan | fact) and skipping unknown kinds, or at least documenting this as a known limitation for the follow-up.
4. Substring dedup can produce false positives
The dedup uses bidirectional contains():
existing.contains(&normalized_query) || normalized_query.contains(&existing)A memory like "User uses Rust" would match an existing "User uses Rust and Python for backend work" which is correct (subsumption). But it also means "User prefers dark mode" would match an existing line containing "User prefers dark mode for IDE but light mode for docs" -- the new shorter memory would be incorrectly suppressed even though it's semantically different (broader). This is acceptable for a first pass but worth noting for the follow-up pruning/compaction work.
Looks good
- Best-effort error handling: all failure paths log warnings and return 0, never blocking compaction.
- No
.unwrap()/.expect()in production code. - Temperature 0.0 and max_tokens 512 are sensible for structured extraction.
- Confidence threshold at 0.75 is reasonable.
#[serde(default)]on evidence field handles LLMs that omit optional fields.- Tests cover both extraction and dedup paths with real workspace instances.
- The Gemini bot feedback has been addressed in the second commit (dedup content extraction, compact_truncate refactoring, serde error logging).
Verdict
Request changes for item #2 (reading MEMORY.md N times in a loop). The rest are minor suggestions or follow-up items. With that fix, this is good to merge.
zmanian
left a comment
zmanian
left a comment
There was a problem hiding this comment.
Requesting changes for the repeated MEMORY.md reads in the dedup loop (see detailed review comment). The rest is solid work.
|
Thank you for pushing structured durable-memory extraction forward.\n\nWe are closing this legacy implementation because Reborn has separated compaction from memory lifecycle ownership. Compaction owns transcript-window reduction; memory providers own interaction recording and any extraction or distillation policy. The active Reborn memory-lifecycle work in #5327 records completed-turn interactions and explicitly defers durable on-run-end summaries to Phase 3.\n\nReferences:\n- https://github.com/nearai/ironclaw/pull/5327\n- https://github.com/nearai/ironclaw/blob/main/crates/ironclaw_agent_loop/src/strategies/compaction.rs\n- https://github.com/nearai/ironclaw/blob/main/crates/ironclaw_memory/src/service.rs\n- https://github.com/nearai/ironclaw/blob/main/crates/ironclaw_memory_native/src/service.rs\n\nThe valuable extraction idea should continue as a provider-owned Phase 3 follow-up to #5327, not as a side effect inside the legacy compaction path. We would be very happy to have you contribute to that Reborn work, with this PR retained as context and attribution. |
3 participants
Implements the first slice of #1474 by extracting high-confidence structured memories during compaction and appending them to MEMORY.md.
What changed:
Validation:
This is intentionally the minimal first pass for #1474; follow-up work can layer compaction/pruning polish on top.