Prevent duplicate Deep Agents messages after continue-as-new - #1804
Prevent duplicate Deep Agents messages after continue-as-new#18041fanwang wants to merge 10 commits into
Conversation
Signed-off-by: 1fanwang <1fannnw@gmail.com>
Signed-off-by: 1fanwang <1fannnw@gmail.com>
The merged message list feeds user agent code whose control flow can branch on it, so replaying a prepend-recorded continuation under replace semantics can emit different commands. Reproduced with a recorded chain: an agent that completed because the duplicated input pushed it over its length threshold replays into a ContinueAsNew command the history does not have (TMPRL1100). patched() keeps pre-upgrade histories on prepend semantics; new executions carry the transcript without duplication. Old history replay and new-code round-trip both verified.
There was a problem hiding this comment.
🟡 Changes recommended
Empty snapshots remain incorrectly restored, backward replay lacks coverage, and the changelog entry is malformed.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Fixes duplicate Deep Agents messages after continue-as-new while preserving replay compatibility.
Changes:
- Replaces input messages with the saved snapshot behind a workflow patch.
- Strengthens continue-as-new regression assertions.
- Documents the fix in the changelog.
File summaries
| File | Description |
|---|---|
temporalio/contrib/deepagents/workflow.py |
Adds patch-gated snapshot restoration. |
tests/contrib/deepagents/test_continue_as_new.py |
Verifies exact message history across rollovers. |
CHANGELOG.md |
Documents the corrected behavior. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 3
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Replace-the-messages merge semantics were wrong at the API level: an externally supplied state_snapshot plus a fresh input message silently dropped the user's new message, and a transcript deliberately compacted to empty resurrected the original input. Instead, keep the prepend merge exactly as before and strip the messages from the input the driver threads through continue_as_new — the snapshot already carries the full transcript, so the internal path resumes without duplicating the original prompt (and without carrying the transcript twice in the payload), while external resume composes carried history plus the new message. This also removes the need for any patch gate: the merge function is unchanged, so histories recorded before this change replay through it identically (verified by recording a duplicating chain on main and replaying its final run here). Also repairs the CHANGELOG entry that a bad merge had split across unrelated bullets, aligns SlowFakeAgent's completion check with FakeAgent's step-count style, and adds unit tests for both merge paths (external snapshot + new message; stripped internal carry).
|
Deep review done (same treatment as #1806) - another valid bug here but a couple changes made: Round 1 - replay determinism (verified empirically). The merged message list feeds user agent code whose control flow branches on it, so replaying a prepend-recorded continuation under replace semantics can emit different commands. I recorded a chain on Round 2 - replace semantics are wrong at the API level, so the gate got superseded. Two confirmed holes: an external caller resuming with a saved Also on the branch: merged latest |
Two regressions in the strip mechanism, both caught by review and
pinned by new e2e tests:
- A non-Mapping input collapsed to {} at continue-as-new, so a
str-typed @workflow.run signature failed argument decoding on the
continued run (permanent workflow-task retry). The carry now derives
from the ORIGINAL input — not the merged dict reassigned on resume,
which also re-prepended carried messages on later boundaries — and a
bare prompt travels as-is with a snapshot marker telling
_merge_snapshot not to re-append it.
- A turn ending with pending todos but an EMPTY transcript lost the
original prompt (the stripped input met an empty snapshot). The
strip now applies only when the transcript is non-empty; otherwise
the input is re-sent unchanged, matching pre-change behavior.
New tests: bare-string input across two boundaries with a str-typed
signature; the continue-as-new command's args verifiably lack
"messages" (transcript rides once, in the snapshot); empty-transcript
carry preserves the prompt. Replay verified against a prepend-era
recorded chain and a new-code round-trip. Also hoists the duplicated
function-local _merge_snapshot imports.
…ompares values - A second-or-later hop returning pending todos with an empty/pruned transcript lost the whole conversation (the stripped carry had no fallback the old merged-input carry provided). The snapshot's messages now fall back to the conversation the agent SAW (the merged input) whenever the result carries none. - The bare-prompt marker now records the carried input VALUE and the merge skips re-appending only on equality, so an externally harvested snapshot plus a fresh prompt composes as documented instead of silently dropping the new prompt. - The empty-transcript test was vacuous (its agent completed on the first turn); it now tracks turns via an activity-backed disk counter, forces a real boundary, and asserts CONTINUED_AS_NEW.
Under cache eviction (e.g. max_cached_workflows=0) a backend_op activity scheduled just before the eviction can start after the evicted TemporalBackend wrapper is garbage-collected, and the replay that would re-register the ref only happens once that activity completes. The GC finalizer now retires the entry into a bounded store that the activity's lookup falls back to, instead of dropping it outright.
TLDR:
run_deep_agentre-appended the original input messages on every continue-as-new rollover, so the initial user prompt appeared again after each boundary. Fixed at the source: the driver stripsmessagesfrom the input it threads throughcontinue_as_new(the snapshot already carries the full transcript), leaving the merge function untouched — so old histories replay identically with no patch gate, and an externally suppliedstate_snapshotplus a fresh input message still composes correctly.What was changed
run_deep_agent's continue-as-new re-invocation carries the input without itsmessages(they are already in the snapshot's transcript). The prepend merge in_merge_snapshotis unchanged.state_snapshotplus a new user message would silently lose that message, and a transcript deliberately compacted to[]would resurrect the original input. Strip-at-source fixes the duplication without either, and halves the CAN payload (the transcript no longer rides in both args).workflow.patched()gate needed: the merge path is byte-identical for inputs recorded under the old code. Verified by recording a duplicating chain onmainand replaying its final run through this branch (clean); the earlier replace+gate iteration was verified the same way before being superseded.Testing
Exact-list regression tests across forced continue-as-new boundaries (fixed threshold and server-suggested modes), unit tests for both merge paths (external snapshot + new message preserved; stripped internal carry resumes from the snapshot alone), record-and-replay verification in both directions. Full deepagents suite: 34 passed.