fix(memory): keep repeated session_input_callback history items out of session input - #4072
fix(memory): keep repeated session_input_callback history items out of session input#4072hsusul wants to merge 2 commits into
Conversation
A session_input_callback that emits the same history object more than once had its repeats persisted as new turn input. _consume_reference() pops the matched history reference, so only the first occurrence carried identity evidence; later occurrences fell through to the new-input branch and the session grew a duplicate on every turn. Track the history object ids separately. Object identity with a copied history item is permanent proof of origin, unlike the per-occurrence reference maps and the content-frequency budget, which stay unchanged so reconstructed items and genuinely new items classify as before.
26788ea to
d0bcb56
Compare
|
@codex review |
|
Security review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
![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: d0bcb56163
ℹ️ 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".
| # Object identity with a copied history item proves the item came from history, and it | ||
| # stays true however many times the callback emits that same object. The reference maps | ||
| # below are consumed per occurrence, so they alone cannot classify a repeat. | ||
| history_object_ids = {id(history_item) for history_item in history_for_callback} |
There was a problem hiding this comment.
When a callback removes a history item before returning it multiple times—for example, item = history.pop(); return [item, item] + new_input—this set is built after the callback and no longer contains that item's identity. Both copies therefore fall through as new input and are persisted on every turn, leaving the compounding session corruption unfixed for callbacks that mutate the provided history copy. Capture and retain the original history objects before invoking the callback so their identities remain available and cannot be recycled.
AGENTS.md reference: AGENTS.md:L127-L128
Useful? React with 👍 / 👎.
| assert [cast(dict[str, Any], item).get("content") for item in session_items] == [ | ||
| "history", | ||
| "new", | ||
| ] |
There was a problem hiding this comment.
When a callback repeats history by reconstructing or deep-copying an item, this assertion requires the extra copy to be persisted as new input, so successive turns can still compound that old item in stored session history. The session-persistence contract explicitly requires existing history not to be re-appended when callbacks deep-copy, duplicate, or reconstruct items; the provenance classification should distinguish these reconstructed repeats instead of locking the corrupting behavior into a regression test.
AGENTS.md reference: AGENTS.md:L128-L128
Useful? React with 👍 / 👎.
seratch
left a comment
seratch
left a comment
There was a problem hiding this comment.
Please capture the original history-object provenance before invoking or awaiting session_input_callback, while retaining strong references to those objects.
The current code builds history_object_ids after the callback returns. Since the documented callback inputs may be safely mutated, a callback can pop an item from history and then return that same item twice; it is no longer present when the IDs are collected, so the duplicate is still persisted as new input. Please add a focused regression test that pops an existing history item and repeats it in the returned model input. Once that case is covered, this should be ready for another review.
The callback owns the lists it receives, so a callback that pops an item out of the history list and still returns it left no evidence that the item came from history. Snapshot the copied history objects before invoking or awaiting the callback and classify against that snapshot, which also keeps the objects alive so their recorded ids cannot be reused.
|
Thanks, you're right, and the pop case was broken in both directions, not just for the repeat. history_objects = list(history_for_callback) is now taken immediately after the deep copy and before session_input_callback is invoked or awaited. history_object_ids comes from that snapshot, and history_refs / history_counts are built from it too, since they had the same problem: they were constructed from the post-callback list, so an item the callback popped out of history had no entry anywhere and was persisted as new input even on its first occurrence. The snapshot list is also what holds the strong references, so the recorded ids stay valid for the whole function and cannot be reused by an unrelated object. For a callback that does not mutate history, the snapshot is element-for-element identical to what those maps were previously built from, so classification is unchanged. Regression tests added in tests/test_agent_runner.py: test_prepare_input_with_session_callback_pops_and_repeats_history_item — the callback pops the existing history item and repeats it in the returned model input; prepared input is ["history", "new", "history"] and nothing is persisted but "new". Full stack re-run on the new commit: make format, make lint, make typecheck clean; make tests → 5967 passed, 3 skipped (parallel) and 45 passed, 4 skipped (serial); make tests-asyncio-stability 5/5; tests/memory/ tests/test_agent_runner*.py → 378 passed, five consecutive runs, no flakes; Python 3.10 → 318 passed. The PR description is updated to match. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6c77a365a0
ℹ️ 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".
| # belong to the new turn and therefore still need to be persisted. | ||
| history_refs = _build_reference_map( | ||
| history_for_callback, | ||
| history_objects, |
There was a problem hiding this comment.
When a callback replaces an entry in the provided history list—for example, history[0] = summarized_item; return history + new_input—building the reference and frequency maps from the pre-callback history_objects snapshot leaves the replacement unclassified, so it falls through into appended_items and is permanently saved as fresh session input. Mutating these copied lists is supported, and before this commit the maps used the post-callback history_for_callback; retain the snapshot for stable original identities, but classify entries still present in the callback's final history list as history.
AGENTS.md reference: AGENTS.md:L127-L128
Useful? React with 👍 / 👎.
2 participants
Summary
A
session_input_callbackthat emits the same history object more than once had every repeat persisted into the session store as new turn input. Because the re-persisted copy becomes part of the next turn's history, the duplication compounds on every turn and permanently corrupts the stored conversation.Affected component:
src/agents/run_internal/session_persistence.py::prepare_input_with_session.Root cause
prepare_input_with_session()classifies each item the callback returns by consuming an object-identity reference fromnew_refs/history_refs, then falling back to content-frequency budgets:_consume_reference()pops the matched object out of the reference list, so identity evidence is available only once per object. When the callback emits the same history object a second time, step 2 no longer matches; step 3 has already had that key's budget decremented by the step-2 match, so it is exhausted too, and the repeat falls through to step 5 and is persisted.The same evidence is lost in a second way: every reference and frequency map was built after the callback returned. The callback owns the lists it receives, so a callback that pops an item out of
historyand still returns that object leaves nothing in those maps to attribute it to history, and it is persisted as new input on the first occurrence too.Behavioral change
A callback may now include the same existing history item as many times as it likes, and may move it out of the
historylist it was handed, without that item being classified or persisted as new input. Withhistory + [history[0]] + newover four turns, the SQLite session used to hold 11 items (u0stored three extra times); it now holds 8. The model input is unchanged — the repeat is still forwarded to the model, since the callback deliberately put it there.Why this fix is safe
The fix snapshots the copied history objects before the callback is invoked or awaited, records their ids once, and treats an id match as permanent proof of origin:
It is deliberately not a value-equality relaxation. Broad content matching would let a genuinely new item that happens to serialize identically to a stored item be silently dropped from persistence. The distinction used here is the real semantic one: the callback receives deep copies of history and of the new input as two disjoint object graphs, so
id(item)being one of the history copies is conclusive, while equal content is only a hint.Everything else is untouched:
history_objectsalso keeps a strong reference to every copied history item for the rest of the function, so an id recorded there cannot be recycled by an unrelated object.history_refsandhistory_countsare built from that same snapshot, for the same reason. For a callback that does not mutatehistory, the snapshot is element-for-element identical to the list those maps were previously built from, so classification is unchanged.history_refsconsumption, so the first occurrence still consumes its reference and decrements the content budget exactly as before. Only surplus occurrences take the new branch, and they intentionally do not decrement the budget again — that budget stays reserved for reconstructed items.OpenAIConversationsSessionkey sanitization, so it behaves identically for both session kinds.Steps 1, 3, 4 and 5 are byte-for-byte unchanged, which is what keeps genuinely new items, reconstructed items, reordering, filtering, and empty callback results behaving exactly as they did.
Tests added
Unit tests in
tests/test_agent_runner.py, alongside the existingprepare_input_with_sessioncases:test_prepare_input_with_session_callback_repeats_history_item— the same history object emitted twice is persisted zero times; ordering of the prepared model input is asserted.test_prepare_input_with_session_callback_pops_and_repeats_history_item— the callback pops an existing history item out of thehistorylist and repeats it in the returned model input; it is still persisted zero times.test_prepare_input_with_session_async_callback_pops_and_repeats_history_item— the same case through an async callback that pops after anawait. This pins the "before invoking or awaiting" requirement: a snapshot taken between the call and the await would pass the sync test and fail this one.test_prepare_input_with_session_callback_repeats_equal_history_items— two distinct history entries with equal content, each repeated, all still classify as history.test_prepare_input_with_session_repeated_history_keeps_equal_new_item— a genuinely new item whose content equals the repeated history item is still persisted exactly once. This is the guard against a value-equality-based fix.test_prepare_input_with_session_extra_reconstructed_item_stays_new_input— documents the intentional boundary: a rebuilt copy beyond the item's multiplicity in history carries no proof of origin and is still persisted. This behavior is unchanged by the PR and is asserted so any future move to unbounded content matching is a conscious decision.Runner-level test in
tests/memory/test_session.py:test_session_callback_repeating_history_does_not_grow_session[run|run_sync|run_streamed]— three turns against a temporarySQLiteSessionwith ahistory + [history[0]] + newcallback, asserting the stored user messages are exactly["user 0", "user 1", "user 2"]and the session holds 6 items. This covers the sync, async, and streamed entry points, which share this code path.No sleeps and no concurrency are involved in these tests; the callbacks are deterministic and the assertions are on stored state. Existing coverage for empty callback results (
test_prepare_input_with_session_callback_drops_new_items,..._ignores_callback_without_history), append-after-history ordering (..._uses_sync_callback,..._awaits_async_callback), reordering (..._callback_reorders_new_items), and content-matched rebuilds (..._matches_copied_items_by_content) all still pass unchanged.Pre-fix proof. With only
src/agents/run_internal/session_persistence.pyat upstreammainand the new tests in place:The two pop cases also fail against the previous revision of this PR, which built the id set after the callback returned. All pass with the current fix.
Compatibility
No public API, exception type, or persisted-schema change. Behavior changes only for callbacks that repeat or relocate a history object, where an item that used to be written to the session store no longer is — the documented intent in
.agents/references/session-persistence.md("Existing history must not be re-appended as new input, even whensession_input_callbackdeep-copies, reorders, filters, duplicates, or reconstructs items"). Model input is unaffected.Non-goals. Callbacks that repeat a new-input object (still persisted per occurrence), unbounded content-based history attribution, and any change to handoff or compaction persistence paths.
Test plan
Run from the repository root on
fix/4069-session-callback-history-repetition(Python 3.12.13, macOS 15.7.3):make format842 files left unchanged;ruff check --fix→All checks passed!make lintAll checks passed!make typecheckSuccess: no issues found in 833 source files; pyright0 errors, 0 warnings, 0 informationsmake tests5967 passed, 3 skipped, 2 warnings(parallel) and45 passed, 4 skipped, 5970 deselected(serial)make tests-asyncio-stabilitygit diff --checkuv run pytest tests/memory/test_session.py -k repeating_history -q3 passeduv run pytest tests/test_agent_runner.py -k prepare_input_with_session -q21 passeduv run pytest tests/memory/ tests/test_agent_runner.py tests/test_agent_runner_streamed.py tests/test_agent_runner_sync.py -q378 passed(run 5× consecutively, no flakes)UV_PROJECT_ENVIRONMENT=.venv_310 uv run --python 3.10 -m pytest tests/memory/ tests/test_agent_runner.py -q318 passedNo OpenAI API key, network access, or paid model call was used; the tests rely on
tests/fake_model.py::FakeModel,tests/utils/simple_session.py::SimpleListSession, and a temporarySQLiteSession.Not run:
make integration-tests*(requires live provider credentials and external services) andmake build-docs(no documentation files changed). No inline snapshots were added or modified. No lockfile or dependency changes.Issue number
Fixes #4069
Checks
.agents/skills/code-change-verification/scripts/run.sh/reviewbefore submitting this PR