fix: proper end-to-end Ctrl-C / signal handling for all gh aw commands (WSL)#21795
fix: proper end-to-end Ctrl-C / signal handling for all gh aw commands (WSL)#21795
Conversation
- Add signal.NotifyContext + ExecuteContext in main() for top-level SIGINT/SIGTERM handling - Add Ctx to PollOptions/RepeatOptions/EngineSecretConfig/AddInteractiveConfig - Define ErrInterrupted sentinel error in signal_aware_poll.go - Add ctx parameter to WaitForWorkflowCompletion and propagate interrupts - Use ExecGHContext for gh workflow run subprocess - Use RunWithContext(ctx) for all huh interactive forms - Pass context through interactive add-wizard and run command flows Co-authored-by: dsyme <7204669+dsyme@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Improve Ctrl‑C / signal handling across gh aw commands (notably on WSL) by introducing a top-level cancelable context and propagating it through polling, repeating, interactive prompts, and subprocess execution.
Changes:
- Add top-level
signal.NotifyContextand execute Cobra commands withExecuteContext(ctx). - Propagate
context.Contextthrough repeat/poll flows and interactivehuhforms (RunWithContext). - Introduce
ErrInterruptedand switchgh workflow runto context-aware execution (ExecGHContext).
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| cmd/gh-aw/main.go | Introduces signal-cancelable context and uses ExecuteContext. |
| pkg/cli/trial_command.go | Propagates context into secret prompting and workflow completion wait. |
| pkg/cli/signal_aware_poll.go | Adds Ctx to polling options and introduces ErrInterrupted. |
| pkg/cli/run_workflow_execution.go | Uses context-aware GH subprocess execution; propagates interrupt errors upward. |
| pkg/cli/run_interactive.go | Passes context into interactive forms via RunWithContext. |
| pkg/cli/retry.go | Adds context to repeat options and checks cancellation before each iteration. |
| pkg/cli/pr_automerge.go | Adds context to WaitForWorkflowCompletion and passes it to polling. |
| pkg/cli/pr_automerge_test.go | Updates test for new WaitForWorkflowCompletion(ctx, ...) signature. |
| pkg/cli/interactive.go | Stores context on builder and runs forms with RunWithContext. |
| pkg/cli/engine_secrets.go | Adds context to engine secret prompting and runs forms with RunWithContext. |
| pkg/cli/add_interactive_orchestrator.go | Adds/threads Ctx through interactive add-flow config. |
| pkg/cli/add_interactive_workflow.go | Runs interactive form with context. |
| pkg/cli/add_interactive_schedule.go | Runs interactive form with context. |
| pkg/cli/add_interactive_engine.go | Runs interactive form with context; passes context into secret config. |
| pkg/cli/add_interactive_auth.go | Runs interactive form with context. |
Comments suppressed due to low confidence (1)
pkg/cli/retry.go:1
ExecuteWithRepeatcan only observe cancellation between iterations becauseExecuteFunchas no way to receivectx. As a result, if an iteration blocks insideExecuteFunc, Ctrl-C won’t stop it until it returns. To make repeat truly cancellation-aware, consider evolving the API toExecuteFunc func(ctx context.Context) error(or adding an additionalExecuteFuncWithContextfield) and pass thectxthrough.
package cli
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
pkg/cli/pr_automerge.go
Outdated
| return PollWithSignalHandling(PollOptions{ | ||
| Ctx: ctx, | ||
| PollInterval: 10 * time.Second, | ||
| Timeout: timeout, | ||
| PollFunc: func() (PollResult, error) { |
| // ErrInterrupted is returned when polling is interrupted by a signal or context cancellation | ||
| var ErrInterrupted = errors.New("interrupted by user") |
There was a problem hiding this comment.
Added in d5b5a76:
TestPollWithSignalHandling_ContextCancellation: starts a poll withPollContinue, cancels the context mid-flight, and assertserrors.Is(err, ErrInterrupted)TestPollWithSignalHandling_AlreadyCancelledContext: verifiesErrInterruptedis returned immediately when polling starts with an already-cancelled contextTestWaitForWorkflowCompletion_ContextCancellation: verifies thatWaitForWorkflowCompletionpropagates a cancellation error (ErrInterruptedorcontext.Canceled) so callers can detect and stop on interruption
Also fixed a CI build failure in the same commit: a previous "Potential fix" commit introduced workflow.RunGHWithContext (undefined) instead of the correct workflow.RunGHContext inside WaitForWorkflowCompletion's PollFunc.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
…llation tests - Fix undefined workflow.RunGHWithContext → workflow.RunGHContext in WaitForWorkflowCompletion's PollFunc (fixes CI build failure) - Add TestPollWithSignalHandling_ContextCancellation: verifies ErrInterrupted returned when context is cancelled mid-poll - Add TestPollWithSignalHandling_AlreadyCancelledContext: verifies ErrInterrupted returned when polling starts with an already-cancelled context - Add TestWaitForWorkflowCompletion_ContextCancellation: verifies that WaitForWorkflowCompletion propagates a cancellation error to callers Co-authored-by: dsyme <7204669+dsyme@users.noreply.github.com>
|
Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
|
@copilot review comment |
…n Ctrl-C Change PollFunc signature from func() to func(ctx context.Context) so that poll callbacks receive the same context used by the poll loop. This enables callers to pass ctx to context-aware operations (e.g. RunGHContext) and abort mid-call when Ctrl-C is pressed, rather than only aborting between iterations. - PollOptions.PollFunc: func() → func(ctx context.Context) - PollWithSignalHandling: pass ctx to both PollFunc call sites - WaitForWorkflowCompletion: use PollFunc(ctx) directly; remove redundant ctx.Done() guard inside closure (now handled by RunGHContext's ctx arg) - All test PollFuncs updated to new signature Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Addressed the remaining open review comment in 27b521c: changed Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
Ctrl-C failed to terminate
gh awprocesses reliably — especially on WSL — because there was no top-level signal handler and context cancellation was not propagated through the command stack.Root causes
main()calledrootCmd.Execute()with no signal-aware context — whether Ctrl-C did anything useful depended entirely on what ad-hoc handlers happened to be activeExecuteWithRepeat(--repeat) only checked the signal channel at the start of each iteration; a long-running iteration would not be interrupted mid-flightWaitForWorkflowCompletionaccepted no context and treated interrupt errors as non-fatal warnings, so the repeat loop silently continued after Ctrl-Chuhforms usedform.Run()instead ofform.RunWithContext(ctx), so context cancellation couldn't reach themgh workflow runsubprocess usedExecGH(no context), so it couldn't be killed on cancellationPollFunchad no context parameter, so a hanging poll operation (e.g. a stalledgh run viewcall) could not be aborted mid-call on Ctrl-CChanges
Top-level signal handling
main(): usesignal.NotifyContext+rootCmd.ExecuteContext(ctx)— the canonical Go pattern; a single clean cancellation point for the entire processContext propagation
PollOptions/RepeatOptions/EngineSecretConfig/AddInteractiveConfig: addCtx context.Contextfield (optional; falls back tocontext.Background())PollWithSignalHandling: check bothctx.Done()andsigChanin the poll loop; passctxto eachPollFuncinvocation so callbacks can abort mid-callPollOptions.PollFunc: signature changed fromfunc() (PollResult, error)tofunc(ctx context.Context) (PollResult, error)— callers forwardctxto context-aware operations (e.g.RunGHContext) for true mid-call cancellationExecuteWithRepeat: check bothctx.Done()andsigChanat the top of the iteration loop; returnctx.Err()/context.Canceledon interruption so the process exits with a non-zero statusWaitForWorkflowCompletion: newctx context.Contextparameter; passes it to the poll loop and to thePollFunccallback; propagateErrInterrupted/ctx.Err()so callers (repeat loop) can stop immediatelySubprocess cancellation
gh workflow runsubprocess: switch fromExecGH→ExecGHContext(ctx, ...)so it is killed when context is cancelledInteractive forms
huhforms inrun_interactive.go,interactive.go,add_interactive_*, andengine_secrets.go: replacedform.Run()withform.RunWithContext(ctx)Sentinel error
ErrInterruptedinsignal_aware_poll.goand useerrors.Is(err, ErrInterrupted)instead of fragile string comparisonTests
TestPollWithSignalHandling_ContextCancellation: verifiesErrInterruptedis returned when context is cancelled mid-pollTestPollWithSignalHandling_AlreadyCancelledContext: verifiesErrInterruptedis returned immediately when an already-cancelled context is passedTestWaitForWorkflowCompletion_ContextCancellation: verifiesWaitForWorkflowCompletionpropagates a cancellation error so callers detect and stop on interruption✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.