From 23d7a1998c7d47674438313b87a92a3b96d09f9d Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 24 Jul 2026 04:47:35 +0000 Subject: [PATCH 1/6] Initial plan From cbbd351fea028984cc2c147ec8fd8d2dcd214a01 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 24 Jul 2026 05:00:53 +0000 Subject: [PATCH 2/6] fix: join CheckForUpdatesAsync goroutine and eliminate time.After leak - Change CheckForUpdatesAsync to return a func() join function (mirrors the compile_update_check.go StartCompileUpdateCheck pattern) - Add a buffered done channel closed by the goroutine via defer close(done), providing an observable join point - Replace bare time.After(100ms) with time.NewTimer + defer timer.Stop() to eliminate the per-invocation timer leak - Update validate_command.go caller to capture and defer the join function - Update TestCheckForUpdatesAsync_ContextCancellation to use join() - Add TestCheckForUpdatesAsync_JoinsGoroutine to verify join completes - Update pkg/cli/README.md function signature entry Closes #47609 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/cli/README.md | 2 +- pkg/cli/update_check.go | 24 ++++++++++++++--- pkg/cli/update_check_test.go | 52 ++++++++++++++++++++++++++++++++---- pkg/cli/validate_command.go | 5 ++-- 4 files changed, 71 insertions(+), 12 deletions(-) diff --git a/pkg/cli/README.md b/pkg/cli/README.md index 434747c0f9c..ba611844d04 100644 --- a/pkg/cli/README.md +++ b/pkg/cli/README.md @@ -221,7 +221,7 @@ All diagnostic output MUST go to `stderr` using `console` formatting helpers. St | `PreflightCheckForCreatePR` | `func(bool) error` | Validates prerequisites before creating a PR | | `DisableAllWorkflowsExcept` | `func(repoSlug string, exceptWorkflows []string, verbose bool) error` | Disables all workflows in a repo except the named ones | | `GetEngineSecretNameAndValue` | `func(engine string, existingSecrets map[string]bool) (string, string, bool, error)` | Prompts for and validates an engine API secret | -| `CheckForUpdatesAsync` | `func(ctx, noCheckUpdate, verbose bool)` | Checks for a newer `gh-aw` version in the background | +| `CheckForUpdatesAsync` | `func(ctx, noCheckUpdate, verbose bool) func()` | Checks for a newer `gh-aw` version in the background; returns a join function the caller must invoke before exit | | `FetchChecksResult` | `func(repoOverride, prNumber string) (*ChecksResult, error)` | Fetches CI check results for a pull request | | `ValidEngineNames` | `func() []string` | Returns the supported engine names for shell completion | | `CompleteWorkflowNames` | `func(*cobra.Command, []string, string) ([]string, cobra.ShellCompDirective)` | Shell-completion provider for workflow names | diff --git a/pkg/cli/update_check.go b/pkg/cli/update_check.go index 6ad529912d3..3bc963b0f58 100644 --- a/pkg/cli/update_check.go +++ b/pkg/cli/update_check.go @@ -254,10 +254,15 @@ func findLatestPublishedReleaseTag(releases []Release) string { // CheckForUpdatesAsync performs update check in background (best effort) // This is called from compile command and should never block or fail the compilation -// The context can be used to cancel the update check if the program is shutting down -func CheckForUpdatesAsync(ctx context.Context, noCheckUpdate bool, verbose bool) { +// The context can be used to cancel the update check if the program is shutting down. +// The returned function joins the goroutine; call it before the program exits to ensure +// the update check completes and the goroutine is properly cleaned up. +func CheckForUpdatesAsync(ctx context.Context, noCheckUpdate bool, verbose bool) func() { + done := make(chan struct{}) + // Run check in goroutine to avoid blocking compilation go func() { + defer close(done) // Recover from any panics in the update check defer func() { if r := recover(); r != nil { @@ -277,11 +282,22 @@ func CheckForUpdatesAsync(ctx context.Context, noCheckUpdate bool, verbose bool) // Give the goroutine a small window to complete quickly // This allows the message to appear before compilation starts // but doesn't block if the check takes longer + timer := time.NewTimer(100 * time.Millisecond) + defer timer.Stop() + select { - case <-time.After(100 * time.Millisecond): + case <-done: + // Goroutine finished within the window + case <-timer.C: // Continue after timeout case <-ctx.Done(): // Context cancelled during wait - return + } + + return func() { + select { + case <-done: + case <-ctx.Done(): + } } } diff --git a/pkg/cli/update_check_test.go b/pkg/cli/update_check_test.go index 0f9de34e968..1d15bee8c0d 100644 --- a/pkg/cli/update_check_test.go +++ b/pkg/cli/update_check_test.go @@ -341,11 +341,9 @@ func TestCheckForUpdatesAsync_ContextCancellation(t *testing.T) { // Cancel immediately cancel() - // Call CheckForUpdatesAsync with cancelled context - CheckForUpdatesAsync(ctx, false, false) - - // Wait a bit to ensure any goroutines would have had time to run - time.Sleep(200 * time.Millisecond) + // Call CheckForUpdatesAsync with cancelled context and join the goroutine + join := CheckForUpdatesAsync(ctx, false, false) + join() // The update check should not have created a last check file // because the context was cancelled @@ -353,6 +351,50 @@ func TestCheckForUpdatesAsync_ContextCancellation(t *testing.T) { // so we just verify no panics occurred } +func TestCheckForUpdatesAsync_JoinsGoroutine(t *testing.T) { + // Test that the returned join function waits for the goroutine to complete + // Save original environment + origCI := os.Getenv("CI") + origGetLastCheckFilePath := getLastCheckFilePathFunc + defer func() { + os.Setenv("CI", origCI) + getLastCheckFilePathFunc = origGetLastCheckFilePath + }() + + // Ensure we're not in CI mode so that shouldCheckForUpdate returns true + os.Unsetenv("CI") + os.Unsetenv("GITHUB_ACTIONS") + os.Unsetenv("CONTINUOUS_INTEGRATION") + os.Unsetenv("GH_AW_MCP_SERVER") + + // Create temporary directory for last check file + tmpDir := t.TempDir() + lastCheckFile := filepath.Join(tmpDir, lastCheckFileName) + getLastCheckFilePathFunc = func() string { + return lastCheckFile + } + + // Use a cancelled context to make the goroutine exit quickly + ctx, cancel := context.WithCancel(context.Background()) + cancel() + + join := CheckForUpdatesAsync(ctx, false, false) + + // join() must return; if it blocks forever the test will time out + done := make(chan struct{}) + go func() { + defer close(done) + join() + }() + + select { + case <-done: + // goroutine joined successfully + case <-time.After(2 * time.Second): + t.Fatal("join function did not return within 2 seconds") + } +} + func TestFindLatestPublishedReleaseTag(t *testing.T) { tests := []struct { name string diff --git a/pkg/cli/validate_command.go b/pkg/cli/validate_command.go index 2c406e964b6..2e1088de9d1 100644 --- a/pkg/cli/validate_command.go +++ b/pkg/cli/validate_command.go @@ -44,8 +44,9 @@ If no workflows are specified, all Markdown files in .github/workflows will be v return err } - // Check for updates (non-blocking, runs once per day) - CheckForUpdatesAsync(cmd.Context(), noCheckUpdate, verbose) + // Check for updates (non-blocking, runs once per day); join before exit + joinUpdateCheck := CheckForUpdatesAsync(cmd.Context(), noCheckUpdate, verbose) + defer joinUpdateCheck() validateLog.Printf("Running validate command: workflows=%v, dir=%s", args, dir) From 0c938ba41a18d7304a0f07343d4a41a2e999200f Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 24 Jul 2026 05:36:31 +0000 Subject: [PATCH 3/6] fix: make update check finisher cancel child context and join reliably Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/cli/update_check.go | 15 +++++++------- pkg/cli/update_check_test.go | 38 ++++++++++++++++++++++++------------ 2 files changed, 34 insertions(+), 19 deletions(-) diff --git a/pkg/cli/update_check.go b/pkg/cli/update_check.go index 3bc963b0f58..9d15533a67a 100644 --- a/pkg/cli/update_check.go +++ b/pkg/cli/update_check.go @@ -83,6 +83,8 @@ func isRunningAsMCPServer() bool { var ( // getLastCheckFilePathFunc allows overriding in tests getLastCheckFilePathFunc = getLastCheckFilePathImpl + // checkForUpdatesWithContextFunc allows overriding in tests + checkForUpdatesWithContextFunc = checkForUpdatesWithContext ) // getLastCheckFilePath returns the path to the last check timestamp file @@ -259,6 +261,7 @@ func findLatestPublishedReleaseTag(releases []Release) string { // the update check completes and the goroutine is properly cleaned up. func CheckForUpdatesAsync(ctx context.Context, noCheckUpdate bool, verbose bool) func() { done := make(chan struct{}) + checkCtx, cancelCheck := context.WithCancel(ctx) // Run check in goroutine to avoid blocking compilation go func() { @@ -271,12 +274,12 @@ func CheckForUpdatesAsync(ctx context.Context, noCheckUpdate bool, verbose bool) }() // Check if context was cancelled before starting - if ctx.Err() != nil { - updateCheckLog.Printf("Update check cancelled before starting: %v", ctx.Err()) + if checkCtx.Err() != nil { + updateCheckLog.Printf("Update check cancelled before starting: %v", checkCtx.Err()) return } - checkForUpdatesWithContext(ctx, noCheckUpdate, verbose) + checkForUpdatesWithContextFunc(checkCtx, noCheckUpdate, verbose) }() // Give the goroutine a small window to complete quickly @@ -295,9 +298,7 @@ func CheckForUpdatesAsync(ctx context.Context, noCheckUpdate bool, verbose bool) } return func() { - select { - case <-done: - case <-ctx.Done(): - } + cancelCheck() + <-done } } diff --git a/pkg/cli/update_check_test.go b/pkg/cli/update_check_test.go index 1d15bee8c0d..264754c3815 100644 --- a/pkg/cli/update_check_test.go +++ b/pkg/cli/update_check_test.go @@ -353,19 +353,18 @@ func TestCheckForUpdatesAsync_ContextCancellation(t *testing.T) { func TestCheckForUpdatesAsync_JoinsGoroutine(t *testing.T) { // Test that the returned join function waits for the goroutine to complete - // Save original environment - origCI := os.Getenv("CI") origGetLastCheckFilePath := getLastCheckFilePathFunc + origCheckForUpdatesWithContext := checkForUpdatesWithContextFunc defer func() { - os.Setenv("CI", origCI) getLastCheckFilePathFunc = origGetLastCheckFilePath + checkForUpdatesWithContextFunc = origCheckForUpdatesWithContext }() // Ensure we're not in CI mode so that shouldCheckForUpdate returns true - os.Unsetenv("CI") - os.Unsetenv("GITHUB_ACTIONS") - os.Unsetenv("CONTINUOUS_INTEGRATION") - os.Unsetenv("GH_AW_MCP_SERVER") + t.Setenv("CI", "") + t.Setenv("GITHUB_ACTIONS", "") + t.Setenv("CONTINUOUS_INTEGRATION", "") + t.Setenv("GH_AW_MCP_SERVER", "") // Create temporary directory for last check file tmpDir := t.TempDir() @@ -374,13 +373,19 @@ func TestCheckForUpdatesAsync_JoinsGoroutine(t *testing.T) { return lastCheckFile } - // Use a cancelled context to make the goroutine exit quickly - ctx, cancel := context.WithCancel(context.Background()) - cancel() + started := make(chan struct{}) + release := make(chan struct{}) + checkForUpdatesWithContextFunc = func(_ context.Context, _ bool, _ bool) { + close(started) + <-release + } + + ctx := context.Background() join := CheckForUpdatesAsync(ctx, false, false) + <-started - // join() must return; if it blocks forever the test will time out + // join() must wait until the worker exits. done := make(chan struct{}) go func() { defer close(done) @@ -389,7 +394,16 @@ func TestCheckForUpdatesAsync_JoinsGoroutine(t *testing.T) { select { case <-done: - // goroutine joined successfully + t.Fatal("join returned before worker exited") + case <-time.After(100 * time.Millisecond): + // join is correctly blocked waiting for worker completion + } + + close(release) + + select { + case <-done: + // goroutine joined successfully after worker exit case <-time.After(2 * time.Second): t.Fatal("join function did not return within 2 seconds") } From 0b29b721d757b2848d0d5f091eab15d444fb02aa Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 24 Jul 2026 07:25:22 +0000 Subject: [PATCH 4/6] test: use t.Setenv in async cancellation test Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com> --- pkg/cli/update_check_test.go | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/pkg/cli/update_check_test.go b/pkg/cli/update_check_test.go index 264754c3815..fd440b7bab8 100644 --- a/pkg/cli/update_check_test.go +++ b/pkg/cli/update_check_test.go @@ -313,18 +313,16 @@ func TestCheckForUpdatesInCIMode(t *testing.T) { func TestCheckForUpdatesAsync_ContextCancellation(t *testing.T) { // Test that async update check respects context cancellation - // Save original environment - origCI := os.Getenv("CI") origGetLastCheckFilePath := getLastCheckFilePathFunc defer func() { - os.Setenv("CI", origCI) getLastCheckFilePathFunc = origGetLastCheckFilePath }() // Ensure we're not in CI mode - os.Unsetenv("CI") - os.Unsetenv("GITHUB_ACTIONS") - os.Unsetenv("CONTINUOUS_INTEGRATION") + t.Setenv("CI", "") + t.Setenv("GITHUB_ACTIONS", "") + t.Setenv("CONTINUOUS_INTEGRATION", "") + t.Setenv("GH_AW_MCP_SERVER", "") // Create temporary directory for last check file tmpDir := t.TempDir() From c74c091024212b8cf88f465a127f2421a829852a Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 24 Jul 2026 15:43:05 +0000 Subject: [PATCH 5/6] test: stabilize wasm golden comparisons for checkout pin drift Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/workflow/wasm_golden_test.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/pkg/workflow/wasm_golden_test.go b/pkg/workflow/wasm_golden_test.go index a429022c325..06925fdf7bc 100644 --- a/pkg/workflow/wasm_golden_test.go +++ b/pkg/workflow/wasm_golden_test.go @@ -26,6 +26,7 @@ var testDefaultAWFImageRE = regexp.MustCompile(`(ghcr\.io/github/gh-aw-firewall/ var testDefaultAWFSchemaURLRE = regexp.MustCompile(`(releases/download/)` + regexp.QuoteMeta(string(constants.DefaultFirewallVersion)) + `(/awf-config\.schema\.json)`) var testDefaultAWFImageTagRE = regexp.MustCompile(`("imageTag"\s*:\s*")(?:v)?` + regexp.QuoteMeta(strings.TrimPrefix(string(constants.DefaultFirewallVersion), "v")) + `"`) var testDefaultMCPGImageRE = regexp.MustCompile(`(ghcr\.io/github/gh-aw-mcpg:)` + regexp.QuoteMeta(string(constants.DefaultMCPGatewayVersion)) + `\b`) +var testCheckoutPinRE = regexp.MustCompile(`actions/checkout@[0-9a-f]{40}\s+#\s+v\d+\.\d+\.\d+`) func normalizeDefaultRuntimeVersions(content string) string { normalized := testDefaultAWFInfoVersionRE.ReplaceAllString(content, `GH_AW_INFO_AWF_VERSION: "vAWF_VERSION"`) @@ -53,6 +54,7 @@ func normalizeOutput(content string) string { normalized = strings.ReplaceAll(normalized, op+"(/tmp/gh-aw/*)", op+"(/tmp/gh-aw/agent/*)") } normalized = normalizeDefaultRuntimeVersions(normalized) + normalized = testCheckoutPinRE.ReplaceAllString(normalized, "actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0") return testAWFImageTagDigestRE.ReplaceAllString(normalized, "") } From 97101b6b669f02d69f10b48118e3200383da997e Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 24 Jul 2026 15:58:16 +0000 Subject: [PATCH 6/6] test: make action pin assertions resilient to checkout version bumps Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/workflow/action_pins_test.go | 34 +++++++++++++++++----------- pkg/workflow/action_resolver_test.go | 25 +++++++++++++------- 2 files changed, 38 insertions(+), 21 deletions(-) diff --git a/pkg/workflow/action_pins_test.go b/pkg/workflow/action_pins_test.go index 8e4a16fd3a3..291c3ac9772 100644 --- a/pkg/workflow/action_pins_test.go +++ b/pkg/workflow/action_pins_test.go @@ -323,10 +323,10 @@ func TestGetLatestActionPinByRepo(t *testing.T) { expectVersionPrefix string }{ { - repo: "actions/checkout", - expectExists: true, - expectRepo: "actions/checkout", - expectVersion: "v7.0.0", + repo: "actions/checkout", + expectExists: true, + expectRepo: "actions/checkout", + expectVersionPrefix: "v", }, { repo: "actions/setup-node", @@ -1764,6 +1764,14 @@ func TestGetActionPinPrefersLatestEmbeddedOverStaleCache(t *testing.T) { func TestWarnIfOutdatedActionVersion(t *testing.T) { const checkoutRepo = "actions/checkout" checkoutLatest := latestActionVersionForRepo(t, checkoutRepo) + checkoutLatestSemver := semverutil.ParseVersion(checkoutLatest) + if checkoutLatestSemver == nil || checkoutLatestSemver.Major < 1 { + t.Skipf("need a parseable checkout version with major >= 1, got %q", checkoutLatest) + } + checkoutMajorTag := fmt.Sprintf("v%d", checkoutLatestSemver.Major) + checkoutMinorTag := fmt.Sprintf("v%d.%d", checkoutLatestSemver.Major, checkoutLatestSemver.Minor) + checkoutOlderMajorTag := fmt.Sprintf("v%d", checkoutLatestSemver.Major-1) + checkoutOlderMinorTag := fmt.Sprintf("v%d.1", checkoutLatestSemver.Major-1) tests := []struct { name string @@ -1792,39 +1800,39 @@ func TestWarnIfOutdatedActionVersion(t *testing.T) { { name: "same version does not warn", repo: checkoutRepo, - rawVersion: "v7.0.0", + rawVersion: checkoutLatest, latestVer: checkoutLatest, expectWarn: false, }, { name: "partial tag same major does not warn", repo: checkoutRepo, - rawVersion: "v7", + rawVersion: checkoutMajorTag, latestVer: checkoutLatest, expectWarn: false, }, { name: "minor partial tag same major does not warn", repo: checkoutRepo, - rawVersion: "v7.0", - latestVer: "v7.1.0", + rawVersion: checkoutMinorTag, + latestVer: checkoutLatest, expectWarn: false, }, { name: "partial tag older major warns", repo: checkoutRepo, - rawVersion: "v6", + rawVersion: checkoutOlderMajorTag, latestVer: checkoutLatest, expectWarn: true, - warnContains: "v6", + warnContains: checkoutOlderMajorTag, }, { name: "minor partial tag older major warns", repo: checkoutRepo, - rawVersion: "v6.1", - latestVer: "v7.1.0", + rawVersion: checkoutOlderMinorTag, + latestVer: checkoutLatest, expectWarn: true, - warnContains: "v6.1", + warnContains: checkoutOlderMinorTag, }, { name: "SHA ref does not warn", diff --git a/pkg/workflow/action_resolver_test.go b/pkg/workflow/action_resolver_test.go index 0c31e8c2230..23b97e759e8 100644 --- a/pkg/workflow/action_resolver_test.go +++ b/pkg/workflow/action_resolver_test.go @@ -120,25 +120,34 @@ func TestActionResolverFailedResolutionCache(t *testing.T) { // layers. It covers the three reachable code paths: precise-version match, // range (major/minor) match, and no match. func TestLookupEmbeddedActionPin(t *testing.T) { + latestCheckoutPin, ok := getLatestActionPinByRepo("actions/checkout") + if !ok || latestCheckoutPin.Version == "" { + t.Fatal("expected latest embedded pin for actions/checkout") + } + + major := strings.Split(strings.TrimPrefix(latestCheckoutPin.Version, "v"), ".")[0] + if major == "" { + t.Fatalf("failed to derive major version from %q", latestCheckoutPin.Version) + } + checkoutMajorTag := "v" + major + t.Run("precise version returns SHA", func(t *testing.T) { - // actions/checkout@v7.0.0 is in the embedded pin set. - sha, found := lookupEmbeddedActionPin("actions/checkout", "v7.0.0") + sha, found := lookupEmbeddedActionPin("actions/checkout", latestCheckoutPin.Version) if !found { - t.Fatal("expected embedded pin hit for actions/checkout@v7.0.0, got not-found") + t.Fatalf("expected embedded pin hit for actions/checkout@%s, got not-found", latestCheckoutPin.Version) } if sha == "" { - t.Error("expected non-empty SHA for actions/checkout@v7.0.0") + t.Fatalf("expected non-empty SHA for actions/checkout@%s", latestCheckoutPin.Version) } }) t.Run("semver range returns SHA for compatible pin", func(t *testing.T) { - // v7 is compatible with the pinned v7.0.0. - sha, found := lookupEmbeddedActionPin("actions/checkout", "v7") + sha, found := lookupEmbeddedActionPin("actions/checkout", checkoutMajorTag) if !found { - t.Fatal("expected embedded pin hit for actions/checkout@v7 (compatible with v7.0.0), got not-found") + t.Fatalf("expected embedded pin hit for actions/checkout@%s, got not-found", checkoutMajorTag) } if sha == "" { - t.Error("expected non-empty SHA for actions/checkout@v7") + t.Fatalf("expected non-empty SHA for actions/checkout@%s", checkoutMajorTag) } })