From d517a9868766fd86fdf58390d52aaceb460695f6 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 7 Aug 2026 19:58:07 +0000 Subject: [PATCH 1/4] Initial plan From 2162b45aec90d39587cb3bc4fa16be7ef4750e8c Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 7 Aug 2026 20:10:22 +0000 Subject: [PATCH 2/4] Improve test quality: migrate tracker_id_integration_test.go to testify Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/workflow/tracker_id_integration_test.go | 164 +++++++++++++------- 1 file changed, 112 insertions(+), 52 deletions(-) diff --git a/pkg/workflow/tracker_id_integration_test.go b/pkg/workflow/tracker_id_integration_test.go index de9ba221540..f11bd6b2e91 100644 --- a/pkg/workflow/tracker_id_integration_test.go +++ b/pkg/workflow/tracker_id_integration_test.go @@ -5,17 +5,25 @@ package workflow import ( "os" "path/filepath" - "strings" "testing" - "github.com/github/gh-aw/pkg/stringutil" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "github.com/github/gh-aw/pkg/stringutil" "github.com/github/gh-aw/pkg/testutil" ) -func TestTrackerIDIntegration(t *testing.T) { - tmpDir := testutil.TempDir(t, "test-*") +// assertScriptUsesRequire asserts that the compiled lock file content sets the +// GH_AW_TRACKER_ID environment variable and loads scripts via require() (file +// mode, not inline). +func assertScriptUsesRequire(t *testing.T, contentStr string) { + t.Helper() + assert.Contains(t, contentStr, "GH_AW_TRACKER_ID", "expected GH_AW_TRACKER_ID environment variable to be set") + assert.Contains(t, contentStr, "require(", "expected scripts to be loaded using require()") +} +func TestTrackerIDIntegration(t *testing.T) { tests := []struct { name string workflowContent string @@ -82,70 +90,122 @@ Create a pull request. shouldHaveInScript: true, expectedTrackerID: "pr-tracker-123", }, + { + name: "Workflow with tracker-id and multiple safe-outputs", + workflowContent: `--- +on: workflow_dispatch +permissions: + contents: read +tracker-id: multi-output-1 +safe-outputs: + create-issue: + create-pull-request: +--- + +# Test Multiple Safe Outputs + +Create an issue and a pull request. +`, + shouldCompile: true, + shouldHaveEnvVar: true, + shouldHaveInScript: true, + expectedTrackerID: "multi-output-1", + }, + { + name: "Workflow with too-short tracker-id", + workflowContent: `--- +on: workflow_dispatch +permissions: + contents: read +tracker-id: short +safe-outputs: + create-issue: +--- + +# Test Short Tracker ID + +Create a test issue. +`, + shouldCompile: false, + }, + { + name: "Workflow with tracker-id containing spaces", + workflowContent: `--- +on: workflow_dispatch +permissions: + contents: read +tracker-id: has spaces +safe-outputs: + create-issue: +--- + +# Test Tracker ID With Spaces + +Create a test issue. +`, + shouldCompile: false, + }, + { + name: "Workflow with tracker-id containing invalid characters", + workflowContent: `--- +on: workflow_dispatch +permissions: + contents: read +tracker-id: bad!chars! +safe-outputs: + create-issue: +--- + +# Test Tracker ID With Invalid Characters + +Create a test issue. +`, + shouldCompile: false, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { + // Each subtest gets its own tmpDir, which testutil.TempDir already + // registers for cleanup via t.Cleanup, so generated workflow and + // lock files are removed automatically even on early failures. + tmpDir := testutil.TempDir(t, "test-*") workflowFile := filepath.Join(tmpDir, "test.md") - err := os.WriteFile(workflowFile, []byte(tt.workflowContent), 0644) - if err != nil { - t.Fatalf("Failed to write test workflow: %v", err) - } + require.NoError(t, os.WriteFile(workflowFile, []byte(tt.workflowContent), 0644)) compiler := NewCompiler() // Use dev mode to test with local action paths compiler.SetActionMode(ActionModeDev) compiler.verbose = false - err = compiler.CompileWorkflow(workflowFile) + err := compiler.CompileWorkflow(workflowFile) - if tt.shouldCompile && err != nil { - t.Fatalf("Expected compilation to succeed, got error: %v", err) - } - if !tt.shouldCompile && err == nil { - t.Fatal("Expected compilation to fail, but it succeeded") + if tt.shouldCompile { + require.NoError(t, err, "expected compilation to succeed") + } else { + require.Error(t, err, "expected compilation to fail") + return } - if tt.shouldCompile { - lockFile := stringutil.MarkdownToLockFile(workflowFile) - content, err := os.ReadFile(lockFile) - if err != nil { - t.Fatalf("Failed to read lock file: %v", err) - } - - contentStr := string(content) - - if tt.shouldHaveEnvVar { - envVarLine := "GH_AW_TRACKER_ID: \"" + tt.expectedTrackerID + "\"" - if !strings.Contains(contentStr, envVarLine) { - t.Errorf("Expected lock file to contain env var '%s', but it didn't", envVarLine) - } - } else { - // The JavaScript code will always read process.env.GH_AW_TRACKER_ID - // but the environment variable should not be set - envVarLine := "GH_AW_TRACKER_ID: \"" - if strings.Contains(contentStr, envVarLine) { - t.Error("Expected lock file to NOT set GH_AW_TRACKER_ID env var, but it did") - } - } - - if tt.shouldHaveInScript { - // Check that tracker-id environment variable is set - if !strings.Contains(contentStr, "GH_AW_TRACKER_ID") { - t.Error("Expected GH_AW_TRACKER_ID environment variable to be set") - } - // Check that scripts are loaded using require() (file mode, not inline) - if !strings.Contains(contentStr, "require(") { - t.Error("Expected scripts to be loaded using require()") - } - } - - // Clean up lock file - os.Remove(lockFile) + lockFile := stringutil.MarkdownToLockFile(workflowFile) + + content, err := os.ReadFile(lockFile) + require.NoError(t, err, "failed to read lock file") + + contentStr := string(content) + + if tt.shouldHaveEnvVar { + envVarLine := "GH_AW_TRACKER_ID: \"" + tt.expectedTrackerID + "\"" + assert.Contains(t, contentStr, envVarLine, "expected lock file to contain tracker-id env var") + } else { + // The JavaScript code will always read process.env.GH_AW_TRACKER_ID + // but the environment variable should not be set + assert.NotContains(t, contentStr, "GH_AW_TRACKER_ID: \"", "expected lock file to NOT set GH_AW_TRACKER_ID env var") } - // Clean up workflow file - os.Remove(workflowFile) + if tt.shouldHaveInScript { + assertScriptUsesRequire(t, contentStr) + } }) } } From d77f109135c0a28de3a2808f7b2c0379a259a675 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 7 Aug 2026 21:08:16 +0000 Subject: [PATCH 3/4] Fix engine definition custom lint findings Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/workflow/engine_definition.go | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/pkg/workflow/engine_definition.go b/pkg/workflow/engine_definition.go index 060cb7bd8a8..56ad488349f 100644 --- a/pkg/workflow/engine_definition.go +++ b/pkg/workflow/engine_definition.go @@ -408,14 +408,20 @@ func downloadKnownEngineImports(ctx context.Context, rawURL string) ([]byte, err // knownEngineImportsTimeout; fetch and parse failures are treated as an empty // catalog so engine validation remains unchanged. func knownEngineImportFor(id string) (string, bool) { - knownEngineImportsMu.Lock() - if knownEngineImportsLoaded { - importPath, ok := knownEngineImports[strings.ToLower(id)] - knownEngineImportsMu.Unlock() + importPath, ok, initialized, download := func() (string, bool, bool, func(context.Context) ([]byte, error)) { + knownEngineImportsMu.Lock() + defer knownEngineImportsMu.Unlock() + + if knownEngineImportsLoaded { + importPath, ok := knownEngineImports[strings.ToLower(id)] + return importPath, ok, true, nil + } + + return "", false, false, knownEngineImportsDownload + }() + if initialized { return importPath, ok } - download := knownEngineImportsDownload - knownEngineImportsMu.Unlock() // Avoid holding the catalog mutex during the network fetch. Concurrent cold // callers may each fetch once, but only the first completed result is cached. @@ -428,7 +434,7 @@ func knownEngineImportFor(id string) (string, bool) { knownEngineImportsLoaded = true } - importPath, ok := knownEngineImports[strings.ToLower(id)] + importPath, ok = knownEngineImports[strings.ToLower(id)] return importPath, ok } @@ -453,7 +459,7 @@ func loadKnownEngineImports(download func(context.Context) ([]byte, error)) map[ for _, engine := range catalog.Engines { id := strings.ToLower(strings.TrimSpace(engine.ID)) importPath := strings.TrimSpace(engine.Import) - if id == "" || importPath == "" { + if len(id) == 0 || importPath == "" { continue } loaded[id] = knownEngineImportWithCompilerRef(importPath) From 77a723c02a03b2945ab7d22a9af0550e83146528 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 7 Aug 2026 21:09:20 +0000 Subject: [PATCH 4/4] Use consistent engine import empty checks Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/workflow/engine_definition.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/workflow/engine_definition.go b/pkg/workflow/engine_definition.go index 56ad488349f..1e78160b3e4 100644 --- a/pkg/workflow/engine_definition.go +++ b/pkg/workflow/engine_definition.go @@ -459,7 +459,7 @@ func loadKnownEngineImports(download func(context.Context) ([]byte, error)) map[ for _, engine := range catalog.Engines { id := strings.ToLower(strings.TrimSpace(engine.ID)) importPath := strings.TrimSpace(engine.Import) - if len(id) == 0 || importPath == "" { + if len(id) == 0 || len(importPath) == 0 { continue } loaded[id] = knownEngineImportWithCompilerRef(importPath)