fix(memory): compact the full session history when the input window is limited - #4293
fix(memory): compact the full session history when the input window is limited#4293Luccacvb wants to merge 6 commits into
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0d38b02831
ℹ️ 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".
seratch
left a comment
There was a problem hiding this comment.
Thanks for reviving #3827 and carrying over the response-ownership fixes. The underlying data-loss issue is valid, but the current fallback is not safe to merge. session_input_callback and call_model_input_filter may intentionally exclude stored history from the model request; this patch records that as incomplete coverage and then sends the full underlying store to responses.compact. That bypasses the filter and can place excluded content into the compacted session state. The new filter tests currently assert this unsafe behavior.
Please reset the fallback contract so full-history input is used automatically only when missing coverage is known to come solely from SessionSettings.limit. When callbacks, model-input filters, handoff rewrites, or resume make the effective input transformed or unknown, skip or defer auto-compaction and preserve the store. Add negative regressions with a filtered sentinel that must never reach responses.compact or the compacted state. Keep explicit "input" and "previous_response_id" behavior unchanged.
|
Thanks for the clear contract. I implemented it as described. The automatic full-history fallback now applies only when missing coverage is known to come solely from I also added negative regressions to ensure filtered sentinels never reach While reviewing the change, I also found that aborted compaction attempts, such as deferral, API failure, or a raising decision hook, could leave an older covered response context reusable by a later manual force retry. Each attempt now publishes its context on exit, with regressions covering those paths. This is the only manual retry behavior change, and it is safer: the retry rebuilds from the full store instead of reusing an unproven response chain One point for confirmation: |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b2bf2dd360
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 12d52eb5f8
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e84ef18102
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 33d16e5091
ℹ️ 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".
|
Thanks for continuing to work through the feedback. There are still unresolved issues. For example, an older compaction attempt can complete after a newer run has persisted items and then replace those newer items or overwrite the newer response context, because the current replacement and context publication do not have a shared ownership or serialization boundary. The current approach does not appear to be converging on a stable design, so please pause work on this PR. The maintainers need to determine whether and how to pursue this change before any further implementation work continues. No further action is needed from you for now. Thank you again for your patience and for all the work you have put into this. |
Summary
OpenAIResponsesCompactionSessionreplaces the entire stored session with the compaction output, but it previously decided what to compact from a limited view._ensure_compaction_candidatesloaded history with a bareget_items(), which limit-aware backends resolve against their effectivesession_settings.limit, so a limited session summarized only the tail and permanently destroyed everything older without ever sending it toresponses.compact. WithSessionSettings(limit=4)over 12 stored items, 4 reached the model and 8 were lost. The same truncated view reachedshould_trigger_compaction, so a limit belowDEFAULT_COMPACTION_THRESHOLDcould also prevent auto-compaction from firing.Candidate loading now reads the full stored history through the existing
_get_all_underlying_session_items()helper, which the sibling reads in this file already use.That alone is not enough. Ordinary
autowith a stored response ID can resolve toprevious_response_id, whose server-side history may not represent everything the local session is about to replace. Coverage is therefore classified where model input is prepared asfull,limit_only, ortransformed.Automatic full-history
inputfallback is used only when missing coverage is known to come solely fromSessionSettings.limit. Ifsession_input_callback,call_model_input_filter, a handoff rewrite, resume, or another transformation makes the effective input differ from or obscure the stored history, auto-compaction skips that turn and preserves the store instead of sending excluded content toresponses.compact.Explicit
compaction_mode="input"and"previous_response_id"retain their released behavior.Coverage metadata travels with the specific response into compaction rather than being inferred from shared session reads.
run_compactioncaptures the response ID and requested mode before awaiting session operations, preventing concurrent attempts from redirecting an in-flight compaction decision. The candidate cache is invalidated after replacement instead of being seeded from the compaction output, so an interleavedadd_itemsis not dropped by the next compaction.The automatic
limit_onlyfallback runs stored history through the same replay sanitization used by successful model requests, preventing reasoning IDs or orphaned calls removed by the normal request path from being reintroduced. Because that sanitization would treat a pending tool call still waiting for its output as an orphan, the fallback defers while a pending call has no persisted output. These behaviors remain scoped to the automatic fallback; explicitcompaction_mode="input"keeps the released v0.19.4 payload unchanged.Compaction attempts now also publish their attempt-local response context on every exit path, including deferral, API failure, and a raising decision hook. This prevents a later manual force retry from reusing an older response chain whose coverage was never proven; the retry rebuilds from the stored session instead.
The new per-response metadata is sent only to the SDK's built-in compaction session; third-party implementations of the released compaction-aware protocol continue to receive the legacy argument payload. The wrapper also delegates
session_settingsdynamically to the underlying store, so runtime limit changes are observed consistently.This revives #3827, which was previously closed for inactivity. The implementation incorporates the review feedback from that PR and keeps the original investigation and design work by @winklemad. Thanks to @seratch for the detailed review there and the guidance on supported behavior.
Known limitations:
compaction_mode="previous_response_id"intentionally bypasses automatic coverage checks; callers selecting it are responsible for ensuring the server-side response history matches the local session.compaction_mode="input"still bypasses replay sanitization, which predates this change and is left unchanged here to preserve released behavior.autowithstore=Falsenow also skips when the effective input is transformed; this is flagged for maintainer confirmation in the review thread.Test plan
Regression tests across
tests/memory/test_openai_responses_compaction_session.pyandtests/test_agent_runner_streamed.pycover full-history candidates under a limit, the decision hook's candidate count, automaticlimit_onlyfallback, limits configured on the wrapped store, transformed inputs fromsession_input_callbackandcall_model_input_filter, handoff rewrites, resume on the same and fresh wrappers, streaming resume, deferred compaction, pending approval/tool calls, attempt-local response IDs under controlled interleavings, cache reload after an interleavedadd_items, fallback replay sanitization, explicit input behavior, supported tool-call types, and explicit modes remaining unchanged under a limit.Negative regressions assert that content excluded by callbacks or model-input filters never reaches
responses.compactor the compacted session state, and that transformed or unknown inputs preserve the underlying store by skipping automatic compaction.Additional regressions cover aborted compaction attempts, including deferral, API failure, and a raising decision hook, ensuring a later manual force retry does not reuse stale response context.
The limit-related regression assertions are expected to fail against
mainbecause the original implementation exposes only the limited tail, for example withassert 2 == 12and an input list containing only the most recent items..agents/skills/code-change-verification/scripts/run.shpasses.Issue number
N/A — this is the success-path sibling of #3116/#3117, which fixed history loss on the restore path and introduced
_get_all_underlying_session_items. Supersedes #3827.Checks
.agents/skills/code-change-verification/scripts/run.sh/reviewbefore submitting this PR