Run evals in parallel with safe_outputs by rewiring compiler job needs#45752
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
evals in parallel with safe_outputs by rewiring compiler job needs
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #45752 does not have the 'implementation' label and has 73 new lines of code in business logic directories (threshold: 100). |
There was a problem hiding this comment.
Pull request overview
This pull request updates the gh-aw workflow compiler to rewire the evals job so it no longer depends on safe_outputs, allowing evals to run in parallel with safe outputs while still preserving required ordering via agent (and detection when enabled).
Changes:
- Reworked
evalsjobneedsto depend onagent+activation, plusdetectionwhen the detection job is enabled. - Aligned the
evalsjob runtime guard to consistently checkneeds.agent.result != 'skipped'. - Added focused unit tests covering evals dependency resolution with and without detection.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/evals_job.go | Rewires evals dependency selection and aligns the if: guard to agent, enabling parallel execution with safe outputs. |
| pkg/workflow/evals_job_test.go | Adds targeted unit tests validating the new needs and if behavior for evals with/without detection. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Low
| // Evals always depends on agent, 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)} |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
The dependency rewiring looks correct. Evals now depends directly on agent (always) and detection (when enabled), enabling parallel execution with safe_outputs while preserving proper ordering. The if condition consistently references needs.agent.result since agent is always in the needs list. Test coverage for both detection-enabled and detection-disabled paths is adequate.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 10.1 AIC · ⌖ 7.67 AIC · ⊞ 5K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd — one minor gap in test coverage; otherwise a clean, well-scoped change. No blocking issues.
📋 Key Themes & Highlights
Key Themes
- Correctness: The dependency rewiring is sound.
IsDetectionJobEnabled(nil)correctly returnsfalse, so the nil safe-outputs path still produces[agent, activation]. - Test coverage: Two focused tests cover detection-disabled and detection-enabled configs. The one missing case is
SafeOutputs == nil(the oldelsebranch) — see inline comment.
Positive Highlights
- ✅ Net simplification: 11 lines of branching replaced by one conditional append
- ✅ Tests are well-structured with clear names and assertion intent
- ✅ Comment updated correctly from
safe_outputs→agentto track the behaviour change - ✅
IsDetectionJobEnabledis the established single source of truth — correct reuse here
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 29.7 AIC · ⌖ 4.48 AIC · ⊞ 6.7K
Comment /matt to run again
| 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") | ||
| } |
There was a problem hiding this comment.
[/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.
There was a problem hiding this comment.
Non-blocking observations
The dependency graph change is well-reasoned and parallel execution is safe — evals only consumes agent artifacts, not safe_outputs outputs.
Findings
Medium — Stale function-level doc comment (evals_job.go:19-20): still describes the old sequential dependency on safe_outputs. Misleads future readers.
Medium — Missing SafeOutputs == nil test (evals_job_test.go): the old code had an explicit branch for this path; the new code handles it implicitly via IsDetectionJobEnabled's nil guard, but no test covers this common case. Add TestBuildEvalsJobNeedsWithNilSafeOutputs.
🔎 Code quality review by PR Code Quality Reviewer · 35.4 AIC · ⌖ 4.56 AIC · ⊞ 5.6K
Comment /review to run again
Comments that could not be inline-anchored
pkg/workflow/evals_job.go:20
Stale function-level doc comment: the description still says the evals job runs after the safe_outputs job, but this PR intentionally changes that. The comment now misleads readers into thinking the old sequential dependency still holds.
<details>
<summary>💡 Suggested fix</summary>
Update lines 19–20:
// buildEvalsJob creates a separate evals job that runs after the agent job
// (and after the detection job when detection is enabled), in parallel with safe_outputs.Without …
pkg/workflow/evals_job_test.go:56
Missing test for SafeOutputs == nil: the old code had an explicit branch for this path; the new code handles it implicitly (nil SafeOutputs means IsDetectionJobEnabled returns false, so only agent+activation needs). This path is the default for many workflows and has no coverage in this PR.
<details>
<summary>💡 Suggested fix</summary>
Add a third test:
func TestBuildEvalsJobNeedsWithNilSafeOutputs(t *testing.T) {
compiler := NewCompiler()
data := &WorkflowData{
A…
</details>
|
@copilot merge main and recompile |
|
Run: https://github.com/github/gh-aw/actions/runs/29426851914
|
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…er-for-evals-job Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done — merged main (commit |
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
|
@copilot merge main and recompile |
…er-for-evals-job Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done — merged latest main (5 new commits including frontmatter extraction refactoring and evals metadata additions) and recompiled all 256 workflows successfully. |
|
|
…er-for-evals-job Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
PR Finisher Summary
Actions taken:
Hand-off: Still needed:
The PR is code-complete, locally validated, and ready for merge pending CI re-trigger. |
|
🎉 This pull request is included in a new release. Release: |
This change updates workflow compilation so the
evalsjob no longer waits onsafe_outputs.evalsnow runs afteragent(anddetectionwhen present), enabling parallel execution with safe outputs while keeping dependency correctness.Dependency graph change
evalsneeds were changed fromsafe_outputs|agent + activationtoagent + activation (+ detection when enabled).safe_outputsis no longer a hard dependency ofevals.Job condition alignment
needs.agent.result != 'skipped', instead of switching betweenagentandsafe_outputs.Targeted compiler coverage