Skip to content

fix: ensure execution task result is shown #3629

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
lifeizhou-ap merged 3 commits into from
Jul 25, 2025
Merged

fix: ensure execution task result is shown #3629

lifeizhou-ap merged 3 commits into from
Jul 25, 2025

Conversation

@lifeizhou-ap
Copy link
Contributor

@lifeizhou-ap lifeizhou-ap commented Jul 24, 2025

Why
fix #3628

What

  • Add extra sleep to make sure the notification has been received and displayed
  • Extract checking token is cancelled to the util module

@lifeizhou-ap lifeizhou-ap requested a review from DOsinga July 24, 2025 14:07
DOsinga
DOsinga approved these changes Jul 24, 2025
View reviewed changes
crates/goose/src/agents/subagent_execution_tool/task_execution_tracker.rs

self.try_send_notification(event, "tasks complete");
// Wait for the notification to be recieved and displayed before clearing the tasks
sleep(Duration::from_millis(COMPLETION_NOTIFICATION_DELAY_MS)).await;
Copy link
Collaborator

@DOsinga DOsinga Jul 24, 2025

Choose a reason for hiding this comment

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

sounds good, but are you saying the message doesn't get there if we complete here?

Copy link
Contributor Author

@lifeizhou-ap lifeizhou-ap Jul 25, 2025

Choose a reason for hiding this comment

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

The extra sleep is to handle the race condition.

The notification flow:

  1. TaskExecutionTracker → try_send → mpsc channel queue
  2. ReceiverStream reads from queue → agent's event stream
  3. Session loop processes from agent stream → CLI display

When the last task completes, notifications is sent async and the sub agent execution tool completes. The extra sleep is to ensure the asynchronous notification processing completes before the tool returns its result to the agent.

Without sleep:
T=1ms: Notifications queued in channel
T=2ms: Tool returns immediately (no sleep)
T=15ms: Agent stream terminates
T=15ms: Session loop exits
T=16ms: Channel dropped with notifications inside

Copy link
Collaborator

@michaelneale michaelneale Jul 25, 2025

Choose a reason for hiding this comment

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

I think this could be a future source of bugs - really no other way?

@lifeizhou-ap
Merge branch 'main' into lifei/fix-ensure-execution-result-shown
59f2517 
* main:
  docs: Quick spotlight fix (#3633)
  alexhancock/rmcp-tools-annotations (#3617)
  fix: clean up subagent (#3565)
  Adds the `WaitingForUserInput` state (#3620)
  docs: update extensions library (#3612)
  Fixing grants path (#3632)
  docs: June 2024 Community All-Stars Spotlight (#3631)
  grant program (#3630)
  Lifei/sub recipe desktop temp (#3576)
@lifeizhou-ap lifeizhou-ap merged commit 1bccd26 into main Jul 25, 2025
8 checks passed
@lifeizhou-ap lifeizhou-ap deleted the lifei/fix-ensure-execution-result-shown branch July 25, 2025 02:30
michaelneale added a commit that referenced this pull request Jul 28, 2025
@michaelneale
Merge branch 'main' into add-inline-python-extension
953ec10 
* main: (25 commits)
  fix: add maintainer, homepage and categories to DEB/RPM package config (#3096)
  blog: agent to agent convo (#3677)
  Possible to disable random thinking messages (#3304)
  Two VS code tutorials (#3603)
  small blog fixes (#3549)
  docs: fix installation command for YouTube Transcript MCP in servers.json (#3595)
  Docs for using Docker Model Runner as a local LLM provider.  (#3509)
  Docs: VS Code Extension move to tutorials (#3601)
  Fix working directory when session has no messages (#3513)
  goose docs MCP server (#3665)
  Remove confusing status output when testing sharing url connection and it shows 404 (#3659)
  chore: use typed notifications from rmcp (#3653)
  feat: convert GetPromptResult from mcp_core to rmcp version (#3650)
  feat: Replace usage of mcp_core Tools/ToolAnnotations in openapi schema (#3649)
  fix: ensure execution task result is shown (#3629)
  docs: Quick spotlight fix (#3633)
  alexhancock/rmcp-tools-annotations (#3617)
  fix: clean up subagent (#3565)
  Adds the `WaitingForUserInput` state (#3620)
  docs: update extensions library (#3612)
  ...
michaelneale added a commit that referenced this pull request Jul 28, 2025
@michaelneale
Merge branch 'main' into micn/install-with-openrouter
9056916 
* main: (27 commits)
  Add inline python extension (#3107)
  fix: add maintainer, homepage and categories to DEB/RPM package config (#3096)
  blog: agent to agent convo (#3677)
  Possible to disable random thinking messages (#3304)
  Two VS code tutorials (#3603)
  small blog fixes (#3549)
  docs: fix installation command for YouTube Transcript MCP in servers.json (#3595)
  Docs for using Docker Model Runner as a local LLM provider.  (#3509)
  Docs: VS Code Extension move to tutorials (#3601)
  Fix working directory when session has no messages (#3513)
  goose docs MCP server (#3665)
  Remove confusing status output when testing sharing url connection and it shows 404 (#3659)
  chore: use typed notifications from rmcp (#3653)
  feat: convert GetPromptResult from mcp_core to rmcp version (#3650)
  feat: Replace usage of mcp_core Tools/ToolAnnotations in openapi schema (#3649)
  fix: ensure execution task result is shown (#3629)
  docs: Quick spotlight fix (#3633)
  alexhancock/rmcp-tools-annotations (#3617)
  fix: clean up subagent (#3565)
  Adds the `WaitingForUserInput` state (#3620)
  ...
michaelneale added a commit that referenced this pull request Jul 28, 2025
@michaelneale
Merge branch 'main' into micn/fix-pricing-tests
cb0f0de 
* main: (69 commits)
  Add inline python extension (#3107)
  fix: add maintainer, homepage and categories to DEB/RPM package config (#3096)
  blog: agent to agent convo (#3677)
  Possible to disable random thinking messages (#3304)
  Two VS code tutorials (#3603)
  small blog fixes (#3549)
  docs: fix installation command for YouTube Transcript MCP in servers.json (#3595)
  Docs for using Docker Model Runner as a local LLM provider.  (#3509)
  Docs: VS Code Extension move to tutorials (#3601)
  Fix working directory when session has no messages (#3513)
  goose docs MCP server (#3665)
  Remove confusing status output when testing sharing url connection and it shows 404 (#3659)
  chore: use typed notifications from rmcp (#3653)
  feat: convert GetPromptResult from mcp_core to rmcp version (#3650)
  feat: Replace usage of mcp_core Tools/ToolAnnotations in openapi schema (#3649)
  fix: ensure execution task result is shown (#3629)
  docs: Quick spotlight fix (#3633)
  alexhancock/rmcp-tools-annotations (#3617)
  fix: clean up subagent (#3565)
  Adds the `WaitingForUserInput` state (#3620)
  ...
atarantino pushed a commit to atarantino/goose that referenced this pull request Aug 5, 2025
@lifeizhou-ap
fix: ensure execution task result is shown (block#3629)
ebe9db0 
Signed-off-by: Adam Tarantino <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@michaelneale michaelneale michaelneale left review comments

@DOsinga DOsinga DOsinga approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

In Goose CLI sub agent execution display does not show the task completion result sometimes

4 participants

@lifeizhou-ap @michaelneale @DOsinga