Fix activation job needs not populated from engine.env needs expressions - #49863
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
|
There was a problem hiding this comment.
Pull request overview
Fixes activation dependencies for custom-job outputs referenced by engine.env.
Changes:
- Detects referenced custom jobs without explicit dependencies.
- Wires them before activation while preventing dependency cycles.
- Adds focused unit coverage.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/compiler_jobs.go |
Adds engine environment reference detection. |
pkg/workflow/compiler_custom_jobs.go |
Prevents circular activation dependencies. |
pkg/workflow/compiler_activation_outputs.go |
Adds referenced jobs to activation needs. |
pkg/workflow/compiler_activation_outputs_test.go |
Tests activation dependency behavior. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 4/4 changed files
- Comments generated: 2
- Review effort level: Balanced
| for _, j := range c.getEngineEnvReferencedCustomJobsWithNoExplicitNeeds(data) { | ||
| promptReferencedJobs[j] = struct{}{} |
| for _, envValue := range data.EngineConfig.Env { | ||
| engineEnvBuilder.WriteByte('\n') | ||
| engineEnvBuilder.WriteString(envValue) |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
The fix is correct and well-structured. The new getEngineEnvReferencedCustomJobsWithNoExplicitNeeds function properly scans engine.env values for needs.<job>.outputs.* expressions and adds those custom jobs as activation prerequisites — mirroring the existing pattern for markdown-body-referenced jobs. Cycle prevention is handled correctly in both configureActivationNeedsAndCondition and getCustomJobDependencySets. Tests cover the key scenarios including nil safety, deduplication, and exclusion of built-in jobs.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 14 AIC · ⌖ 11.2 AIC · ⊞ 5.4K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — requesting changes on two small but concrete gaps.
📋 Key Themes & Highlights
Issues Found
-
Missing cycle-guard regression test — the
hasNeedsfilter correctly prevents jobs with explicitneeds(e.g.needs: activation) from being added to activation deps, but there's no test that pins this invariant. A future refactor could silently break it. -
Non-deterministic result ordering —
getEngineEnvReferencedCustomJobsWithNoExplicitNeedsiterates overdata.EngineConfig.Env(amap[string]string), which gives no ordering guarantee.getReferencedCustomJobsalready sorts its output viasort.Strings; the new helper should do the same to keep compiled YAML deterministic.
Positive Highlights
- ✅ Root cause is correctly diagnosed and fixed — activation was referencing job outputs before those jobs ran
- ✅ The circular-dependency guard in
getCustomJobDependencySetsis a thoughtful companion change - ✅ Test coverage is comprehensive: nil safety, case() expressions, deduplication, built-in job exclusion
- ✅ New helper mirrors the structure of the existing
getCustomJobsReferencedInPromptWithNoActivationDep, keeping the codebase consistent
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 38.8 AIC · ⌖ 7.49 AIC · ⊞ 7.1K
Comment /matt to run again
| preActivationJob: false, | ||
| } | ||
|
|
||
| assert.NotPanics(t, func() { |
There was a problem hiding this comment.
[/diagnosing-bugs] Missing regression test for the cycle-prevention invariant: a job with needs: activation referenced in engine.env must NOT be added to activation needs. The hasNeeds filter handles it correctly today, but without a test this can silently regress.
💡 Suggested test case
t.Run("engine.env reference to job with explicit needs is not added to activation needs", func(t *testing.T) {
c := NewCompiler()
data := &WorkflowData{
EngineConfig: &EngineConfig{
Env: map[string]string{
"SOME_VAR": "${{ needs.post_job.outputs.token }}",
},
},
Jobs: map[string]any{
"post_job": map[string]any{
"runs-on": "ubuntu-latest",
"needs": "activation",
},
},
}
ctx := &activationJobBuildContext{data: data}
c.configureActivationNeedsAndCondition(ctx)
assert.NotContains(t, ctx.activationNeeds, "post_job")
})@copilot please address this.
| referencedJobs := c.getReferencedCustomJobs(engineEnvBuilder.String(), data.Jobs) | ||
| var result []string | ||
| for _, jobName := range referencedJobs { | ||
| jobConfig, ok := data.Jobs[jobName].(map[string]any) |
There was a problem hiding this comment.
[/diagnosing-bugs] The env-value iteration order is non-deterministic (Go map iteration). When two env values reference different custom jobs, the order they are appended to result (and therefore customJobsBeforeActivation) will vary across runs. This is benign today — slices.Contains deduplicates — but it can produce non-deterministic needs: ordering in the compiled YAML. Consider sorting result before returning, matching the pattern used by getCustomJobsReferencedInPromptWithNoActivationDep (line 172) and getReferencedCustomJobs (which already calls sort.Strings).
💡 One-line fix
// at end of getEngineEnvReferencedCustomJobsWithNoExplicitNeeds, before return:
sort.Strings(result)
return result@copilot please address this.
…tion Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
🧪 Test Quality Sentinel Report✅ Test Quality Score: 81/100 — Excellent
📊 Metrics (7 tests)
|
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (252 new lines in 📄 Draft ADR committed:
📋 What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. ❓ Why ADRs Matter
ADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
|
@copilot Please address the remaining blockers on this PR, then run the Outstanding review items:
Branch refresh may help once you are ready.
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Implemented in
|
|
🎉 This pull request is included in a new release. Release: |
Follow-up to #30239: the
activationjob was not gainingneedsentries for custom jobs referenced vianeeds.<job>.outputs.*inengine.envvalues (e.g. overridingCOPILOT_GITHUB_TOKEN), even though theagentjob already did. The activation step using the engine.env override would reference a job output before that job had run.Changes
compiler_jobs.go— NewgetEngineEnvReferencedCustomJobsWithNoExplicitNeedshelper: scansengine.envvalues forneeds.<job>.outputs.*patterns, returning only custom jobs with no explicitneedsdeclaration (same filter logic asgetCustomJobsReferencedInPromptWithNoActivationDep).compiler_activation_outputs.go—configureActivationNeedsAndConditionnow calls the new helper, adding referenced engine.env jobs tocustomJobsBeforeActivation(the activation job'sneedslist).compiler_custom_jobs.go—getCustomJobDependencySetsnow includes engine.env-referenced (no-explicit-needs) jobs in the pre-activation set, preventingactivationfrom being auto-added to those jobs. Without this, the compiler would create a circular dependency:activation → custom_token → activation.compiler_activation_outputs_test.go— Unit tests for the new behavior: no-explicit-needs jobs,case()expressions, with/withoutpre_activation, nil safety, deduplication, and cycle-safe exclusion of built-in job references.Before / After
Given:
Before:
After:
Run: https://github.com/github/gh-aw/actions/runs/30771772506