fix(compression): preserve snapshot role alternation - #116960
fangliquanflq wants to merge 10 commits into
Conversation
kvnloo
left a comment
There was a problem hiding this comment.
[P1] Do not fold the durable TODO snapshot into ephemeral scaffolding while retaining the scaffolding flag.
The new else branch handles any trailing synthetic user row, including _empty_recovery_synthetic, _verification_stop_synthetic, _pre_verify_synthetic, and _dropped_toolcall_nudge, by appending the TODO text and preserving that flag.
Those flags have stronger downstream semantics than “not a real human turn”:
agent/session_persistence.py::_EPHEMERAL_SCAFFOLDING_FLAGSmakes the entire row non-durable._todo_snapshot_syntheticis deliberately NOT in that tuple.agent/turn_final_response.pypops a trailing row carrying any of those ephemeral flags before the final response is appended.
So, for the added test's exact shape:
{"role":"user",
"content":"Continue after recovering...",
"_empty_recovery_synthetic": true}
this PR folds the live TODO snapshot into that row, leaves _empty_recovery_synthetic=true, and the next persistence/finalization boundary can skip/pop the snapshot together with the recovery nudge. The role-alternation assertion goes green, but the post-compression TODO-retention contract can be lost.
The old comment's “scaffolding tails must not absorb it” was load-bearing for more than provenance: the two payloads have different lifecycles.
Please keep those lifecycles separate. The regression should drive a real ephemeral-tail lifecycle, not only call _fold_todo_snapshot twice:
- start with an
_empty_recovery_synthetic(and ideally verification-nudge) user tail; - fold a pending TODO snapshot;
- run the production persistence/final-response cleanup boundary;
- assert the ephemeral nudge disappears but the TODO snapshot remains represented in the durable/live post-compression context, with strict role alternation preserved.
I would not clear the ephemeral flag just to keep the row: that would turn internal retry/verification text into durable user evidence, which #69292/#65919 explicitly prevent. The fix needs a sequence shape that preserves both strict alternation and the distinct durable-vs-ephemeral ownership of the two pieces.
|
The production change is broader than the new regression, so there is one useful test-shape upgrade before this becomes a strong salvage atom. The new That predicate is not just
The new regression proves only the The smallest stronger invariant would parameterize representative synthetic categories, not add more implementation:
For each case, assert:
That pins the actual contract this hunk changes: every synthetic user tail may absorb the snapshot without becoming human intent or breaking role alternation, rather than only one current producer of synthetic rows. |
…snapshot-alternation # Conflicts: # agent/conversation_compression.py
|
Follow-up after the new commits: the broader synthetic-tail coverage exposed a real dependency that changes the salvage shape of this PR.
Its core fix folds the TODO snapshot into an ephemeral synthetic user row to preserve strict role alternation. But those retry/verification rows are intentionally removed later by persistence/final-response cleanup. Without the later work, the fix can therefore preserve alternation while silently dropping the durable TODO state. The two follow-up commits are correctness dependencies, not optional cleanup:
So the integration unit is now the three-commit chain (or a different simpler implementation that proves both invariants):
The new parameterized synthetic-category regression plus It would also help to update the PR summary before merge/carrierization, because the current body still reads like a two-file role-alternation fix while the branch now changes the durable compaction/persistence contract across five files. |
|
Addressed all three points.
Validation:
|
Summary
Testing
scripts/run_tests.sh tests/agent/test_compression_rotation_state.py -k TodoSnapshotScaffoldingTails(16 passed)scripts/run_tests.sh tests/agent/test_verification_continuation_budget.py tests/agent/test_salvage_grown_transcript.py tests/agent/test_message_sequence_repair.py(75 passed)Related Issue
Closes #116880