diff --git a/.github/workflows/metrics-collector.lock.yml b/.github/workflows/metrics-collector.lock.yml index 73a229a1f72..f1713c33de1 100644 --- a/.github/workflows/metrics-collector.lock.yml +++ b/.github/workflows/metrics-collector.lock.yml @@ -1196,6 +1196,7 @@ jobs: env: GH_AW_SETUP_WORKFLOW_NAME: "Metrics Collector - Infrastructure Agent" GH_AW_CURRENT_WORKFLOW_REF: ${{ github.repository }}/.github/workflows/metrics-collector.lock.yml@${{ github.ref }} + GH_AW_INFO_VERSION: "1.0.40" - name: Download agent output artifact id: download-agent-output continue-on-error: true diff --git a/pkg/cli/compile_engine_env_needs_integration_test.go b/pkg/cli/compile_engine_env_needs_integration_test.go new file mode 100644 index 00000000000..f0c4a41eea6 --- /dev/null +++ b/pkg/cli/compile_engine_env_needs_integration_test.go @@ -0,0 +1,60 @@ +//go:build integration + +package cli + +import ( + "os" + "os/exec" + "path/filepath" + "testing" + + goyaml "github.com/goccy/go-yaml" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestCompileEngineEnvNeedsExpression verifies that engine.env values containing +// needs..outputs.* expressions cause the referenced custom job to be added +// as a direct dependency of the agent job. +func TestCompileEngineEnvNeedsExpression(t *testing.T) { + setup := setupIntegrationTest(t) + defer setup.cleanup() + + srcPath := filepath.Join(projectRoot, "pkg/cli/workflows/test-engine-env-needs.md") + dstPath := filepath.Join(setup.workflowsDir, "test-engine-env-needs.md") + + srcContent, err := os.ReadFile(srcPath) + require.NoError(t, err, "Failed to read source workflow fixture") + require.NoError(t, os.WriteFile(dstPath, srcContent, 0644), "Failed to write workflow fixture") + + cmd := exec.Command(setup.binaryPath, "compile", dstPath) + output, err := cmd.CombinedOutput() + require.NoError(t, err, "Compile failed:\n%s", string(output)) + + lockFilePath := filepath.Join(setup.workflowsDir, "test-engine-env-needs.lock.yml") + lockContent, err := os.ReadFile(lockFilePath) + require.NoError(t, err, "Failed to read lock file") + + var workflow map[string]any + require.NoError(t, goyaml.Unmarshal(lockContent, &workflow), "Lock file should be valid YAML") + + jobs, ok := workflow["jobs"].(map[string]any) + require.True(t, ok, "Compiled workflow should include jobs map") + + agentJob, ok := jobs["agent"].(map[string]any) + require.True(t, ok, "Compiled workflow should include agent job") + + needsRaw, ok := agentJob["needs"].([]any) + require.True(t, ok, "agent job should have a needs list") + + needs := make([]string, 0, len(needsRaw)) + for _, need := range needsRaw { + require.IsType(t, "", need, "agent needs entries should be strings") + needs = append(needs, need.(string)) + } + + assert.Contains(t, needs, "provide_value_to_agent", + "agent job must depend on provide_value_to_agent referenced in engine.env") + assert.Contains(t, needs, "activation", + "agent job must still depend on activation") +} diff --git a/pkg/cli/workflows/test-engine-env-needs.md b/pkg/cli/workflows/test-engine-env-needs.md new file mode 100644 index 00000000000..80153845564 --- /dev/null +++ b/pkg/cli/workflows/test-engine-env-needs.md @@ -0,0 +1,30 @@ +--- +name: Test Engine Env Needs Expression +on: + workflow_dispatch: +permissions: + contents: read +engine: + id: copilot + env: + RECEIVED_VALUE: ${{ needs.provide_value_to_agent.outputs.provided_value }} +strict: false +jobs: + provide_value_to_agent: + runs-on: ubuntu-latest + outputs: + provided_value: ${{ steps.provide.outputs.provided_value }} + steps: + - id: provide + run: echo "provided_value=hello" >> "$GITHUB_OUTPUT" +--- + +# Test Engine Env Needs Expression + +This workflow tests that `engine.env` values containing `needs..outputs.*` expressions +cause the referenced custom job to be added as a direct dependency of the agent job. + +The `provide_value_to_agent` job must appear in the agent job's `needs` list so that +`RECEIVED_VALUE` evaluates correctly at runtime. + +Please echo the value of the `RECEIVED_VALUE` environment variable. diff --git a/pkg/workflow/compiler_jobs_test.go b/pkg/workflow/compiler_jobs_test.go index b2fb361cd85..eef7700ebaf 100644 --- a/pkg/workflow/compiler_jobs_test.go +++ b/pkg/workflow/compiler_jobs_test.go @@ -3529,3 +3529,170 @@ Test content` require.NotEmpty(t, conclusionSection, "conclusion job should be present") assert.Contains(t, conclusionSection, "push_experiments_state", "conclusion job should depend on push_experiments_state") } + +// TestBuildMainJobEngineEnvNeedsExpression verifies that when engine.env values contain +// needs..outputs.* expressions, the referenced custom job is added as a direct +// dependency of the agent job (issue: agent 'needs' does not incorporate jobs in engine.env). +func TestBuildMainJobEngineEnvNeedsExpression(t *testing.T) { + compiler := NewCompiler() + compiler.stepOrderTracker = NewStepOrderTracker() + + workflowData := &WorkflowData{ + Name: "Test Workflow", + AI: "copilot", + RunsOn: "runs-on: ubuntu-latest", + Permissions: "permissions:\n contents: read", + EngineConfig: &EngineConfig{ + ID: "copilot", + Env: map[string]string{ + "RECEIVED_VALUE": "${{ needs.provide_value_to_agent.outputs.provided_value }}", + }, + }, + Jobs: map[string]any{ + "provide_value_to_agent": map[string]any{ + "runs-on": "ubuntu-latest", + "needs": "pre_activation", + "steps": []any{ + map[string]any{ + "run": `echo "provided_value=hello" >> "$GITHUB_OUTPUT"`, + }, + }, + }, + }, + } + + job, err := compiler.buildMainJob(workflowData, true) + require.NoError(t, err, "buildMainJob should succeed") + + // The agent job must directly depend on provide_value_to_agent because engine.env + // references its outputs; without this, needs.provide_value_to_agent would be undefined. + assert.Contains(t, job.Needs, "provide_value_to_agent", + "agent job must directly depend on provide_value_to_agent referenced in engine.env") + assert.Contains(t, job.Needs, string(constants.ActivationJobName), + "agent job must also depend on activation") +} + +// TestBuildMainJobEngineEnvNeedsNotDuplicated verifies that a job referenced in both +// engine.env and regular job dependencies is not duplicated in the agent's needs list. +func TestBuildMainJobEngineEnvNeedsNotDuplicated(t *testing.T) { + compiler := NewCompiler() + compiler.stepOrderTracker = NewStepOrderTracker() + + workflowData := &WorkflowData{ + Name: "Test Workflow", + AI: "copilot", + RunsOn: "runs-on: ubuntu-latest", + Permissions: "permissions:\n contents: read", + EngineConfig: &EngineConfig{ + ID: "copilot", + Env: map[string]string{ + "MY_VALUE": "${{ needs.custom_job.outputs.result }}", + }, + }, + Jobs: map[string]any{ + // custom_job has no explicit needs so it becomes a direct agent dependency + "custom_job": map[string]any{ + "runs-on": "ubuntu-latest", + "steps": []any{ + map[string]any{"run": "echo result=hello >> $GITHUB_OUTPUT"}, + }, + }, + }, + } + + job, err := compiler.buildMainJob(workflowData, true) + require.NoError(t, err, "buildMainJob should succeed") + + count := 0 + for _, need := range job.Needs { + if need == "custom_job" { + count++ + } + } + assert.Equal(t, 1, count, "custom_job should appear exactly once in agent needs") +} + +// TestBuildMainJobEngineEnvNeedsIntegration is an end-to-end integration test that compiles +// a workflow where engine.env references a custom job output, and verifies that the +// compiled lock file includes the custom job as a direct dependency of the agent job. +func TestBuildMainJobEngineEnvNeedsIntegration(t *testing.T) { + tmpDir := testutil.TempDir(t, "engine_env_needs_test") + + // This workflow matches the bug report: engine.env references provide_value_to_agent + // which in turn depends on pre_activation. Without the fix, the agent job would only + // have `needs: activation` and runtime evaluation of needs.provide_value_to_agent + // would silently return an empty string. + frontmatter := `--- +on: issues +permissions: + contents: read + issues: read +engine: + id: copilot + env: + RECEIVED_VALUE: ${{ needs.provide_value_to_agent.outputs.provided_value }} +strict: false +jobs: + provide_value_to_agent: + runs-on: ubuntu-latest + needs: pre_activation + outputs: + provided_value: ${{ steps.provide.outputs.provided_value }} + steps: + - id: provide + run: echo "provided_value=hello" >> "$GITHUB_OUTPUT" +--- + +# Test Workflow + +This workflow tests that engine.env needs expressions create agent job dependencies. +` + + testFile := filepath.Join(tmpDir, "engine-env-needs.md") + require.NoError(t, os.WriteFile(testFile, []byte(frontmatter), 0644), "write test file") + + compiler := NewCompiler() + require.NoError(t, compiler.CompileWorkflow(testFile), "compile workflow") + + lockFile := filepath.Join(tmpDir, "engine-env-needs.lock.yml") + content, err := os.ReadFile(lockFile) + require.NoError(t, err, "read lock file") + + yamlStr := string(content) + + // The agent job must directly depend on provide_value_to_agent + agentSection := extractJobSection(yamlStr, "agent") + require.NotEmpty(t, agentSection, "agent job section should be present in lock file") + + assert.Contains(t, agentSection, "provide_value_to_agent", + "agent job must list provide_value_to_agent in its needs (referenced via engine.env)") +} + +// TestBuildMainJobEngineEnvActivationNoFalseWarning verifies that referencing the activation +// built-in job in engine.env does NOT emit a warning, since activation is always a direct +// dependency of the agent job and the expression is valid. +func TestBuildMainJobEngineEnvActivationNoFalseWarning(t *testing.T) { + compiler := NewCompiler() + compiler.stepOrderTracker = NewStepOrderTracker() + + workflowData := &WorkflowData{ + Name: "Test Workflow", + AI: "copilot", + RunsOn: "runs-on: ubuntu-latest", + Permissions: "permissions:\n contents: read", + EngineConfig: &EngineConfig{ + ID: "copilot", + Env: map[string]string{ + // activation is a valid direct dependency — no warning should be emitted + "MODEL": "${{ needs.activation.outputs.model }}", + }, + }, + } + + initialWarnings := compiler.GetWarningCount() + _, err := compiler.buildMainJob(workflowData, true) + require.NoError(t, err, "buildMainJob should succeed") + + assert.Equal(t, initialWarnings, compiler.GetWarningCount(), + "no warning should be emitted for activation which is already a direct agent dependency") +} diff --git a/pkg/workflow/compiler_main_job.go b/pkg/workflow/compiler_main_job.go index 7e49e8f40aa..791534a5c15 100644 --- a/pkg/workflow/compiler_main_job.go +++ b/pkg/workflow/compiler_main_job.go @@ -3,10 +3,13 @@ package workflow import ( "fmt" "maps" + "os" "slices" + "sort" "strconv" "strings" + "github.com/github/gh-aw/pkg/console" "github.com/github/gh-aw/pkg/constants" "github.com/github/gh-aw/pkg/logger" ) @@ -118,12 +121,28 @@ func (c *Compiler) buildMainJob(data *WorkflowData, activationJobCreated bool) ( // (e.g., ${{ needs.search_issues.outputs.* }}), we MUST add them as direct dependencies. // This is required for GitHub Actions expression evaluation and actionlint validation. // Also check custom steps from the frontmatter, which are also added to the agent job. + // Also check engine.env values, which may contain needs..outputs.* expressions. var contentBuilder strings.Builder contentBuilder.WriteString(data.MarkdownContent) if data.CustomSteps != "" { contentBuilder.WriteByte('\n') contentBuilder.WriteString(data.CustomSteps) } + // Compute engine.env content once; reuse for both the dependency scan and the built-in + // job reference warning below. + var engineEnvContent string + if data.EngineConfig != nil && len(data.EngineConfig.Env) > 0 { + var engineEnvBuilder strings.Builder + for _, envValue := range data.EngineConfig.Env { + engineEnvBuilder.WriteByte('\n') + engineEnvBuilder.WriteString(envValue) + } + engineEnvContent = engineEnvBuilder.String() + // Include engine.env values so that needs..outputs.* expressions there are also + // scanned for custom job dependencies that must be added to the agent job's needs list. + contentBuilder.WriteString(engineEnvContent) + compilerMainJobLog.Printf("Including %d engine.env values in agent job dependency scan", len(data.EngineConfig.Env)) + } referencedJobs := c.getReferencedCustomJobs(contentBuilder.String(), data.Jobs) for _, jobName := range referencedJobs { // Skip built-in jobs as they are handled separately and should not become custom dependencies. @@ -136,7 +155,42 @@ func (c *Compiler) buildMainJob(data *WorkflowData, activationJobCreated bool) ( // Add it if not already present if !alreadyDepends { depends = append(depends, jobName) - compilerMainJobLog.Printf("Added direct dependency on custom job '%s' because it's referenced in workflow content", jobName) + compilerMainJobLog.Printf("Added direct dependency on custom job '%s' because it's referenced in workflow content or engine.env", jobName) + } + } + + // Warn when built-in job names appear in needs expressions inside engine.env values. + // engine.env values are emitted as step-level environment variables in the agent job; + // for a needs expression like ${{ needs.X.outputs.Y }} to evaluate correctly at runtime, + // X must be a direct dependency of the agent job. Built-in jobs (e.g., detection, + // safe_outputs) are managed by the compiler and cannot be added as direct dependencies, + // so referencing them here will silently produce empty strings at runtime. + // Exception: skip any built-in that is already in `depends` (e.g., `activation`), + // as those expressions are valid and will evaluate correctly. + if engineEnvContent != "" { + builtinNames := make([]string, 0, len(constants.KnownBuiltInJobNames)) + for name := range constants.KnownBuiltInJobNames { + builtinNames = append(builtinNames, name) + } + sort.Strings(builtinNames) + builtinsWarned := make(map[string]bool) + for _, builtinJobName := range builtinNames { + // Skip built-ins that are already direct dependencies (e.g., activation) — + // their outputs are accessible and the expression is valid. + if slices.Contains(depends, builtinJobName) { + continue + } + if !builtinsWarned[builtinJobName] && strings.Contains(engineEnvContent, fmt.Sprintf("needs.%s.", builtinJobName)) { + builtinsWarned[builtinJobName] = true + warningMsg := fmt.Sprintf( + "engine.env references built-in job '%s' in a needs expression. "+ + "Built-in jobs are managed by the compiler and cannot be added as direct agent dependencies; "+ + "this expression will silently evaluate to an empty string at runtime.", + builtinJobName, + ) + fmt.Fprintln(os.Stderr, console.FormatWarningMessage(warningMsg)) + c.IncrementWarningCount() + } } }