fix: propagate context through GitHub CLI subprocess calls to fix Windows MCP server timeout - #51426
Conversation
…P server timeout The TestMCPServer_WindowsSmokeCommands integration test on Windows was failing with a 120-second context deadline exceeded error when calling the 'status' MCP tool. Root cause: fetchGitHubWorkflows and fetchLatestRunsByRef called workflow.ExecGH() without a context, so 'gh workflow list' could hang indefinitely (no GH token in the test environment). The MCP tool handler's 2-minute context eventually expired with no way to cancel the subprocess. Fix: thread context.Context through the full call chain: - fetchGitHubWorkflows(ctx, ...) uses ExecGHContext - fetchLatestRunsByRef(ctx, ...) uses ExecGHContext - GetWorkflowStatuses(ctx, ...) passes ctx to both fetch functions - StatusWorkflows(ctx, ...) passes ctx to GetWorkflowStatuses - getWorkflowStatus(ctx, ...) passes ctx to fetchGitHubWorkflows - handleWorkflowEnablement(ctx, ...) passes ctx to getWorkflowStatus - All callers and test files updated accordingly Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR does not have the 'implementation' label and has ≤100 new lines of code in business logic directories (41 additions across 13 files).
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Test Quality Sentinel completed test quality analysis. No new or modified behavioral tests in this PR. Test files were updated only to maintain compatibility with production code changes. Test Quality Sentinel analysis skipped.
|
|
✅ PR Code Quality Reviewer completed the code quality review. Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "api.individual.githubcopilot.com"See Network Configuration for more information.
|
There was a problem hiding this comment.
Pull request overview
Propagates request context into GitHub CLI subprocesses to prevent Windows MCP status requests from hanging.
Changes:
- Uses
ExecGHContextfor workflow and run-status queries. - Threads context through CLI, MCP, enablement, and forecast paths.
- Updates API documentation, tests, and mocks for new signatures.
Show a summary per file
| File | Description |
|---|---|
pkg/cli/workflows.go |
Adds context-aware workflow fetching. |
pkg/cli/status.go |
Passes Cobra command context to status handling. |
pkg/cli/status_remote_test.go |
Updates remote-status calls. |
pkg/cli/status_mcp_integration_test.go |
Updates MCP status calls. |
pkg/cli/status_command.go |
Propagates context through status queries. |
pkg/cli/status_command_test.go |
Updates status tests. |
pkg/cli/run_workflow_execution.go |
Threads context through enablement checks. |
pkg/cli/README.md |
Documents context-aware signatures. |
pkg/cli/mcp_tools_readonly.go |
Passes MCP request context. |
pkg/cli/forecast_resolution.go |
Propagates context through forecast fetching. |
pkg/cli/forecast_montecarlo_test.go |
Updates forecast mock signature. |
pkg/cli/enable.go |
Passes enablement context to workflow fetching. |
pkg/cli/commands_test.go |
Updates affected command tests. |
Review details
Tip
Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
Suppressed comments (3)
pkg/cli/status_command.go:63
- A cancellation during
gh run listis also treated as an ordinary recoverable API failure, after which status generation continues and reports success. Preserve the best-effort behavior for GitHub errors, but return the context error when the subprocess was stopped by cancellation or deadline.
latestRunsByWorkflow, err = fetchLatestRunsByRef(ctx, ref, repoOverride, false)
if err != nil {
statusLog.Printf("Failed to fetch workflow runs for ref %s: %v", ref, err)
latestRunsByWorkflow = make(map[string]*WorkflowRun)
pkg/cli/run_workflow_execution.go:226
- The context reaches
getWorkflowStatus, but cancellation from that call is immediately suppressed as an optional status-check failure. Execution then proceeds through the rest of workflow preparation with an already-cancelled context, so this does not provide the claimed end-to-end cancellation behavior. Returnctx.Err()before retaining the existing warning-and-continue behavior for non-context failures.
wf, err := getWorkflowStatus(ctx, workflowIdOrName, opts.RepoOverride, opts.Verbose)
if err != nil {
if opts.Verbose {
fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Could not check workflow status: %v", err)))
}
return workflowEnableState{}, nil
pkg/cli/enable.go:88
- If this fetch is cancelled, the error is downgraded to a warning and the function continues into the enable/disable loop, whose subsequent
ExecGHcalls do not use this context. A cancelled operation can therefore still mutate workflow state or hang in another subprocess. Return the context error before falling back for ordinary remote-status failures.
githubWorkflows, err := fetchGitHubWorkflows(ctx, repoOverride, false)
if err != nil {
enableLog.Printf("Failed to fetch GitHub workflows: %v", err)
fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Unable to fetch GitHub workflows (gh CLI may not be authenticated): %v", err)))
githubWorkflows = make(map[string]*GitHubWorkflow)
- Files reviewed: 13/13 changed files
- Comments generated: 3
- Review effort level: Balanced
| githubWorkflows, err := fetchGitHubWorkflows(ctx, repoOverride, false) | ||
| if err != nil { | ||
| statusLog.Printf("Failed to fetch GitHub workflows: %v", err) | ||
| githubWorkflows = make(map[string]*GitHubWorkflow) |
|
|
||
| // Get workflow statuses | ||
| statuses, err := GetWorkflowStatuses(pattern, ref, labelFilter, repoOverride) | ||
| statuses, err := GetWorkflowStatuses(ctx, pattern, ref, labelFilter, repoOverride) |
| args = append(args, "--repo", repoOverride) | ||
| } | ||
| cmd := workflow.ExecGH(args...) | ||
| cmd := workflow.ExecGHContext(ctx, args...) |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: Context Propagation Through GitHub CLI Subprocess Calls
This PR cleanly and correctly propagates context.Context through GitHub CLI subprocess calls by switching from workflow.ExecGH(args...) to workflow.ExecGHContext(ctx, args...). This is the right fix for Windows MCP server timeout issues where context cancellation was not being honored.
What was reviewed:
- All changed functions (
fetchGitHubWorkflows,getWorkflowStatus,GetWorkflowStatuses,StatusWorkflows,handleWorkflowEnablement) correctly threadctxthrough the call chain - Both
ExecGH→ExecGHContextmigrations are complete with no remaining unconverted call sites in the diff - Tests updated to use
t.Context()consistently - No
context.Background()in non-test production code that should use the propagated context
No blocking issues found. ✅
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 25 AIC · ⌖ 7.16 AIC · ⊞ 5.5K
There was a problem hiding this comment.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 46.4 AIC · ⌖ 7.53 AIC · ⊞ 7.1K
Comment /matt to run again
Comments that could not be inline-anchored
pkg/cli/workflows.go:218
[/diagnosing-bugs] restoreWorkflowState still calls workflow.ExecGH (no context), so it can hang indefinitely on Windows — the same root cause this PR is fixing.
<details>
<summary>💡 Suggested fix</summary>
Add ctx context.Context as the first parameter and propagate it:
func restoreWorkflowState(ctx context.Context, workflowIdOrName string, workflowID int64, repoOverride string, verbose bool) {
// ...
cmd := workflow.ExecGHContext(ctx, args...)Update call site…
pkg/cli/enable.go:217
[/diagnosing-bugs] toggleWorkflowsByNames already receives ctx but the workflow.enable/workflow.disable ExecGH calls on lines 217–245 still don't propagate it — same hang risk on Windows.
<details>
<summary>💡 Suggested fix</summary>
Replace these four workflow.ExecGH(args...) calls with workflow.ExecGHContext(ctx, args...). The ctx variable is already in scope from the function signature.
</details>
@copilot please address this.
PR Triage
Fixes a Windows MCP hang caused by missing context propagation through
|
|
@copilot Quick triage nudge for this PR. Please address the remaining review feedback below, refresh the branch if GitHub can update it cleanly, run the Open review context (newest first):
Branch refresh was requested. Run: https://github.com/github/gh-aw/actions/runs/31272330768
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
TestMCPServer_WindowsSmokeCommandswas timing out on Windows after 120s becausefetchGitHubWorkflowsandfetchLatestRunsByRefcalledworkflow.ExecGH()without a context — sogh workflow listwould hang indefinitely when no GH token is available, with no way to cancel the subprocess when the MCP request context expired.Changes
fetchGitHubWorkflows(ctx, ...)— replacesExecGHwithExecGHContext; addsctx context.Contextas first paramfetchLatestRunsByRef(ctx, ...)— same patternGetWorkflowStatuses(ctx, ...)— threads ctx to both fetch functions aboveStatusWorkflows(ctx, ...)— threadscmd.Context()from the Cobra command through toGetWorkflowStatusesgetWorkflowStatus(ctx, ...)/handleWorkflowEnablement(ctx, ...)— full propagation so the workflow-run path is also coveredenable.go— passes existingctxfromtoggleWorkflowsByNamesforecast_resolution.go— updatesforecastFetchGitHubWorkflowsfunction variable type to include ctx; passes ctx infetchWorkflowsWithBackofft.Context()orcontext.Background(); forecast mock signature updated