From 1d4760e956af7f814a5b33f550630cb3ddee72d5 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sun, 5 Jul 2026 15:13:57 +0000 Subject: [PATCH 1/3] Initial plan From 3935f27e37c71f90ab2f50ddaed27e25f104caeb Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sun, 5 Jul 2026 15:52:11 +0000 Subject: [PATCH 2/3] fix: auto-add pre_activation to safe_outputs/conclusion job needs when messages reference it Fixes actionlint errors in skillet.lock.yml at lines 1385, 1411, 1882. The safe_outputs and conclusion jobs referenced needs.pre_activation.outputs.* in GH_AW_SAFE_OUTPUT_MESSAGES but did not declare pre_activation in their needs. - Add messagesContainPreActivationRef() helper in compiler_safe_outputs_job.go - Auto-add pre_activation to safe_outputs job needs when messages reference it - Auto-add pre_activation to conclusion job needs when messages reference it - Recompile all workflows (skillet.lock.yml now correct) Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- .github/workflows/skillet.lock.yml | 2 + pkg/workflow/compiler_safe_outputs_job.go | 48 +++++++++++++++++++++++ pkg/workflow/notify_comment.go | 12 ++++++ 3 files changed, 62 insertions(+) diff --git a/.github/workflows/skillet.lock.yml b/.github/workflows/skillet.lock.yml index d3e18d488ec..a20f4aa499b 100644 --- a/.github/workflows/skillet.lock.yml +++ b/.github/workflows/skillet.lock.yml @@ -1135,6 +1135,7 @@ jobs: - activation - agent - detection + - pre_activation - safe_outputs if: > always() && (needs.agent.result != 'skipped' || needs.activation.outputs.lockdown_check_failed == 'true' || @@ -1858,6 +1859,7 @@ jobs: - activation - agent - detection + - pre_activation if: (!cancelled()) && needs.agent.result != 'skipped' && needs.detection.result == 'success' runs-on: ubuntu-slim permissions: diff --git a/pkg/workflow/compiler_safe_outputs_job.go b/pkg/workflow/compiler_safe_outputs_job.go index 317d9ea26e9..8fd947a4602 100644 --- a/pkg/workflow/compiler_safe_outputs_job.go +++ b/pkg/workflow/compiler_safe_outputs_job.go @@ -19,6 +19,41 @@ var consolidatedSafeOutputsJobLog = logger.New("workflow:compiler_safe_outputs_j // step starts in job.Steps (6-space indent + "- name: "). const stepNameLinePrefix = " - name: " +// messagesContainPreActivationRef reports whether any message template in cfg +// contains a reference to a needs.pre_activation.outputs.* expression. +// When true, the safe_outputs and conclusion jobs must declare pre_activation +// in their needs so that GitHub Actions can resolve the expression at runtime. +func messagesContainPreActivationRef(cfg *SafeOutputMessagesConfig) bool { + if cfg == nil { + return false + } + preActivationOutputsRef := "needs." + string(constants.PreActivationJobName) + ".outputs." + for _, field := range []string{ + cfg.Footer, + cfg.FooterInstall, + cfg.FooterWorkflowRecompile, + cfg.FooterWorkflowRecompileComment, + cfg.StagedTitle, + cfg.StagedDescription, + cfg.ActivationComments, + cfg.RunStarted, + cfg.RunSuccess, + cfg.RunFailure, + cfg.DetectionFailure, + cfg.PullRequestCreated, + cfg.IssueCreated, + cfg.CommitPushed, + cfg.AgentFailureIssue, + cfg.AgentFailureComment, + cfg.BodyHeader, + } { + if strings.Contains(field, preActivationOutputsRef) { + return true + } + } + return false +} + // buildConsolidatedSafeOutputsJob builds a single job containing all safe output operations // as separate steps within that job. This reduces the number of jobs in the workflow // while maintaining observability through distinct step names, IDs, and outputs. @@ -599,6 +634,19 @@ func (c *Compiler) buildSafeOutputsJobFromParts( } } + // If any message template references needs.pre_activation.outputs.*, add pre_activation + // as a dependency so that GitHub Actions can resolve the expression at runtime. + if data.SafeOutputs != nil && messagesContainPreActivationRef(data.SafeOutputs.Messages) { + if _, exists := c.jobManager.GetJob(string(constants.PreActivationJobName)); exists { + preActName := string(constants.PreActivationJobName) + if !setutil.Contains(seenNeeds, preActName) { + needs = append(needs, preActName) + seenNeeds[preActName] = struct{}{} + consolidatedSafeOutputsJobLog.Print("Added pre_activation dependency to safe_outputs job (messages reference pre_activation outputs)") + } + } + } + // Extract workflow ID from markdown path for GH_AW_WORKFLOW_ID workflowID := GetWorkflowIDFromPath(markdownPath) diff --git a/pkg/workflow/notify_comment.go b/pkg/workflow/notify_comment.go index cdcd8112359..2e18fd613a8 100644 --- a/pkg/workflow/notify_comment.go +++ b/pkg/workflow/notify_comment.go @@ -2,6 +2,7 @@ package workflow import ( "fmt" + "slices" "strings" "github.com/github/gh-aw/pkg/constants" @@ -61,6 +62,17 @@ func (c *Compiler) buildConclusionJob(data *WorkflowData, mainJobName string, sa steps = append(steps, c.generateScriptModeCleanupStep()) } needs := buildConclusionJobNeeds(data, mainJobName, safeOutputJobNames) + // If any message template references needs.pre_activation.outputs.*, add pre_activation + // as a dependency so that GitHub Actions can resolve the expression at runtime. + if data.SafeOutputs != nil && messagesContainPreActivationRef(data.SafeOutputs.Messages) { + if _, exists := c.jobManager.GetJob(string(constants.PreActivationJobName)); exists { + preActName := string(constants.PreActivationJobName) + if !slices.Contains(needs, preActName) { + needs = append(needs, preActName) + notifyCommentLog.Print("Added pre_activation dependency to conclusion job (messages reference pre_activation outputs)") + } + } + } notifyCommentLog.Printf("Job built successfully: dependencies_count=%d", len(needs)) return &Job{ Name: "conclusion", From 18144ee9d2a454f07084f4f49a4dc569f5b837f8 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sun, 5 Jul 2026 16:49:46 +0000 Subject: [PATCH 3/3] test: add coverage for pre_activation needs wiring via message templates Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com> --- .../compiler_safe_outputs_job_test.go | 90 +++++++++++++++ pkg/workflow/notify_comment_test.go | 103 ++++++++++++++++++ 2 files changed, 193 insertions(+) diff --git a/pkg/workflow/compiler_safe_outputs_job_test.go b/pkg/workflow/compiler_safe_outputs_job_test.go index d7768088c1b..62902461135 100644 --- a/pkg/workflow/compiler_safe_outputs_job_test.go +++ b/pkg/workflow/compiler_safe_outputs_job_test.go @@ -266,6 +266,96 @@ func TestBuildConsolidatedSafeOutputsJobNeedsIncludesConfiguredDependencies(t *t assert.Equal(t, 1, secretsFetcherCount, "duplicate configured dependencies should be deduplicated") } +// TestBuildConsolidatedSafeOutputsJobNeedsPreActivationFromMessages tests that +// pre_activation is automatically added to the safe_outputs job's needs when any +// message template references needs.pre_activation.outputs.*. +func TestBuildConsolidatedSafeOutputsJobNeedsPreActivationFromMessages(t *testing.T) { + tests := []struct { + name string + messages *SafeOutputMessagesConfig + registerPreAct bool + expectPreActNeeds bool + }{ + { + name: "adds pre_activation when Footer references its outputs", + messages: &SafeOutputMessagesConfig{ + Footer: "Skill: ${{ needs.pre_activation.outputs.skill_name }}", + }, + registerPreAct: true, + expectPreActNeeds: true, + }, + { + name: "adds pre_activation when RunSuccess references its outputs", + messages: &SafeOutputMessagesConfig{ + RunSuccess: "Done — skill: ${{ needs.pre_activation.outputs.skill_name }}", + }, + registerPreAct: true, + expectPreActNeeds: true, + }, + { + name: "does not add pre_activation when messages have no reference", + messages: &SafeOutputMessagesConfig{ + Footer: "No expression here", + }, + registerPreAct: true, + expectPreActNeeds: false, + }, + { + name: "does not add pre_activation when job is not registered", + messages: &SafeOutputMessagesConfig{ + Footer: "Skill: ${{ needs.pre_activation.outputs.skill_name }}", + }, + registerPreAct: false, + expectPreActNeeds: false, + }, + { + name: "does not add pre_activation when messages is nil", + messages: nil, + registerPreAct: true, + expectPreActNeeds: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + compiler := NewCompiler() + compiler.jobManager = NewJobManager() + + if tt.registerPreAct { + preActJob := &Job{Name: string(constants.PreActivationJobName)} + require.NoError(t, compiler.jobManager.AddJob(preActJob)) + } + + workflowData := &WorkflowData{ + Name: "Test Workflow", + SafeOutputs: &SafeOutputsConfig{ + CreateIssues: &CreateIssuesConfig{TitlePrefix: "[Test] "}, + Messages: tt.messages, + }, + } + + job, _, err := compiler.buildConsolidatedSafeOutputsJob(workflowData, string(constants.AgentJobName), "test.md") + require.NoError(t, err) + require.NotNil(t, job) + + preActName := string(constants.PreActivationJobName) + if tt.expectPreActNeeds { + assert.Contains(t, job.Needs, preActName, "safe_outputs job should depend on pre_activation when messages reference its outputs") + // Ensure no duplicate + count := 0 + for _, n := range job.Needs { + if n == preActName { + count++ + } + } + assert.Equal(t, 1, count, "pre_activation should appear exactly once in needs") + } else { + assert.NotContains(t, job.Needs, preActName, "safe_outputs job should not depend on pre_activation when messages don't reference its outputs") + } + }) + } +} + // TestBuildConsolidatedSafeOutputsJobTimeoutMinutes tests that the timeout-minutes field // is correctly applied to the safe_outputs job, with 45 minutes as the default. func TestBuildConsolidatedSafeOutputsJobTimeoutMinutes(t *testing.T) { diff --git a/pkg/workflow/notify_comment_test.go b/pkg/workflow/notify_comment_test.go index 6e80571e390..a63a123bda5 100644 --- a/pkg/workflow/notify_comment_test.go +++ b/pkg/workflow/notify_comment_test.go @@ -1366,3 +1366,106 @@ func TestConclusionJobIncludesUsageArtifactSteps(t *testing.T) { t.Errorf("Expected 'Download safe outputs items manifest' to appear before 'Collect usage artifact files'.\ndownloadIdx=%d collectIdx=%d\nGenerated steps:\n%s", downloadIdx, collectIdx, allSteps) } } + +// TestConclusionJobNeedsPreActivationFromMessages tests that pre_activation is automatically +// added to the conclusion job's needs when any message template references +// needs.pre_activation.outputs.*, and that it is not duplicated. +func TestConclusionJobNeedsPreActivationFromMessages(t *testing.T) { + tests := []struct { + name string + messages *SafeOutputMessagesConfig + registerPreAct bool + expectPreActNeeds bool + }{ + { + name: "adds pre_activation when Footer references its outputs", + messages: &SafeOutputMessagesConfig{ + Footer: "Skill: ${{ needs.pre_activation.outputs.skill_name }}", + }, + registerPreAct: true, + expectPreActNeeds: true, + }, + { + name: "adds pre_activation when RunFailure references its outputs", + messages: &SafeOutputMessagesConfig{ + RunFailure: "Failed in ${{ needs.pre_activation.outputs.skill_name }}", + }, + registerPreAct: true, + expectPreActNeeds: true, + }, + { + name: "does not add pre_activation when messages have no reference", + messages: &SafeOutputMessagesConfig{ + Footer: "No expression here", + }, + registerPreAct: true, + expectPreActNeeds: false, + }, + { + name: "does not add pre_activation when job is not registered", + messages: &SafeOutputMessagesConfig{ + Footer: "Skill: ${{ needs.pre_activation.outputs.skill_name }}", + }, + registerPreAct: false, + expectPreActNeeds: false, + }, + { + name: "does not add pre_activation when messages is nil", + messages: nil, + registerPreAct: true, + expectPreActNeeds: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + compiler := NewCompiler() + + if tt.registerPreAct { + preActJob := &Job{Name: string(constants.PreActivationJobName)} + if err := compiler.jobManager.AddJob(preActJob); err != nil { + t.Fatalf("Failed to register pre_activation job: %v", err) + } + } + + workflowData := &WorkflowData{ + Name: "Test Workflow", + SafeOutputs: &SafeOutputsConfig{ + AddComments: &AddCommentsConfig{ + BaseSafeOutputConfig: BaseSafeOutputConfig{Max: strPtr("1")}, + }, + Messages: tt.messages, + }, + } + + job, err := compiler.buildConclusionJob(workflowData, string(constants.AgentJobName), []string{}) + if err != nil { + t.Fatalf("buildConclusionJob returned error: %v", err) + } + if job == nil { + t.Fatal("Expected conclusion job to be non-nil") + } + + preActName := string(constants.PreActivationJobName) + if tt.expectPreActNeeds { + if !slices.Contains(job.Needs, preActName) { + t.Errorf("Expected pre_activation in conclusion job needs, got: %v", job.Needs) + } + // Ensure no duplicate + count := 0 + for _, n := range job.Needs { + if n == preActName { + count++ + } + } + if count != 1 { + t.Errorf("pre_activation appears %d times in conclusion job needs (expected 1): %v", count, job.Needs) + } + } else { + if slices.Contains(job.Needs, preActName) { + t.Errorf("Did not expect pre_activation in conclusion job needs, got: %v", job.Needs) + } + } + }) + } +}