fix(compiler): auto-add pre_activation to safe_outputs/conclusion needs when messages reference its outputs#43570
Conversation
…n messages reference it Fixes actionlint errors in skillet.lock.yml at lines 1385, 1411, 1882. The safe_outputs and conclusion jobs referenced needs.pre_activation.outputs.* in GH_AW_SAFE_OUTPUT_MESSAGES but did not declare pre_activation in their needs. - Add messagesContainPreActivationRef() helper in compiler_safe_outputs_job.go - Auto-add pre_activation to safe_outputs job needs when messages reference it - Auto-add pre_activation to conclusion job needs when messages reference it - Recompile all workflows (skillet.lock.yml now correct) Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
pre_activation to safe_outputs/conclusion needs when messages reference its outputs
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #43570 does not have the 'implementation' label and has ≤100 new lines of code in business logic directories (60 additions across 3 files). |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. Test Quality Sentinel skipped. |
There was a problem hiding this comment.
Pull request overview
Fixes a workflow compilation gap where safe_outputs and conclusion jobs could embed ${{ needs.pre_activation.outputs.* }} inside GH_AW_SAFE_OUTPUT_MESSAGES without declaring pre_activation in needs, causing actionlint expression errors in generated lock YAML.
Changes:
- Added a helper to detect
needs.pre_activation.outputs.*references insafe-outputs.messagestemplates and conditionally wirepre_activationinto jobneeds. - Applied this conditional dependency wiring to both the
safe_outputsjob and theconclusionjob (with deduping forconclusion). - Recompiled
skillet.lock.ymlto reflect the correctedneedslists.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/compiler_safe_outputs_job.go | Detects needs.pre_activation.outputs.* in message templates and auto-adds pre_activation to safe_outputs job needs. |
| pkg/workflow/notify_comment.go | Applies the same conditional needs wiring to the conclusion job, with slices.Contains dedup. |
| .github/workflows/skillet.lock.yml | Regenerated lock output showing pre_activation added to safe_outputs and conclusion job needs. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 3/3 changed files
- Comments generated: 2
- Review effort level: Low
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs, /tdd, and /codebase-design — requesting changes on two maintainability risks.
📋 Key Themes & Highlights
Key Themes
- Missing regression protection:
messagesContainPreActivationRef— the heart of the fix — has no unit tests. A future contributor adding a field toSafeOutputMessagesConfigwon't know to update the scanned list, reintroducing this class of bug silently. - Manually maintained field list: The slice of 16 field values in the helper is a structural copy of the struct. Using reflection in a coverage test (or in the helper itself) would make omissions a compile/test failure rather than a runtime surprise.
- Duplicated guard pattern with inconsistent dedup primitive: The 9-line block (nil check → jobManager check → dedup → append) appears verbatim in two files, but uses
setutil.Containsin one place andslices.Containsin the other. ExtractingappendPreActivationNeedIfRequiredwould remove the duplication and standardise the primitive.
Positive Highlights
- ✅ Root cause properly diagnosed: the serializer embeds message templates verbatim, so the dependency must be wired at compile time.
- ✅ The fix is general and automatic — no per-workflow markdown changes needed.
- ✅
jobManager.GetJobguard is correctly placed to avoid wiring the dep when the job doesn't exist. - ✅ Both affected jobs (
safe_outputsandconclusion) are covered. - ✅ The lock file recompilation confirms end-to-end correctness.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 62.6 AIC · ⌖ 5.37 AIC · ⊞ 6.7K
Comment /matt to run again
| 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) { |
There was a problem hiding this comment.
[/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.
There was a problem hiding this comment.
Review
The fix is correct and addresses the root cause: safe_outputs and conclusion jobs embedding needs.pre_activation.outputs.* expressions without declaring the dependency.
One non-blocking concern: messagesContainPreActivationRef enumerates all string fields of SafeOutputMessagesConfig by hand. Currently the only omission is AppendOnlyComments (a bool field, which cannot contain such a reference — fine). However, if a new string field is added to the struct in the future, this function will silently miss it — the compiler will not warn and no existing test will fail. Using reflect to iterate string-typed fields, or adding a canary test that asserts the hardcoded field count matches the struct's actual string-field count, would keep the scan complete automatically.
Everything else looks good: the seenNeeds dedup in compiler_safe_outputs_job.go is consistent with existing patterns, slices.Contains in notify_comment.go is the right choice for a plain slice, the jobManager.GetJob guard prevents wiring when pre_activation was never created, and the recompiled skillet.lock.yml diff confirms the intended effect.
Checked
SafeOutputMessagesConfigfields vs scanned fields: onlyAppendOnlyComments(bool) absent — expected- Dedup logic in both jobs: correct
buildConclusionJobNeedsdoes not already insertpre_activation— new code is the only insertion point- No existing test covers the new code path (inline comment filed)
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 88.1 AIC · ⌖ 6.47 AIC · ⊞ 4.9K
There was a problem hiding this comment.
Verdict: request changes — two correctness bugs and missing test coverage
The logic is sound for the happy path (skillet.lock.yml confirms it compiles correctly when pre_activation exists), but there are two silent-failure bugs and a structural fragility that will cause problems as the codebase evolves.
Blocking issues (2)
1. Silent no-op when pre_activation job is absent (both paths)
Both buildSafeOutputsJobFromParts and buildConclusionJob silently skip adding the needs edge when GetJob(PreActivationJobName) returns false, with no error. A workflow that has needs.pre_activation.outputs.skill_name in a footer template but no pre_activation job will compile without error, emit a valid-looking YAML, and then produce empty/broken message bodies at runtime. The fix is to return a fmt.Errorf in both paths when the job is absent — the compiler already propagates errors from both call sites.
2. No unit tests for the new correctness-critical function
messagesContainPreActivationRef has zero test coverage despite gating the needs-edge generation. The pre_activation injection in buildConclusionJob also has no tests. Correctness regressions here produce silently broken compiled workflows.
Non-blocking issues (2)
3. Field enumeration will silently miss future struct fields
The hard-coded string-field slice in messagesContainPreActivationRef will silently skip any new fields added to SafeOutputMessagesConfig. A reflection-based exhaustiveness test (or using reflect.Value iteration in the function itself) would catch this at test time.
4. Inconsistent deduplication between the two paths
safe_outputs uses seenNeeds map[string]struct{}, conclusion uses slices.Contains. Both work, but the inconsistency is a maintenance hazard.
🔎 Code quality review by PR Code Quality Reviewer · 178.9 AIC · ⌖ 8.38 AIC · ⊞ 5.4K
Comment /review to run again
| // 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 { |
There was a problem hiding this comment.
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.
| if data.SafeOutputs != nil && messagesContainPreActivationRef(data.SafeOutputs.Messages) { | ||
| if _, exists := c.jobManager.GetJob(string(constants.PreActivationJobName)); exists { | ||
| preActName := string(constants.PreActivationJobName) | ||
| if !slices.Contains(needs, preActName) { |
There was a problem hiding this comment.
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:
- Extract a shared
addUniqueNeed(needs *[]string, seen map[string]struct{}, name string)helper that both call-sites use, or - Build a local
seenNeedsmap inbuildConclusionJobusing the same approach asbuildSafeOutputsJobFromParts.
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).
|
@copilot please run the
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Done. Added |
|
@copilot please run the skill, address unresolved review comments, and rerun checks once the branch is up to date. Unresolved review feedback to address:
|
The two review threads are already addressed — |
skillet.lock.ymlproduced three actionlintexpressionerrors becausesafe_outputsandconclusionjobs embedded${{ needs.pre_activation.outputs.skill_name }}inGH_AW_SAFE_OUTPUT_MESSAGESwithout declaringpre_activationin theirneeds.The compiler serializes
safe-outputs.messagestemplates verbatim into every job that embeds that env var, but the dependency wiring was never propagated forpre_activation(which is explicitly excluded from the normal auto-wiring paths for both jobs).Changes
compiler_safe_outputs_job.go— addsmessagesContainPreActivationRef(*SafeOutputMessagesConfig) boolhelper that scans all message template fields forneeds.pre_activation.outputs.references; used to conditionally appendpre_activationto thesafe_outputsjob's needs list after explicit needs are resolvednotify_comment.go— same guard applied inbuildConclusionJobafterbuildConclusionJobNeeds; usesslices.Containsfor dedupskillet.lock.yml— recompiled;pre_activationnow appears in theneedsof bothsafe_outputsandconclusionThe fix is general: any workflow whose
safe-outputs.messagestemplates referenceneeds.pre_activation.outputs.*will automatically get the dependency wired, without requiring changes to the source markdown.