Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
# ADR-30070: Propagate context.Context Through Action SHA Resolution and Compilation Pipeline

**Date**: 2026-05-04
**Status**: Draft
**Deciders**: Unknown (AI-generated draft — review and finalize before accepting)

---

## Part 1 — Narrative (Human-Friendly)

### Context

The `gh-aw` CLI resolves GitHub Actions SHA pins by calling GitHub's REST API at multiple points during workflow compilation and the `add` command pipeline. These calls were previously made using hardcoded `context.Background()` contexts, which made every API call immune to cancellation and timeouts. When GitHub's API is slow or unreachable, the tool would hang indefinitely with no way for the caller — including Cobra command handlers, tests, or programmatic users — to interrupt the operation. Go's standard convention for cancellable I/O is to accept a `context.Context` as the first parameter and pass it through to all downstream I/O calls.

### Decision

We will thread `context.Context` as the first parameter through every function in the action SHA resolution and workflow compilation call stacks that performs or delegates I/O, replacing all internal uses of `context.Background()` with the caller-supplied context. Where threading context through a method signature is impractical (specifically for `Compiler` internal methods called from many dispatch sites), we store the context on the `Compiler` struct via `SetContext(ctx)` with a `context.Background()` fallback, making the intent visible while preserving backward compatibility for call sites that have no available context.

### Alternatives Considered

#### Alternative 1: Keep Hardcoded `context.Background()` (Status Quo)

Every I/O call would continue to use `context.Background()`, ignoring any cancellation signal from the caller. This is simple and requires no API changes, but it means timeouts and context cancellations from Cobra (e.g., Ctrl-C) have no effect on in-flight GitHub API calls, causing potential indefinite hangs in CI and watch-mode scenarios.

#### Alternative 2: Package-Level or Global Context Variable

Store the "current" context in a package-level variable, updated at command entry points. This avoids changing dozens of function signatures but is not idiomatic Go, introduces implicit state shared across goroutines (data-race risk), and makes testing harder since tests cannot inject isolated contexts. This approach was rejected because the signature-threading approach, while verbose, is the standard Go pattern and is safe under concurrent test execution.

#### Alternative 3: Full Context on `Compiler` Struct (Struct-Only Storage)

Store context exclusively on the `Compiler` struct and access it through a method everywhere, avoiding parameter changes entirely. This would be more consistent but would hide context flow from function signatures, making it harder for readers to see which functions perform I/O. The hybrid approach chosen — threading context via parameters for most functions and falling back to struct storage only for compiler-internal methods — keeps the call graph readable for the common case.

### Consequences

#### Positive
- Callers (Cobra commands, integration tests, background agents) can now cancel or time out all GitHub API calls by cancelling the context they supply.
- `TestCheckActionSHAUpdates_ContextCancellation` demonstrates and enforces the new behavior: a pre-cancelled context causes resolution to skip cleanly rather than hang.
- The code is now conformant with Go's standard context propagation idiom throughout the I/O paths.

#### Negative
- Every function in the changed call stack has a new leading `context.Context` parameter, which is a breaking API change for any external callers of the affected public functions (`AddResolvedWorkflows`, `CompileWorkflowWithValidation`, `CompileWorkflowDataWithValidation`, `CheckActionSHAUpdates`, `ValidateActionSHAsInLockFile`).
- The `Compiler` struct now carries mutable state (the stored context), which can be surprising in concurrent uses of a single `Compiler` instance.
- Call sites that have no meaningful context (e.g., `enable.go` watch-mode compilation) must explicitly pass `context.Background()`, adding boilerplate without behavioral change.

#### Neutral
- `FetchDefaultBranch` switches from `RunGH` to `RunGHContext` as a consequence of receiving a real context; callers see no change in behavior when the context is not cancelled.
- The `Compiler.context()` accessor falls back to `context.Background()` if `SetContext` was never called, preserving existing behavior for callers that have not been updated yet.

---

## Part 2 — Normative Specification (RFC 2119)

> The key words **MUST**, **MUST NOT**, **REQUIRED**, **SHALL**, **SHALL NOT**, **SHOULD**, **SHOULD NOT**, **RECOMMENDED**, **MAY**, and **OPTIONAL** in this section are to be interpreted as described in [RFC 2119](https://www.rfc-editor.org/rfc/rfc2119).

### Context Propagation in I/O Functions

1. Any function that performs or delegates a GitHub API call, git remote operation, or other network I/O **MUST** accept a `context.Context` as its first parameter.
2. Functions **MUST NOT** replace a caller-supplied context with `context.Background()` before passing it to a downstream I/O call unless the function is explicitly documented as a fire-and-forget background operation.
3. Callers that have no meaningful context available (e.g., watch-mode loops, legacy wrappers) **SHOULD** pass `context.Background()` explicitly and document why they cannot propagate a richer context.
4. New internal helpers that perform only pure computation (no I/O) **MAY** omit the context parameter.

### Compiler Context Storage

1. When threading context through every method signature of `Compiler` is impractical, implementations **MUST** call `compiler.SetContext(ctx)` before invoking compilation so that compiler-internal I/O inherits the caller's context.
2. `Compiler.context()` **MUST** return `context.Background()` as a fallback when no context has been set via `SetContext`, ensuring the `Compiler` is safe to use without explicit context injection.
3. `Compiler` instances **SHOULD NOT** be shared across goroutines after `SetContext` has been called, because the stored context field is mutable and not protected by a mutex.

### Testing

1. Tests for functions that accept a context **MUST** pass a real (non-nil) context, and **SHOULD** include at least one test case that verifies behavior under a pre-cancelled context.
2. Tests **MUST NOT** pass `nil` as a context argument; they **MUST** pass at minimum `context.Background()`.

### Conformance

An implementation is considered conformant with this ADR if it satisfies all **MUST** and **MUST NOT** requirements above. Failure to meet any **MUST** or **MUST NOT** requirement constitutes non-conformance.

---

*This is a DRAFT ADR generated by the [Design Decision Gate](https://github.com/github/gh-aw/actions/runs/25323021162) workflow. The PR author must review, complete, and finalize this document before the PR can merge.*
15 changes: 15 additions & 0 deletions pkg/actionpins/data/action_pins.json
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,11 @@
"version": "v5.0.5",
"sha": "27d5ce7f107fe9357f9df03efb73ab90386fccae"
},
"actions/cache/save@v4": {
"repo": "actions/cache/save",
"version": "v4",
"sha": "0057852bfaa89a56745cba8c7296529d2fc39830"
},
"actions/cache/save@v5.0.5": {
"repo": "actions/cache/save",
"version": "v5.0.5",
Expand All @@ -48,6 +53,11 @@
"version": "v5.0.5",
"sha": "27d5ce7f107fe9357f9df03efb73ab90386fccae"
},
"actions/checkout@v4": {
"repo": "actions/checkout",
"version": "v4",
"sha": "34e114876b0b11c390a56381ad16ebd13914f8d5"
},
"actions/checkout@v6.0.2": {
"repo": "actions/checkout",
"version": "v6.0.2",
Expand Down Expand Up @@ -88,6 +98,11 @@
"version": "v6.4.0",
"sha": "48b55a011bda9f5d6aeb4c2d9c7362e8dae4041e"
},
"actions/setup-python@v5": {
"repo": "actions/setup-python",
"version": "v5",
"sha": "a26af69be951a213d495a4c3e4e4022e16d87065"
},
"actions/setup-python@v6.2.0": {
"repo": "actions/setup-python",
"version": "v6.2.0",
Expand Down
28 changes: 14 additions & 14 deletions pkg/cli/add_command.go
Original file line number Diff line number Diff line change
Expand Up @@ -184,13 +184,13 @@ func AddWorkflows(ctx context.Context, workflows []string, opts AddOptions) (*Ad
return nil, err
}

return AddResolvedWorkflows(workflows, resolved, opts)
return AddResolvedWorkflows(ctx, workflows, resolved, opts)
}

// AddResolvedWorkflows adds workflows using pre-resolved workflow data.
// This allows callers to resolve workflows early (e.g., to show descriptions) and then add them later.
// The opts.Quiet parameter suppresses detailed output (useful for interactive mode where output is already shown).
func AddResolvedWorkflows(workflowStrings []string, resolved *ResolvedWorkflows, opts AddOptions) (*AddWorkflowsResult, error) {
func AddResolvedWorkflows(ctx context.Context, workflowStrings []string, resolved *ResolvedWorkflows, opts AddOptions) (*AddWorkflowsResult, error) {
addLog.Printf("Adding workflows: count=%d, engineOverride=%s, createPR=%v, noGitattributes=%v, opts.WorkflowDir=%s, noStopAfter=%v, stopAfter=%s", len(workflowStrings), opts.EngineOverride, opts.CreatePR, opts.NoGitattributes, opts.WorkflowDir, opts.NoStopAfter, opts.StopAfter)

result := &AddWorkflowsResult{}
Expand Down Expand Up @@ -222,7 +222,7 @@ func AddResolvedWorkflows(workflowStrings []string, resolved *ResolvedWorkflows,
// Handle PR creation workflow
if opts.CreatePR {
addLog.Print("Creating workflow with PR")
prNumber, prURL, err := addWorkflowsWithPR(resolved.Workflows, opts)
prNumber, prURL, err := addWorkflowsWithPR(ctx, resolved.Workflows, opts)
if err != nil {
return nil, err
}
Expand All @@ -233,11 +233,11 @@ func AddResolvedWorkflows(workflowStrings []string, resolved *ResolvedWorkflows,

// Handle normal workflow addition - pass resolved workflows with content
addLog.Print("Adding workflows normally without PR")
return result, addWorkflows(resolved.Workflows, opts)
return result, addWorkflows(ctx, resolved.Workflows, opts)
}

// addWorkflows handles workflow addition using pre-fetched content
func addWorkflows(workflows []*ResolvedWorkflow, opts AddOptions) error {
func addWorkflows(ctx context.Context, workflows []*ResolvedWorkflow, opts AddOptions) error {
addLog.Printf("Adding %d workflow(s) to repository", len(workflows))
// Create file tracker for all operations
tracker, err := NewFileTracker()
Expand All @@ -248,11 +248,11 @@ func addWorkflows(workflows []*ResolvedWorkflow, opts AddOptions) error {
}
tracker = nil
}
return addWorkflowsWithTracking(workflows, tracker, opts)
return addWorkflowsWithTracking(ctx, workflows, tracker, opts)
}

// addWorkflows handles workflow addition using pre-fetched content
func addWorkflowsWithTracking(workflows []*ResolvedWorkflow, tracker *FileTracker, opts AddOptions) error {
// addWorkflowsWithTracking handles workflow addition using pre-fetched content
func addWorkflowsWithTracking(ctx context.Context, workflows []*ResolvedWorkflow, tracker *FileTracker, opts AddOptions) error {
addLog.Printf("Adding %d workflow(s) with tracking: force=%v, disableSecurityScanner=%v", len(workflows), opts.Force, opts.DisableSecurityScanner)
// Ensure .gitattributes is configured unless flag is set
if !opts.NoGitattributes {
Expand All @@ -278,7 +278,7 @@ func addWorkflowsWithTracking(workflows []*ResolvedWorkflow, tracker *FileTracke
fmt.Fprintln(os.Stderr, console.FormatProgressMessage(fmt.Sprintf("Adding workflow %d/%d: %s", i+1, len(workflows), resolved.Spec.WorkflowName)))
}

if err := addWorkflowWithTracking(resolved, tracker, opts); err != nil {
if err := addWorkflowWithTracking(ctx, resolved, tracker, opts); err != nil {
return fmt.Errorf("failed to add workflow '%s': %w", resolved.Spec.String(), err)
}
}
Expand All @@ -291,7 +291,7 @@ func addWorkflowsWithTracking(workflows []*ResolvedWorkflow, tracker *FileTracke
}

// addWorkflowWithTracking adds a workflow using pre-fetched content with file tracking
func addWorkflowWithTracking(resolved *ResolvedWorkflow, tracker *FileTracker, opts AddOptions) error {
func addWorkflowWithTracking(ctx context.Context, resolved *ResolvedWorkflow, tracker *FileTracker, opts AddOptions) error {
workflowSpec := resolved.Spec
sourceContent := resolved.Content
sourceInfo := resolved.SourceInfo
Expand Down Expand Up @@ -363,7 +363,7 @@ func addWorkflowWithTracking(resolved *ResolvedWorkflow, tracker *FileTracker, o

// For remote workflows, fetch and save all dependencies (includes, imports, dispatch workflows, resources)
if !isLocalWorkflowPath(workflowSpec.WorkflowPath) {
if err := fetchAllRemoteDependencies(context.Background(), string(sourceContent), workflowSpec, githubWorkflowsDir, opts.Verbose, opts.Force, tracker); err != nil {
if err := fetchAllRemoteDependencies(ctx, string(sourceContent), workflowSpec, githubWorkflowsDir, opts.Verbose, opts.Force, tracker); err != nil {
return err
}
} else if sourceInfo != nil && sourceInfo.IsLocal {
Expand Down Expand Up @@ -536,15 +536,15 @@ func addWorkflowWithTracking(resolved *ResolvedWorkflow, tracker *FileTracker, o
// Compile any dispatch-workflow .md dependencies that were just fetched and lack a
// .lock.yml. The dispatch-workflow validator requires every .md dispatch target to be
// compiled before the main workflow can be validated.
compileDispatchWorkflowDependencies(destFile, opts.Verbose, opts.Quiet, opts.EngineOverride, tracker)
compileDispatchWorkflowDependencies(ctx, destFile, opts.Verbose, opts.Quiet, opts.EngineOverride, tracker)

// Compile the workflow
if tracker != nil {
if err := compileWorkflowWithTracking(destFile, opts.Verbose, opts.Quiet, opts.EngineOverride, tracker); err != nil {
if err := compileWorkflowWithTracking(ctx, destFile, opts.Verbose, opts.Quiet, opts.EngineOverride, tracker); err != nil {
fmt.Fprintln(os.Stderr, console.FormatErrorChain(err))
}
} else {
if err := compileWorkflow(destFile, opts.Verbose, opts.Quiet, opts.EngineOverride); err != nil {
if err := compileWorkflow(ctx, destFile, opts.Verbose, opts.Quiet, opts.EngineOverride); err != nil {
fmt.Fprintln(os.Stderr, console.FormatErrorChain(err))
}
}
Expand Down
5 changes: 3 additions & 2 deletions pkg/cli/add_command_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,7 @@ func TestAddResolvedWorkflows(t *testing.T) {

opts := AddOptions{}
_, err := AddResolvedWorkflows(
context.Background(),
[]string{"test/repo/test-workflow"},
resolved,
opts,
Expand Down Expand Up @@ -461,7 +462,7 @@ func TestAddWorkflowWithTracking_SourceFieldVariants(t *testing.T) {
}
opts := AddOptions{DisableSecurityScanner: true}

err := addWorkflowWithTracking(resolved, nil, opts)
err := addWorkflowWithTracking(context.Background(), resolved, nil, opts)
require.NoError(t, err, "addWorkflowWithTracking should succeed")

written, err := os.ReadFile(filepath.Join(workflowsDir, tt.spec.WorkflowName+".md"))
Expand Down Expand Up @@ -515,7 +516,7 @@ func TestAddWorkflowWithTracking_UsesActualFetchedPath(t *testing.T) {
opts := AddOptions{
DisableSecurityScanner: true,
}
err := addWorkflowWithTracking(resolved, nil, opts)
err := addWorkflowWithTracking(context.Background(), resolved, nil, opts)
require.NoError(t, err, "addWorkflowWithTracking should succeed")

// Read the written file
Expand Down
7 changes: 4 additions & 3 deletions pkg/cli/add_gitattributes_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
package cli

import (
"context"
"os"
"os/exec"
"path/filepath"
Expand Down Expand Up @@ -89,7 +90,7 @@ This is a test workflow.`

// Call addWorkflows with noGitattributes=false
opts := AddOptions{}
err := addWorkflows([]*ResolvedWorkflow{resolved}, opts)
err := addWorkflows(context.Background(), []*ResolvedWorkflow{resolved}, opts)
if err != nil {
// Log any error but don't fail - we're testing gitattributes behavior
t.Logf("Note: workflow addition returned: %v", err)
Expand Down Expand Up @@ -119,7 +120,7 @@ This is a test workflow.`

opts := AddOptions{NoGitattributes: true}
// Call addWorkflows with noGitattributes=true
err := addWorkflows([]*ResolvedWorkflow{resolved}, opts)
err := addWorkflows(context.Background(), []*ResolvedWorkflow{resolved}, opts)
if err != nil {
// Log any error but don't fail - we're testing gitattributes behavior
t.Logf("Note: workflow addition returned: %v", err)
Expand All @@ -142,7 +143,7 @@ This is a test workflow.`

opts := AddOptions{NoGitattributes: true}
// Call addWorkflows with noGitattributes=true
err := addWorkflows([]*ResolvedWorkflow{resolved}, opts)
err := addWorkflows(context.Background(), []*ResolvedWorkflow{resolved}, opts)
if err != nil {
// Log any error but don't fail - we're testing gitattributes behavior
t.Logf("Note: workflow addition returned: %v", err)
Expand Down
2 changes: 1 addition & 1 deletion pkg/cli/add_interactive_git.go
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ func (c *AddInteractiveConfig) createWorkflowPRAndConfigureSecret(ctx context.Co
StopAfter: c.StopAfter,
DisableSecurityScanner: false,
}
result, err := AddResolvedWorkflows(c.WorkflowSpecs, c.resolvedWorkflows, opts)
result, err := AddResolvedWorkflows(ctx, c.WorkflowSpecs, c.resolvedWorkflows, opts)
if err != nil {
return fmt.Errorf("failed to add workflow: %w", err)
}
Expand Down
Loading
Loading