Repository navigation
feat(memory): add conversation memory header contract no-op hook - #1134
Conversation
github-actions
Bot
added
model-gateway
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 46 minutes and 0 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe pull request introduces conversation memory configuration support by adding header extraction infrastructure, configuration data structures, and integration into the response routing pipeline. A new Changes
Sequence DiagramsequenceDiagram
participant Client
participant route_responses
participant header_utils
participant inject_memory
participant request_body
Client->>route_responses: HTTP Request + x-conversation-memory-config Header
route_responses->>header_utils: extract_conversation_memory_config(headers)
header_utils->>header_utils: Parse JSON, Normalize Fields
header_utils-->>route_responses: ConversationMemoryConfig
route_responses->>route_responses: Emit Debug Log (if memory enabled)
route_responses->>inject_memory: inject_memory_context(config, request_body)
inject_memory->>inject_memory: Emit Debug Log (if memory enabled)
inject_memory-->>route_responses: (no mutations)
route_responses->>request_body: Continue Processing
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
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)
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 |
|
Hi @spalimpaaces-star, the DCO sign-off check has failed. All commits must include a To fix existing commits: # Sign off the last N commits (replace N with the number of unsigned commits)
git rebase HEAD~N --signoff
git push --force-with-leaseTo sign off future commits automatically:
|
![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 the infrastructure for conversation memory configuration by adding a new ConversationMemoryConfig struct and a utility to extract this configuration from the x-conversation-memory-config JSON header. It also includes a stub for injecting memory context into OpenAI response requests and provides comprehensive unit tests for the new functionality. Feedback was provided regarding a discrepancy in the documentation for ConversationMemoryConfig, which incorrectly mentions support for legacy headers that are not currently handled by the extraction logic.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/common/header_utils.rs`:
- Around line 20-21: The comment claims population from legacy x-smg-memory-*
headers but the implementation only reads x-conversation-memory-config JSON;
either implement legacy header mapping or tighten the doc: if you implement
mapping, update the header parsing logic that currently reads the
`x-conversation-memory-config` JSON to also look for `x-smg-memory-*` flat
headers (e.g., `x-smg-memory-type`, `x-smg-memory-enabled`, etc.),
convert/normalize those values into the same MemoryConfig struct fields, merge
them with the JSON payload with JSON taking precedence, and add unit tests
exercising both JSON-only, legacy-only and mixed inputs; otherwise simply change
the comment above the population function to state that only
`x-conversation-memory-config` JSON is supported and remove the legacy header
claim.
In `@model_gateway/src/routers/openai/responses/history.rs`:
- Line 320: The test currently only verifies the enum variant with
assert!(matches!(request.input, ResponseInput::Text(_))) but doesn't ensure the
inner string wasn't mutated; update the assertion to extract the inner text and
compare it to the original expected value. For example, pattern-match or use if
let / match on request.input (ResponseInput::Text(inner)) and assert_eq!(inner,
expected_text) (referencing request.input and the ResponseInput::Text variant
and the expected_text/original_input variable) to guarantee the payload is
unchanged.
- Around line 276-283: The debug log in history.rs currently logs the raw
subject_id (in the debug! call), which risks identifier leakage; change the
debug! invocation to avoid printing the raw value by logging presence/state
instead (e.g., use config.ltm_subject_id.is_some() or map to a constant like
"<redacted>" or "<none>" via as_deref().map(|_| "<redacted>") ), leaving the
other fields (ltm_embedding_model_id, ltm_extraction_model_id) unchanged and
keep the same message "LTM recall requested — retrieval not yet implemented".
🪄 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: c6697e8d-e0a6-4f10-963b-a5a45f74b169
📒 Files selected for processing (3)
model_gateway/src/routers/common/header_utils.rsmodel_gateway/src/routers/openai/responses/history.rsmodel_gateway/src/routers/openai/responses/route.rs
936ad3d to
fe1425d
Compare
fe1425d to
18dce25
Compare
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/common/header_utils.rs`:
- Around line 61-75: The extract_conversation_memory_config function currently
maps JSON fields straight into ConversationMemoryConfig, preserving empty
strings; update the mapping in extract_conversation_memory_config to trim and
convert any "" or whitespace-only values from MemoryConfigJson (e.g.,
long_term_memory.subject_id, embedding_model_id, extraction_model_id, and
short_term_memory.condenser_model_id) into None/empty by using a helper
normalizer (or inline trim check) before assigning to ConversationMemoryConfig
so blank values aren’t treated as set by inject_memory_context; add a regression
test that parses header JSON with "" and whitespace-only values to assert they
result in the default/None-equivalent fields in ConversationMemoryConfig.
🪄 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: aec9ea51-1253-4979-b38d-8d6a0e5289b5
📒 Files selected for processing (3)
model_gateway/src/routers/common/header_utils.rsmodel_gateway/src/routers/openai/responses/history.rsmodel_gateway/src/routers/openai/responses/route.rs
18dce25 to
d028dfb
Compare
d028dfb to
9761c77
Compare
|
Hi @spalimpaaces-star, the DCO sign-off check has failed. All commits must include a To fix existing commits: # Sign off the last N commits (replace N with the number of unsigned commits)
git rebase HEAD~N --signoff
git push --force-with-leaseTo sign off future commits automatically:
|
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/common/header_utils.rs`:
- Around line 61-79: In extract_conversation_memory_config, when
serde_json::from_str::<MemoryConfig>(raw) fails, add a debug/warn log that
includes the HEADER_CONVERSATION_MEMORY_CONFIG name, the raw header value and
the parsing error (e.g., via log::warn! or tracing::debug!/warn!), then continue
returning the existing default ConversationMemoryConfig; this uses the existing
symbols extract_conversation_memory_config, extract_header_value,
HEADER_CONVERSATION_MEMORY_CONFIG, MemoryConfig and ConversationMemoryConfig to
locate where to insert the logging.
🪄 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: 8e8578c8-e426-4fdc-9c0b-eeebffa38a31
📒 Files selected for processing (1)
model_gateway/src/routers/common/header_utils.rs
![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: 4f6fbdad06
ℹ️ 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".
59590c1 to
b85711c
Compare
|
DCO, unit test, and pre-commit all failed. |
05aa31c to
75a292c
Compare
85ec39d to
9f546af
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/openai/responses/header_utils.rs`:
- Around line 83-121: Add two tests covering absent header and whitespace
normalization: 1) create a test named like
extract_conversation_memory_config_with_no_header_returns_defaults that calls
extract_conversation_memory_config(None) and asserts
cfg.long_term_memory.enabled and cfg.short_term_memory.enabled are false
(defaults) and all optional IDs are None; 2) create a test named like
extract_conversation_memory_config_with_blank_values_normalizes_to_none that
inserts "x-conversation-memory-config" with either a blank string value (" ")
and/or JSON where fields are whitespace-only (e.g.,
{"long_term_memory":{"enabled":true,"subject_id":" ","embedding_model_id":"
","extraction_model_id":"
"},"short_term_memory":{"enabled":true,"condenser_model_id":" "}}), call
extract_conversation_memory_config(Some(&headers)), assert boolean flags reflect
the enabled bits and that subject_id, embedding_model_id, extraction_model_id,
and condenser_model_id are normalized to None (use as_deref() comparisons as in
existing tests).
- Around line 43-46: The code currently drops invalid UTF-8 header values when
calling to_str().ok() on HEADER_CONVERSATION_MEMORY_CONFIG, creating a blind
spot; update the raw extraction to detect to_str() errors, log a diagnostic
(including the header name HEADER_CONVERSATION_MEMORY_CONFIG and the to_str()
error) via the existing logger (e.g., tracing::warn/error) before continuing,
and only proceed with the valid UTF-8 string for the subsequent filter and JSON
parsing; adjust the expression using a match/map_err or if let Err(...) to emit
the log and return None for malformed values so JSON parse failures remain
logged as before.
In `@model_gateway/src/routers/openai/responses/route.rs`:
- Around line 124-135: The parsed memory_config returned from
extract_conversation_memory_config is only used for debug logging and never
forwarded into the request injection path; update the routing code so the
memory_config is passed into the upstream/injection call (the same place where
hooks or request metadata are injected before dispatch) instead of being
dropped—locate extract_conversation_memory_config and the upstream
dispatch/injection function in route.rs and thread memory_config through that
call so the new no-op integration point receives it.
🪄 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: 72acb213-8bd4-4b15-911c-c67105ae427f
📒 Files selected for processing (3)
model_gateway/src/routers/openai/responses/header_utils.rsmodel_gateway/src/routers/openai/responses/mod.rsmodel_gateway/src/routers/openai/responses/route.rs
0380431 to
6f98aeb
Compare
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/openai/responses/history.rs`:
- Around line 301-305: The debug log has a typo "STMO" — update the message
string in the debug! call guarded by config.short_term_memory.enabled (where it
also logs has_condenser_model =
config.short_term_memory.condenser_model_id.is_some()) to read "STM recall
requested - retrieval not yet implemented" so it matches the project's STM
terminology.
🪄 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: a018f034-7632-4188-8778-5e8513f5c361
📒 Files selected for processing (4)
model_gateway/src/routers/openai/responses/header_utils.rsmodel_gateway/src/routers/openai/responses/history.rsmodel_gateway/src/routers/openai/responses/mod.rsmodel_gateway/src/routers/openai/responses/route.rs
|
Hi @spalimpaaces-star, the DCO sign-off check has failed. All commits must include a To fix existing commits: # Sign off the last N commits (replace N with the number of unsigned commits)
git rebase HEAD~N --signoff
git push --force-with-leaseTo sign off future commits automatically:
|
0a22eb4 to
fb2b09a
Compare
| use serde::Deserialize; | ||
| use tracing::debug; | ||
|
|
||
| static HEADER_CONVERSATION_MEMORY_CONFIG: http::header::HeaderName = |
There was a problem hiding this comment.
header shouldnt be part of router
we have header code in common
There was a problem hiding this comment.
moved it to the common module
| /// Memory configuration parsed from the `x-conversation-memory-config` request header. | ||
| /// | ||
| /// Returns defaults when the header is absent or unparsable. | ||
| #[derive(Debug, Clone, Default, Deserialize)] |
There was a problem hiding this comment.
why do we have ltm memory config here?
didnt we define this elsewhere?
There was a problem hiding this comment.
Are you taking about the other PR? it was not merged yet. Is the concern to move these objects under some new module(memory)?
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod tests { |
There was a problem hiding this comment.
same here
these should be moved to header
There was a problem hiding this comment.
Sure, moved it now.
Signed-off-by: saikiranpalimpati <34260562+saikiranpalimpati@users.noreply.github.com>
Signed-off-by: saikiranpalimpati <34260562+saikiranpalimpati@users.noreply.github.com>
…ove inject memory, Signed-off-by: saikiranpalimpati <34260562+saikiranpalimpati@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: saikiranpalimpati <34260562+saikiranpalimpati@users.noreply.github.com>
fb2b09a to
3de64bf
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3de64bff06
ℹ️ 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".
| use crate::routers::openai::responses::header_utils::{ | ||
| ConversationMemoryConfig, LongTermMemoryConfig, ShortTermMemoryConfig, | ||
| }; |
There was a problem hiding this comment.
The new unit test imports ConversationMemoryConfig from crate::routers::openai::responses::header_utils, but this commit defines those types in crate::routers::common::header_utils. In any cfg(test) build (e.g. cargo test -p model_gateway), this unresolved path makes the test module fail to compile, so the added coverage cannot run.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
model_gateway/src/routers/common/header_utils.rs (1)
356-426: 🛠️ Refactor suggestion | 🟠 MajorModule location appears inconsistent with the intended move.
Per a prior review thread, these memory-config types were to be moved out of
routers::commoninto a responses-specific header module (author: "moved them to responses specific headers file"). However,ConversationMemoryConfig, its sub-structs, andextract_conversation_memory_configare still defined here inrouters::common::header_utils. This also conflicts with the test inopenai/responses/history.rs(see separate comment), which imports these types fromcrate::routers::openai::responses::header_utils.Please either:
- Actually move this block to
model_gateway/src/routers/openai/responses/header_utils.rsand declare it viapub mod header_utils;inresponses/mod.rs, updatingroute.rsandhistory.rsimports accordingly; or- Leave it in
commonand update the PR description,history.rstest imports, and past discussion outcome to match.Option 1 aligns with the PR description and the Responses-specific nature of the fields.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@model_gateway/src/routers/common/header_utils.rs` around lines 356 - 426, The memory-config types and helper (ConversationMemoryConfig, LongTermMemoryConfig, ShortTermMemoryConfig, extract_conversation_memory_config, normalize_optional_string) were supposed to live in the Responses-specific header utilities but remain in routers::common::header_utils; move the entire block to the responses header_utils module (create/modify responses/header_utils.rs), export it via pub mod header_utils; in any files that import these symbols (e.g., route.rs and the test in history.rs) update imports to point to the new responses::header_utils location, run tests, and remove the duplicate definitions from routers::common::header_utils; alternatively if you intentionally want to keep them in common, update the PR description and adjust the history.rs test imports to reference routers::common::header_utils instead (but prefer moving to responses as described).
🤖 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/openai/responses/history.rs`:
- Around line 309-317: The test module currently imports
ConversationMemoryConfig, LongTermMemoryConfig, and ShortTermMemoryConfig from
crate::routers::openai::responses::header_utils which doesn't exist; update the
use statement inside the tests mod (the block containing inject_memory_context
and ResponseInput/ResponsesRequest) to import these types from
crate::routers::common::header_utils instead so it matches the non-test code
import and the actual definitions.
---
Duplicate comments:
In `@model_gateway/src/routers/common/header_utils.rs`:
- Around line 356-426: The memory-config types and helper
(ConversationMemoryConfig, LongTermMemoryConfig, ShortTermMemoryConfig,
extract_conversation_memory_config, normalize_optional_string) were supposed to
live in the Responses-specific header utilities but remain in
routers::common::header_utils; move the entire block to the responses
header_utils module (create/modify responses/header_utils.rs), export it via pub
mod header_utils; in any files that import these symbols (e.g., route.rs and the
test in history.rs) update imports to point to the new responses::header_utils
location, run tests, and remove the duplicate definitions from
routers::common::header_utils; alternatively if you intentionally want to keep
them in common, update the PR description and adjust the history.rs test imports
to reference routers::common::header_utils instead (but prefer moving to
responses as described).
🪄 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: fd1913ba-bbd7-456f-b203-cdbc80bcb148
📒 Files selected for processing (3)
model_gateway/src/routers/common/header_utils.rsmodel_gateway/src/routers/openai/responses/history.rsmodel_gateway/src/routers/openai/responses/route.rs
3de64bf to
8f98cc2
Compare
Move ConversationMemoryConfig, LongTermMemoryConfig, ShortTermMemoryConfig, and extract_conversation_memory_config from responses/header_utils.rs to routers/common/header_utils.rs where shared header parsing utilities live. Signed-off-by: saikiranpalimpati <34260562+saikiranpalimpati@users.noreply.github.com>
8f98cc2 to
44f6ae2
Compare
5 participants
Description
Problem
We need a clear request contract in SMG for conversation memory settings, including both long-term memory and short-term memory.
Solution
Add a thin contract layer in the responses flow:
Changes
Changes needed:
Implementation:
model_gateway/src/routers/openai/responses/header_utils.rs (new file)
** Added ConversationMemoryConfig, LongTermMemoryConfig, ShortTermMemoryConfig — nested structs that directly deserialize the x-conversation-memory-config JSON wire format
** Added extract_conversation_memory_config(headers) — returns defaults when header is absent or unparseable; normalizes blank/whitespace string fields to None; emits debug! log on parse failure
** Unit tests: valid JSON → all fields populated, invalid JSON → safe defaults
model_gateway/src/routers/openai/responses/route.rs
** Calls extract_conversation_memory_config in the responses request path after history loading
** Emits a debug! log with presence flags (not raw values) when memory is requested; actual memory injection is deferred to a follow-up PR
model_gateway/src/routers/openai/responses/mod.rs
** Exposes the new header_utils module
Test Plan
Unit tests only as this was just a no op change
Checklist
cargo +nightly fmtpassescargo clippy --all-targets --all-features -- -D warningspassesSummary by CodeRabbit
Release Notes