fix: load full history when compacting a limited session

[winklemad]

What & why

OpenAIResponsesCompactionSession._ensure_compaction_candidates loads the underlying history with a bare underlying_session.get_items(). For a session configured with a limit (SessionSettings(limit=N)), get_items() resolves the limit to session_settings.limit and returns only the latest N items.

In input/auto compaction mode those truncated items are exactly what gets sent to responses.compact, and run_compaction then 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 (no force needed), so this bites any user who pairs a limited session with compaction.

Minimal repro:

underlying = SQLiteSession("s", session_settings=SessionSettings(limit=3))
await underlying.add_items([...8 items...])
session = OpenAIResponsesCompactionSession("s", underlying, client=mock, compaction_mode="input")
await session.run_compaction({"force": True})
# responses.compact receives only the latest 3 items; after replacement the
# session holds a single summary of those 3 — items 0..4 are gone.

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 real SQLiteSession(limit=3) holding 8 items must send all 8 to responses.compact (fails before the fix — only the latest 3 arrive). Full tests/memory/ suite (146 tests) passes; ruff and mypy clean.

Found by reading the code; no linked issue.

[seratch]

@codex review

[chatgpt-codex-connector]

Codex Review: Didn't find any major issues. Already looking forward to the next diff.

Reviewed commit: d6c8424884

ℹ️ 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".

[seratch]

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.

[winklemad]

Thanks @seratch — you're right, the previous_response_id path was still exposed. Pushed a follow-up:

• Auto now falls back to full-history input compaction when the resolved mode would be previous_response_id but the underlying session's default (limit-bounded) view doesn't cover the full stored history. That way the clear+replace can't drop unrepresented items. An explicitly requested previous_response_id compaction is left opt-in and unchanged.
• Detection is a small _underlying_view_covers_history() helper comparing the default get_items() view against the full view; the extra read only happens on the auto + previous_response_id branch.
• Added test_run_compaction_auto_falls_back_to_input_when_history_truncated — a no-force test covering the auto path before replacement (12 candidates over a limit=3 session trigger compaction via the threshold and must send all 12 to responses.compact rather than resolving to previous_response_id).
• Also fixed the typecheck (pyright reportOptionalIterable) failure by guarding the test's input assertions.

Full tests/memory/ suite passes; ruff, mypy, and pyright are clean on the changed files. PTAL.

[chatgpt-codex-connector]

💡 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".

[chatgpt-codex-connector]

💡 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".

[winklemad]

@seratch ready for another look when you get a chance — the previous_response_id path you flagged now falls back to full-history input compaction whenever the session's limit-bounded view doesn't cover the full stored history, so the clear+replace can't drop unrepresented items. The two follow-up nits since are folded in as well. Thanks!

[seratch]

Thanks for addressing the previous-response fallback, unsafe ID reuse, and per-run limits. The remaining blocker is ownership of the per-run limit.

_last_prepared_input_limit is one mutable field updated by every public get_items() call, rather than state tied to a specific run or response. If A prepares with limit=3, B prepares with limit=None, and A then completes, A can read B's limit and use previous_response_id even though response A omitted older history. A fresh compaction wrapper on RunState resume has the same problem because resume skips session input preparation.

Please carry the effective history-window metadata with the specific run/response into both compaction and deferred compaction instead of inferring it from shared get_items() side effects. Please also add controlled-overlap and fresh-wrapper-resume regressions that assert the surviving operation and full stored history.

[winklemad]

Thanks @seratch — good catch on the ownership issue. Reworked it so the history window travels with the specific response instead of being inferred from shared get_items() side effects:

• Added input_history_limit to OpenAIResponsesCompactionArgs. save_result_to_session resolves the effective window (session settings + per-run RunConfig) through a shared resolve_session_history_limit helper and passes it alongside the response into both run_compaction and _defer_compaction.
• Removed _last_prepared_input_limit entirely; _underlying_view_covers_history now takes the window as an argument. So the controlled-overlap case (A prepares with limit=3, B with limit=None) can no longer let A read B's limit, and a fresh wrapper on resume — which skips input preparation — still gets the right window because the Runner carries it through save_result_to_session / save_resumed_turn_items (threaded session_settings into those paths).

Added the two regressions you asked for, both asserting the surviving operation compacts from the full stored history:

• test_run_compaction_uses_per_response_limit_under_interleaved_get_items (controlled overlap)
• test_save_result_to_session_carries_run_limit_to_fresh_wrapper_on_resume (fresh wrapper on resume, driven through the Runner save path)

Both fail on the previous commit and pass now. Full tests/memory/ + runner suites pass; ruff, mypy, and pyright are clean. PTAL.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d79002827f

ℹ️ 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".

[winklemad]

Follow-up: I widened the fix so the per-run window now flows through every save path that can reach compaction, not just the direct save_result_to_session calls. save_turn_items_if_needed (the final-output / run-again paths) and the streaming _save_stream_items / _save_resumed_stream_items closures now thread session_settings too, so a limited run compacts the full stored history on those paths as well.

Added regressions for each: the final-output path (test_save_turn_items_if_needed_carries_run_limit), the deferred-under-limit path (test_deferred_compaction_under_limit_uses_full_history, covering the "into both compaction and deferred compaction" point), and the streaming path (test_save_stream_items_carries_run_limit). Each fails before its threading and passes after. ruff, mypy, and pyright are clean; the full tests/memory/ + runner suites pass. PTAL @seratch.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e2ace6c9ec

ℹ️ 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".

[winklemad]

One more refinement on the efficiency edge: a limited run whose pre-existing history already fit within the window shouldn't be forced onto full-history input just because this turn grows the stored session past the limit — the response genuinely saw everything, so previous_response_id is still safe.

Coverage is now resolved against the pre-save store (the state the response was actually prepared from) in save_result_to_session, and carried as input_covered_full_history rather than recomputed from the post-save session length. The check is a single bounded read that never reports "covered" when it can't prove it, so it stays safe (never drops history) while avoiding the needless oversized input request at the limit boundary. Added test_compaction_reuses_previous_response_id_when_pre_save_history_fits (fails before / passes after). ruff, mypy, and pyright are clean; full tests/memory/ + runner suites pass.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 2788e93e1c

ℹ️ 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".

[winklemad]

This round is a correctness fix, not just efficiency. When the compaction session wraps a store that carries its own SessionSettings(limit=N) — rather than the wrapper or a RunConfig — resolve_session_history_limit previously saw None (it only read the wrapper's settings) and recorded input_covered_full_history=True, even though get_items(None) delegates to that underlying store and its limit truncates the input. A normal save could then keep previous_response_id and drop the hidden older items.

It now walks the delegate chain (underlying_session) to find the effective limit, so the truncation is detected and compaction falls back to full-history input. Added test_compaction_honors_underlying_session_limit — a save whose limit lives only on the wrapped store must not reuse previous_response_id (fails before / passes after). ruff, mypy, and pyright are clean; the full tests/memory/ + runner suites pass.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 5570d8091a

ℹ️ 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".

[winklemad]

Tightened the pre-save coverage check to be exact. It now reads one item beyond the window (get_items(limit + 1)), so a history that exactly fills the limit counts as covered rather than truncated — the earlier len < limit form treated an exact fill as truncated and could force an unnecessary full-history input at the boundary. Since an explicit limit arg wins over a session's own default (resolve_session_limit), get_items(limit + 1) still surfaces a real overflow item, so truncation (including the wrapped-store case) is still detected and it never reports "covered" without proof. Added test_compaction_reuses_previous_response_id_at_exact_limit (fails before / passes after). ruff, mypy, and pyright are clean; the full tests/memory/ + runner suites pass.

[winklemad]

@seratch this is ready for another look — consolidating where it landed, since the fixes ended up spread across a few commits.

The ownership blocker is resolved. The history-window state no longer lives in a shared mutable field (_last_prepared_input_limit is gone). save_result_to_session now computes, per response, whether that response's input window covered the full stored history (input_covered_full_history), measured against the pre-save store the response was actually prepared from, and carries that boolean into run_compaction / _defer_compaction. Because it travels with the specific response, interleaved runs and fresh wrappers on resume can no longer read each other's window.

Both regressions you asked for are in:

• test_run_compaction_uses_per_response_limit_under_interleaved_get_items — A prepares with a limit, B with none; asserts A still uses full-history input and the full stored history survives the replace.
• test_save_result_to_session_carries_run_limit_to_fresh_wrapper_on_resume — a fresh wrapper on resume, asserting the per-run limit is honored with no prepare step.

Also hardened along the way: per-run RunConfig(session_settings=…) limits now flow through every save path that reaches compaction (final-output, run-again, streaming); an underlying store's own SessionSettings(limit=N) is honored via the delegate chain; and the pre-save coverage check reads one item beyond the window so an exact-fill window counts as covered rather than being needlessly forced onto input compaction.

Green on the current head: the compaction suite (59 tests) plus ruff, mypy, and pyright clean on the changed files.

[seratch]

Thanks, the previous shared-limit blocker is substantially addressed: the limit state is no longer stored by get_items(), and the coverage result is threaded through the normal, resumed, streaming, and deferred save paths.

One ownership issue remains in run_compaction(). The call writes its response_id into shared self._response_id, awaits candidate loading, and then combines that shared value with this call's local input_covered_full_history. If B suspends with covered=True, A overwrites the field with a limited response ID and returns without compacting, B can resume and send A's ID under B's coverage decision.

Please keep the response ID, coverage, and mode decision attempt-local across awaits. Also replace the current sequential "interleaved" test with an event-controlled two-call regression that proves the request uses its own response ID and that full stored history survives.

[winklemad]

@seratch addressed — thanks for the precise catch.

run_compaction no longer reads the shared self._response_id after awaiting. It captures the response id in a local at the top (before the candidate-loading await) and uses that local for the mode decision, the previous_response_id it sends, and the unstored marking. self._response_id is still updated for external state but is never read within the call, so an interleaved run can no longer redirect this call to another call's id/coverage. Coverage (input_covered_full_history) and the mode were already locals.

Replaced the sequential interleaved test with an event-controlled two-call regression (test_run_compaction_keeps_response_id_and_coverage_attempt_local): an asyncio.Event parks one call at its first await while the other run completes and overwrites the shared field, then asserts each call compacts under its own response id and the full stored history survives. It fails on the pre-fix code and passes now; full compaction suite green, ruff + mypy clean.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 8a43af3ff3

ℹ️ 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".

[chatgpt-codex-connector]

Preserve the original session limit on resume

On a RunState resume, this uses the resume call's RunConfig rather than the settings that prepared the interrupted response. If the original run used RunConfig(session_settings=SessionSettings(limit=N)) and the resume call omits or changes that setting with a fresh OpenAIResponsesCompactionSession, save_result_to_session marks input_covered_full_history as true for a response_id that only saw the limited window, so auto compaction can use previous_response_id and replace the full local history with a server summary missing older items. Carry the original turn's coverage/limit in RunState instead of recomputing it from the resume config.

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Keep unsafe response IDs unsafe when store is true

Fresh evidence after the prior unsafe-ID fix: the same _last_unstored_response_id slot is now used for response IDs that did not cover full history, but this branch clears it whenever a later compaction of the same ID passes store=True. After an input fallback has shrunk the session inside the limit, that later call can pass the live coverage check and switch back to previous_response_id, replacing the full-history summary with server history that never saw the hidden older items. Track coverage-unsafe IDs separately, or do not clear them based only on store=True.

Useful? React with 👍 / 👎.

[winklemad]

@seratch — the latest commit has the attempt-local response id fix plus the event-controlled two-call regression you asked for. Two of the Codex suggestions on this push I'd rather get your read on before expanding scope, since one looks unreachable and the other is broader than this PR:

1. Clearing _last_unstored_response_id on store=True (openai_responses_compaction_session.py). I don't think this is reachable through the Runner. save_result_to_session always calls run_compaction with the current turn's response id, and the store=True clear only fires when response_id == self._last_unstored_response_id. So a coverage-unsafe id marked on one turn is never revisited with store=True on a later turn — the later turn has a new id. I checked a limited-then-stored sequence with distinct ids and the earlier id stays marked; it would only clear if the same response id were compacted twice with differing store, which I don't see happening outside a direct run_compaction call. Do you see a path I'm missing? If not, I'd leave it as-is rather than add a second tracking slot.

2. Resume uses the resume call's RunConfig (run.py). This one does look reachable: if a run is interrupted under RunConfig(session_settings=SessionSettings(limit=N)) and resumed without (or with a different) limit, save_result_to_session recomputes coverage from the resume config for a response that only saw the limited window, so auto compaction could reuse previous_response_id. Fixing it properly means carrying the original turn's coverage/limit on RunState instead of recomputing on resume — a bit broader than this PR's scope. Happy to do it here, or split it into a follow-up. Which would you prefer?

Kept this commit focused on your ownership fix rather than folding both in speculatively — let me know how you'd like to scope 1 and 2.

[winklemad]

Thanks for the detailed review @seratch — this is addressed in the latest commit (8a43af3f).

run_compaction now snapshots the response id into an attempt-local variable (alongside input_covered_full_history and the resolved mode) and uses that snapshot for the mode resolution, the should-compact decision, and the compact call. self._response_id is still updated when an id is passed, so a later call without an explicit id keeps falling back to the most recent one — but a concurrent call can no longer make an attempt act on another call's id and coverage.

I also replaced the sequential interleaved test with an event-controlled two-call regression (test_run_compaction_keeps_response_id_and_coverage_attempt_local): it parks the full-coverage call on its first await while a limited call overwrites the shared id, then asserts each call compacts with its own response id and that full stored history survives. It fails on the previous code and passes now.

[winklemad]

Thanks @seratch — addressed both points:

• run_compaction (and _defer_compaction) now resolve the mode from the attempt's own values before the candidate-loading await, and I dropped the shared self._response_id field entirely, so each call decides only from its own response id — a concurrent call with no id of its own can no longer inherit another run's id.
• Replaced the interleaving test with an event-controlled two-call regression: it parks one call mid-await, runs a no-id call in between, and asserts it falls back to full-history input instead of inheriting the parked call's id (fails on the previous code, passes now).

Also merged main to clear the conflict.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 80afea6095

ℹ️ 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".

[chatgpt-codex-connector]

Don't count pre-saved turn input as missing history

Fresh evidence after the earlier exact-limit fix: in the normal Runner paths the user's input is already persisted before the model response is saved (for example run.py 771/864 and streaming run_loop.py 1516), so this length check is no longer measuring only the pre-response history. With a limited OpenAIResponsesCompactionSession, if the pre-turn history exactly filled the limit (or a multi-item user input pushes the stored length past it), the response did see all prior history plus the current input, but this marks the response ID unsafe and forces full-history input compaction; for large histories this can turn a safe server-side compaction into an oversized or unnecessarily expensive request.

Useful? React with 👍 / 👎.