refactor: migrate remaining RunGHWithHost call sites to RunGHContextWithHost - #49554
Conversation
…ithHost Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
PR TriageCategory: refactor · Risk: medium · Priority: medium · Score: 42/100 (impact 20, urgency 10, quality 12) Recommended action: Mechanical migration of
|
PR Triage
Draft; large call-site migration (RunGHWithHost -> RunGHContextWithHost), moderate blast radius, still draft.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #49554 does not have the implementation label and has only 25 new lines of code in business logic directories (threshold: 100). |
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. Test Quality Sentinel skipped. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
|
There was a problem hiding this comment.
Pull request overview
Migrates cross-host gh operations toward context-aware execution, though several calls still use non-cancellable background contexts.
Changes:
- Deprecates
RunGHWithHost. - Threads caller contexts through PR creation paths.
- Replaces remaining legacy calls in package and outcome evaluation paths.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/github_cli.go |
Deprecates the legacy host wrapper. |
pkg/cli/pr_command.go |
Adds context-aware PR operations. |
pkg/cli/pr_helpers.go |
Propagates context into PR creation. |
pkg/cli/add_workflow_pr.go |
Passes the existing workflow context. |
pkg/cli/add_package_manifest.go |
Migrates package lookup calls. |
pkg/cli/outcome_eval.go |
Migrates outcome API calls. |
pkg/cli/update_command.go |
Passes contexts when creating update PRs. |
pkg/cli/upgrade_command.go |
Passes the command context. |
pkg/cli/upgrade_org.go |
Passes the target-repository context. |
pkg/cli/deploy_command.go |
Threads context through deploy PR creation. |
pkg/cli/init.go |
Passes initialization context. |
pkg/cli/README.md |
Documents the updated helper signature. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Suppressed comments (3)
pkg/cli/outcome_eval.go:308
- The GraphQL request is still unboundable because
context.Background()never observes caller cancellation or deadlines. Propagate the command/audit context to this helper rather than replacing the legacy wrapper with a background context.
output, err = workflow.RunGHContextWithHost(context.Background(), "Checking outcome...", host, args...)
pkg/cli/add_package_manifest.go:990
- Although this lambda accepts a context,
resolveRepositoryPackageLatestReleaseinvokes it only viaresolveLatestReleaseWithDeps(context.Background(), ...)below, whileresolveRepositoryPackagealready has a real caller context. The release lookup consequently remains uncancellable; thread that existing context through the helper and function variable.
return workflow.RunGHContextWithHost(ctx, "Fetching releases...", host, args...)
pkg/cli/outcome_eval.go:287
- This background context preserves the legacy call's non-cancellable behavior for array API requests, contrary to the migration's purpose. Pass the originating command/audit context through the outcome evaluator chain instead.
output, err = workflow.RunGHContextWithHost(context.Background(), "Checking outcome...", host, args...)
- Files reviewed: 12/12 changed files
- Comments generated: 2
- Review effort level: Balanced
| var err error | ||
| if host != "" { | ||
| output, err = workflow.RunGHWithHost("Checking outcome...", host, args...) | ||
| output, err = workflow.RunGHContextWithHost(context.Background(), "Checking outcome...", host, args...) |
| var err error | ||
| if host != "" { | ||
| output, err = workflow.RunGHWithHost("Fetching repo info...", host, args...) | ||
| output, err = workflow.RunGHContextWithHost(context.Background(), "Fetching repo info...", host, args...) |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Clean, consistent context-propagation refactor. All call sites that already had a context thread it through correctly; the three context.Background() uses in outcome_eval.go are an acceptable placeholder given those functions do not yet accept a context parameter (pre-existing design).
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 11.8 AIC · ⌖ 13.7 AIC · ⊞ 5.4K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design — no blocking issues, two minor TODO comment suggestions.
📋 Summary
Positive highlights
- ✅ Clean mechanical refactor — every
RunGHWithHostcall inpkg/clinow uses the context-aware variant - ✅
Deprecated:godoc annotation onRunGHWithHostcorrectly guides future callers - ✅ Context is properly threaded from
cmd.Context()all the way down throughCreatePRWithChanges→createPR - ✅ README kept in sync with the updated
CreatePRWithChangessignature
Suggestions (non-blocking)
-
pkg/cli/outcome_eval.golines 262, 287, 308 — threecontext.Background()calls are acknowledged deferrals, but lack aTODOcomment. Without one, future readers cannot distinguish intentional deferral from an oversight. -
pkg/cli/add_package_manifest.goline 938 —context.Background()is constrained by the test-mock var signature (getRepositoryPackageDefaultBranchhas noctx). A brief comment explaining this constraint would help the next refactor.
Neither issue affects correctness; both affect long-term maintainability.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 26.3 AIC · ⌖ 7.92 AIC · ⊞ 7.1K
Comment /matt to run again
| var err error | ||
| if host != "" { | ||
| output, err = workflow.RunGHWithHost("Fetching repo info...", host, args...) | ||
| output, err = workflow.RunGHContextWithHost(context.Background(), "Fetching repo info...", host, args...) |
There was a problem hiding this comment.
[/codebase-design] context.Background() here is forced by the test-mock var (getRepositoryPackageDefaultBranch) whose signature has no ctx. The constraint is real but silent — a // TODO: add ctx to getRepositoryPackageDefaultBranch once the mock var signature supports it comment would surface this for the next refactor pass rather than leaving an unexplained context.Background().
@copilot please address this.
| var err error | ||
| if host != "" { | ||
| output, err = workflow.RunGHWithHost("Checking outcome...", host, args...) | ||
| output, err = workflow.RunGHContextWithHost(context.Background(), "Checking outcome...", host, args...) |
There was a problem hiding this comment.
[/codebase-design] context.Background() here silently discards any cancellation or deadline from the caller. The PR body notes this is deferred — a // TODO: thread real ctx once the eval* call chain is refactored comment on lines 262, 287, and 308 would make the intent visible and prevent the pattern from being copied as a template.
@copilot please address this.
|
@copilot make wasm golden more resilient Fix https://github.com/github/gh-aw/actions/runs/30715022301/job/91409111260 |
…lity Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Fixed in commit Added |
|
@copilot resolve the merge conflicts on this branch. |
|
@copilot quick triage for this PR:
|
…ION normalization Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Resolved. Merged |
…paths - outcomeEvaluator type now includes ctx context.Context as first param - All eval* functions (evalCreatePullRequest, evalCreateIssue, evalAddComment, evalReplaceLabel, evalAddLabels, evalAssignToAgent, evalAddReviewer, evalSubmitPullRequestReview, evalCloseSticky, evalGenericSticky, evalDispatchWorkflow, evalUpdateDiscussion, evalUpdateIssue, evalUpdatePullRequest, evalRetainedUpdate) now take ctx - ghAPIGet/ghAPIGetArray/ghAPIGraphQL now take ctx; context.Background() removed - mutableStateLoader type includes ctx; extractCurrentIssueUpdateState and extractCurrentPullRequestUpdateState take ctx - EvaluateOutcomes and RunOutcomes take ctx; cobra passes cmd.Context() - buildAuditData/addAuditOutcomeSummary take ctx; audit.go passes real ctx - resolveRepositoryPackageDefaultBranch/LatestRelease take ctx; callers pass ctx - All test files updated to match new signatures Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Done. Context threading is now complete: Outcome eval subsystem — Package manifest —
All test files updated to match new signatures, |
|
@copilot
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Fixed in commit Root cause:
|
|
🎉 This pull request is included in a new release. Release: |
89% of cross-host
ghcall sites inpkg/cliwere using the legacyworkflow.RunGHWithHost, making GHES/data-residency operations non-cancellable and unboundable by caller timeouts.Changes
pkg/workflow/github_cli.go— marksRunGHWithHostas// Deprecated: Use RunGHContextWithHostpkg/cli/pr_command.go— addsctx context.ContexttocreatePR; bothRunGHWithHostcalls replaced withRunGHContextWithHost(ctx, ...)pkg/cli/pr_helpers.go— addsctxtoCreatePRWithChanges, threaded through tocreatePRpkg/cli/add_workflow_pr.go,update_command.go,upgrade_command.go,upgrade_org.go,deploy_command.go,init.go— callers ofCreatePRWithChangesupdated to passcmd.Context()or existingctxpkg/cli/outcome_eval.go— 3RunGHWithHostcalls replaced withRunGHContextWithHost(context.Background(), ...); real ctx threading deferred since theeval*call chain has no ctx without a larger refactorpkg/cli/add_package_manifest.go— lambda at line 990 already hadctxand now passes it through; line 938 usescontext.Background()to preserve the test-mocked var signatureAfter this change
grep -rn "workflow\.RunGHWithHost(" pkg/cli --include='*.go' | grep -v _test.goreturns zero results.run: https://github.com/github/gh-aw/actions/runs/30716547955
Run URL: https://github.com/github/gh-aw/actions/runs/30719603092