Compaction follow-up: JSON-serialise shrunk tool returns, strengthen guard test, DRY threshold (#1824)

[JSv4]

Closes #1824.

Code-review follow-up for the in-run history compaction processor shipped in #1817. Each of the five findings was assessed against the code before acting.

Findings assessment & resolution

#	Finding	Verdict	Action
1	test_thinking_only_modelresponse_is_not_emptied passes via the no-op early-return, not the empty-parts guard it names	Valid	Strengthened the fixture + added telemetry assertions
2	_shrink_tool_return_part stringifies non-string content with str() instead of json.dumps()	Valid	New shared _stringify_tool_content helper using json.dumps(..., default=str)
3	Shrunk content can exceed target_chars by the trim-notice length; undocumented	Valid (issue recommends documenting)	Documented on the constant + helper docstring
4	Tests reach into a pydantic-ai internal (root_capability.capabilities)	No change needed (issue says so — documented + tripwired)	Left as-is
5	_on_in_run_shrink closure captures user_msg_id/llm_msg_id by late-binding — worth a comment	Valid	Added a comment confirming the IDs are stable for the turn

Changes

• Finding Bump traefik from v2.8.7 to v2.9.1 in /compose/production/traefik #2 (correctness). ToolReturnPart.content can be a dict/list; the shrink path serialised it with str() (Python repr — single-quoted keys, True/None), which the model can misparse. New _stringify_tool_content serialises non-strings with json.dumps(content, default=str) (falling back to str() only if JSON serialisation fails) and is reused by both the token estimator and the shrink, so the pre-shrink estimate matches the post-shrink payload. This matches the existing json.dumps(tool_result, default=str) convention already used to build tool-result content in pydantic_ai_agents.py.
• Finding Bump postgres from 14.5 to 15.0 in /compose/production/postgres #1 (test correctness). The old fixture had no shrinkable content in the older prefix, so the processor hit the tool_returns_shrunk == 0 and thinking_parts_dropped == 0 early-return and the test passed because nothing ran. It now places a large ToolReturnPart alongside the thinking-only response so the shrink actually runs, and asserts via the emitted InRunShrinkEvent (tool_returns_shrunk == 1, thinking_parts_dropped == 0) that the empty-parts guard is the reason the ThinkingPart survives. Added test_non_string_tool_return_serialized_as_json to pin the Bump traefik from v2.8.7 to v2.9.1 in /compose/production/traefik #2 fix.
• DRY (the "while you're in there" ask). New context_window_and_threshold(model_name, threshold_ratio) is the single definition of the compaction kick-in point, replacing three copies of int(get_context_window_for_model(...) * ratio) across should_compact, compact_message_history, and shrink_old_artifacts_processor. This formalises the shared-threshold_ratio design the review praised. Also unified the two str(content) stringification sites behind _stringify_tool_content.
• Finding Bump actions/setup-node from 3.4.1 to 3.5.1 #3 / Bump actions/checkout from 3.0.2 to 3.1.0 #5 (docs). Documented that IN_RUN_TOOL_RETURN_TARGET_CHARS bounds the preserved prefix (the trim notice adds a small fixed overhead) on both the constant and _shrink_tool_return_part; added the closure-stability comment in _stream_core.

Testing

Ran the affected pure-unit suites in an isolated venv (pydantic-ai-slim 1.104.0, matching the repo pin):

• test_history_processors.py: 14/14 passed (incl. the strengthened guard test + new JSON test).
• Pure compaction-logic classes in test_context_guardrails.py (TestShouldCompact, TestCompactMessageHistory, TestGetContextWindowForModel, TestTruncateToolOutput, …): 61/61 passed, exercising the context_window_and_threshold refactor.

black, isort, and flake8 pass on all changed files. No production behaviour changes beyond the str() → json.dumps() serialisation of non-string shrunk tool returns; the threshold refactor is byte-for-byte identical arithmetic.

[claude]

Code Review

This is a well-motivated follow-up that addresses real correctness and quality issues. The core fix (finding #2) is sound, the DRY refactor is clean, and the test improvements are genuinely valuable. A few notes below, ordered by severity.

---

Minor: Hardcoded line numbers in comment (pydantic_ai_agents.py)

The added closure-stability comment reads:

# The closure captures ``user_msg_id`` / ``llm_msg_id`` by Python
# late-binding, but both are resolved above (lines ~866-889) and
# never reassigned afterwards …

The ~866-889 reference will drift silently as code is inserted or removed nearby — the very kind of comment staleness the project's CLAUDE.md warns about. The intent is clear without it; suggest dropping the line-range qualifier:

# … both are resolved above (at the start of this function) and
# never reassigned afterwards …

---

Minor: RecursionError not guarded in _stringify_tool_content

# history_processors.py
try:
    return json.dumps(content, default=str)
except (TypeError, ValueError):
    return str(content)

json.dumps raises RecursionError on circular structures, which escapes the current guard. In practice, pydantic-ai tool returns are unlikely to be self-referential, but since the fallback intent is "never crash on exotic content," the catch clause should be broadened:

except (TypeError, ValueError, RecursionError):
    return str(content)

---

Observation: context_window unused in should_compact / compact_message_history

After the refactor, both callers discard the first element:

_, threshold = context_window_and_threshold(model_name, threshold_ratio)

The get_context_window_for_model call is cheap (dict lookup + fallback), so there's no performance concern. But if future callers consistently ignore context_window, it might read more clearly to offer a thin _threshold_for(model_name, ratio) alias. Not required — the current form is correct and the _ convention is idiomatic Python.

---

Observation: Test could assert the positive None → null transform

In test_non_string_tool_return_serialized_as_json, the None → null fix is checked only negatively:

assert "None" not in part.content

Adding the corresponding positive assertion makes the discriminator symmetric and more readable to future readers:

assert "null" in part.content      # JSON null, not Python None
assert "None" not in part.content

---

What's working well

• _stringify_tool_content — clean, defensive, correctly handles default=str for non-serialisable scalars. Using it in both the estimator and the shrinker ensures estimate ↔ payload consistency, which was the original correctness gap.
• context_window_and_threshold — good single-definition refactor; the docstring makes the design intent clear to future maintainers. The __all__ update is not forgotten.
• Test for finding Bump postgres from 14.5 to 15.0 in /compose/production/postgres #1 — the fixture improvement is the right approach: the old test proved nothing because the processor exited early. The three-part assertion structure (thinking-only intact, sibling shrunk, telemetry counters) gives real confidence in the guard path.
• IN_RUN_TOOL_RETURN_TARGET_CHARS documentation — the clarification that the constant bounds the prefix, not the final string, is exactly the kind of "hidden constraint" comment CLAUDE.md asks for.
• Changelog — entries are well-placed, specific, and reference files by path.

The RecursionError gap is the only real correctness concern; the rest are style or test-completeness suggestions. Happy to approve once that's addressed (or if the team judges circular tool returns impossible by design).

[claude]

Code Review: Compaction Follow-up (#1824)

Clean, well-scoped PR. All five findings were correctly triaged, and the two correctness fixes are real improvements. A few minor observations below.

---

Overview

This follow-up correctly addresses the five code-review findings from #1817:

• Bump traefik from v2.8.7 to v2.9.1 in /compose/production/traefik #2 (correctness): str() to json.dumps() for non-string ToolReturnPart.content — the most impactful change.
• Bump postgres from 14.5 to 15.0 in /compose/production/postgres #1 (test correctness): Strengthened the guard test that was passing for the wrong reason.
• DRY: context_window_and_threshold consolidates the duplicated threshold arithmetic into one place.
• Bump actions/setup-node from 3.4.1 to 3.5.1 #3 and Bump actions/checkout from 3.0.2 to 3.1.0 #5 (docs): Clear constant annotation and closure-stability comment.

---

Correctness

_stringify_tool_content (history_processors.py:88) — well-designed. The json.dumps(content, default=str) path is the right fix; it matches the existing convention in pydantic_ai_agents.py. Good that it is shared by both the token estimator (line 121) and the shrink (line 194) so the pre-shrink estimate and post-shrink payload are serialised identically.

RecursionError catch — intentional and documented, but worth a second look: with default=str, json.dumps already absorbs non-serialisable scalars, so TypeError and ValueError are effectively unreachable in practice. Only RecursionError (circular self-reference) can still escape. Catching the other two is harmless but slightly misleading — the comment says "absorbs stray non-JSON-serialisable values" which is what default=str does, not what the except clause adds. Could tighten to just except RecursionError, but this is a nit.

context_window_and_threshold (context_guardrails.py:126) — confirmed context_window is still consumed at line 339 (InRunShrinkEvent construction), so the tuple unpack is not introducing an unused variable. Flake8 would catch that anyway, but good to confirm.

---

Tests

test_thinking_only_modelresponse_is_not_emptied — the strengthened fixture is correct. Placing the large ToolReturnPart alongside the thinking-only response forces the real shrink path, and the three-part assertion structure (guard survives, sibling truncated, telemetry confirms) gives tight coverage. This is a meaningful improvement over the original.

test_non_string_tool_return_serialized_as_json — well-designed discriminator test. Using Python repr True/None vs JSON true/null as the distinguishing signals is clean and unambiguous. The double-quoted vs single-quoted key check reinforces it.

One minor fragility: the "None" not in part.content assertion would fail if the trim-notice template ever happened to contain the word "None". It will not with the current numeric/fixed-text notice format, but a short comment noting why this is safe would future-proof the assertion.

context_window_and_threshold test coverage — the new function has no dedicated unit test, but it is exercised transitively through TestShouldCompact and TestCompactMessageHistory (the author's 61/61 run). For a one-liner wrapper this is fine.

_stringify_tool_content test coverage — tested only indirectly through the integration test. Acceptable for a private helper, and the new JSON test pins the observable behaviour. A small direct unit test would make regressions on the helper itself immediately obvious, but not a blocker.

---

Documentation and Style

All good. The constant annotation, _shrink_tool_return_part docstring update, and _stream_core closure comment each add real signal. The CHANGELOG entries are precise (file paths, line numbers, behavioural impact) and well-placed.

---

Summary

Approve with nits. The correctness fix is valid and the test improvements close the gap the original review identified. The only actionable suggestions are:

1. Consider narrowing the except clause in _stringify_tool_content to just RecursionError (since default=str already handles TypeError/ValueError).
2. Add a comment on the "None" not in part.content assertion explaining why the trim notice cannot contain that string.

Neither is a blocker.

[claude]

Code Review - PR 1853. Overview: Focused, well-scoped follow-up to PR 1817 in-run history compaction. Addresses five findings: a correctness bug (str vs json.dumps), a broken test, a DRY violation, and two documentation gaps. All changes are surgical. No blocking issues. Ready to merge. Full review to follow in next comment.

[claude]

Correctness Fix - _stringify_tool_content (history_processors.py): The bug was real -- str on a dict produces Python repr (single-quoted keys, True/None literals) that a model could misparse. json.dumps produces valid JSON (true/null, double-quoted keys). The exception triple (TypeError, ValueError, RecursionError) is well-chosen; inline comments justify each. Consistency win: _stringify_tool_content is shared by both _estimate_total_tokens and _shrink_tool_return_part, so the pre-shrink estimate and post-shrink payload are serialised identically. Consistent with the existing json.dumps(tool_result, default=str) convention at line 2110 of pydantic_ai_agents.py. Minor note: ValueError may not always be reachable with Python default encoder (float nan encodes as NaN without raising) -- the comment overstates reachability slightly, but keeping it is defensible.

[claude]

DRY Refactor - context_window_and_threshold (context_guardrails.py): Clean extraction. The three callers (should_compact, compact_message_history, shrink_old_artifacts_processor) all computed int(get_context_window_for_model(model_name) * threshold_ratio) identically. The tuple return (window, threshold) with callers using _, threshold is idiomatic -- the two turn-level callers only need threshold; the in-run processor genuinely uses both. Added to all correctly. from future import annotations is already at line 19 so the tuple[int, int] return type annotation is safe on Python < 3.9.

[claude]

Test Fix - test_thinking_only_modelresponse_is_not_emptied: The original test was genuinely broken as described -- with no shrinkable content in the older prefix, the processor hit the early-return and the test passed trivially via no-op, not via the empty-parts guard it claimed to test. Adding the large ToolReturnPart forces the processor to enter the shrink path. The three-part assertion structure (a/b/c) is easy to follow. assert len(deps.events) == 1 is a tight assertion that verifies exactly one shrink event fired, not just at least one.

[claude]

New Test - test_non_string_tool_return_serialized_as_json: Well-designed discriminator test. Using True/None in the payload gives a clean true/null vs True/None distinction that str() cannot produce. One observation: the assertion checking that the string None does not appear in part.content relies on _TRIM_NOTICE_TEMPLATE not containing the literal word None. This holds today and the inline comment documents the assumption. A more robust alternative would check only the JSON prefix before the trim notice marker, but the current approach is acceptable given the documentation. tool_call_id correlation preserved across serialise+shrink is a nice extra assertion.

[claude]

Documentation Changes: IN_RUN_TOOL_RETURN_TARGET_CHARS docstring clarification (target bounds the preserved prefix, not the final string) is accurate and useful for anyone tuning the constant down to a tight ceiling. The _on_in_run_shrink closure comment about late-binding stability is correct: _stream_core handles a single user turn and neither user_msg_id nor llm_msg_id is reassigned after the closure is defined. Overall verdict: no blocking issues. The str-to-json.dumps fix is a genuine correctness improvement consistent with existing conventions. The DRY refactor is byte-for-byte identical arithmetic. The test fix means the test now actually exercises the guard it claims to test. Ready to merge.

[claude]

Code Review

This is a solid, targeted follow-up that addresses the five findings cleanly. The changes are small, self-contained, and well-documented. A few observations:

---

What works well

_stringify_tool_content helper (history_processors.py:88–115)
The three-case exception tuple (TypeError, ValueError, RecursionError) is fully justified by the inline comment — this is one of the rare cases where documenting the why on each exception is genuinely useful rather than noise. The shared use by both the token estimator and the shrink path (_estimate_total_tokens + _shrink_tool_return_part) is exactly right: the pre-shrink estimate and the post-shrink payload now use identical serialisation, removing the silent mismatch that existed before.

context_window_and_threshold (context_guardrails.py:126–146)
Clean DRY extraction. Destructuring (_, threshold) at the two call sites where only threshold is needed is idiomatic Python. The shrink_old_artifacts_processor call sites correctly destructure both values since context_window is also used for the telemetry payload, which is the right pattern.

Test fixture strengthening (test_thinking_only_modelresponse_is_not_emptied)
The three-part assertion structure (a/b/c) makes it clear exactly why the ThinkingPart survived. The telemetry check (evt.tool_returns_shrunk == 1, evt.thinking_parts_dropped == 0) is the strongest possible discriminator between "guard fired" and "early-return no-op".

test_non_string_tool_return_serialized_as_json
Using True/None as discriminators between json.dumps and str() serialisation is elegant — it's a minimal payload that gives a concrete, readable assertion rather than a length check.

---

Minor observations

_stringify_tool_content — bytes input
ToolReturnPart.content is typed Any. If a tool ever returns bytes (raw binary response, base64 payload), json.dumps(b"...") will raise TypeError and fall through to str(b"..."), yielding "b'...'" with the b' prefix. That's probably fine for the shrink path (the model will see something, not crash), but it's worth noting if tools ever return binary content. No action needed unless that pattern exists in the codebase — just something to keep in mind.

context_window_and_threshold — should_compact discards context_window
should_compact uses _, threshold = context_window_and_threshold(...) and never uses context_window. The helper calls get_context_window_for_model unconditionally. This is a negligible cost (a dict lookup), but if should_compact is on a hot path (called per-message), the underscore discard is correct as written — just worth acknowledging there's a tiny hidden cost relative to the pre-refactor inline call.

Missing __all__ export in history_processors.py
_stringify_tool_content is correctly name-mangled as private (leading underscore), so no __all__ entry needed. But InRunShrinkEvent is a public dataclass used by callers setting deps.on_in_run_shrink. It's exported via import in pydantic_ai_agents.py already, so no issue — just confirming the current state is correct.

Changelog whitespace nit
The diff removes a blank line before - **Corpus document versioning...** in the Fixed section (around line 128 of CHANGELOG.md). This appears unrelated to the PR's changes and leaves the bullet list slightly less visually grouped. Trivially cosmetic, but if the intent was formatting cleanup, it's worth keeping consistent.

---

Test coverage assessment

• Finding Bump postgres from 14.5 to 15.0 in /compose/production/postgres #1: test_thinking_only_modelresponse_is_not_emptied now genuinely exercises the empty-parts guard path. Telemetry assertions make it brittle-proof (the test will fail if the guard is accidentally removed). ✓
• Finding Bump traefik from v2.8.7 to v2.9.1 in /compose/production/traefik #2: test_non_string_tool_return_serialized_as_json pins the json.dumps path with discriminating assertions. ✓
• Finding Bump actions/setup-node from 3.4.1 to 3.5.1 #3: Documented on the constant and in the docstring — no executable assertion possible for a documentation-only clarification. Appropriate. ✓
• Finding Bump actions/checkout from 3.0.2 to 3.1.0 #5: Comment added. Appropriate — the late-binding behaviour is provably benign within a single turn, so a comment is the right artefact, not a test. ✓

The reported 14/14 and 61/61 pass counts are consistent with the scope of changes. No new async tool calls were introduced, so no risk of SynchronousOnlyOperation from the CLAUDE.md pitfall list.

---

Summary

All five findings are correctly addressed. The correctness fix (Finding #2, str() → json.dumps) is the most impactful and is properly guarded by a pinning test. The DRY refactor (context_window_and_threshold) is behaviour-identical and verified by the existing 61-test suite. No security, performance, or architectural concerns.

Approved.

[claude]

Code Review

Summary: This is a solid follow-up PR that correctly addresses all five code-review findings from #1817. The changes are well-scoped, the fixes are correct, and the test improvements genuinely test what they claim to. Below are a few minor observations worth noting before merge.

---

Strengths

• _stringify_tool_content is well-designed. The choice of json.dumps(content, default=str) over str() is the right fix — Python repr with single-quoted keys and True/None literals is not something the model should receive. Using the same helper in both the estimator and the shrink path so pre-shrink estimates and post-shrink payloads are serialised identically is a particularly nice touch.

• context_window_and_threshold cleanly removes the triplication. The arithmetic was duplicated verbatim in three places; having one definition makes it impossible for the callers to diverge. The _, threshold = unpacking idiom at the two callers that only need the threshold is idiomatic and readable.

• Strengthened test is now actually testing the guard. The old fixture hit the no-op early-return, so test_thinking_only_modelresponse_is_not_emptied was never running the guard path it named. The telemetry assertion at (c) — evt.tool_returns_shrunk == 1, evt.thinking_parts_dropped == 0 — is the right discriminator.

---

Minor issues

1. Inaccurate comment on RecursionError in _stringify_tool_content (history_processors.py)

The comment currently says:

# - RecursionError: a self-referential (circular) tool return.

With CPython's default check_circular=True, a circular reference raises ValueError: Circular reference detected, not RecursionError. RecursionError is raised for very deeply nested (non-circular) structures that exhaust Python's call stack before the encoder finishes. The exception is still correctly caught, but the comment attributes it to the wrong scenario. Suggested rewording:

# - RecursionError: a very deeply nested (non-circular) tool return that
#     exhausts Python's call stack inside the encoder. (Circular
#     references with the default check_circular=True raise ValueError
#     instead, which the case above already catches.)

2. test_thinking_only_modelresponse_is_not_emptied: structurally invalid conversation (cosmetic, not a bug)

The new fixture places a ModelRequest(ToolReturnPart) immediately after a ModelResponse(ThinkingPart), but the ThinkingPart response issued no ToolCallPart, so there is no tool call the return could correspond to. The processor doesn't validate conversation structure, so the test is correct and passes, but a future reader might be confused. A brief comment would help:

# Structurally odd (no matching ToolCallPart) but valid for this unit
# test — the processor only cares about the presence of a shrinkable
# ToolReturnPart, not conversation pairing.

3. test_non_string_tool_return_serialized_as_json: implicit dict-ordering assumption

The assertions that "flag", "true", and "null" appear in the truncated output rely on Python 3.7+ dict insertion-order guarantees — the small keys come first in the JSON string, before the 700 000-char blob eats all the budget. This is fine given the project's minimum Python version, but a one-line comment noting the dependency would make the test more self-explanatory:

# Dict iteration order (Python 3.7+) means flag/missing come before
# blob in the JSON, so both appear in the preserved 4 K prefix.

4. context_window_and_threshold: no direct unit test (optional)

The function is tested indirectly through its three callers, which is sufficient. A direct test would be trivial but is entirely optional given the function's simplicity.

---

Verdict

Ready to merge with minor nit on the RecursionError comment (item 1). Items 2–4 are optional polish. No behaviour regressions, no security issues, no architectural concerns.