Skip to content
Merged
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
2 changes: 2 additions & 0 deletions .github/workflows/skillet.lock.yml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

48 changes: 48 additions & 0 deletions pkg/workflow/compiler_safe_outputs_job.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Comment thread
pelikhan marked this conversation as resolved.
Comment thread
pelikhan marked this conversation as resolved.
if cfg == nil {
return false
}
preActivationOutputsRef := "needs." + string(constants.PreActivationJobName) + ".outputs."
for _, field := range []string{
Comment thread
pelikhan marked this conversation as resolved.
Comment thread
pelikhan marked this conversation as resolved.
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,
Comment thread
pelikhan marked this conversation as resolved.
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.
Expand Down Expand Up @@ -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 {
Comment thread
pelikhan marked this conversation as resolved.
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)")
Comment thread
pelikhan marked this conversation as resolved.
}
}
}

// Extract workflow ID from markdown path for GH_AW_WORKFLOW_ID
workflowID := GetWorkflowIDFromPath(markdownPath)

Expand Down
90 changes: 90 additions & 0 deletions pkg/workflow/compiler_safe_outputs_job_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
12 changes: 12 additions & 0 deletions pkg/workflow/notify_comment.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package workflow

import (
"fmt"
"slices"
"strings"

"github.com/github/gh-aw/pkg/constants"
Expand Down Expand Up @@ -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) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/codebase-design] The guard block here is structurally identical to the one in compiler_safe_outputs_job.go (lines 639–647). The same pattern — check nil, check jobManager, dedup-append — is now duplicated across two files with slightly different dedup primitives (setutil.Contains vs slices.Contains). Consider extracting a shared helper or at minimum aligning the two call sites on the same dedup primitive to reduce cognitive load.

💡 Suggested refactor direction

A helper like:

// appendPreActivationNeedIfRequired appends pre_activation to needs if:
//   1. any message template references needs.pre_activation.outputs.*
//   2. the pre_activation job actually exists in the job manager
// It is a no-op if pre_activation is already present.
func (c *Compiler) appendPreActivationNeedIfRequired(
    data *WorkflowData, needs []string,
) []string {
    if data.SafeOutputs == nil || !messagesContainPreActivationRef(data.SafeOutputs.Messages) {
        return needs
    }
    name := string(constants.PreActivationJobName)
    if _, exists := c.jobManager.GetJob(name); !exists {
        return needs
    }
    if slices.Contains(needs, name) {
        return needs
    }
    return append(needs, name)
}

Both call sites collapse to a single line and the dedup primitive is consistent.

@copilot please address this.

if _, exists := c.jobManager.GetJob(string(constants.PreActivationJobName)); exists {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same silent failure in conclusion job path: when GetJob(PreActivationJobName) returns false, the conclusion job is emitted with unresolvable needs.pre_activation.outputs.* template expressions and no error. This is the same bug as the safe_outputs path above.

💡 Suggested fix

Mirror the fix from the safe_outputs path — return an error instead of a no-op:

if data.SafeOutputs != nil && messagesContainPreActivationRef(data.SafeOutputs.Messages) {
	if _, exists := c.jobManager.GetJob(string(constants.PreActivationJobName)); !exists {
		return nil, fmt.Errorf(
			"safe-outputs messages reference %s outputs but no %s job exists",
			constants.PreActivationJobName, constants.PreActivationJobName,
		)
	}
	preActName := string(constants.PreActivationJobName)
	if !slices.Contains(needs, preActName) {
		needs = append(needs, preActName)
	}
}

Note: the conclusion job already returns (*Job, error), so the error return is already wired up.

preActName := string(constants.PreActivationJobName)
if !slices.Contains(needs, preActName) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inconsistent deduplication strategy: the safe_outputs path uses an O(1) seenNeeds map[string]struct{}, while this conclusion-job path uses slices.Contains (O(n) linear scan). Both are correct for current list sizes, but the inconsistency is a maintenance hazard — contributors will copy one pattern without knowing there are two, and future code audits will waste time questioning whether the difference is intentional.

💡 Suggested fix

Either:

  1. Extract a shared addUniqueNeed(needs *[]string, seen map[string]struct{}, name string) helper that both call-sites use, or
  2. Build a local seenNeeds map in buildConclusionJob using the same approach as buildSafeOutputsJobFromParts.

The map-based approach is also naturally extensible if the needs list ever grows large (e.g. multi-job workflows with many safe-output dependencies).

needs = append(needs, preActName)
notifyCommentLog.Print("Added pre_activation dependency to conclusion job (messages reference pre_activation outputs)")
}
}
}
Comment thread
pelikhan marked this conversation as resolved.
notifyCommentLog.Printf("Job built successfully: dependencies_count=%d", len(needs))
return &Job{
Name: "conclusion",
Expand Down
103 changes: 103 additions & 0 deletions pkg/workflow/notify_comment_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
}
})
}
}
Loading