diff --git a/pkg/workflow/activation_checkout_test.go b/pkg/workflow/activation_checkout_test.go deleted file mode 100644 index e7b884750ea..00000000000 --- a/pkg/workflow/activation_checkout_test.go +++ /dev/null @@ -1,152 +0,0 @@ -//go:build integration - -package workflow - -import ( - "os" - "path/filepath" - "strings" - "testing" - - "github.com/github/gh-aw/pkg/stringutil" - - "github.com/github/gh-aw/pkg/testutil" -) - -// TestActivationJobNoCheckoutStep tests that the activation job uses GitHub API -// instead of checking out the repository for the timestamp check -func TestActivationJobNoCheckoutStep(t *testing.T) { - tests := []struct { - name string - frontmatter string - description string - }{ - { - name: "basic workflow has no checkout in activation", - frontmatter: `--- -on: - issues: - types: [opened] -permissions: - contents: read - issues: read -engine: claude -strict: false ----`, - description: "Activation job should not include checkout step - uses GitHub API instead", - }, - { - name: "workflow without contents permission has no checkout in activation", - frontmatter: `--- -on: - issues: - types: [opened] -permissions: - issues: read -engine: claude -strict: false ----`, - description: "Activation job should not include checkout - uses GitHub API instead", - }, - { - name: "workflow with reaction has no checkout in activation", - frontmatter: `--- -on: - issues: - types: [opened] - reaction: eyes -permissions: - issues: read -engine: claude -strict: false ----`, - description: "Activation job with reaction should not include checkout - uses GitHub API instead", - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - tmpDir := testutil.TempDir(t, "activation-checkout-test") - - testContent := tt.frontmatter + "\n\n# Test Workflow\n\nTest workflow content." - testFile := filepath.Join(tmpDir, "test-workflow.md") - if err := os.WriteFile(testFile, []byte(testContent), 0644); err != nil { - t.Fatal(err) - } - - compiler := NewCompiler(WithVersion("dev")) - // Use dev mode to use local action paths - compiler.SetActionMode(ActionModeDev) - - // Compile the workflow - if err := compiler.CompileWorkflow(testFile); err != nil { - t.Fatalf("Failed to compile workflow: %v", err) - } - - // Calculate the lock file path - lockFile := stringutil.MarkdownToLockFile(testFile) - - // Read the generated lock file - lockContent, err := os.ReadFile(lockFile) - if err != nil { - t.Fatalf("Failed to read lock file: %v", err) - } - - lockContentStr := string(lockContent) - - // Verify activation job exists - if !strings.Contains(lockContentStr, "activation:") { - t.Error("Expected activation job to be present") - } - - // Extract the activation job section - activationJobStart := strings.Index(lockContentStr, "activation:") - if activationJobStart == -1 { - t.Fatal("Activation job not found in compiled workflow") - } - - // Find the next job or end of file - activationJobEnd := len(lockContentStr) - nextJobIdx := strings.Index(lockContentStr[activationJobStart+11:], "\n ") - if nextJobIdx != -1 { - searchStart := activationJobStart + 11 + nextJobIdx - for idx := searchStart; idx < len(lockContentStr); idx++ { - if lockContentStr[idx] == '\n' { - lineStart := idx + 1 - if lineStart < len(lockContentStr) && lineStart+2 < len(lockContentStr) { - if lockContentStr[lineStart:lineStart+2] == " " && lockContentStr[lineStart+2] != ' ' { - colonIdx := strings.Index(lockContentStr[lineStart:], ":") - if colonIdx > 0 && colonIdx < 50 { - activationJobEnd = idx - break - } - } - } - } - } - } - - activationJobSection := lockContentStr[activationJobStart:activationJobEnd] - - // In dev mode, checkout may be present for setup action, but should be minimal - // In release mode (which we no longer test here), there would be no checkout - // The key is that we're NOT checking out the full .github/workflows directory - // for timestamp checking - that uses GitHub API instead - - // Verify it does NOT checkout .github/workflows for timestamp checking - if strings.Contains(activationJobSection, "Checkout workflows") { - t.Errorf("%s: Should not have 'Checkout workflows' step - uses GitHub API for timestamp checking", tt.description) - } - - // Verify timestamp check step is present - if !strings.Contains(activationJobSection, "Check workflow lock file") { - t.Errorf("%s: Should contain timestamp check step", tt.description) - } - - // Verify scripts are loaded via require() (not inlined) - if !strings.Contains(activationJobSection, "require(") { - t.Errorf("%s: Should load scripts via require()", tt.description) - } - }) - } -} diff --git a/pkg/workflow/compiler_activation_checkout_test.go b/pkg/workflow/compiler_activation_checkout_test.go new file mode 100644 index 00000000000..4e293a003bd --- /dev/null +++ b/pkg/workflow/compiler_activation_checkout_test.go @@ -0,0 +1,141 @@ +//go:build integration + +package workflow + +import ( + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/github/gh-aw/pkg/stringutil" + + "github.com/github/gh-aw/pkg/testutil" +) + +// TestActivationJobNoCheckoutStep tests that the activation job uses GitHub API +// instead of checking out the repository for the timestamp check +func TestActivationJobNoCheckoutStep(t *testing.T) { + tests := []struct { + name string + frontmatter string + }{ + { + name: "basic workflow has no checkout in activation", + frontmatter: `--- +on: + issues: + types: [opened] +permissions: + contents: read + issues: read +engine: claude +strict: false +---`, + }, + { + name: "workflow without contents permission has no checkout in activation", + frontmatter: `--- +on: + issues: + types: [opened] +permissions: + issues: read +engine: claude +strict: false +---`, + }, + { + name: "workflow with reaction has no checkout in activation", + frontmatter: `--- +on: + issues: + types: [opened] + reaction: eyes +permissions: + issues: read +engine: claude +strict: false +---`, + }, + { + // Top-level workflow permissions cannot grant write scopes directly (enforced by + // validateDangerousPermissions), but safe-outputs such as create-pull-request require + // contents: write in their own downstream job. This case verifies that even when the + // workflow needs write-capable safe-outputs, the activation job itself still only + // performs the sparse .github checkout - it never checks out the full repository. + name: "workflow with write-capable safe-outputs still has no full checkout in activation", + frontmatter: `--- +on: + issues: + types: [opened] +permissions: + contents: read + issues: read +engine: claude +strict: false +safe-outputs: + create-pull-request: +---`, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + tmpDir := testutil.TempDir(t, "activation-checkout-test") + + testContent := tt.frontmatter + "\n\n# Test Workflow\n\nTest workflow content." + testFile := filepath.Join(tmpDir, "test-workflow.md") + require.NoError(t, os.WriteFile(testFile, []byte(testContent), 0644)) + + compiler := NewCompiler(WithVersion("dev")) + // Use dev mode to use local action paths + compiler.SetActionMode(ActionModeDev) + + // Compile the workflow + require.NoError(t, compiler.CompileWorkflow(testFile), "Failed to compile workflow") + + // Calculate the lock file path + lockFile := stringutil.MarkdownToLockFile(testFile) + + // Read the generated lock file + lockContent, err := os.ReadFile(lockFile) + require.NoError(t, err, "Failed to read lock file") + + lockContentStr := string(lockContent) + + // Verify activation job exists + require.Contains(t, lockContentStr, "activation:", "Expected activation job to be present") + + // Extract the activation job section using the shared job-boundary helper + activationJobSection := extractJobSection(lockContentStr, "activation") + require.NotEmpty(t, activationJobSection, "Activation job section should not be empty") + + // The activation job always sparse-checks-out .github/.agents (and, in dev mode, + // actions/setup) so it can load helper scripts and engine config - this is + // unaffected by the workflow's permissions or triggers (including contents: write, + // via write-capable safe-outputs). What must never happen is a full checkout of + // the repository, or a checkout of .github/workflows for timestamp checking - + // that always uses the GitHub API instead. + + // Verify the activation checkout is the sparse .github/.agents checkout + assert.Contains(t, activationJobSection, "name: Checkout .github and .agents folders", "Should use the sparse .github/.agents checkout step") + assert.Contains(t, activationJobSection, "sparse-checkout: |", "Sparse checkout should be configured") + assert.Contains(t, activationJobSection, "sparse-checkout-cone-mode: true", "Sparse checkout cone mode should be enabled") + + // Verify it does NOT perform a full repository checkout + assert.NotContains(t, activationJobSection, "name: Checkout repository", "Should not have a full repository checkout step") + + // Verify it does NOT checkout .github/workflows for timestamp checking + assert.NotContains(t, activationJobSection, "Checkout workflows", "Should not have 'Checkout workflows' step - uses GitHub API for timestamp checking") + + // Verify timestamp check step is present + assert.Contains(t, activationJobSection, "Check workflow lock file", "Should contain timestamp check step") + + // Verify scripts are loaded via require() (not inlined) + assert.Contains(t, activationJobSection, "require(", "Should load scripts via require()") + }) + } +} diff --git a/pkg/workflow/compiler_activation_job_test.go b/pkg/workflow/compiler_activation_job_test.go index 8d7a051c59f..d8dc7aafd15 100644 --- a/pkg/workflow/compiler_activation_job_test.go +++ b/pkg/workflow/compiler_activation_job_test.go @@ -1273,3 +1273,70 @@ func TestResolveSymlinkExtraPaths(t *testing.T) { assert.Equal(t, 1, count, "already-present path should not be duplicated") }) } + +func TestActivationEventSet(t *testing.T) { + t.Run("string on value", func(t *testing.T) { + events, ok := activationEventSet("on: issues") + require.True(t, ok) + assert.Equal(t, map[string]struct{}{"issues": {}}, events) + }) + + t.Run("list on value", func(t *testing.T) { + events, ok := activationEventSet("on: [issues, pull_request]") + require.True(t, ok) + assert.Equal(t, map[string]struct{}{"issues": {}, "pull_request": {}}, events) + }) + + t.Run("map on value excludes metadata trigger fields", func(t *testing.T) { + onSection := "on:\n issues:\n types: [opened]\n reaction: eyes\n stop-after: +48h\n" + events, ok := activationEventSet(onSection) + require.True(t, ok) + assert.Equal(t, map[string]struct{}{"issues": {}}, events, "metadata fields like reaction/stop-after should be excluded") + }) + + t.Run("invalid yaml returns not ok", func(t *testing.T) { + events, ok := activationEventSet("on: [unterminated") + assert.False(t, ok) + assert.Empty(t, events) + }) + + t.Run("missing on key returns not ok", func(t *testing.T) { + events, ok := activationEventSet("permissions:\n contents: read\n") + assert.False(t, ok) + assert.Empty(t, events) + }) + + t.Run("unsupported on value type returns not ok", func(t *testing.T) { + events, ok := activationEventSet("on: 5\n") + assert.False(t, ok) + assert.Empty(t, events) + }) +} + +func TestBuildCentralizedCommandOnSection(t *testing.T) { + t.Run("single event produces synthetic on section", func(t *testing.T) { + result := buildCentralizedCommandOnSection([]string{"issues"}) + assert.Equal(t, "on:\n issues:\n types: [created]\n", result) + }) + + t.Run("pull_request_comment and issue_comment dedupe to issue_comment", func(t *testing.T) { + result := buildCentralizedCommandOnSection([]string{"pull_request_comment", "issue_comment"}) + assert.Equal(t, "on:\n issue_comment:\n types: [created]\n", result) + }) + + t.Run("unknown identifiers produce empty on section", func(t *testing.T) { + result := buildCentralizedCommandOnSection([]string{"not-a-real-event"}) + assert.Empty(t, result) + }) + + t.Run("empty input defaults to all comment events", func(t *testing.T) { + expected := "on:\n" + + " issues:\n types: [created]\n" + + " issue_comment:\n types: [created]\n" + + " pull_request:\n types: [created]\n" + + " pull_request_review_comment:\n types: [created]\n" + + " discussion:\n types: [created]\n" + + " discussion_comment:\n types: [created]\n" + assert.Equal(t, expected, buildCentralizedCommandOnSection(nil)) + }) +} diff --git a/pkg/workflow/compiler_test_helpers_test.go b/pkg/workflow/compiler_test_helpers_test.go index b71707aa5f5..b92fe7ea7c1 100644 --- a/pkg/workflow/compiler_test_helpers_test.go +++ b/pkg/workflow/compiler_test_helpers_test.go @@ -1,6 +1,11 @@ package workflow -import "strings" +import ( + "strings" + "testing" + + "github.com/stretchr/testify/assert" +) func containsInNonCommentLines(content, search string) bool { lines := strings.SplitSeq(content, "\n") @@ -67,3 +72,33 @@ func extractJobSection(yamlContent, jobName string) string { return strings.Join(jobLines, "\n") } + +// TestExtractJobSection exercises the job-boundary parsing helper used throughout the +// test suite to isolate a single job's YAML from a full compiled lock file. +func TestExtractJobSection(t *testing.T) { + t.Run("job at end of file", func(t *testing.T) { + yamlContent := "jobs:\n build:\n runs-on: ubuntu-latest\n steps:\n - run: echo hi\n" + got := extractJobSection(yamlContent, "build") + assert.Equal(t, " build:\n runs-on: ubuntu-latest\n steps:\n - run: echo hi\n", got) + }) + + t.Run("job followed by another job", func(t *testing.T) { + yamlContent := "jobs:\n activation:\n runs-on: ubuntu-latest\n steps:\n - run: echo activation\n agent:\n runs-on: ubuntu-latest\n" + got := extractJobSection(yamlContent, "activation") + assert.Equal(t, " activation:\n runs-on: ubuntu-latest\n steps:\n - run: echo activation", got) + assert.NotContains(t, got, "agent:") + }) + + t.Run("job with nested multi-level indentation", func(t *testing.T) { + yamlContent := "jobs:\n activation:\n steps:\n - name: Check\n with:\n nested:\n deeply: true\n agent:\n runs-on: ubuntu-latest\n" + got := extractJobSection(yamlContent, "activation") + assert.Contains(t, got, "deeply: true") + assert.NotContains(t, got, "agent:") + }) + + t.Run("job not present returns empty string", func(t *testing.T) { + yamlContent := "jobs:\n build:\n runs-on: ubuntu-latest\n" + got := extractJobSection(yamlContent, "missing") + assert.Empty(t, got) + }) +}