fork-only: memory_persistence backward compatibility - #2
Conversation
📝 WalkthroughWalkthroughThe changes introduce support for legacy top-level memory_persistence configuration while encouraging migration to defaults.memory_persistence, add a new channel_recall tool to the prompt, refine memory_recall guidance for recent queries, and include unit tests validating the configuration behavior. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d255715fff
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| .memory_persistence | ||
| .or(toml.memory_persistence) |
There was a problem hiding this comment.
Merge legacy memory_persistence fields instead of whole table
Using Option::or here makes [defaults.memory_persistence] win as an all-or-nothing table, so when both locations are present and the new table is only partially populated, values from legacy [memory_persistence] are silently dropped and replaced by hardcoded defaults (for example, message_interval reverts to 50). This breaks backward-compatibility during incremental migrations; precedence should be applied per field (enabled/message_interval) rather than by selecting one entire table.
Useful? React with 👍 / 👎.
fork-only change.\n\nadds backward compatibility for top-level [memory_persistence] while keeping [defaults.memory_persistence] as preferred config location.\n\nno upstream PR for this.