Skip to content

fix(agent): mark handled tool output final - #721

Open
bogdanovich wants to merge 2 commits into
mainfrom
fix/response-handled-final-marker
Open

fix(agent): mark handled tool output final#721
bogdanovich wants to merge 2 commits into
mainfrom
fix/response-handled-final-marker

Conversation

@bogdanovich

Copy link
Copy Markdown
Owner

Summary

  • mark response-handled tool delivery as final outbound output
  • preserve immediate-continue tool delivery as interim
  • cover the durable channel-manager path used by user-only delegation

Validation

  • CGO_ENABLED=0 go test -tags goolm,stdjson ./pkg/agent ./pkg/channels/mintclaw ./cmd/mintclaw/internal/agent

Regression evidence

A live first-party browser delegation completed its child turn and parent turn, but mintclaw agent live waited until its outer timeout because the user-only handled output had no final outbound marker. Trace trace-turn-4527da22779651f94037e5a7 records the child result at +110860ms and parent end at +110982ms.

@bogdanovich bogdanovich left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Automated review by Reviewer agent.

Comment thread pkg/agent/agent_outbound.go Outdated
bus.OutboundMetadata{OutboundKind: bus.OutboundKindInterim}.ApplyToContext(outboundCtx)
kind := ""
switch {
case result.ResponseHandled || result.DeliveryIntent == toolshared.DeliveryFinalHandled:

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Automated review note: This marks every response-handled result final before the delivery route is known. For implicit media without a direct channel-manager route, applySyncToolResultDelivery queues the media and then clears ResponseHandled so the model produces a follow-up answer. The queued media still carries outbound_kind=final, so browser/live consumers can terminate before that later answer arrives. Select the marker using the effective post-route ownership, keeping direct handled media final but queued media that triggers model fallback interim, and assert the metadata in the existing queued-media fallback test.

Comment thread pkg/agent/agent_outbound.go Outdated
bus.OutboundMetadata{OutboundKind: bus.OutboundKindInterim}.ApplyToContext(outboundCtx)
kind := ""
switch {
case result.ResponseHandled || result.DeliveryIntent == toolshared.DeliveryFinalHandled:

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Automated review note: The final branch runs before DeliveryIntent == immediate_continue. WithResponseHandled().WithImmediateDelivery() is a valid construction that leaves ResponseHandled true while setting the canonical intent to immediate-continue; this code now labels it final even though the turn continues. Before this change, ImmediateDelivery made the output interim. Check a non-empty canonical intent first, then fall back to legacy fields, and preserve interim behavior for legacy conflicts.

@bogdanovich

Copy link
Copy Markdown
Owner Author

Automated review by Reviewer agent.

Review Summary

The text path is fixed, but the new final-marker selection can terminate clients too early for queued handled media and for canonical immediate-continue results carrying a stale legacy handled flag.

Findings

  1. [major] Do not mark queued media final when delivery falls back to the model (pkg/agent/agent_outbound.go:696)

This marks every response-handled result final before the delivery route is known. For implicit media without a direct channel-manager route, applySyncToolResultDelivery queues the media and then clears ResponseHandled so the model produces a follow-up answer. The queued media still carries outbound_kind=final, so browser/live consumers can terminate before that later answer arrives. Select the marker using the effective post-route ownership, keeping direct handled media final but queued media that triggers model fallback interim, and assert the metadata in the existing queued-media fallback test.

  1. [major] Give canonical immediate intent precedence over stale legacy flags (pkg/agent/agent_outbound.go:696)

The final branch runs before DeliveryIntent == immediate_continue. WithResponseHandled().WithImmediateDelivery() is a valid construction that leaves ResponseHandled true while setting the canonical intent to immediate-continue; this code now labels it final even though the turn continues. Before this change, ImmediateDelivery made the output interim. Check a non-empty canonical intent first, then fall back to legacy fields, and preserve interim behavior for legacy conflicts.

@bogdanovich

Copy link
Copy Markdown
Owner Author

Automated review by Reviewer agent.

Review Summary

No high-confidence issues found. The current head resolves both prior findings: queued implicit handled media is marked interim before model fallback, and canonical immediate-continue intent takes precedence over stale legacy handled state. Prior findings are resolved.

The previously reported findings are resolved in the current PR head.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant