feat: report failed non-builtin jobs as issues from conclusion job - #49959
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
… and template Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #49959 does not have the 'implementation' label and has only 48 new lines of code in business logic directories (threshold: 100). |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
🧪 Test Quality Sentinel Report
Verdict
|
There was a problem hiding this comment.
Pull request overview
Adds conclusion-job reporting for failed non-builtin workflow jobs.
Changes:
- Adds the reporting script and issue template.
- Adds
report-failed-jobsconfiguration and required permissions. - Regenerates affected workflow lockfiles.
Show a summary per file
| File | Description |
|---|---|
actions/setup/js/report_failed_jobs.cjs |
Queries failed jobs and creates issues. |
actions/setup/md/failed_jobs_issue.md |
Defines the failure issue body. |
pkg/workflow/safe_outputs_config_types.go |
Adds the configuration field. |
pkg/workflow/safe_outputs_config_global.go |
Parses the configuration value. |
pkg/workflow/notify_comment.go |
Adds the step and permissions. |
pkg/workflow/notify_comment_conclusion_helpers.go |
Generates the reporting step. |
.github/workflows/*.lock.yml (all changed lockfiles) |
Regenerates conclusion steps and permissions. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 275/275 changed files
- Comments generated: 6
- Review effort level: Balanced
| ReportFailureAsIssue any `yaml:"report-failure-as-issue,omitempty"` // Controls failure issue creation: bool, templatable expression string, or []interface{} categories (parsed to ReportFailureAsIssueCategories/ExcludedCategories). Default: true | ||
| ReportFailureAsIssueCategories []string `yaml:"-"` // Parsed failure categories for report-failure-as-issue (internal use only, included categories) | ||
| ReportFailureAsIssueExcludedCategories []string `yaml:"-"` // Parsed excluded failure categories for report-failure-as-issue (internal use only, categories starting with "!") | ||
| ReportFailedJobs *bool `yaml:"report-failed-jobs,omitempty"` // Controls whether to report failed non-builtin jobs as issues (default: true). Set to false to disable. |
| if reportFailedJobs, exists := outputMap["report-failed-jobs"]; exists { | ||
| if reportFailedJobsBool, ok := reportFailedJobs.(bool); ok { | ||
| config.ReportFailedJobs = &reportFailedJobsBool | ||
| safeOutputsConfigLog.Printf("Report failed jobs: %t", reportFailedJobsBool) |
| repo, | ||
| title: issueTitle, | ||
| body: issueBody, | ||
| labels: ["agentic-workflows"], |
| return nil, err | ||
| } | ||
| steps = append(steps, agentFailureSteps...) | ||
| steps = append(steps, c.buildConclusionReportFailedJobsStep(data, mainJobName)...) |
| const workflowName = process.env.GH_AW_WORKFLOW_NAME || "unknown"; | ||
| const workflowSourceURL = process.env.GH_AW_WORKFLOW_SOURCE_URL || ""; | ||
| const runUrl = process.env.GH_AW_RUN_URL || ""; | ||
| const { owner, repo } = context.repo; |
| async function main() { | ||
| try { | ||
| // Check if reporting is enabled | ||
| const reportFailedJobs = parseBoolTemplatable(process.env.GH_AW_REPORT_FAILED_JOBS, true); |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: feat: report failed non-builtin jobs as issues from conclusion job
The feature is well-structured and fills a real gap — failed non-builtin jobs were previously invisible unless you inspected the run directly. The compiler-side changes (permission injection, frontmatter field, step builder) look correct.
Two blocking issues in actions/setup/js/report_failed_jobs.cjs:
🔴 No deduplication (blocking)
Every invocation unconditionally creates a new issue. A scheduled workflow that fails on back-to-back runs, or a manual re-run, will accumulate [aw] Failed jobs: <workflow> issues for the same event. handle_agent_failure.cjs avoids this with findExistingFailureIssue, which searches for a recent open issue matching the workflow's XML marker. The same guard needs to be applied here before calling github.rest.issues.create.
🟡 timed_out jobs silently missed (blocking for correctness)
Line 89 filters conclusion === 'failure' only. GitHub Actions also uses timed_out (job-level timeout) as a failure-like conclusion for non-builtin jobs. Those will not be reported. Extend the check to include timed_out (and optionally action_required).
🔵 Permission-error matching is over-broad (non-blocking)
Both isIssueWritePermissionError and isActionsReadPermissionError match on '403' in the message string. A 403 from a repo with issues disabled (not a permissions problem) will be silently swallowed as a permission error instead of surfaced as a warning. Consider using a more precise match (e.g. checking the HTTP status code directly or matching "Resource not accessible by integration" exactly).
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 83.5 AIC · ⌖ 7.84 AIC · ⊞ 5.4K
| setupGlobals(core, github, context, exec, io, getOctokit); | ||
| const { main } = require('${{ runner.temp }}/gh-aw/actions/handle_agent_failure.cjs'); | ||
| await main(); | ||
| - name: Report failed jobs |
There was a problem hiding this comment.
No deduplication of failure issues — report_failed_jobs.cjs unconditionally creates a new issue every time this step runs. On re-runs or repeated scheduled failures the same workflow will accumulate multiple open [aw] Failed jobs: <workflow> issues.
handle_agent_failure.cjs uses findExistingFailureIssue to search for a recent open issue with a matching XML marker before creating a new one. The same dedup window check should be applied in report_failed_jobs.cjs.
@copilot please address this.
| const { main } = require('${{ runner.temp }}/gh-aw/actions/handle_agent_failure.cjs'); | ||
| await main(); | ||
| - name: Report failed jobs | ||
| id: report_failed_jobs |
There was a problem hiding this comment.
timed_out jobs silently ignored — In report_failed_jobs.cjs (line 89), only conclusion === 'failure' is checked. GitHub Actions jobs can also reach timed_out and action_required states that indicate a non-successful outcome. timed_out non-builtin jobs will not be reported.
Consider extending the filter to ['failure', 'timed_out'].includes(job.conclusion).
@copilot please address this.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /codebase-design — requesting changes on correctness and maintainability issues.
📋 Key Themes & Highlights
Key Themes
- Duplicate issues on re-run: No deduplication guard means each conclusion-job re-run creates a new issue for the same run.
- Overlapping error detectors:
isIssueWritePermissionErrorandisActionsReadPermissionErrorshare identical string patterns — a 403 from the jobs API can silently early-exit instead of warning. - Unbounded pagination: No page-cap safety net in
getFailedNonBuiltinJobs. - Redundant env var emission: The
elsebranch emittingGH_AW_REPORT_FAILED_JOBS: "true"is dead code since the JS already defaults totrue. - Dual job-name forms: The builtin-names Set holds both
pre_activationandpre-activation; normalise or document.
Positive Highlights
- ✅ Graceful permission error handling — skips instead of failing the conclusion job
- ✅ Rate-limit telemetry before/after job queries is good observability practice
- ✅ Clean
always()step condition ensures failures are captured even on partial runs - ✅ Opt-out via
report-failed-jobs: falsefrontmatter is well-placed - ✅ Compiler correctly upgrades conclusion-job permissions without downgrading existing higher levels
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 67.7 AIC · ⌖ 11.3 AIC · ⊞ 7.1K
Comment /matt to run again
| * @param {unknown} error | ||
| * @returns {boolean} | ||
| */ | ||
| function isIssueWritePermissionError(error) { |
There was a problem hiding this comment.
[/tdd] isIssueWritePermissionError and isActionsReadPermissionError share identical body logic — a 403 from the jobs API could match the issue-write check, silently skipping reporting when it should warn instead.
💡 Suggestion
Add unit tests for each function with errors that should only match one of the two, and consider tightening the issue-write check (e.g., scope the 403 match to a more specific string, or check status on a structured error object).
@copilot please address this.
| page, | ||
| filter: "latest", | ||
| }); | ||
|
|
There was a problem hiding this comment.
[/tdd] The pagination loop has no upper bound — if jobs.length never falls below perPage (e.g. due to an API bug or very large run), this loops forever without a timeout or page cap.
💡 Suggestion
Add a safety cap, e.g. if (page > 100) { core.warning('Too many pages, aborting'); break; }, and add a test that simulates exactly perPage jobs on the last page to confirm the loop exits.
@copilot please address this.
| } | ||
| core.warning(`Failed to query jobs for run: ${getErrorMessage(error)}`); | ||
| return; | ||
| } finally { |
There was a problem hiding this comment.
[/tdd] fetchAndLogRateLimit is called inside the finally block even when getFailedNonBuiltinJobs throws a permission error and the function returns early — this causes an extra API call after the early-return path, which may be surprising and could itself fail.
💡 Suggestion
Move the after-rate-limit log to after the try/catch, only calling it on the success path, or document why it's intentional in the finally block. Add a test for the permission-error path to confirm the expected number of rate-limit calls.
@copilot please address this.
| * by the handle_agent_failure step (the agent job) or are framework-internal | ||
| * (conclusion = current job, pre_activation/activation = reported via agent failure issue flags). | ||
| */ | ||
| const BUILTIN_REPORTED_JOB_NAMES = new Set(["agent", "conclusion", "activation", "pre_activation", "pre-activation"]); |
There was a problem hiding this comment.
[/codebase-design] BUILTIN_REPORTED_JOB_NAMES contains both pre_activation and pre-activation (underscore and hyphen variants). If the canonical job name changes or a new alias is added, this Set must be manually kept in sync across two string forms.
💡 Suggestion
Normalise job names before checking (e.g. job.name.replace(/-/g, '_')) and store only the underscore variant, or add a comment documenting where these names come from so future maintainers know how to update them.
@copilot please address this.
| core.info(`Found ${failedJobs.length} failed non-builtin job(s): ${failedJobs.map(j => j.name).join(", ")}`); | ||
|
|
||
| // Render the issue body from template | ||
| const failedJobsList = formatFailedJobsList(failedJobs); |
There was a problem hiding this comment.
[/tdd] No deduplication: if the conclusion job runs twice (retry/re-run), a second [aw] Failed jobs: <workflow> issue is created for the same run. Existing failure-issue logic in this repo typically searches for an open issue with the same title before creating.
💡 Suggestion
Search for an existing open issue with the same title ([aw] Failed jobs: <workflow>) and update it instead of always creating a new one, consistent with report_failure.cjs patterns in this codebase.
@copilot please address this.
| } | ||
| var envVars []string | ||
| envVars = append(envVars, buildWorkflowMetadataEnvVarsWithTrackerID(data.Name, data.Source, data.TrackerID, buildLocalWorkflowSourceURL(c.markdownPath))...) | ||
| envVars = append(envVars, " GH_AW_RUN_URL: ${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }}\n") |
There was a problem hiding this comment.
[/codebase-design] GH_AW_REPORT_FAILED_JOBS is emitted as "true" unconditionally when ReportFailedJobs is nil (default), but the JS already defaults to true via parseBoolTemplatable. The branch on lines 567-571 is dead code for the nil case — emitting the variable when it was not explicitly set adds noise and could confuse future readers who expect env vars to only be set when they carry a non-default value.
💡 Suggestion
Only append GH_AW_REPORT_FAILED_JOBS to envVars when data.SafeOutputs.ReportFailedJobs != nil (i.e., when the user explicitly set it), and remove the else branch that adds the redundant "true" line.
@copilot please address this.
|
@copilot sous-chef triage: Failed checks:
Please refresh the branch if needed and then run the
|
…int lint error Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
🔍 PR TriageCategory: feature · Risk: high · Total score: 80/100
Adds conclusion-job reporting for failed non-builtin jobs across 275 compiled workflow files — high blast radius (touches all lock.yml outputs) but high value for observability. CI pending. Recommended action:
|
|
🎉 This pull request is included in a new release. Release: |
The conclusion job had no mechanism to surface failures in non-agent jobs (custom safe-output jobs,
safe_outputs,detection, etc.). Failed jobs other thanagent/activationwould silently disappear unless someone inspected the run directly.Changes
JavaScript (
actions/setup/js/report_failed_jobs.cjs)listJobsForWorkflowRunfor the current run, filtered toconclusion === 'failure'handle_agent_failure:agent,activation,pre_activation,conclusionfetchAndLogRateLimitrenderTemplateFromFile; creates[aw] Failed jobs: <workflow>issueactions:readorissues:writepermissionsTemplate (
actions/setup/md/failed_jobs_issue.md)Separate template file with
{workflow_name},{workflow_source_url},{run_url},{failed_jobs_list}placeholders.Compiler
safe_outputs_config_types.go/safe_outputs_config_global.go: newReportFailedJobs *boolfield backingreport-failed-jobsfrontmatter (default:true)notify_comment_conclusion_helpers.go:buildConclusionReportFailedJobsStep— emits the step, skips when explicitly disablednotify_comment.go: wires the step afterhandle_agent_failure; ensures conclusion job has at leastactions: read+issues: writewhen the feature is on (preserves higher existing levels)Opt-out
run: https://github.com/github/gh-aw/actions/runs/30815680828