Skip to content

fix(agent): guard merged assistant compaction handoffs - #85394

Closed
abundantbeing wants to merge 2 commits into
NousResearch:mainfrom
abundantbeing:fix/compaction-merged-assistant-handoff
Closed

fix(agent): guard merged assistant compaction handoffs#85394
abundantbeing wants to merge 2 commits into
NousResearch:mainfrom
abundantbeing:fix/compaction-merged-assistant-handoff

Conversation

@abundantbeing

@abundantbeing abundantbeing commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

What does this PR do?

Extends the #80622 active-turn guard to the merged assistant carrier shape that still escaped it.

When compaction merges its summary into an assistant tail row, the completed carrier identifies itself with role="assistant", classify_summary_content(...) == "merged", and its own finish_reason="stop". That carrier's preserved prose is historical assistant output, not a fresh user request.

The guard is deliberately narrow:

  • only a merged assistant carrier with its own completed finish_reason="stop" is reference-only
  • a merged carrier with pending tool_calls remains live and must continue the tool chain
  • a merged carrier without the completed-stop boundary remains in flight
  • a real user turn after the carrier still proceeds
  • a distinct later assistant tool-call row still proceeds
  • standalone handoffs continue to use the existing guard

This avoids the rejected adjacent-row heuristic: the message before a merged carrier may be unrelated, while the carrier's own finish state is the direct production invariant.

Issue linkage

Tests

RED first:

  • completed merged carrier reproduced as an unintended next-model-call driver
  • pending merged tool-call carrier reproduced as a false reference-only stop

GREEN:

  • 20 passed in the focused active-turn suite
  • 195 passed across the adjacent compaction surface
  • ruff clean
  • compileall clean
  • git diff --check clean

Checklist

  • I have read the contributing guidelines
  • This PR targets main
  • pytest tests/ -q passes locally
  • Relevant comments updated where needed
  • No dependencies added
  • The diff is limited to the active-turn guard and regression tests

I ran the complete adjacent compaction surface rather than claiming an unrelated full-suite run.

@abundantbeing

Copy link
Copy Markdown
Contributor Author

CI's only failure is unrelated to this two-file compaction patch: test_run_prompt_submit_requeues_all_unstarted_notifications_with_real_threading in tests/test_tui_gateway_server.py timed out while requeueing proc_batch_3 (550 tests in that slice passed). The touched compaction surface is locally green at 195/195. I do not have upstream permission to rerun the failed job.

@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint area/compression Context compression and continuation sessions sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state labels Aug 13, 2026
@abundantbeing

Copy link
Copy Markdown
Contributor Author

Latest-head CI has one unrelated failure in tests/run_agent/test_primary_runtime_restore.py::TestTryRecoverPrimaryTransport::test_allowed_for_nous_anthropic_messages: the runner resolves an empty Nous model to a cached 32,000-token context window, then agent_init.py correctly enforces the 64,000-token minimum. The same untouched test fails with the same traceback on both #85394 and #85442 even though #85394 changes only agent/context_compressor.py plus its focused test and #85442 changes only gateway/platforms/api_server.py plus its projection test. All other Python slices, OS jobs, lints, security checks, E2E, and Docker builds passed; the touched surfaces remain locally green at 195/195 for #85394 and 52/52 for #85442.

@Enough1122

Copy link
Copy Markdown
Contributor

AI code review — automated review for reference, author can ignore or act on any point.

PR: fix(agent): guard merged assistant compaction handoffs

  1. finish_reason == "stop" is provider-specific — backends that emit "end_turn"/"eos"/"length" instead of "stop" would classify a completed merged carrier as still-live, reverting to the pre-fix skip behavior for those providers. Consider normalizing finish_reason or treating any non-"tool_calls" value as completed.
  2. classify_summary_content(message.get("content")) runs for every assistant message in the scan loop, including ones where _handoff_carries_live_user_content will be False (wasted classification) and ones whose content may be None (pure tool-call rows). Confirm classify_summary_content handles None gracefully — if it assumes a string, a None-content carrier could raise mid-scan.
  3. The and not message.get("tool_calls") guard means a completed carrier (finish_reason == "stop") that nevertheless retains a stale tool_calls array is treated as live — correct and defensive; the test test_live_tool_call_carrier_after_completed_stop_stays_in_flight covers it. Good.
  4. Behavioral note: this changes the semantics of last_driving_handoff for merged carriers — a completed merged assistant handoff now drives the next model call even without an adjacent user stop. The tests cover the main shapes; a test with a standalone summary followed by a merged carrier (both in one message list) would confirm the loop picks the later driver as intended.

@abundantbeing
abundantbeing force-pushed the fix/compaction-merged-assistant-handoff branch 2 times, most recently from bf55f4a to db16c76 Compare August 19, 2026 15:34
@abundantbeing

Copy link
Copy Markdown
Contributor Author

CI note for the maintainers: the failing slice on this head is unrelated to this PR's diff and reproduces on clean main (b5455fdd1):

  • tests/tools/test_image_generation.py::TestFalCatalog::test_upscale_defaults_are_all_off — reproduced locally against the branch's code, which does not touch test_image_generation.py or the FAL catalog; the assertion fails on the xai/grok-imagine-image/v2.0 catalog entry itself.
  • Clean main's own CI also reports a failing Python-tests slice (slice 12/12 on b5455fdd1).
  • Other flakes observed in the same runs: test_transcription_tools.py::TestRunCommandSttIdleTimeout (timing), test_goal_continuation_drain.py (queue key), and a GitHub 429 during checkout.

A CI re-run (maintainer-only; the fork token gets 403 on actions/rerun) should clear it. Happy to force-push a no-op refresh if that's easier.

@abundantbeing
abundantbeing force-pushed the fix/compaction-merged-assistant-handoff branch from b3cfbcf to 746a7d2 Compare August 20, 2026 00:46
Treat a merged assistant-role summary carrier as the driving reference handoff when it immediately follows a completed assistant stop. Its preserved prose and stale tool_calls are assistant continuity, not a fresh live user request.

Keep legitimate in-flight behavior unchanged when there is no completed stop, a real user turn follows, or a distinct later assistant tool-call row continues the loop.

Extends the NousResearch#80622 active-turn guard for the merged-carrier shape reported under NousResearch#42768.
Identify a completed merged assistant handoff from the carrier's own stop state instead of an unrelated adjacent history row. Keep carriers with pending tool calls actionable so compaction cannot abort a live tool chain.

@andrexibiza andrexibiza left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed exact head 57f567adf3bc9acfd1ad9547592f710c98331cde.

Code verdict: no blocking findings.

The production trace supports the narrow predicate:

  • merge-into-tail preserves the assistant carrier's own normalized finish_reason and tool_calls;
  • role="assistant" + merged-summary classification + finish_reason="stop" + no tool_calls therefore identifies completed historical assistant output rather than a live request;
  • pending carrier tool calls, a later real user turn, and a distinct later assistant tool-call row remain actionable.

The regression matrix covers both sides of that boundary. Provider-native stop reasons are normalized before this row is persisted, so the "stop" check is not Anthropic-specific. Exact-head CI is green across CI, Docker Build/Test/Publish, and Nix flake check.

Merge disposition: #91516 is a content-identical cherry-pick of both commits onto current main (c481a5f4f345eef3ee9435e632a440fa4ed987a8) with @abundantbeing's authorship preserved; both changed-file blob SHAs are identical. This head is now 263 commits behind current main. Merge exactly one vehicle. Given live state, #91516 is the current-base integration copy; close #85394 as superseded after #91516 lands, or update this original onto current main and close #91516. Do not merge both.

Copy link
Copy Markdown
Contributor

Topology closure: #91516 is the sole current-base landing vehicle for this implementation.

Closing #85394 unmerged as superseded would leave one landing authority without erasing the original contribution/review chronology. Do not merge both.

@alt-glitch alt-glitch added the duplicate This issue or pull request already exists label Aug 21, 2026
@kshitijk4poor

Copy link
Copy Markdown
Collaborator

Merged via #91516 — both of your commits cherry-picked onto current main with your authorship preserved (rebase merge, no squash). The guard verified exactly as described: side-by-side E2E against main confirmed the completed merged carrier was re-driving the model, and your fix flips precisely that case while live tool-call carriers stay in flight. 20/20 tests green, mutation-checked. Thanks for the sharp fix, @abundantbeing!

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

Labels

area/compression Context compression and continuation sessions comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint duplicate This issue or pull request already exists P2 Medium — degraded but workaround exists sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants