Uh oh!
There was an error while loading. Please reload this page.
Fix actionlint error: agent job referencing needs.approval_allowlist it never depends on - #54028
Conversation
…gent job's safe-outputs config Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…gentSafeOutputsConfig Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
PR TriageCategory: bug · Risk: medium · Score: 57/100 (impact 25 + urgency 20 + quality 12) Root-caused compiler bug behind recurring PR Sous Chef job failures; affects generated workflow correctness. Automated triage — see run report for full details.
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
✅ Ponytail Reviewer completed successfully!
|
✅ PR Code Quality Reviewer completed the code quality review.
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
❌ Design Decision Gate 🏗️ failed during design decision gate check. Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details.
|
There was a problem hiding this comment.
The fix is clean and correct. sanitizeAgentSafeOutputsConfig properly neutralizes needs.<job>.* expressions that are unresolvable in the agent job config copy, replacing templated allowed_pull_requests values with empty arrays. Only the agent job copy is affected; the handler job config retains the real expression. The test is well-targeted and the lock file update confirms end-to-end correctness.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 18.4 AIC · ⌖ 8.8 AIC · ⊞ 5.7K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — approving with minor suggestions.
📋 Key Themes & Highlights
Key Themes
- Asymmetric neutralization: the
stringexpression branchdeletes the key while thetemplatableJSONExpressionbranch replaces it with[]any{}— worth making consistent and testing. - Test coverage: the new test covers the happy path well but misses the
string-expression and early-exit branches ofsanitizeAgentSafeOutputsConfig.
Positive Highlights
- ✅ Root cause is correctly diagnosed and clearly explained in the PR description
- ✅ The fix is surgical — handler config is intentionally untouched
- ✅ Regression test added alongside the fix
- ✅ Recompile confirms actionlint is clean
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 28.7 AIC · ⌖ 9.95 AIC · ⊞ 7.8K
Comment /matt to run again
| return isExpression(v) && referencesUnresolvableJob(v) | ||
| } | ||
| return false | ||
| } |
There was a problem hiding this comment.
[/diagnosing-bugs] The string-expression branch deletes the key entirely (delete(v, key)), while the templatableJSONExpression branch replaces with []any{}. For a non-slice field expressed as a string template, silently dropping the key could behave differently from setting it to an empty value, and there is no test exercising this code path.
💡 Suggested fix
Consider replacing rather than deleting for consistency:
case string:
ifisExpression(fv) &&referencesUnresolvableJob(fv) {
v[key] =""// explicit empty rather than absent
}Or add a test case that covers a string-typed expression field to document the deletion as intentional.
@copilot please address this.
| func TestGenerateSafeOutputsConfigCommentMemoryToolsOnly(t *testing.T) { | ||
| data := &WorkflowData{ | ||
| CommentMemoryConfig: &CommentMemoryConfig{ |
There was a problem hiding this comment.
[/tdd] The new test only exercises AllowedPullRequests (the []any / templatableJSONExpression path). Neither the string expression path nor the "no unresolvable jobs" early-exit path is covered. Adding those cases would turn the test into a complete specification of sanitizeAgentSafeOutputsConfig.
💡 Suggested additional test cases
// empty unresolvableJobs — config must be unchangedfuncTestSanitizeAgentSafeOutputsConfigNoOp(t*testing.T) { ... }
// string-typed expression field — verify deletion vs empty-string behaviourfuncTestSanitizeAgentSafeOutputsConfigStringExpr(t*testing.T) { ... }@copilot please address this.
There was a problem hiding this comment.
Pull request overview
Fixes invalid needs references in agent-job safe-output configuration while preserving handler-job expressions.
Changes:
- Sanitizes unresolvable safe-output expressions.
- Adds regression coverage.
- Regenerates the PR Sous Chef workflow.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/safe_outputs_config_generation.go | Applies agent config sanitization. |
pkg/workflow/compiler_safe_outputs_builder.go | Implements recursive expression sanitization. |
pkg/workflow/safe_outputs_config_generation_test.go | Tests expression neutralization. |
.github/workflows/pr-sous-chef.lock.yml | Updates generated agent configuration. |
Review details
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 4/4 changed files
- Comments generated: 3
- Review effort level: Balanced
| referencesUnresolvableJob := func(expr string) bool { | ||
| for _, job := range unresolvableJobs { | ||
| if strings.Contains(expr, "needs."+job+".") { | ||
| return true | ||
| } | ||
| } | ||
| return false | ||
| } |
| case []any: | ||
| // A slice field is only ever templated as a whole (a single-element | ||
| // expression slice); replace the entire slice if any element references | ||
| // an unresolvable job, since individual elements cannot be mutated | ||
| // in place through the parent map. | ||
| replaced := false | ||
| for _, item := range fv { | ||
| if itemReferencesUnresolvableJob(item) { | ||
| v[key] = []any{} | ||
| replaced = true | ||
| break | ||
| } | ||
| } | ||
| if !replaced { | ||
| for _, item := range fv { | ||
| visit(item) | ||
| } | ||
| } |
Uh oh!
There was an error while loading. Please reload this page.
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment.
|
There was a problem hiding this comment.
Verdict: comment
No blocking issues stood up in the changed lines after reviewing the config sanitization path and its regression test.
Notes
- The new sanitization is scoped to the agent job's copy of
GH_AW_SAFE_OUTPUTS_CONFIG, while the handler config keeps the realneeds.*expression. - I did not find a changed-line regression that would break
approve_workflow_runsemantics beyond the intended agent-side neutralization. - The requested
grumpy-codersub-agent was not available in this harness, so its advisory pass was skipped.
🔎 Code quality review by PR Code Quality Reviewer · gpt54 · 9.44 AIC · ⌖ 9.96 AIC · ⊞ 7K
Comment /review to run again
There was a problem hiding this comment.
Ponytail review (over-engineering only).
L195: yagni: recursive generic tree walker (visit handles arbitrary nested map[string]any/[]any) but only ever called on a config whose real shapes are flat (tool -> field map -> scalar/slice). A single non-recursive loop over each tool's fields covers every actual case; the recursion for nested []any/map depths never occurs here.
net: -20 lines possible.
Generated by ✂️ Ponytail Reviewer for #54028 · auto · 48.6 AIC · ⌖ 3.96 AIC · ⊞ 7.3K
Comment /ponytail to run again
| } | ||
| return false | ||
| } | ||
| var visit func(value any) |
There was a problem hiding this comment.
yagni: recursive generic tree walker (visit handles arbitrary nested map[string]any/[]any) but only ever called on a config whose actual shapes are flat: each safe-output tool maps to a flat field map like allowed_pull_requests. Nothing here nests deeper than tool -> field -> scalar/slice. A single non-recursive loop over each tool's field map covers every real case; the default: visit(fieldValue) and top-level case []any recursion branches handle depths that never occur in this config.
gh-aw-bot
commented
Aug 19, 2026
@copilot This PR has unresolved review feedback and failing checks that still need a forward-progress pass.
|
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>
Addressed the sanitizer feedback in commit |
pelikhan
commented
Aug 19, 2026
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Fixed in commit |
gh-aw-bot
commented
Aug 20, 2026
@copilot This PR still needs a forward-progress pass.
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Completed the requested forward-progress pass in commit |
PR Triage
Fixes an actionlint validation error affecting agent job dependency wiring platform-wide. mergeable_state:blocked pending checks; already labeled needs-review. Automated triage by PR Triage Agent.
|
🎉 This pull request is included in a new release. Release: |
The 2026-08-19 static analysis report flagged a new actionlint error at
pr-sous-chef.lock.yml:837:${{ toJSON(needs.approval_allowlist.outputs.eligible_pull_request_numbers) }}references a job that isn't in theagentjob's dependency graph, likely explaining a series of open "Failed jobs: PR Sous Chef" issues.Root cause
generateSafeOutputsConfigbuilds theagentjob's own copy ofconfig.json(GH_AW_SAFE_OUTPUTS_CONFIG) by reusing the same handler-registry output as the safe-outputs handler job's config (GH_AW_SAFE_OUTPUTS_HANDLER_CONFIG). Jobs listed undersafe-outputs.needs(e.g.approval_allowlist) are only ever wired as dependencies of the later handler job (buildSafeOutputsJobNeeds), never ofagent. So anyneeds.<job>expression carried into the agent job's config is structurally unresolvable and trips actionlint's undefined-property check.Fix
sanitizeAgentSafeOutputsConfig(compiler_safe_outputs_builder.go), invoked fromgenerateSafeOutputsConfig, which walks the agent-job config map and neutralizes any templated field — single expression or slice-of-expressions — that referencesneeds.<job>for a job indata.SafeOutputs.Needs, replacing it with an empty value.GH_AW_SAFE_OUTPUTS_HANDLER_CONFIGis untouched, since that job legitimately depends on these custom jobs and still resolves the real expression at runtime.Before/after for the agent job's config:
Recompiling
pr-sous-chef.mdconfirms actionlint no longer reports the undefined-property error, while the handler config retains the real allow-list expression.