Python: exclude encrypted reasoning payloads from compaction token counts - #8354
Conversation
…unts The compaction token counter serialises each message and tokenises the JSON. Content.protected_data (OpenAI encrypted_content, Anthropic thinking signature, Gemini thought_signature, OpenAI reasoning_details) is opaque ciphertext the model replays but never tokenises, so counting it inflated every token-aware compaction decision. The same blob can also ride in additional_properties as encrypted_content. Both are now excluded from serialised content, matching the existing exclusions for raw_representation and mirrored items. Fixes microsoft#8348
There was a problem hiding this comment.
🔵 Needs a closer look
Previously cached token counts can remain inflated and must be invalidated or recomputed.
Pull request overview
This PR prevents opaque encrypted reasoning payloads from inflating Python compaction token estimates.
Changes:
- Excludes
protected_dataandadditional_properties["encrypted_content"]. - Adds regression tests for both payload locations.
- Cached token-count annotations still require invalidation or recomputation.
File summaries
| File | Summary |
|---|---|
python/packages/core/tests/core/test_compaction.py |
Adds regression tests for both encrypted payload locations. |
python/packages/core/agent_framework/_compaction.py |
Filters encrypted reasoning payloads during token serialization. |
Review details
Suppressed comments (1)
python/packages/core/agent_framework/_compaction.py:707
- This only changes newly computed counts. Token counts are cached in the
_groupannotation, andannotate_token_countsskips messages whose count is already present; those annotations are also preserved byMessage.to_dict()/from_dict(). A transcript annotated before this fix (or one whose reasoning payload is added after annotation) will therefore continue to use the inflated count and can still trigger premature compaction. Please invalidate/version the cached count when the serialization basis changes, or otherwise force a retokenization for such stale annotations.
payload.pop("protected_data", None)
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Lite (auto)
Note
Copilot is running an experiment and ran this review at Lite.
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
|
@microsoft-github-policy-service agree |
|
Thanks for the update. Before this is ready, could you please:
Once those are addressed, please re-request review. Thanks! |
…ale token caches Review follow-up for the protected-data token exclusion: - reasoning_details stored in protected_data are JSON-serialised and replayed to the provider as clear text (summary, reasoning_text and nested reasoning.text), so only opaque members (encrypted_content) are excluded from the estimate; non-JSON payloads (Anthropic signature, Responses API encrypted blobs) are still dropped entirely. - group token counts are stamped with a serialization basis version; counts cached under an older basis (e.g. sessions annotated before a serialization change) are recomputed instead of reused.
|
Thanks Eduard van Valkenburg (@eavanvalkenburg) Evan Mattson (@moonbox3) — all three points are addressed in efb44d8:
Tests: the two original regression tests remain, plus two new ones (clear-text reasoning counted while opaque members are excluded; stale-basis counts recomputed and re-stamped). Re-requesting review. |
|
Please fix the failing CI/CD checks. |
|
Thanks for the update. The current head is not green: |
- annotate the recursive reasoning-payload filter with explicit dict[Any, Any] / list[Any] casts so iterating the narrowed Any dict does not leak Unknown types under pyright strict - cast additional_properties before the encrypted_content filter - assert non-None token counts in the clear-text regression test so the ordering comparison does not involve optional integers
|
Thanks Eduard van Valkenburg (@eavanvalkenburg) Evan Mattson (@moonbox3) — the pyright/mypy failures are fixed in 48bde32:
Local verification: One note: CI is re-running on the new head; re-requesting review. |
Eduard van Valkenburg (eavanvalkenburg)
left a comment
There was a problem hiding this comment.
Lubaoshuai Reviewed the complete diff and found no actionable issues. All reported checks for the current head are green.
Motivation & Context
The compaction token counter serialises each message to JSON and tokenises the string. That serialisation included provider reasoning payloads: opaque encrypted blobs (OpenAI Responses
encrypted_content, Anthropic thinkingsignature) and the JSON-serialisedreasoning_detailsstored by the Chat Completions client. Encrypted blobs are replayed opaquely and never tokenised by the model, so they inflated local token estimates and could trigger premature compaction (#8348).Description & Review Guide
_serialize_contentexcludes provider-encrypted reasoning payloads from the token-estimation serialisation:protected_data/additional_properties["encrypted_content"]blobs are dropped.reasoning_details(Chat Completions) keep their clear-text members (summary,reasoning_text, nestedreasoning.text) — the client replays those to the provider as input, so only their opaqueencrypted_contentmembers are excluded (review follow-up; previously the whole structure was dropped).token_count_basis). Counts cached under an older basis — e.g. sessions annotated before this change, since annotations surviveMessage.to_dict()/from_dict()whileannotate_token_countsskips messages that already have a count — are recomputed instead of reused (review follow-up)._strip_opaque_reasoning_payload) and the basis-version handling in_write_group_annotation/_token_count/_write_token_count.Related Issue
Fixes #8348
Contribution Checklist
breaking changelabel (or add "[BREAKING]" to the title prefix, before or after any language prefix) — a workflow keeps the label and title prefix in sync automatically.