Skip to content

fix(compaction): preserve provider reasoning replay state - #917

Open
breedx wants to merge 2 commits into
mpfaffenberger:mainfrom
breedx:fix/upstream-reasoning-replay-state
Open

fix(compaction): preserve provider reasoning replay state#917
breedx wants to merge 2 commits into
mpfaffenberger:mainfrom
breedx:fix/upstream-reasoning-replay-state

Conversation

@breedx

@breedx breedx commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

Compaction can discard reasoning items that have no visible text but still carry provider replay state. Preserve those items so later requests retain the state they need, while continuing to remove genuinely empty reasoning parts.

Problem

The history cleanup checks only content and signature. An empty ThinkingPart with an id or provider_details is therefore treated as disposable, including when it is the only part of a response. Visible text is not a reliable test for whether provider replay state is present.

Change

  • Use one emptiness predicate that checks content, signature, ID and provider details.
  • Preserve opaque state in both standalone and mixed reasoning/text responses.
  • Keep removing truly empty parts and dropping responses left with no parts.
  • Correct the existing test that classified an ID-bearing item as empty.

This is a history-cleanup correction, not a new compaction strategy or a change to prompt/retry custody. It does not depend on the open capability refactors; #829 touches the same module and may need to retain this predicate when rebased.

Validation

Tested on Linux / Python 3.13.13 with the unchanged upstream dependency lock (pydantic-ai-slim 2.35.0, harness 0.23.0), based on 1d25d696.

  • New regression file on unpatched base: 4 failed, 6 passed. Failures demonstrate loss of ID-only and provider-details-only reasoning, both alone and alongside text.
  • python -m pytest -q -o addopts= tests/agents/test_reasoning_replay_state.py tests/agents/test_compaction.py tests/test_compaction_reasoning.py: 39 passed.
  • python -m pytest -q -o addopts= tests/agents tests/test_compaction_reasoning.py: 476 passed, 2 warnings.
  • Ruff lint, format check and git diff --check pass for the changed files.

Tests ran with disposable HOME/XDG directories, no inherited credentials, and socket connect/DNS/bind blocked. The two unawaited-coroutine warnings come from test_fire_stream_event_import_error and test_fire_stream_event_exception; both reproduce on the unpatched base. They were not suppressed.

Compatibility and limits

No dependency, package-version or public API changes. Tests use actual Pydantic message objects; no live-provider replay or end-to-end provider acceptance claim is made.

@breedx

breedx commented Sep 7, 2026

Copy link
Copy Markdown
Contributor Author

The focused fix remains independently testable, but full CI is red on an existing completion assertion: a quoted ~/... insertion is checked as if it were unquoted. I reproduced the same failure on the unpatched base; the correction is already proposed separately in #916, so this PR stays narrowly scoped.

Details:

  • Quality and Windows encoding checks passed.
  • The macOS test job finished with 1 failed, 7,818 passed, 14 skipped. Its sole failure is tests/test_completions_and_small_modules.py::TestFilePathCompleterMissedLines::test_tilde_prefix_display.
  • On unpatched 1d25d696, python -m pytest -q -o addopts= tests/test_completions_and_small_modules.py::TestFilePathCompleterMissedLines::test_tilde_prefix_display reproduces the same assertion: the insertion is "'~/.../test.txt'", so .startswith("~") is false.
  • This PR changes neither the completion implementation nor that test. fix(completion): apostrophes in prose no longer hide later @file refs #916 updates the assertion to decode the quoted insertion before checking the tilde path.
  • CI's job label says Python 3.13, but its test environment/log paths show Python 3.14.7. The local focused/control results in the PR body were run on Python 3.13.13; these are different environments.

No failing checks or warnings were suppressed. The full CI run also reports deprecation/resource/coroutine warnings beyond the locally reproduced stream warnings; I am not claiming that all of those have independent base controls. No merge-ready or all-green CI claim is made. Once the separate completion correction lands, these unchanged fixes can be requalified on the updated base.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant