Thread context.Context through all ResolveSHA call sites#32295
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…ll chains Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…ction name strings Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR threads context.Context through SHA resolution and related compile/add/init/update flows so gh api calls can be canceled or timed out by callers.
Changes:
- Adds context parameters to workflow SHA resolution helpers, maintenance workflow generation, compile orchestration, add/update/init/enable paths, and CLI command boundaries.
- Adds compiler context storage via
WithContext/SetContext. - Updates many tests and one generated workflow lock file to match the new APIs/output.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/side_repo_maintenance.go |
Passes context through side-repo maintenance generation. |
pkg/workflow/maintenance_workflow.go |
Adds context to maintenance workflow and setup CLI action resolution. |
pkg/workflow/maintenance_workflow_yaml.go |
Passes context into generated maintenance YAML action references. |
pkg/workflow/maintenance_workflow_test.go |
Updates tests for context-aware maintenance APIs. |
pkg/workflow/lock_validation.go |
Adds context to action SHA validation. |
pkg/workflow/compiler_types.go |
Adds compiler context field and setters/options. |
pkg/workflow/compiler_custom_actions_test.go |
Updates setup action reference tests. |
pkg/workflow/central_slash_command_workflow.go |
Adds context to centralized slash-command generation. |
pkg/workflow/central_slash_command_workflow_test.go |
Updates centralized workflow tests. |
pkg/workflow/action_sha_validation_test.go |
Updates lock SHA validation test call. |
pkg/workflow/action_sha_checker.go |
Passes context to ResolveSHA. |
pkg/workflow/action_sha_checker_test.go |
Updates SHA checker test call. |
pkg/workflow/action_reference.go |
Adds context to setup action reference resolution. |
pkg/workflow/action_reference_test.go |
Updates setup action reference tests. |
pkg/cli/upgrade_command.go |
Threads context through upgrade compilation/setup paths. |
pkg/cli/update_workflows.go |
Passes context during workflow recompilation. |
pkg/cli/update_command_test.go |
Updates compile-with-refresh test calls. |
pkg/cli/update_actions.go |
Passes context during action update recompilation. |
pkg/cli/init.go |
Adds InitOptions.Ctx and threads context into setup/maintenance. |
pkg/cli/init_test.go |
Updates maintenance helper test call. |
pkg/cli/init_mcp_test.go |
Updates Copilot setup helper calls. |
pkg/cli/init_command.go |
Passes Cobra command context into init. |
pkg/cli/file_tracker_test.go |
Updates tracking compile helper tests. |
pkg/cli/error_formatting_test.go |
Updates compile validation test call. |
pkg/cli/enable.go |
Adds context to enable/disable workflow flows. |
pkg/cli/copilot_setup.go |
Threads context through Copilot setup action resolution. |
pkg/cli/copilot_setup_test.go |
Updates Copilot setup tests for context APIs. |
pkg/cli/compile_workflow_processor.go |
Passes context into workflow-data compilation. |
pkg/cli/compile_watch.go |
Threads context into watch-mode compile calls. |
pkg/cli/compile_validation.go |
Adds context to compile validation and SHA validation. |
pkg/cli/compile_update_discussion_test.go |
Updates compile validation test call. |
pkg/cli/compile_security_benchmark_test.go |
Updates benchmark compile calls. |
pkg/cli/compile_post_processing.go |
Passes context into post-processing workflow generation. |
pkg/cli/compile_pipeline.go |
Threads context through compile pipeline stages. |
pkg/cli/compile_orchestrator.go |
Sets compiler context and passes it through orchestration. |
pkg/cli/compile_guard_policy_test.go |
Updates guard policy compile tests. |
pkg/cli/compile_file_operations.go |
Adds context to single/all/dependency compile helpers. |
pkg/cli/commands_file_watching_test.go |
Updates watch/file compile helper tests. |
pkg/cli/add_workflow_pr.go |
Passes context through PR-based workflow addition. |
pkg/cli/add_workflow_compilation.go |
Adds context to add-time compile helpers. |
pkg/cli/add_interactive_git.go |
Passes interactive context into add flow. |
pkg/cli/add_gitattributes_test.go |
Updates add workflow helper tests. |
pkg/cli/add_command.go |
Threads context through resolved workflow addition and dependencies. |
pkg/cli/add_command_test.go |
Updates add command helper tests. |
cmd/gh-aw/main.go |
Passes Cobra command context to enable/disable commands. |
.github/workflows/linter-miner.lock.yml |
Regenerates the linter-miner workflow lock output. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (1)
pkg/cli/compile_validation.go:110
- This signature change is not applied consistently: pkg/cli/compile_command_test.go still calls CompileWorkflowDataWithValidation without the new context argument, so the package tests will not compile. Also apply ctx to the compiler before CompileWorkflowData so SHA resolution during data compilation observes caller cancellation.
func CompileWorkflowDataWithValidation(ctx context.Context, compiler *workflow.Compiler, workflowData *workflow.WorkflowData, filePath string, verbose bool, runZizmorPerFile bool, runPoutinePerFile bool, runActionlintPerFile bool, strict bool, validateActionSHAs bool) error {
- Files reviewed: 46/46 changed files
- Comments generated: 7
|
|
||
| // Compiler handles converting markdown workflows to GitHub Actions YAML | ||
| type Compiler struct { | ||
| ctx context.Context // Context for network operations (e.g. SHA resolution); defaults to context.Background() |
|
|
||
| // ValidateActionSHAsInLockFile validates action SHAs in a lock file and emits warnings | ||
| func ValidateActionSHAsInLockFile(lockFilePath string, cache *ActionCache, verbose bool) error { | ||
| func ValidateActionSHAsInLockFile(ctx context.Context, lockFilePath string, cache *ActionCache, verbose bool) error { |
| // repoSlug is the owner/repo slug used to determine the default branch for the push | ||
| // trigger; pass an empty string to fall back to "main". | ||
| func GenerateMaintenanceWorkflow(workflowDataList []*WorkflowData, workflowDir string, version string, actionMode ActionMode, actionTag string, verbose bool, repoConfig *RepoConfig, repoSlug string) error { | ||
| func GenerateMaintenanceWorkflow(ctx context.Context, workflowDataList []*WorkflowData, workflowDir string, version string, actionMode ActionMode, actionTag string, verbose bool, repoConfig *RepoConfig, repoSlug string) error { |
|
|
||
| // EnableWorkflowsByNames enables workflows by specific names, or all if no names provided | ||
| func EnableWorkflowsByNames(workflowNames []string, repoOverride string) error { | ||
| func EnableWorkflowsByNames(ctx context.Context, workflowNames []string, repoOverride string) error { |
| - name: Setup Scripts | ||
| id: setup | ||
| uses: github/gh-aw-actions/setup@v0.72.1 | ||
| uses: ./actions/setup |
|
|
||
| // CompileWorkflowWithValidation compiles a workflow with always-on YAML validation for CLI usage | ||
| func CompileWorkflowWithValidation(compiler *workflow.Compiler, filePath string, verbose bool, runZizmorPerFile bool, runPoutinePerFile bool, runActionlintPerFile bool, strict bool, validateActionSHAs bool) error { | ||
| func CompileWorkflowWithValidation(ctx context.Context, compiler *workflow.Compiler, filePath string, verbose bool, runZizmorPerFile bool, runPoutinePerFile bool, runActionlintPerFile bool, strict bool, validateActionSHAs bool) error { |
| // This is a convenience wrapper around compileWorkflowWithRefresh. | ||
| func compileWorkflow(filePath string, verbose bool, quiet bool, engineOverride string) error { | ||
| return compileWorkflowWithRefresh(filePath, verbose, quiet, engineOverride, false) | ||
| func compileWorkflow(ctx context.Context, filePath string, verbose bool, quiet bool, engineOverride string) error { |
🧪 Test Quality Sentinel ReportTest Quality Score: N/A
Summary
Nature of Test ChangesAll 18 modified test files follow an identical mechanical pattern:
These are signature-adaptation changes required by the production code refactoring — no behavioral changes to test logic. Verdict
References: §25916052602
|
|
Commit pushed:
|
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (313 new lines across AI has analyzed the PR diff and generated a draft ADR to help you get started: 📄 Draft ADR: The draft has been committed to this PR branch. What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. Why ADRs Matter
ADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in References:
|
|
@copilot merge main and review all comments |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /zoom-out and /tdd — this is a pure refactor that threads context.Context through a call chain, with architectural implications worth examining.
Key Themes
1. Split context ownership (important)
CompileWorkflowWithValidation now takes a ctx parameter and passes it to ValidateActionSHAsInLockFile, but compiler.CompileWorkflow (line 46) uses compiler.ctx — a different context stored in the struct. Callers who inject a timeout context into CompileWorkflowWithValidation will only get partial cancellation coverage. See inline comment.
2. context.Context as a struct field (Go anti-pattern)
The Go team explicitly discourages storing Context in structs. A long-lived Compiler reused across multiple compiles is a good example of why — each compile call may warrant a different deadline. The cleaner path is to pass ctx per-call through the public compile entry points and remove c.ctx / WithContext / SetContext. See inline comment.
3. Missing cancellation test
All test call sites pass context.Background(). There is no test that verifies a cancelled context is actually propagated and honoured. The PR's stated value — "making it impossible for callers to inject timeouts or cancel hung gh api network calls" — is unverified. See inline comment.
Positive Highlights
- ✅ Clean, mechanical refactor with no logic changes — easy to review
- ✅ Consistent call-site updates across all 5 production sites
- ✅ Good PR description explains the motivation clearly
- ✅
WithContextfunctional option follows the establishedCompilerOptionpattern
Verdict
The threading is mechanically correct and the goal is valuable. The split-context issue is the most actionable — it means the PR solves the stated problem only partially as-is. Flagging as COMMENT since the issues are architectural observations rather than blockers, but the split-context concern is worth resolving before this ships.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · ● 17.6M
| // Use the compiler's shared action cache to benefit from cached resolutions | ||
| actionCache := compiler.GetSharedActionCache() | ||
| if err := workflow.ValidateActionSHAsInLockFile(lockFile, actionCache, verbose); err != nil { | ||
| if err := workflow.ValidateActionSHAsInLockFile(ctx, lockFile, actionCache, verbose); err != nil { |
There was a problem hiding this comment.
[/zoom-out] Split context ownership: CompileWorkflowWithValidation receives ctx and passes it to ValidateActionSHAsInLockFile (line 77), but line 46's compiler.CompileWorkflow(filePath) uses compiler.ctx internally — a different context. A caller who does:
ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
defer cancel()
CompileWorkflowWithValidation(ctx, compiler, filePath, ...)will time out SHA validation but not the compile step (which uses compiler.ctx = context.Background() by default).
To eliminate this inconsistency, either:
- Drop the
ctxparameter fromCompileWorkflowWithValidationentirely and callcompiler.SetContext(ctx)at the call site before invoking this function, so a single context governs everything, or - Forward
ctxto the compiler at the top of the function:compiler.SetContext(ctx)
|
|
||
| // Compiler handles converting markdown workflows to GitHub Actions YAML | ||
| type Compiler struct { | ||
| ctx context.Context // Context for network operations (e.g. SHA resolution); defaults to context.Background() |
There was a problem hiding this comment.
[/zoom-out] Storing context.Context as a struct field is explicitly discouraged by the Go team ([context docs]((pkg.go.dev/redacted) "Do not store Contexts inside a struct type; instead, pass a Context explicitly to each function that needs it."
This approach forces callers to think about per-operation context vs per-compiler-instance context, and leads to the split-ownership issue noted in compile_validation.go. A long-lived Compiler may be reused across multiple compile calls with different deadlines/cancellation scopes.
Consider removing c.ctx and requiring callers to pass ctx directly to each compile entry point. The existing WithContext / SetContext API can be removed once the context is passed per-call instead.
|
|
||
| // Check for updates | ||
| checks := CheckActionSHAUpdates(actions, resolver) | ||
| checks := CheckActionSHAUpdates(context.Background(), actions, resolver) |
There was a problem hiding this comment.
[/tdd] The entire motivation of this PR is to allow callers to inject timeouts/cancellation into ResolveSHA calls. But every test site simply passes context.Background(). There is no test that verifies the context is actually honoured.
Consider adding a test that cancels the context and verifies the operation respects it:
t.Run("respects cancelled context", func(t *testing.T) {
ctx, cancel := context.WithCancel(context.Background())
cancel() // cancel immediately
checks := CheckActionSHAUpdates(ctx, actions, resolver)
// All checks should fail or be skipped — not hang indefinitely
for _, c := range checks {
assert.False(t, c.UpdateAvailable, "should not resolve SHAs with cancelled context")
}
})Without this, the refactor is correct by inspection but not by evidence.
…gation-fix Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…integration test call sites Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done. Merged
All tests pass ( |
Enhancement
5 production call sites hardcoded
context.Background()when callingActionResolver.ResolveSHA(), making it impossible for callers to inject timeouts or cancel hunggh apinetwork calls. This threadsctxend-to-end from cobracmd.Context()down to everyResolveSHAinvocation.pkg/workflow/— core fixesaction_sha_checker.go:CheckActionSHAUpdates(ctx, actions, resolver)action_reference.go:ResolveSetupActionReference(ctx, ...)/resolveSetupActionRef(ctx, ...)maintenance_workflow.go:resolveActionRef(ctx, ...),generateInstallCLISteps(ctx, ...),GenerateMaintenanceWorkflow(ctx, ...)lock_validation.go:ValidateActionSHAsInLockFile(ctx, ...)central_slash_command_workflow.go,maintenance_workflow_yaml.go,side_repo_maintenance.go: corresponding generator functions takectxcompiler_types.go: addedctx context.Contextfield toCompilerwithWithContextoption andSetContextmethod; defaults tocontext.Background()inNewCompiler()pkg/cli/— call chain propagationcopilot_setup.go: all 7 internal functions (getActionRef,ensureCopilotSetupSteps,upgradeCopilotSetupSteps, etc.) takectxCompileWorkflowWithValidation,CompileWorkflowDataWithValidation,compileWorkflowFile,compileSingleFile,compileAllWorkflowFiles,watchAndCompileWorkflows, and orchestrator functions all takectxadd_command.go:AddResolvedWorkflows,addWorkflows,addWorkflowsWithTracking,addWorkflowWithTrackingtakectx;fetchAllRemoteDependenciesupdated to usectxenable.go:EnableWorkflowsByNames/DisableWorkflowsByNamestakectxinit.go:InitOptionsgainsCtx context.Context;InitRepositoryfalls back tocontext.Background()if nilupgrade_command.go,update_workflows.go,update_actions.go: call sites updatedcmd/gh-aw/main.go— boundaryCobra
RunEhandlers passcmd.Context()(signal-aware) instead ofcontext.Background().Before / After
context.Background()now only appears at top-level CLI entry points and in tests.