fix(memory): tighten persistence rules and add conversational events#522
Open
fix(memory): tighten persistence rules and add conversational events#522
Conversation
Contributor
WalkthroughThis PR extends the memory persistence system by introducing two new event types ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/tools/memory_persistence_complete.rs (1)
203-205: Consider logging when an unrecognized event type falls back toSystem.When
WorkingMemoryEventType::parse()returnsNone, the code silently falls back toSystem. This could mask LLM prompt drift or schema mismatches. A debug/trace log would aid troubleshooting without impacting normal operation.🔍 Optional: Add trace-level logging for unrecognized event types
- let event_type = - crate::memory::WorkingMemoryEventType::parse(&event_input.event_type) - .unwrap_or(crate::memory::WorkingMemoryEventType::System); + let event_type = match crate::memory::WorkingMemoryEventType::parse(&event_input.event_type) { + Some(et) => et, + None => { + tracing::trace!( + raw_event_type = %event_input.event_type, + "unrecognized event_type, falling back to System" + ); + crate::memory::WorkingMemoryEventType::System + } + };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools/memory_persistence_complete.rs` around lines 203 - 205, The code silently falls back to WorkingMemoryEventType::System when WorkingMemoryEventType::parse(&event_input.event_type) returns None; add a trace- or debug-level log in that branch to record the original event_input.event_type and that the System fallback was used. Modify the logic around the parse call that sets event_type (referencing WorkingMemoryEventType::parse, event_input.event_type, and WorkingMemoryEventType::System) to detect the None case and emit a trace/debug message (e.g., using tracing::trace! or log::debug!) before assigning System so unrecognized event types are visible during troubleshooting.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/tools/memory_persistence_complete.rs`:
- Around line 203-205: The code silently falls back to
WorkingMemoryEventType::System when
WorkingMemoryEventType::parse(&event_input.event_type) returns None; add a
trace- or debug-level log in that branch to record the original
event_input.event_type and that the System fallback was used. Modify the logic
around the parse call that sets event_type (referencing
WorkingMemoryEventType::parse, event_input.event_type, and
WorkingMemoryEventType::System) to detect the None case and emit a trace/debug
message (e.g., using tracing::trace! or log::debug!) before assigning System so
unrecognized event types are visible during troubleshooting.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6792852a-4058-476f-b3b6-d0df5f911645
📒 Files selected for processing (6)
prompts/en/memory_persistence.md.j2prompts/en/tools/memory_save_description.md.j2src/config/types.rssrc/llm/model.rssrc/memory/working.rssrc/tools/memory_persistence_complete.rs
![tembo[bot]](https://avatars.githubusercontent.com/in/1223475?s=60&v=4){kind=small}
vsumner
force-pushed
the
codex/memory-observation-p1
branch
from
f47982a to
9850d4d
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
prompts/en/memory_persistence.md.j2 (1)
38-41: Clarify where normalized timestamps should be written.Since events currently have no explicit timestamp field, specify that the absolute timestamp should be embedded in
summarywhen time matters.Proposed wording tweak
- - Normalize relative time references to absolute dates/times with timezone + - Normalize relative time references to absolute dates/times with timezone (for example `2026-03-31T14:20:00-04:00`) so downstream memory checks are stable across sessions. + - If a timestamp is relevant, include it directly in `summary` (there is no separate event_time field).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@prompts/en/memory_persistence.md.j2` around lines 38 - 41, The prompt should explicitly state that when normalizing relative times to absolute timestamps, the generated absolute timestamp must be embedded into the event's summary field (i.e., write the ISO-8601 timestamp into the existing summary string) because events lack an explicit timestamp property; update the text in the memory_persistence.md.j2 prompt to instruct authors to append or include the normalized timestamp in `summary` (e.g., "…include normalized time as ISO-8601 in the `summary` field") and give a brief example format to use.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@prompts/en/memory_persistence.md.j2`:
- Around line 38-41: The prompt should explicitly state that when normalizing
relative times to absolute timestamps, the generated absolute timestamp must be
embedded into the event's summary field (i.e., write the ISO-8601 timestamp into
the existing summary string) because events lack an explicit timestamp property;
update the text in the memory_persistence.md.j2 prompt to instruct authors to
append or include the normalized timestamp in `summary` (e.g., "…include
normalized time as ISO-8601 in the `summary` field") and give a brief example
format to use.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 19c2aa77-5e14-423d-9c14-ea844e5294fc
📒 Files selected for processing (5)
prompts/en/memory_persistence.md.j2prompts/en/tools/memory_save_description.md.j2src/config/types.rssrc/memory/working.rssrc/tools/memory_persistence_complete.rs
✅ Files skipped from review due to trivial changes (1)
- src/config/types.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- prompts/en/tools/memory_save_description.md.j2
- src/memory/working.rs
- src/tools/memory_persistence_complete.rs
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.
Why
This is the first packet of the memory-evolution work. The immediate goal is to tighten what the persistence branch is allowed to save and to start recording the most important conversational corrections explicitly.
Before this change, the persistence prompt still allowed too much derivable or noisy state into durable memory, and the working-memory event surface did not have first-class types for user corrections or revised decisions. That made the next observation-memory work harder because the system was still losing important conversational pivots while retaining lower-value chatter.
Scope
This PR is intentionally narrow.
It includes prompt-discipline changes plus the first conversational working-memory event semantics. It does not yet expand the broader event vocabulary or improve working-memory rendering end to end.
What Changed
user_correctionanddecision_revisedas working-memory event types and allowedmemory_persistence_completeto persist themclippycleanup insrc/llm/model.rsbecause repo gates were red on this branch otherwiseTesting
just preflightjust gate-prcargo test -p spacebot persists_conversational_events -- --test-threads=1 --nocapturecargo test -p spacebot test_event_type_roundtrip -- --test-threads=1 --nocaptureNotes
src/llm/model.rschange is not part of the memory packet itself. It is the smallest repo-level lint fix needed to getjust gate-prgreen.Note
Memory persistence now enforces stricter guard rails on what gets saved durably. The core changes include introducing
user_correctionanddecision_revisedevent types to capture conversational inflection points, normalizing relative dates to absolute timestamps for cache stability, and filtering out derivable/transient data (repo state, git history, ephemeral task output). These constraints ensure memory captures semantically valuable pivots rather than noise. TheMemoryPersistenceConfigdocs now clarify that active persistence thresholds are configured viaWorkingMemoryConfigrather than the legacy branch cadence. Updated tests verify both new event types round-trip correctly through the persistence pipeline.Written by Tembo for commit af64604