feat(memory): add runtime-gated conversation memory substrate - #1149
Conversation
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRefactors storage factory to return a Changes
Sequence DiagramsequenceDiagram
participant Client as HTTP Request
participant Server as Route Handler
participant Middleware as Storage Middleware
participant MemoryCtx as MemoryExecutionContext
participant App as AppContext
participant Writer as ConversationMemoryWriter
Client->>Server: request + headers
Server->>Middleware: pass HeaderMap
Middleware->>MemoryCtx: build_memory_execution_context(config, headers)
MemoryCtx->>MemoryCtx: parse policy header (trim, ci)
MemoryCtx->>MemoryCtx: determine store/recall state (Active/GatedOff/NotRequested)
Middleware-->>Server: MemoryExecutionContext
Server->>App: access AppContext (includes optional writer)
App-->>Server: conversation_memory_writer (Option)
Server->>Server: process items with memory context and optional writer
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
github-actions
Bot
added
data-connector
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Clean scaffolding PR that threads memory plumbing through the system with proper runtime gating. The StorageTuple → StorageBundle upgrade, validation in AppContextBuilder, and header-driven MemoryExecutionContext are all well-structured.
Summary: 0 🔴 Important · 3 🟡 Nit · 0 🟣 Pre-existing
Nits:
- Split
impl Policyblocks inmemory/context.rs— consolidate into one - Doc comment after
#[derive]inmemory.rs— move before derives per convention refresh_memory_execution_contextispubwith no callers — consider annotating as scaffolding
![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 `@model_gateway/src/routers/conversations/handlers.rs`:
- Around line 328-331: The code constructs a MemoryExecutionContext via
middleware::build_memory_execution_context but immediately drops it, so
header-derived memory headers are ignored; modify the call sites so the returned
MemoryExecutionContext is threaded into the downstream ingestion flow (e.g.,
pass it into create_conversation_items_with_headers or attach it to the
request/context object that is handed to the ingestion pipeline) and persist or
clone it as needed so create_conversation_items_with_headers can read and act on
the x-smg-ltm-memory-* headers; ensure all places that call
middleware::build_memory_execution_context (and the
create_conversation_items_with_headers signature) are updated to accept and
propagate the MemoryExecutionContext.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: bcf16ed0-a079-439a-9e92-271028d57a24
📒 Files selected for processing (18)
crates/data_connector/src/factory.rscrates/data_connector/src/lib.rscrates/data_connector/src/memory.rscrates/data_connector/src/noop.rsmodel_gateway/src/app_context.rsmodel_gateway/src/config/builder.rsmodel_gateway/src/config/types.rsmodel_gateway/src/config/validation.rsmodel_gateway/src/lib.rsmodel_gateway/src/memory/context.rsmodel_gateway/src/memory/mod.rsmodel_gateway/src/middleware.rsmodel_gateway/src/routers/common/header_utils.rsmodel_gateway/src/routers/conversations/handlers.rsmodel_gateway/src/routers/openai/context.rsmodel_gateway/src/routers/openai/router.rsmodel_gateway/src/server.rsmodel_gateway/src/service_discovery.rs
💤 Files with no reviewable changes (1)
- model_gateway/src/config/validation.rs
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
model_gateway/src/routers/conversations/handlers.rs (1)
319-325:⚠️ Potential issue | 🟠 MajorHeader-derived memory context is still a no-op here.
Line 403 makes
MemoryExecutionContextintentionally unused, so/v1/conversations/{conversation_id}/itemsstill ignores the newx-smg-ltm-memory-*headers. Either trigger the memory store/recall behavior from this ingestion path, or defer the header-aware API until the context actually affects processing.Also applies to: 350-357, 398-404
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@model_gateway/src/routers/conversations/handlers.rs` around lines 319 - 325, The handler create_conversation_items_with_headers accepts a MemoryExecutionContext but currently ignores it (marked unused) so the x-smg-ltm-memory-* headers have no effect; update the ingestion path to apply the memory context by invoking the existing memory recall/store APIs inside create_conversation_items_with_headers (and the related handlers referenced around the same area) before persisting items: extract the relevant flags/fields from MemoryExecutionContext, call the memory recall function to retrieve any preloaded memory to include in item processing, and after item creation call the memory store function when the context indicates storing is required, making sure to use the MemoryExecutionContext type and existing memory store/recall method names to tie header-derived behavior to the item ingestion flow rather than leaving the parameter unused.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/data_connector/src/memory.rs`:
- Around line 282-307: Add a unit test that verifies the happy path for
MemoryConversationMemoryWriter: construct a MemoryConversationMemoryWriter, call
create_memory with a minimal NewConversationMemory, assert the returned
ConversationMemoryId starts with "mem_" and that the writer's inner store
contains the inserted NewConversationMemory under that id (access inner via
MemoryConversationMemoryWriter.inner read lock). Use the create_memory async
method (await it) and check cloning/lookup via id.clone(); reference
MemoryConversationMemoryWriter, create_memory, ConversationMemoryId,
NewConversationMemory, and inner in the test.
---
Duplicate comments:
In `@model_gateway/src/routers/conversations/handlers.rs`:
- Around line 319-325: The handler create_conversation_items_with_headers
accepts a MemoryExecutionContext but currently ignores it (marked unused) so the
x-smg-ltm-memory-* headers have no effect; update the ingestion path to apply
the memory context by invoking the existing memory recall/store APIs inside
create_conversation_items_with_headers (and the related handlers referenced
around the same area) before persisting items: extract the relevant flags/fields
from MemoryExecutionContext, call the memory recall function to retrieve any
preloaded memory to include in item processing, and after item creation call the
memory store function when the context indicates storing is required, making
sure to use the MemoryExecutionContext type and existing memory store/recall
method names to tie header-derived behavior to the item ingestion flow rather
than leaving the parameter unused.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: e32d1935-0e66-471b-9d3a-f2146e3c4e8b
📒 Files selected for processing (5)
crates/data_connector/src/memory.rsmodel_gateway/src/memory/context.rsmodel_gateway/src/routers/conversations/handlers.rsmodel_gateway/src/routers/openai/context.rsmodel_gateway/src/server.rs
56a9503 to
7b6bc6a
Compare
7b6bc6a to
a0e48e7
Compare
a0e48e7 to
7926d37
Compare
Signed-off-by: Daisy Zhou <zhoug9127@gmail.com>
Signed-off-by: Daisy Zhou <zhoug9127@gmail.com>
Signed-off-by: Daisy Zhou <zhoug9127@gmail.com>
7926d37 to
6768b71
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/data_connector/src/factory.rs`:
- Around line 249-254: Update the factory tests that call create_storage and
unpack the StorageBundle (where they currently destructure
bundle.response_storage, bundle.conversation_storage,
bundle.conversation_item_storage) to also assert the new
conversation_memory_writer contract: verify that when HistoryBackend::Memory is
configured the returned bundle.conversation_memory_writer is Some(writer) and
when HistoryBackend::None is configured the field is None (to enforce the
startup precheck), and ensure any hook-wrapped bundles preserve that writer
presence/absence rather than dropping or populating it incorrectly.
In `@model_gateway/src/memory/context.rs`:
- Around line 25-31: The code currently warns when headers.policy is present but
unrecognized even if it is blank/whitespace; update the logic around
headers.policy and Policy::Unspecified so that you first trim raw_policy (e.g.,
let trimmed = raw_policy.trim()) and if trimmed.is_empty() treat it as
unspecified with no warn!, and only call warn! when trimmed is non-empty and the
parsed policy still matched Policy::Unspecified; ensure you continue to pass the
original raw value (or the trimmed value) into the log for context when warning.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d1f08e88-d128-4ec6-8061-b99644e5f83b
📒 Files selected for processing (6)
crates/data_connector/src/factory.rscrates/data_connector/src/lib.rscrates/data_connector/src/memory.rsmodel_gateway/src/app_context.rsmodel_gateway/src/memory/context.rsmodel_gateway/src/routers/conversations/handlers.rs
slin1237
left a comment
slin1237
left a comment
There was a problem hiding this comment.
Thanks for putting this together, Daisy! The shape of the design (header → context → request pipeline → storage handle) is solid, and the StorageTuple → StorageBundle refactor is a nice cleanup.
Left a few small inline suggestions focused on type design and consistency with patterns the codebase already uses — none are blockers, just foundation tightening that's cheaper to do now than after consumer code lands. The MemoryExecutionContext enum suggestion is probably the highest-leverage one.
One bigger-picture note that's worth a sentence in the PR description: the memory_runtime flag has no user-facing way to be enabled today (no CLI flag, no Python binding, no config-file path). If staged rollout is intentional, calling that out would help reviewers calibrate. If not, wiring the CLI flag here would close the loop.
+1 to CodeRabbit's still-open observation about _memory_execution_context being unused at handlers.rs:402 — happy to defer until the consumer lands, but a // TODO(memory): wire into ingestion comment would keep it from getting lost.
Great scaffolding work, looking forward to seeing the consumer wire up!
| pub subject_id: Option<String>, | ||
| pub embedding_model: Option<String>, | ||
| pub extraction_model: Option<String>, | ||
| } |
There was a problem hiding this comment.
Nice job gating store/recall through here. One small refactor that would tighten the type: the (requested, active) pairs encode 4 representable states but only 3 are valid — (requested=false, active=true) is impossible, but the type allows it because all fields are pub.
An enum like LtmStoreState::{NotRequested, GatedOff, Active} would let the type system enforce what your constructor enforces today, and would let downstream consumers match on intent instead of remembering to read _active (not _requested) at the right spot. Small thing, but it'll prevent a future correctness bug as the consumer lands.
There was a problem hiding this comment.
Thanks! I followed the pattern LtmStoreState::{NotRequested, GatedOff, Active} and replaced the old pairs.
Please check: model_gateway/src/memory/context.rs line 14 with latest commit.
|
|
||
| fn disables_ltm(self) -> bool { | ||
| matches!(self, Self::None) | ||
| } |
There was a problem hiding this comment.
Small observation: Policy::None and Policy::Unspecified produce identical (store_requested=false, recall_requested=false) results in the constructor above, so disables_ltm() and the else branch are currently equivalent. Either collapse the two variants, or make None observably different (e.g. a tracing::info! on explicit opt-out, or surfacing it in metrics) — worth pinning down before two variants drift.
There was a problem hiding this comment.
That's true - I didn't collapse the two variants since semantically they are not identical.
Fixed through the new MemoryPolicyMode from last comment now the mode is:
- none -> MemoryPolicyMode::ExplicitNone (memory policy design allowed this "none" as a conversation privacy mode)
- missing header -> MemoryPolicyMode::Unspecified (header not present at all, behaves as no explicit memory request)
- invalid/blank present value -> MemoryPolicyMode::Unrecognized (just invalid value..)
| pub conversation_storage: Arc<dyn ConversationStorage>, | ||
| pub conversation_item_storage: Arc<dyn ConversationItemStorage>, | ||
| pub conversation_memory_writer: Option<Arc<dyn ConversationMemoryWriter>>, | ||
| } |
There was a problem hiding this comment.
Tiny pattern alignment — the rest of this file uses NoOp* impls so callers can call unconditionally (NoOpResponseStorage, NoOpConversationStorage, etc). A NoOpConversationMemoryWriter would let this field be Arc<dyn ConversationMemoryWriter> (required) instead of Option<...>, and would remove the if let Some(writer) branches that'll otherwise show up in every consumer downstream. Same cost now, less friction later.
There was a problem hiding this comment.
Hi Simo I have implemented per your suggestion. Added NoOpConversationMemoryWriter and moved writer plumbing to required Arc in the core path, so callers can invoke unconditionally without Option branching.
This keeps behavior unchanged (NoOp on unsupported backends) while reducing downstream friction.
| &router_config, | ||
| self.conversation_memory_writer.is_some(), | ||
| ) | ||
| .map_err(AppContextBuildError::InvalidConfig)?; |
There was a problem hiding this comment.
Heads-up: validate_memory_writer_configuration is also called at line 397 in from_config, but with a different available signal — backend_supports_memory_writer(...) (static allow-list) vs self.conversation_memory_writer.is_some() (dynamic Arc check). They can disagree, and a caller using the builder directly (not via from_config) only sees this dynamic branch and skips the backend allow-list check. Consolidating to one call at one lifecycle point would be safer and easier to reason about.
There was a problem hiding this comment.
Fix this by consolidating to a single call in build now, see model_gateway/src/app_context.rs line 321. Inside validate_memory_writer_configuration at app_context.rs line 691, both signals are now checked in one place.
| } | ||
|
|
||
| /// Configure memory runtime feature flags for store/recall behavior. | ||
| pub fn memory_runtime_config(mut self, config: MemoryRuntimeConfig) -> Self { |
There was a problem hiding this comment.
Heads-up — this builder method is currently the only way to flip memory_runtime.enabled: main.rs::to_router_config doesn't call it, and bindings/python/src/lib.rs::to_router_config doesn't either, so the flag isn't reachable from the CLI or Python today. If staged rollout is the intent, a sentence in the PR description would set reviewer expectations. If not, adding a --memory-runtime-enabled CLI flag (and the matching Python binding) would close the loop in the same PR.
There was a problem hiding this comment.
Good catch!. All similar TODO comment issues mentioned above have been fixed. Also updated the PR description.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fc4860a7f9
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
…ector Both have been the consistent primary authors of recent PRs in these subsystems: - @zhoug9127 (Daisy): #1168, #1149, #1065, #1061, #976 — mcp + data_connector - @zhaowenzi (Ziwen): #1174, #1163, #1123 — mcp Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
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 `@crates/data_connector/src/memory.rs`:
- Around line 277-308: Update the file header "Structure:" comment to reflect
the new PART 3 entry for MemoryConversationMemoryWriter and renumber subsequent
parts (e.g., MemoryResponseStorage now PART 4); locate the top-of-file structure
comment and insert an entry referencing MemoryConversationMemoryWriter (and
adjust part numbers for MemoryResponseStorage and any following PART labels) so
the comment matches the actual section order in the file.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2639f38a-db57-4b95-bcb4-62915aedabfc
📒 Files selected for processing (11)
crates/data_connector/src/factory.rscrates/data_connector/src/memory.rsmodel_gateway/src/app_context.rsmodel_gateway/src/config/builder.rsmodel_gateway/src/config/types.rsmodel_gateway/src/memory/context.rsmodel_gateway/src/routers/common/header_utils.rsmodel_gateway/src/routers/conversations/handlers.rsmodel_gateway/src/routers/openai/context.rsmodel_gateway/src/server.rsmodel_gateway/src/service_discovery.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c9e162d478
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
c9e162d to
f24d8ed
Compare
f24d8ed to
fa3a18b
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fa3a18be42
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Signed-off-by: Daisy Zhou <zhoug9127@gmail.com>
…ory-substrate Signed-off-by: Daisy Zhou <zhoug9127@gmail.com>
Signed-off-by: Daisy Zhou <zhoug9127@gmail.com>
…ory-substrate Signed-off-by: Daisy Zhou <zhoug9127@gmail.com>
Signed-off-by: Daisy Zhou <zhoug9127@gmail.com>
b000f54 to
b173500
Compare
2 participants
Description
Problem
SMG needed safe, runtime-gated memory substrate plumbing so memory intent can be parsed from request headers and carried through request handling without changing existing behavior.
We also needed startup-time guardrails to fail fast on invalid runtime/backend/hook combinations instead of failing later during execution.
Solution
This PR adds memory substrate plumbing behind a runtime gate (
memory_runtime.enabled, defaultfalse) and keeps default behavior unchanged.MemoryRuntimeConfigonRouterConfig(enabled: bool, default off).memorymodule withMemoryExecutionContextand execution state modeling:NotRequestedGatedOffActivex-conversation-memory-config(JSON), including:long_term_memory.enabledlong_term_memory.policylong_term_memory.subject_idlong_term_memory.embedding_model_idlong_term_memory.extraction_model_idshort_term_memory.*)store_only,store_and_recall,recall_only, andnonewith safe fallback for unrecognized values.create_storagenow returnsStorageBundle(instead of tuple), includingconversation_memory_writer.HistoryBackend::MemoryprovidesMemoryConversationMemoryWriter.NoOpConversationMemoryWriter.AppContextBuilderto reject unsupported combinations:memory_runtime.enabled=truewith backends that do not provide a real memory writer.memory_runtime.enabled=truewithstorage_hook_wasm_path(currently unsupported).create_conversation_items_with_headers(...)and server wiring to passMemoryExecutionContext(reserved for follow-up ingestion logic).Scope Notes
memory_runtime.enabled=false).Changes by Area
data-connectorStorageBundleandbackend_supports_memory_writer(...).conversation_memory_writerthrough storage factory outputs.MemoryConversationMemoryWriterforHistoryBackend::Memory.NoOpConversationMemoryWriterfor non-memory backends.model_gatewaymemorymodule (MemoryExecutionContext).header_utils.AppContextBuilder.Test Plan
Reproducible validation run locally:
Results:
Checklist
cargo +nightly fmtpassescargo clippy --all-targets --all-features -- -D warningspassesSummary by CodeRabbit