Onboard build failure analysis workflow - #132609
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Azure Pipelines: Successfully started running 1 pipeline(s). 15 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
|
Tagging subscribers to this area: @dotnet/runtime-infrastructure |
There was a problem hiding this comment.
Pull request overview
This PR adds an agentic GitHub Actions workflow pair for analyzing Azure Pipelines (runtime, definitionId 129) build failures by downloading the failed/canceled legs’ Logs_Build_* artifacts, extracting *.binlog, and delegating analysis to a repository-scoped build-failure analyst agent that reads the binlogs via the binlog-mcp MCP server and posts results via safe-outputs.
Changes:
- Introduces a shared prompt body and an agent playbook for consistent build-failure analysis behavior.
- Adds two workflows: an automatic
check_run-triggered workflow and a maintainer/analyze-build-failureslash-command workflow, both reusing failed-job binlogs instead of rebuilding. - Pins the
binlog-mcpcontainer digest in the actions lock data and includes the compiled.lock.ymloutput.
Reviewed changes
Copilot reviewed 5 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| .github/workflows/shared/build-failure-analysis-shared.md | Shared agent prompt body imported by the two workflows. |
| .github/workflows/build-failure-analysis.md | Automatic workflow that fetches failed/canceled job binlogs from ADO and runs the analysis agent with safe-outputs. |
| .github/workflows/build-failure-analysis.lock.yml | gh-aw compiled lock workflow (generated) with pinned actions/containers and emitted jobs. |
| .github/workflows/build-failure-analysis-command.md | Slash-command workflow to rerun analysis on a PR’s latest failed build. |
| .github/aw/actions-lock.json | Adds a pinned container digest entry for the binlog-mcp image. |
| .github/agents/build-failure-analyst.agent.md | Adds the reusable build-failure analyst agent playbook used by the workflows. |
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 7 changed files in this pull request and generated no new comments.
Suppressed comments (1)
.github/workflows/build-failure-analysis.md:130
mcp-servers.binlog-mcp.allowed: ["*"]grants the agent access to every tool thebinlog-mcpMCP server exposes. Even with a digest-pinned container, this is a broader capability surface than necessary and makes futurebinlog-mcptool additions automatically callable by the agent. Consider narrowing this allowlist to only the specificbinlog_*tools the analyst is expected to use (and/or a supported wildcard that only matchesbinlog_*).
binlog-mcp:
container: "mcr.microsoft.com/dotnet-buildtools/prereqs:azurelinux-3.0-binlog-mcp-amd64"
mounts:
- "/tmp/binlogs:/data/binlogs:ro"
allowed: ["*"]
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 7 changed files in this pull request and generated no new comments.
Suppressed comments (2)
.github/workflows/build-failure-analysis.md:519
- The per-artifact binlog staging counter
iis only incremented on a successfulcp. If onecpfails and the artifact contains multiple*.binlogfiles, the next file will reuse the same destination name (e.g.${ai}_0_...) and can overwrite a previously staged binlog, contradicting the "destinations unique" comment and potentially hiding the real failing leg.
# on `needs.fetch-binlog.outputs.binlog-found == 'true'`, these only run once
# binlogs have been retrieved from the failed Azure DevOps build.
steps:
- name: Download analysis artifact
uses: actions/download-artifact@v8.0.1
.github/workflows/build-failure-analysis-command.md:460
- The per-artifact binlog staging counter
iis only incremented whencpsucceeds. If an artifact contains multiple*.binlogfiles and an early copy fails, subsequent files can reuse the same${ai}_${i}_...destination name and overwrite staged data, which can make the analysis incomplete or misleading.
# Info-ZIP prepends warnings on STDOUT for a recoverable archive,
# and a multi-line value would still pass the `grep -qE` check
# below, since `grep -q` matches if ANY line matches. `timeout`
# bounds a hostile archive; pipefail + fail-closed because a killed
# probe's partial output can end in a numeric column and undercount.
|
Addressed both review comments in b785d9c by using sanitized artifact names for every workflow-command warning. A subsequent fork E2E run also exposed and fixed the generated safe-output target expression in 0e84cd0. Full E2E proof: https://github.com/YuliiaKovalova/runtime/actions/runs/32471218479 |
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Addressed the Runtime artifact-layout feedback in
Fork E2E: https://github.com/YuliiaKovalova/runtime/actions/runs/33157134057 The run replayed Runtime build 1561838 and selected 15 of 68 build-log artifacts (previous exact matching selected 9), including all six display-only suffix cases. It staged 74 binlogs from all 15 artifacts, used hlx timeline evidence for the unmatched
Both source workflows compile cleanly with |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 7 changed files in this pull request and generated no new comments.
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
.github/workflows/build-failure-analysis.md:487
- The PIPESTATUS checks after
unzip -Z1 | grep -qE ...can misclassify the "suspicious path" case as an unzip failure. Whengrep -qmatches early it closes the pipe andunzipmay exit non-zero due to SIGPIPE; checkingzscan_rc[0]first will then hit the "could not list" branch instead of the intended "suspicious entry path" branch.
if [ "${zscan_rc[0]}" -ne 0 ]; then
echo "::warning::Skipping ${safe_name}: could not list archive entries (unzip -Z1 rc=${zscan_rc[0]})."; continue
fi
if [ "${zscan_rc[1]}" -eq 0 ]; then
echo "::warning::Skipping ${safe_name}: archive has a suspicious (absolute or ..) entry path."; continue
.github/workflows/build-failure-analysis-command.md:541
- The PIPESTATUS checks after
unzip -Z1 | grep -qE ...can misclassify the "suspicious path" case as an unzip failure. Whengrep -qmatches early it closes the pipe andunzipmay exit non-zero due to SIGPIPE; checkingzscan_rc[0]first will then hit the "could not list" branch instead of the intended "suspicious entry path" branch.
if [ "${zscan_rc[0]}" -ne 0 ]; then
echo "::warning::Skipping ${safe_name}: could not list archive entries (unzip -Z1 rc=${zscan_rc[0]})."; continue
fi
if [ "${zscan_rc[1]}" -eq 0 ]; then
echo "::warning::Skipping ${safe_name}: archive has a suspicious (absolute or ..) entry path."; continue
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
`curl --retry` can only rewind seekable output. Through a pipe or a command substitution a retry appends to whatever the failed attempt already wrote, so a *successful* retry produces a corrupt two-response payload: - The artifact download streamed through `head -c` into the zip. A 503 error page followed by a successful retry yielded `<error page><zip>`, which could still pass the size and path guards and then make `unzip` return warning status 1 -- read as failure, leaving staged_legs short and suppressing the whole analysis. It now downloads with `-o` to a freshly removed file, behind `--fail` so HTTP error bodies never reach it and `ulimit -f` as the disk backstop. - `ado_get` and the completeness-check timeline fetch captured curl in a command substitution. A partial body plus a successful retry parsed as neither document, so a recoverable blip was reported as a data-resolution failure. Both write to a file that curl truncates before each attempt. Tests assert the invariant rather than the text: no retrying curl may be piped or captured, every one writes to a file, and every one uses `--fail`. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 7 changed files in this pull request and generated no new comments.
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
.github/workflows/build-failure-analysis.md:56
- This workflow currently has no fork guard. Other agentic workflows in this repo use a root-level
if: (!github.event.repository.fork)to avoid running (and attempting to use the PAT pool / safe-outputs writeback) in forks. Adding the fork guard here prevents unexpected runs/costs/failures when someone enables the workflow in a fork.
# A Runtime compile failure does not always publish a `Logs_Build_*` artifact.
# Activate once the failed build is verified; `binlog-mcp` handles available
# binlogs and `hlx` provides the bounded Azure DevOps task-log fallback.
if: needs.fetch-binlog.outputs.analysis-ready == 'true'
The Azure DevOps response and the artifact archive were written to fixed paths under /tmp. On a runner that is not exclusively ours -- self-hosted, or simply running two of these jobs at once -- a pre-existing file or symlink at a known path decides where curl writes and what gets read back into ADO_DOC. Both now come from `mktemp`. The download reuses one scratch file across artifacts and truncates it per iteration rather than removing and recreating it, so the private file we were handed stays the one we write to. BINLOG_DIR keeps its fixed path deliberately: it is an interface, set by the workflow and read by the upload step, and it is created and cleared rather than written through. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 7 changed files in this pull request and generated no new comments.
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
.github/workflows/build-failure-analysis-command.md:224
- The comment says to keep this logic in sync with
on.command.name, but this workflow’s trigger config useson.slash_command.name(and it’s in the frontmatter, not “below”). This mismatch makes future maintenance error-prone when renaming the command.
# KEEP IN SYNC with `on.command.name` below.
- `ulimit -f` was allowed to fail silently. A shell that refuses to apply
the limit left responses with no usable Content-Length free to fill the
disk before the post-download size check ran. It now exits the subshell,
so the leg fails and the completeness check reports it.
- testfx/sdk discarded the transfer exit status, so a timed-out or
size-limited curl was accepted whenever the partial file happened to
parse as a ZIP -- including a file left exactly at the cap, which the
`-gt` check does not reject. The status is captured and the leg skipped.
- `{"records": null}` satisfied `has("records")` and marked the timeline
readable; the extraction then yielded an empty missing-legs value that
looked like verified completeness. Requires an actual array.
- The extraction directory was a fixed /tmp path, so the mktemp reasoning
applied to it too. It is `mktemp -d`, cleaned per artifact and released
after the loop.
Also corrects a guard comment that still described the `head -c` pipe the
download no longer uses.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The counter is charged from the file left on disk, but the comment claimed it recorded "the bytes that actually crossed the wire". Since `-o` truncates before each retry, failed attempts are not counted, so the comment overstated what the number means to anyone auditing the caps. It is a disk and extraction budget. Network transfer is bounded by DOWNLOAD_DEADLINE through the `timeout` wrapper, and every individual attempt is capped at ZIP_CAP by `ulimit -f`. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Two ways the download guards were weaker than advertised. bash's `ulimit -f` counts in 1024-byte units, not the POSIX 512-byte block. Computing blocks as `(ZIP_CAP + 511) / 512` therefore asked for twice the intended limit, so a single scratch file could reach nearly 4 GB against a 2 GB ZIP_CAP -- past the 3 GB cumulative cap on its own. The post-download size check still rejected the file, but the backstop that exists for responses with no usable Content-Length was not enforcing what it claimed. The wall-clock deadline bounded transfers only. Extraction is separately bounded per artifact, so a run that spent most of its budget downloading could still queue one extraction per remaining artifact and walk past `timeout-minutes` without reaching the controlled no-op. Extraction now reads the same deadline, stops when it is gone, and clamps its own timeout to what is left. DOWNLOAD_DEADLINE is renamed FETCH_DEADLINE because it now bounds the whole phase. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 7 changed files in this pull request and generated no new comments.
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
.github/workflows/build-failure-analysis-command.md:131
- This comment says
activationrequiresbinlog-found == 'true', but the generated workflow actually gates activation onanalysis-ready == 'true'(binlog download can be absent, with hlx as the fallback). This is misleading for troubleshooting why the agent ran without staged binlogs.
# `pre_activation` remains the authoritative role + command-position check,
# and `activation` additionally requires `binlog-found == 'true'`.
bash counts `ulimit -f` in 1024-byte units, but in POSIX mode it counts 512-byte blocks -- so the same arithmetic means two different caps depending on how the runner's shell was invoked. Under POSIX mode the 1024-based value would halve the limit and truncate downloads that fit, which drops a leg and suppresses the analysis. `set +o posix` pins it. Artifact names come from Azure DevOps metadata and are echoed straight into `::warning::`, where a crafted name could forge workflow commands in the log. A sanitized copy already existed for this reason; these call sites were still using the raw value. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 7 changed files in this pull request and generated no new comments.
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
.github/workflows/build-failure-analysis.md:482
- The comment says anything left in /tmp/binlogs from an earlier run would otherwise be uploaded and attributed to this build, but the cleanup only removes
*.binlog. Any other leftover files would still be uploaded with the artifact. Consider clearing the directory contents instead of only binlogs to match the stated invariant.
# Only binlogs extracted by this run may be analyzed. Anything left in
# the directory by an earlier run on the same runner would otherwise be
# uploaded and attributed to this build.
rm -f /tmp/binlogs/*.binlog
.github/workflows/build-failure-analysis-command.md:537
- The comment says anything left in /tmp/binlogs from an earlier run would otherwise be uploaded and attributed to this build, but the cleanup only removes
*.binlog. Any other leftover files would still be uploaded with the artifact. Consider clearing the directory contents instead of only binlogs to match the stated invariant.
# Only binlogs extracted by this run may be analyzed. Anything left in
# the directory by an earlier run on the same runner would otherwise be
# uploaded and attributed to this build.
rm -f /tmp/binlogs/*.binlog
| EVENT_NAME: ${{ github.event_name }} | ||
| CHECK_DETAILS_URL: ${{ github.event.check_run.details_url }} | ||
| CHECK_HEAD_SHA: ${{ github.event.check_run.head_sha }} | ||
| CHECK_PR_NUMBER: ${{ github.event.check_run.pull_requests[0].number }} |
There was a problem hiding this comment.
This silently disables automatic analysis for fork-origin PRs. GitHub check runs for fork heads can have check_run.pull_requests=[], so this produces an empty PR number, analysis-ready=false, and no safe-output target even though the workflow declares roles: all. Please derive the PR number from the fetched Azure Pipelines metadata (triggerInfo["pr.number"] or refs/pull/<N>/merge), bind it to the event-owned check_run.head_sha via triggerInfo["pr.sourceSha"], retain the current PR/head validation, expose it from fetch-binlog, and use that output for both safe-output targets.
Note
This review comment was generated by GitHub Copilot.
| # kill an on-demand analysis a maintainer had just asked for. Each still | ||
| # collapses its own repeat invocations for a PR. | ||
| group: build-failure-analysis-cmd-${{ github.event.issue.number || github.event.pull_request.number || fromJSON(github.event.inputs.aw_context || github.event.client_payload.aw_context || '{}').item_number || github.run_id }} | ||
| cancel-in-progress: true |
There was a problem hiding this comment.
This concurrency group is acquired for every created or edited issue comment before the slash command is matched. As a result, an unrelated comment can cancel an active /analyze-build-failure run; the new run then fails command activation and leaves no replacement analysis. Please set cancel-in-progress: false here and handle genuine duplicate commands through idempotency instead of broad event-level cancellation.
Note
This review comment was generated by GitHub Copilot.
| # is needed because JS `.trim()` and `\s` treat CR as whitespace while | ||
| # awk's default field splitting does not. | ||
| # KEEP IN SYNC with `on.command.name` below. | ||
| first_word=$(printf '%s' "${COMMENT_BODY}" | tr -d '\r' | awk 'NF {print $1; exit}') |
There was a problem hiding this comment.
This pre-gate does not match the generated activation predicate. awk 'NF {print $1; exit}' skips leading spaces and blank lines, but the pinned gh-aw matcher tests the untrimmed comment and requires the command at byte zero. Such a comment passes this gate and pays for the ADO artifact download, then is guaranteed to be rejected during activation. Please make this predicate exactly match the generated slash-command predicate and update the explanatory comment accordingly.
Note
This review comment was generated by GitHub Copilot.
Summary
runtimedefinition (129) and download only exact failed/canceled-jobLogs_Build_*artifactsbinlog-mcpcontainerValidation
gh-aw v0.86.2using--validate1561838; selected 9 exact failed-job build artifacts without the successfulNativeAOT_Librariesprefix collisionsExample
The example of the output given by the workflow:
dotnet/arcade#17348 (comment)
microsoft/testfx#10637 (comment)
Full fork E2E run