-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Listen for ctrl-c during provider request #5585
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.
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR refactors interrupt handling to centralize Ctrl+C signal handling through a cancellation token pattern, and adds support for EOF (End of File) signal handling in user input.
Key changes:
- Centralized Ctrl+C handling via a spawned task that cancels a token instead of directly handling the signal in the select loop
- Modified the event loop to listen for cancellation token instead of raw Ctrl+C signals
- Added EOF signal handling to exit the session gracefully
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| crates/goose-cli/src/session/mod.rs | Refactored interrupt handling to use a spawned task for Ctrl+C that triggers cancellation token, and updated the select loop to listen for cancellation instead of raw signals |
| crates/goose-cli/src/session/input.rs | Added EOF (Ctrl+D) signal handling to exit the session, matching the behavior of Ctrl+C when the line is empty |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let cancel_token_interrupt = cancel_token.clone(); | ||
| tokio::spawn(async move { | ||
| if tokio::signal::ctrl_c().await.is_ok() { | ||
| cancel_token_interrupt.cancel(); | ||
| } | ||
| }); |
Copilot
AI
Nov 5, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The spawned task is not tracked or joined, which could lead to it outliving the function scope. If process_agent_response returns before Ctrl+C is pressed, the spawned task will continue running and may trigger cancellation on an already-consumed token. Consider storing the task handle and aborting it before the function returns, or using tokio::select! with a scoped spawn pattern to ensure proper cleanup.
crates/goose-cli/src/session/mod.rs
Outdated
| tokio::spawn(async move { | ||
| if tokio::signal::ctrl_c().await.is_ok() { | ||
| cancel_token_interrupt.cancel(); |
Copilot
AI
Nov 5, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The spawned task silently ignores any errors from the spawn itself and only checks if ctrl_c succeeded. Consider handling potential spawn failures or at minimum documenting why they can be safely ignored. Also, the task only handles a single Ctrl+C signal - subsequent signals won't be caught. Consider using a loop to handle multiple interrupts or documenting this single-signal behavior.
| tokio::spawn(async move { | |
| if tokio::signal::ctrl_c().await.is_ok() { | |
| cancel_token_interrupt.cancel(); | |
| // Spawn a background task to listen for Ctrl+C signals and cancel the token. | |
| // We loop to handle multiple interrupts, and log any errors from ctrl_c. | |
| // Note: tokio::spawn is infallible unless the runtime is shutting down, in which case | |
| // the process is likely exiting anyway, so we can safely ignore errors from spawn. | |
| tokio::spawn(async move { | |
| loop { | |
| match tokio::signal::ctrl_c().await { | |
| Ok(_) => { | |
| cancel_token_interrupt.cancel(); | |
| } | |
| Err(e) => { | |
| // Log the error and break the loop. | |
| eprintln!("Failed to listen for Ctrl+C: {}", e); | |
| break; | |
| } | |
| } |
* main: (41 commits) Add pending extension indicator to extension panel (#5493) Add environment subsition for auth blocks (#5439) acp: ToolCallLocations and working cancellation (#5588) feat(providers): add Mistral AI provider (#5009) Listen for ctrl-c during provider request (#5585) Also accept null as description, not just missing (#5589) Document missing recipe param types (#5584) docs: description required for "Add Extension" in cli (#5573) fix: Add schema-aware numeric coercion for MCP tool arguments (#5478) Add uv for uvx in Justfile (#5581) Keep llm logs in place (#5577) bump to 1.12.0 (#5580) automate more of the release process (#5409) add clippy warning for string_slice (#5422) improve linux tray icon support (#5425) feat: log rotation (#5561) use app.isPackaged instead of checking for node env development (#5465) disable RPM build-ID generation to prevent package conflicts (#5563) Add Diagnostics Info to Q&A and Bug Report Templates (#5565) fix: improve server error messages to include HTTP status code (#5532) ...
* main: (53 commits) acp: ToolCallLocations and working cancellation (#5588) feat(providers): add Mistral AI provider (#5009) Listen for ctrl-c during provider request (#5585) Also accept null as description, not just missing (#5589) Document missing recipe param types (#5584) docs: description required for "Add Extension" in cli (#5573) fix: Add schema-aware numeric coercion for MCP tool arguments (#5478) Add uv for uvx in Justfile (#5581) Keep llm logs in place (#5577) bump to 1.12.0 (#5580) automate more of the release process (#5409) add clippy warning for string_slice (#5422) improve linux tray icon support (#5425) feat: log rotation (#5561) use app.isPackaged instead of checking for node env development (#5465) disable RPM build-ID generation to prevent package conflicts (#5563) Add Diagnostics Info to Q&A and Bug Report Templates (#5565) fix: improve server error messages to include HTTP status code (#5532) improvement: add useful error message when attempting to use unauthenticated cursor-agent (#5300) fix: unblock acp via databricks (#5562) ...
* main: fix: customised recipe to yaml string to avoid minininjia parsing error (#5494) Add pending extension indicator to extension panel (#5493) Add environment subsition for auth blocks (#5439) acp: ToolCallLocations and working cancellation (#5588) feat(providers): add Mistral AI provider (#5009) Listen for ctrl-c during provider request (#5585) Also accept null as description, not just missing (#5589) Document missing recipe param types (#5584) docs: description required for "Add Extension" in cli (#5573) fix: Add schema-aware numeric coercion for MCP tool arguments (#5478) Add uv for uvx in Justfile (#5581) Keep llm logs in place (#5577) bump to 1.12.0 (#5580) automate more of the release process (#5409)
* origin/main: (75 commits) fix: customised recipe to yaml string to avoid minininjia parsing error (#5494) Add pending extension indicator to extension panel (#5493) Add environment subsition for auth blocks (#5439) acp: ToolCallLocations and working cancellation (#5588) feat(providers): add Mistral AI provider (#5009) Listen for ctrl-c during provider request (#5585) Also accept null as description, not just missing (#5589) Document missing recipe param types (#5584) docs: description required for "Add Extension" in cli (#5573) fix: Add schema-aware numeric coercion for MCP tool arguments (#5478) Add uv for uvx in Justfile (#5581) Keep llm logs in place (#5577) bump to 1.12.0 (#5580) automate more of the release process (#5409) add clippy warning for string_slice (#5422) improve linux tray icon support (#5425) feat: log rotation (#5561) use app.isPackaged instead of checking for node env development (#5465) disable RPM build-ID generation to prevent package conflicts (#5563) Add Diagnostics Info to Q&A and Bug Report Templates (#5565) ...
* main: (60 commits) fix: add standard context menu items to prevent empty right-click menu (#5616) Bump openapi in prepare-release (#5611) docs: add access control section to Developer tutorial (#5615) Token state not showing on load, or after message is finished. (#5606) Change the other location too (#5608) feat(ui): bring back quick launcher (#5144) Support platform tools through CLI (#5570) Avoid web double write (#5601) fix: gemini flash -> pro for mcp smoke tests (#5574) Manual compaction test and fix (#5568) fix: tidy up claude cli handling (#5594) Remove jetbrains (#5602) feat(githubcopilot): add support for newer Copilot AI Models (#5603) fix: customised recipe to yaml string to avoid minininjia parsing error (#5494) Add pending extension indicator to extension panel (#5493) Add environment subsition for auth blocks (#5439) acp: ToolCallLocations and working cancellation (#5588) feat(providers): add Mistral AI provider (#5009) Listen for ctrl-c during provider request (#5585) Also accept null as description, not just missing (#5589) ...
Signed-off-by: fbalicchia <[email protected]>
Signed-off-by: Blair Allan <[email protected]>
Prior, we'd only start listening during the stream. So if the user hit ctrl-c while we waited for the provider response (before any streaming started), we wouldn't handle it.
fixes #4265
Also treats ctrl-D as exit, so we don't print "Error: EOF" when you exit this way.