fix: load full history when compacting a limited session#3827
fix: load full history when compacting a limited session#3827winklemad wants to merge 4 commits into
Conversation
OpenAIResponsesCompactionSession._ensure_compaction_candidates loaded the underlying history with a bare underlying_session.get_items(), which resolves to session_settings.limit and returns only the latest N items. In input/auto mode those truncated items are what gets sent to responses.compact, and run_compaction then clears the session and replaces it with the compacted summary — so any history older than the limit window is silently and permanently lost. The restore/replace paths already read the full history through _get_all_underlying_session_items() (limit=_ALL_SESSION_ITEMS_LIMIT), added in openai#3117; the candidate-loading path was missed. Use the same helper there so compaction always operates on the complete stored history.
|
@codex review |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
seratch
left a comment
seratch
left a comment
There was a problem hiding this comment.
Using _get_all_underlying_session_items() is correct for explicit input compaction. However, the current change also loads full history for thresholding while ordinary auto with a stored response_id still resolves to previous_response_id. That request does not include the full local history, but the session is then cleared and replaced, so older items can still be silently dropped; this patch can also make that compaction trigger earlier.
Please make auto fall back to full-history input compaction whenever the stored history is not fully represented by the limited session view, while leaving explicitly requested previous_response_id behavior opt-in. Add a no-force regression test covering that auto path before replacement.
…imited In auto mode with a stored response_id, compaction resolves to previous_response_id, which does not send the local history. When the underlying session's default view is limit-bounded and does not cover the full stored history, clearing and replacing the session with the server-derived summary can still drop the unrepresented items. Auto now falls back to full-history input compaction whenever the limited view does not cover the stored history; an explicitly requested previous_response_id compaction stays opt-in. Add a no-force regression test for the auto path, and guard the test assertions against an Optional input value for pyright.
|
Thanks @seratch — you're right, the
Full |
![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: 5469581c1c
ℹ️ 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".
| and resolved_mode == "previous_response_id" | ||
| and not await self._underlying_view_covers_history() | ||
| ): | ||
| resolved_mode = "input" |
There was a problem hiding this comment.
When this auto-mode fallback is taken for a limited session, self._response_id still points at the same server response that was considered unsafe because it did not represent the hidden local history. If a user forces compaction again before another model response and the compacted output now fits within the limit, _underlying_view_covers_history() returns true and the next call switches back to previous_response_id, replacing the full-history summary with a server-derived summary that can omit those older hidden items. Treat this response ID like the store=False path, or clear it, after falling back to input.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch, thanks. Fixed in 10471fd: when the auto path falls back to input, I now mark that response_id unsafe (mirroring the store=False handling), so a later compaction won't switch back to previous_response_id with it once the session shrinks back within the limit.
Added test_run_compaction_auto_does_not_reuse_unsafe_response_id_after_input_fallback covering exactly that sequence: a first compaction falls back to input, and after the session is replaced with the small summary a forced second compaction must still use input rather than reuse the stale id. tests/memory/ is green; ruff, mypy, and pyright are clean.
When auto compaction falls back to input because the limited session view did not cover the full history, the stored response_id still points at a server response that omits the hidden items. Once the session later shrinks back within the limit, a forced compaction would otherwise switch back to previous_response_id and replace the full-history summary with a server-derived one that drops those items. Mark the response_id unsafe on fallback, mirroring the store=False path, and cover it with a regression test.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 10471fd7f5
ℹ️ 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".
|
|
||
| False when a session limit hides older items from the default view. | ||
| """ | ||
| default_view = await self.underlying_session.get_items() |
There was a problem hiding this comment.
When the limit is supplied through RunConfig(session_settings=SessionSettings(limit=N)) instead of the underlying session's own default, the model input is built from session.get_items(limit=resolved_settings.limit) in prepare_input_with_session, but this check calls the underlying session with no limit. For an otherwise-unlimited store this returns the full history, so auto mode still uses previous_response_id even though that response only saw the limited window; replacing the local store with the compacted server output can still drop the older items this change is trying to preserve. The fallback needs to know the effective limit used to produce the response, not just whether the underlying default view is truncated.
Useful? React with 👍 / 👎.
The auto->input fallback checked the underlying session's default view, which misses a limit supplied per run (e.g. RunConfig session settings) on an otherwise-unlimited store: the response only saw the limited window, but the check reported full coverage and kept using previous_response_id. Remember the limit the Runner uses to prepare each turn's input via get_items and compare against that window instead, so the fallback triggers whenever the response did not see the full history.
2 participants
What & why
OpenAIResponsesCompactionSession._ensure_compaction_candidatesloads the underlying history with a bareunderlying_session.get_items(). For a session configured with a limit (SessionSettings(limit=N)),get_items()resolves the limit tosession_settings.limitand returns only the latest N items.In
input/autocompaction mode those truncated items are exactly what gets sent toresponses.compact, andrun_compactionthen clears the underlying session and replaces it with the compacted summary. So every item older than the limit window is silently and permanently lost. Compaction runs automatically once the candidate threshold is reached (noforceneeded), so this bites any user who pairs a limited session with compaction.Minimal repro:
Fix
The restore/replace paths already read the complete history through
_get_all_underlying_session_items()(limit=_ALL_SESSION_ITEMS_LIMIT), introduced in #3117; the candidate-loading path was missed. Use the same helper so compaction always operates on the full stored history.Tests
Added
test_run_compaction_uses_full_history_when_underlying_session_has_limit: a realSQLiteSession(limit=3)holding 8 items must send all 8 toresponses.compact(fails before the fix — only the latest 3 arrive). Fulltests/memory/suite (146 tests) passes; ruff and mypy clean.Found by reading the code; no linked issue.