Skip to content
Merged
6 changes: 4 additions & 2 deletions .github/workflows/architecture-guardian.lock.yml

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

5 changes: 3 additions & 2 deletions .github/workflows/auto-triage-issues.lock.yml

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

6 changes: 4 additions & 2 deletions .github/workflows/blog-auditor.lock.yml

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

5 changes: 3 additions & 2 deletions .github/workflows/breaking-change-checker.lock.yml

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

6 changes: 4 additions & 2 deletions .github/workflows/ci-coach.lock.yml

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

5 changes: 3 additions & 2 deletions .github/workflows/contribution-check.lock.yml

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

6 changes: 4 additions & 2 deletions .github/workflows/daily-doc-healer.lock.yml

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

6 changes: 4 additions & 2 deletions .github/workflows/daily-issues-report.lock.yml

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

6 changes: 4 additions & 2 deletions .github/workflows/daily-news.lock.yml

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

6 changes: 4 additions & 2 deletions .github/workflows/draft-pr-cleanup.lock.yml

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

6 changes: 4 additions & 2 deletions .github/workflows/plan.lock.yml

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

5 changes: 3 additions & 2 deletions .github/workflows/smoke-copilot.lock.yml

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

6 changes: 4 additions & 2 deletions .github/workflows/stale-pr-cleanup.lock.yml

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

6 changes: 4 additions & 2 deletions .github/workflows/weekly-blog-post-writer.lock.yml

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

25 changes: 9 additions & 16 deletions pkg/workflow/evals_job.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,8 @@ func evalsBranchName(workflowID string) string {
return WorkflowStateBranchName(constants.EvalsBranchPrefix, workflowID)
}

// buildEvalsJob creates a separate evals job that runs after the safe_outputs job
// (or directly after the agent job if safe_outputs is not configured).
// buildEvalsJob creates a separate evals job that runs after the agent job (and detection
// job when enabled), allowing it to run in parallel with safe_outputs.
// The job downloads the agent artifact to access output files, runs a BinEval
// multi-question evaluation via an agentic engine, and uploads evals.jsonl as an artifact.
// Returns nil if evals are not declared in the workflow frontmatter.
Expand Down Expand Up @@ -53,26 +53,19 @@ func (c *Compiler) buildEvalsJob(data *WorkflowData) (*Job, error) {
steps = append(steps, c.buildEvalsJobSteps(data)...)

// Determine job dependencies.
// Evals runs after safe_outputs when it is configured; otherwise directly after agent.
var needs []string
if data.SafeOutputs != nil {
needs = []string{string(constants.SafeOutputsJobName), string(constants.ActivationJobName)}
} else {
needs = []string{string(constants.AgentJobName), string(constants.ActivationJobName)}
// Evals always depends on agent and activation, and additionally on detection when the detection job is enabled.
// This allows evals to run in parallel with safe_outputs.
needs := []string{string(constants.AgentJobName), string(constants.ActivationJobName)}
if IsDetectionJobEnabled(data.SafeOutputs) {
needs = append(needs, string(constants.DetectionJobName))
}
evalsJobLog.Printf("Evals job dependencies resolved: needs=%v", needs)

// Evals job condition: always run but skip if the upstream job was skipped.
// Evals job condition: always run but skip if the agent job was skipped.
// This matches the detection job pattern so conclusion still sees a non-skipped evals result.
var upstreamJobName string
if data.SafeOutputs != nil {
upstreamJobName = string(constants.SafeOutputsJobName)
} else {
upstreamJobName = string(constants.AgentJobName)
}
alwaysFunc := BuildFunctionCall("always")
upstreamNotSkipped := BuildNotEquals(
BuildPropertyAccess(fmt.Sprintf("needs.%s.result", upstreamJobName)),
BuildPropertyAccess(fmt.Sprintf("needs.%s.result", constants.AgentJobName)),
BuildStringLiteral("skipped"),
)
jobConditionNode := BuildAnd(alwaysFunc, upstreamNotSkipped)
Expand Down
66 changes: 66 additions & 0 deletions pkg/workflow/evals_job_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
package workflow

import (
"testing"

"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"

"github.com/github/gh-aw/pkg/constants"
)

func TestBuildEvalsJobNeedsWithoutDetection(t *testing.T) {
compiler := NewCompiler()

data := &WorkflowData{
AI: "copilot",
Evals: &EvalsConfig{
Questions: []EvalDefinition{
{ID: "q1", Question: "Does it build?"},
},
},
SafeOutputs: &SafeOutputsConfig{},
}

job, err := compiler.buildEvalsJob(data)
require.NoError(t, err)
require.NotNil(t, job)

assert.ElementsMatch(t, []string{
string(constants.AgentJobName),
string(constants.ActivationJobName),
}, job.Needs)
assert.NotContains(t, job.Needs, string(constants.SafeOutputsJobName))
assert.NotContains(t, job.Needs, string(constants.DetectionJobName))
assert.Contains(t, job.If, "needs.agent.result")
assert.NotContains(t, job.If, "needs.safe_outputs.result")
}

func TestBuildEvalsJobNeedsWithDetection(t *testing.T) {
compiler := NewCompiler()

data := &WorkflowData{
AI: "copilot",
Evals: &EvalsConfig{
Questions: []EvalDefinition{
{ID: "q1", Question: "Does it build?"},
},
},
SafeOutputs: &SafeOutputsConfig{
ThreatDetection: &ThreatDetectionConfig{},
},
}

job, err := compiler.buildEvalsJob(data)
require.NoError(t, err)
require.NotNil(t, job)

assert.ElementsMatch(t, []string{
string(constants.AgentJobName),
string(constants.ActivationJobName),
string(constants.DetectionJobName),
}, job.Needs)
assert.NotContains(t, job.Needs, string(constants.SafeOutputsJobName))
assert.Contains(t, job.If, "needs.agent.result")
assert.NotContains(t, job.If, "needs.safe_outputs.result")
}

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.

[/tdd] Missing test for the SafeOutputs == nil path — the old code had an explicit if data.SafeOutputs != nil branch covering this case.

The new code handles it correctly via IsDetectionJobEnabled(nil) == false, but a test with SafeOutputs: nil would lock that in as a regression guard.

💡 Suggested test
func TestBuildEvalsJobNeedsWithoutSafeOutputs(t *testing.T) {
    compiler := NewCompiler()
    data := &WorkflowData{
        AI:          "copilot",
        Evals:       &EvalsConfig{Questions: []EvalDefinition{{ID: "q1", Question: "Does it build?"}}},
        SafeOutputs: nil,
    }
    job, err := compiler.buildEvalsJob(data)
    require.NoError(t, err)
    assert.ElementsMatch(t, []string{
        string(constants.AgentJobName),
        string(constants.ActivationJobName),
    }, job.Needs)
}

@copilot please address this.

Loading