Add first-class agent job gating via jobs.agent.needs and jobs.agent.if - #49814
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
jobs.agent.needs and jobs.agent.if
|
Hey However, this PR mixes several unrelated changes that could be split out:
Once split, this PR will be clean and ready for review. The core feature is well-implemented with tests and docs included! Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "patchdiff.githubusercontent.com"See Network Configuration for more information.
|
There was a problem hiding this comment.
Pull request overview
Adds direct gating of compiler-generated jobs through jobs.<built-in>.needs and jobs.<built-in>.if.
Changes:
- Merges user conditions with generated job conditions.
- Adds unit and compile-level coverage.
- Documents built-in job customization.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/compiler_custom_jobs.go |
Implements built-in job if augmentation. |
pkg/workflow/compiler_custom_jobs_test.go |
Tests condition extraction and merging. |
pkg/workflow/compiler_agent_job_gating_test.go |
Tests compiled agent gating. |
docs/src/content/docs/reference/steps-jobs.md |
Documents gating syntax and example. |
docs/src/content/docs/reference/frontmatter.md |
Describes built-in job customization. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 5/5 changed files
- Comments generated: 3
- Review effort level: Balanced
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel analysis complete: PR #49814 — Test Quality Score 82/100 (Acceptable). All 4 new tests verify design invariants with strong error coverage. 0% implementation tests (threshold: 30%). No guideline violations. Analysis report available in response. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
There was a problem hiding this comment.
REQUEST_CHANGES — jobs.agent.if augmentation has 3 unresolved correctness issues
💡 Summary of blocking concerns (already flagged by prior review, still unaddressed)
extractBuiltinJobIfAugmentationdoesn't strip an"if: "prefix (compiler_custom_jobs.go:687-699) — inconsistent with the establishedJob.Ifcontract used elsewhere in this same file for custom jobs, which strips that prefix before storing. Authors who copy theif: <expr>custom-job syntax intojobs.agent.ifwill silently get a broken/garbage condition string embedded with the literalif:text.- Misleading error message when only
ifis configured (compiler_custom_jobs.go:735) — the error always saysjobs.%s.needs: cannot augment...even when the user only set.ifand never touched.needs. This will confuse users debugging a config error on a built-in job that isn't generated by their workflow (e.g.jobs.safe_outputs.ifon a workflow with no safe-outputs). - Verbatim
&&-combination of userifwith compiler condition can silently bypass compiler-managed gating (compiler_custom_jobs.go:768, usingcombineJobIfConditions) — GitHub Actions disables the implicitsuccess()check on a job when itsif:expression contains a status-check function (always(),failure(),cancelled(),success()). SincecombineJobIfConditions/BuildAndjust AND-joins the raw user expression with the compiler-generated condition, a user-suppliedif: failure()(a very plausible pattern per this PR's own example, "gate agent when build fails") could change the overall implicit-success semantics that the compiler currently relies on for other terms in the combined expression, not just add a constraint. This needs an explicit safety check or documented caveat — it's not just a theoretical edge case, it's the headline use case in this PR's docs example (needs.build.outputs.outcome == 'failure'is a workaround for this exact problem, but a more naturalif: failure()would hit it directly).
These three points were already raised as review comments on this PR at the corresponding lines (compiler_custom_jobs.go:698, 725, 768) and remain unresolved in the current diff — re-flagging here rather than duplicating comments.
No new comments added since the above are already tracked on the PR; the review is being submitted as REQUEST_CHANGES because these are correctness/semantics-affecting issues in a feature that directly manipulates job gating conditions (a security/reliability-sensitive area for the agent job).
🔎 Code quality review by PR Code Quality Reviewer · auto · 30.6 AIC · ⌖ 5.39 AIC · ⊞ 7.9K
Comment /review to run again
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /codebase-design — commenting on a few targeted improvements.
📋 Key Themes & Highlights
Positive Highlights
- ✅ Clean, additive-only design: user
ifandneedsextend but never replace compiler-managed behaviour - ✅ Good type-safety guard in
extractBuiltinJobIfAugmentationwith a clear error message - ✅ Three layers of tests (unit type-error, unit condition merging, integration compile) — solid coverage
- ✅ Docs example matches the code exactly
Issues Found
- Function name drift (
/codebase-design):applyBuiltinJobNeedsAugmentationsnow owns bothneedsandifaugmentation. Rename toapplyBuiltinJobAugmentationsto keep the codebase navigable. - Integration test assertion (
/tdd): The compile-level test assertsagentJob["if"] == user expression, but the documented behaviour is a logical-AND merge with any compiler-generated conditions. If the agent job gains a compiler condition in the future the test will silently pass with only the user fragment. Preferassert.Containsor add a variant that pre-seeds a compiler condition. - Existing Copilot review comments on line 698 (if-expression normalisation), 725 (error message field name), and 768 (implicit
success()bypass) raise important correctness concerns — thesuccess()bypass issue in particular is worth careful consideration before merging.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 34.7 AIC · ⌖ 8.01 AIC · ⊞ 7.1K
Comment /matt to run again
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (147 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
|
There was a problem hiding this comment.
Review: jobs.agent.needs and jobs.agent.if gating
The feature direction is correct and the test coverage is solid. Three blocking issues exist in compiler_custom_jobs.go (flagged as inline comments):
-
ifexpression not normalized (line 698) —extractBuiltinJobIfAugmentationstores the raw value without stripping an"if: "prefix or${{ }}wrapper. Custom jobs normalize this before combining; built-in job augmentation must do the same orcombineJobIfConditionswill produce malformed expressions. -
Wrong field name in error message (line 735) — when only
ifis set (noneeds), the error still reportsjobs.\<name\>.needs. It should reflect the actual field triggering the error. -
Implicit
success()bypass (line 768) — when the user-providedifcontains a status function (e.g.failure()), GitHub Actions suppresses the implicitsuccess()check for downstream jobs in the chain, which can silently allowsafe_outputsor other compiler-managed jobs to run after a failure. The compiler should reject or warn on expressions containing status functions, or wrap the compiler-managed side with an explicitsuccess() &&.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 40.7 AIC · ⌖ 7.55 AIC · ⊞ 5.4K
|
@copilot Please continue triage on this PR. Remaining reviewer-visible blockers (newest first):
Run details: https://github.com/github/gh-aw/actions/runs/30760589044
|
… bypass for builtin job augmentation - Strip 'if: ' prefix from jobs.<built-in>.if values before storing in Job.If, matching the Job.If contract (bare expression only) and preventing 'if: if: ...' in the compiled output - Report the actual field name (.if or .needs) in the 'cannot augment' error so authors can identify the misconfigured field - Add guardIfAgainstStatusFuncBypass helper that inserts explicit needs.<compiler-job>.result == 'success' guards when user condition contains a status function (always/failure/cancelled/success), preventing bypass of compiler-managed activation/pre_activation permission checks - Add ifExpressionContainsStatusFunc helper for string-based status function detection (the expression parser tokenises failure() as an ExpressionNode literal, so AST-based detection is not reliable here) - Add tests for prefix stripping, field-specific error messages, and status function bypass guarding - Update ADR-49814: status changed from Draft to Accepted, guard mechanism documented Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed all three correctness issues (commit 1. 2. Error field reporting — The "cannot augment" error now reports the field the author actually configured: 3. Status function bypass guard — Added Seven new tests cover all three fixes. ADR-49814 updated to Accepted with the guard mechanism documented (including the string-based detection rationale and its known limitations). Maintainer handoff:
|
|
@copilot Please continue triage on this PR. Remaining reviewer-visible blockers appear addressed in your latest response, but the PR still shows blocking review state. Please:
Run details: https://github.com/github/gh-aw/actions/runs/30761726325
|
…Augmentations; strengthen integration test if assertion Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
All reviewer-visible blockers are now addressed and reflected in the commits. Here's the final state: Fixes from commit 929b8da:
Fixes from the latest commit:
Local validation: Maintainer hand-off: CI must be re-triggered (close/reopen or |
|
@copilot Please continue triage on this PR. The latest author handoff says the blocking review items were fixed on the current head, but the PR is still in unstable/blocking review state. Please:
Run details: https://github.com/github/gh-aw/actions/runs/30763117443
|
All blocking review items are resolved on the current HEAD ( Resolved in commit
Resolved in the following commit (
Local validation: Remaining manual actions:
|
|
@copilot The reviewer-visible fixes appear landed, and the remaining blocker is stale review state plus maintainer follow-through. Please run the
Newest unresolved review threads to address first:
Run details: https://github.com/github/gh-aw/actions/runs/30764198337
|
|
🎉 This pull request is included in a new release. Release: |
Document jobs.agent.needs/if built-in job gating (#49814) in syntax-agentic.md, and split token-optimization.md (423 -> 384 lines) by extracting OpenTelemetry/ AgenticOps and harness-experience-learning content into a new token-optimization-observability.md, bringing it back under the 400-line target.
This adds an explicit frontmatter path for gating the generated
agentjob on custom setup jobs, instead of relying on theon.needs+ top-levelifcascade pattern. Workflow authors can now express agent-level dependency and skip logic directly injobs.agent.Compiler support
jobs.<built-in>.ifin addition to additiveneedsjobs.agent.ifto the generated agent job by combining it with compiler-generated conditions via logical&&needsremain additive onlyAgent job gating
agentjob:needs.<job>.outputs.*to be referenced from the agent job without routing the control flow throughpre_activation/activationValidation and coverage
ifaugmentation, condition merging, and invalidiftypesjobs.agent.needs+jobs.agent.iftogetherDocs
jobs:can target compiler-generated built-in jobs for additive customizationjobs.agent.needs/jobs.agent.ifexamples to the steps/jobs referenceRun: https://github.com/github/gh-aw/actions/runs/30761726325