Skip to content

Gate the build-failure binlog download on the commenter's write access - #10364

Open
YuliiaKovalova wants to merge 3 commits into
microsoft:mainfrom
YuliiaKovalova:harden-build-failure-analysis
Open

Gate the build-failure binlog download on the commenter's write access#10364
YuliiaKovalova wants to merge 3 commits into
microsoft:mainfrom
YuliiaKovalova:harden-build-failure-analysis

Conversation

@YuliiaKovalova

Copy link
Copy Markdown
Contributor

Problem

The slash command's fetch-binlog job has no if: gate at all.

gh-aw's role check (roles: [admin, maintainer, write]) is compiled into the
pre_activation job, and that job is generated with needs: fetch-binlog
(build-failure-analysis-command.lock.yml). So authorization runs after
the binlogs have already been downloaded — ~220 MB compressed / 1.2 GB
extracted for a typical build. route_slash_command.cjs in the central
dispatcher performs no permission check either, so any commenter on any PR
could trigger the full download.

Inverting the dependency would create a cycle, so the check is repeated as a
pre-gate inside fetch-binlog.

Because the command uses strategy: centralized it is started via
workflow_dispatch and has no github.event.comment payload, so the actor
is read from aw_context.actor — the same place item_number already
comes from. role_name and permission are both consulted: role_name
keeps maintain/triage distinct, while permission still reports push
access for custom org roles. Any API failure returns a document with neither
field and lands in the deny branch.

Also backported from the dotnet/sdk port (dotnet/sdk#55539)

  • Missing-leg detection. Failed legs that published no logs artifact are
    passed to the agent as GH_AW_MISSING_LEGS so it cannot conclude
    no build failure from the legs that happened to upload.
    Matching is token-based, not prefix-based: timeline job names and
    artifact names are not spelled alike — Linux Debug publishes
    Logs_Build_Linux_Debug but Windows Debug publishes
    Logs_Build_Windows_NT_Debug. A prefix rule would report a missing leg on
    every Windows failure.
  • Download bounds. At most 40 artifacts and 3 GB compressed in total. The
    existing caps were per-artifact plus a cumulative uncompressed budget, so
    many mid-sized archives could still be pulled over the network first.
  • unzip exit 11 (no files matched) is no longer reported as an
    extraction failure — the leg published logs, they just hold no binlog.
  • An artifact with no download URL now warns instead of being silently skipped.

Validation — all in real runs, nothing assumed

Permission gate, executed in Actions on this branch:

Path Result
write access 'YuliiaKovalova' has 'admin' access …; proceeding. — download ran
no write access 'octocat' does not have write access … (resolved role 'read')Download binlogs => skipped

Full chain against real failed build 1534452 (PR #10358), run end-to-end in
Actions: Extracted 6 binlog(s) from 6/6 legs, no false missing-leg
warning for Windows Debug, GH_AW_MISSING_LEGS exported empty, and the
agent read all 6 binlogs via binlog-mcp and correctly called noop
("All 6 build legs … compiled cleanly — non-build failure"), which is right:
that build failed in integration tests, not compilation.

The new bounds were also exercised against the same real build: forcing the
compressed budget to 100 MB stops at leg 4 and fails closed; forcing the
artifact cap to 2 refuses the build; a build with no Logs_Build_* artifacts
emits binlog-found=false.

Note on the actions/checkout pin

The regenerated locks keep actions/checkout on the v7.0.1 SHA recorded
in .github/aw/actions-lock.json. Compiling on the repo's pinned v0.83.1
toolchain rewrites it to v7.0.0 — that is #10258, and it reproduces on
untouched workflows too (I confirmed by recompiling link-checker), so the
pin is realigned rather than regressed, exactly as #10174 did.
python .github/scripts/check_action_pins.py passes: 2076 references across
45 files
.

Compiled with gh aw compile --strict (v0.83.1): 0 errors, 0 warnings.

The slash command's `fetch-binlog` job had no `if:` at all. gh-aw's role
check (`roles: [admin, maintainer, write]`) is compiled into the
`pre_activation` job, and that job is generated with `needs: fetch-binlog`,
so it only runs *after* the binlogs are downloaded -- ~220 MB compressed /
1.2 GB extracted for a typical build. `route_slash_command.cjs` in the
central dispatcher performs no permission check either, so any commenter on
any PR could trigger the full download. Inverting the dependency would create
a cycle, so the check is repeated as a pre-gate inside `fetch-binlog`.

Because the command uses `strategy: centralized` it is started via
`workflow_dispatch` and has no `github.event.comment` payload, so the
actor is read from `aw_context.actor` (the same place `item_number`
already comes from). `role_name` and `permission` are both consulted:
`role_name` keeps `maintain` and `triage` distinct while `permission`
still reports push access for custom org roles. Any API failure yields a
document with neither field and lands in the deny branch.

Also backports the hardening from the dotnet/sdk port:

* Detect failed legs that published no logs artifact and pass them to the
  agent as `GH_AW_MISSING_LEGS`, so it cannot conclude `no build failure`
  from the legs that happened to upload. Timeline job names and artifact names
  are not spelled alike -- `Linux Debug` publishes `Logs_Build_Linux_Debug`
  but `Windows Debug` publishes `Logs_Build_Windows_NT_Debug` -- so the
  match is token-based rather than prefix-based.
* Bound the download: at most 40 artifacts and 3 GB compressed in total. The
  existing caps were per-artifact plus a cumulative *uncompressed* budget, so
  many mid-sized archives could still be pulled over the network first.
* Warn instead of silently skipping an artifact with no download URL.
* Stop reporting `unzip` exit 11 (`no files matched`) as an extraction
  failure -- the leg published logs, they just hold no binlog.

The regenerated locks keep `actions/checkout` on the v7.0.1 pin recorded in
.github/aw/actions-lock.json. Compiling on the pinned v0.83.1 toolchain
rewrites it to v7.0.0; that is microsoft#10258 and reproduces on
untouched workflows too, so the pin is realigned rather than regressed.
`python .github/scripts/check_action_pins.py` passes.
Copilot AI review requested due to automatic review settings July 31, 2026 16:11

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds authorization and resource safeguards to build-failure binlog analysis.

Changes:

  • Gates slash-command downloads on write access.
  • Detects missing legs and improves archive handling.
  • Adds download limits and regenerates workflows.

Reviewed changes

Copilot reviewed 5 out of 5 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 Documents missing-leg handling.
.github/workflows/build-failure-analysis.md Adds detection and download safeguards.
.github/workflows/build-failure-analysis.lock.yml Regenerates the automatic workflow.
.github/workflows/build-failure-analysis-command.md Adds the permission gate and safeguards.
.github/workflows/build-failure-analysis-command.lock.yml Regenerates the command workflow.
Suppressed comments (2)

.github/workflows/build-failure-analysis.md:365

  • The cumulative cap is bypassed by oversized artifacts: each one downloads MAX_ZIP_BYTES + 1, then continues before incrementing TOTAL_ZIP_BYTES. With 40 oversized artifacts this can transfer roughly 20 GB despite the intended 3 GB total bound. Compute the remaining total budget before curl, cap head to that remaining amount, and account every downloaded byte before any per-artifact skip.
            if [ $((TOTAL_ZIP_BYTES + ZIP_BYTES)) -gt "${MAX_TOTAL_ZIP_BYTES}" ]; then
              echo "::warning::Cumulative compressed download budget ${MAX_TOTAL_ZIP_BYTES} reached at ${name}; stopping."; break
            fi
            TOTAL_ZIP_BYTES=$((TOTAL_ZIP_BYTES + ZIP_BYTES))

.github/workflows/build-failure-analysis-command.md:342

  • The cumulative cap is bypassed by oversized artifacts: each one downloads MAX_ZIP_BYTES + 1, then continues before incrementing TOTAL_ZIP_BYTES. With 40 oversized artifacts this can transfer roughly 20 GB despite the intended 3 GB total bound. Compute the remaining total budget before curl, cap head to that remaining amount, and account every downloaded byte before any per-artifact skip.
            if [ $((TOTAL_ZIP_BYTES + ZIP_BYTES)) -gt "${MAX_TOTAL_ZIP_BYTES}" ]; then
              echo "::warning::Cumulative compressed download budget ${MAX_TOTAL_ZIP_BYTES} reached at ${name}; stopping."; break
            fi
            TOTAL_ZIP_BYTES=$((TOTAL_ZIP_BYTES + ZIP_BYTES))

Comment thread .github/workflows/build-failure-analysis.md Outdated
Comment thread .github/workflows/build-failure-analysis-command.md Outdated
The missing-leg detection turned every timeline failure into an empty list,
which is indistinguishable from `every failed leg published its logs`. A
failed request, an HTML error page and an ADO error document all took that
path, so a transient outage could leave the agent concluding `non-build
failure` from an artifact set whose completeness was never established --
the exact case the detection exists to prevent.

Probe for the `records` array before trusting the response and emit an
explicit `(unknown ...)` marker otherwise. The shared prompt now tells the
agent to treat that marker like a non-empty missing-leg list.
Copilot AI review requested due to automatic review settings July 31, 2026 16:26

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

Suppressed comments (2)

.github/workflows/build-failure-analysis.md:379

  • The cumulative limit is enforced only after the artifact has already been downloaded, and downloads rejected by the per-artifact check are never added to TOTAL_ZIP_BYTES. Consequently, 40 oversized artifacts can still transfer roughly 20 GB, while a normal artifact can overshoot the 3 GB limit by up to 500 MB. Cap head -c using the remaining cumulative allowance and account every downloaded byte before any continue.
            if [ $((TOTAL_ZIP_BYTES + ZIP_BYTES)) -gt "${MAX_TOTAL_ZIP_BYTES}" ]; then
              echo "::warning::Cumulative compressed download budget ${MAX_TOTAL_ZIP_BYTES} reached at ${name}; stopping."; break
            fi
            TOTAL_ZIP_BYTES=$((TOTAL_ZIP_BYTES + ZIP_BYTES))

.github/workflows/build-failure-analysis-command.md:356

  • The cumulative limit is enforced only after the artifact has already been downloaded, and downloads rejected by the per-artifact check are never added to TOTAL_ZIP_BYTES. Consequently, 40 oversized artifacts can still transfer roughly 20 GB, while a normal artifact can overshoot the 3 GB limit by up to 500 MB. Cap head -c using the remaining cumulative allowance and account every downloaded byte before any continue.
            if [ $((TOTAL_ZIP_BYTES + ZIP_BYTES)) -gt "${MAX_TOTAL_ZIP_BYTES}" ]; then
              echo "::warning::Cumulative compressed download budget ${MAX_TOTAL_ZIP_BYTES} reached at ${name}; stopping."; break
            fi
            TOTAL_ZIP_BYTES=$((TOTAL_ZIP_BYTES + ZIP_BYTES))

@Evangelink Evangelink left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed the whole change (both .md sources, both regenerated locks, and the shared prompt) at 0426f61.

The headline change — the write-access pre-gate — is sound and correctly implemented. It defaults to deny, every failure path (empty actor, gh error, 404, unparseable JSON) lands in the deny branch, it is ordered after gh-aw's injected Configure GH_HOST step so gh api targets the right host, and the actor is passed via env: rather than interpolated into run:. I also checked the pin claim independently: no uses: line changes in either lock, and python .github/scripts/check_action_pins.py passes on this branch (2076 references across 45 files). The duplicated blocks in the command and non-command workflows are byte-identical — no drift. The timeline_ok probe added in the latest push is the right call: an unreadable timeline reporting as "nothing missing" was the sharpest edge of the missing-leg feature.

My remaining concerns are all in the backported part, not the permission gate. Two I'd want addressed before merge; the rest are smaller. Details inline; below is what does not map to a diff line.


GH_AW_MISSING_LEGS is missing from the agent playbook's variable table

shared/build-failure-analysis-shared.md step 1 adds GH_AW_MISSING_LEGS to the list the agent must read, then tells it to cat .github/agents/build-failure-analyst.agent.md. That file's table (lines 25-33) is introduced with "The caller also sets the environment variables below. You must read all of them before doing anything else." — and the new variable is not in it. Worth a row so the playbook stays authoritative, especially now that it carries a sentinel value ((unknown …) whose semantics are documented only in the shared prompt.

Prompt question: silent noop when legs are missing

When GH_AW_MISSING_LEGS is non-empty and every visible leg compiled cleanly, the guidance is "name them in your noop reason" — so the incomplete-picture warning never reaches a human on the PR at all. Deliberate, or would a short comment be better in that specific case? Genuine question.

Residual: only the role half of pre_activation is replicated

The pre-gate checks the role but not the command position, so a write-access user writing blah /analyze-build-failure still triggers the full download and is only refused afterwards by check_command_position. Fine trade-off, but a sentence in the comment block would stop a future reader from reading it as an oversight.

Structural alternative (take it or leave it)

A tiny authorize job that fetch-binlog declares needs: on would make the gate a first-class node in the job graph — visible in the Actions UI, skippable as a whole, no runner spun for the fetch — without recreating the pre_activation cycle. Functionally equivalent to the inline step; purely a legibility call.

Duplication

~75 lines are now copy-pasted verbatim across build-failure-analysis.md and build-failure-analysis-command.md. They are identical today, but this is the third block to be duplicated, and the latest push had to make the same timeline_ok edit twice. If gh-aw can share a steps: fragment the way imports: already shares the prompt, this seems like the moment.

Comment thread .github/workflows/build-failure-analysis-command.md Outdated
Comment thread .github/workflows/build-failure-analysis-command.md Outdated
Comment thread .github/workflows/build-failure-analysis-command.md Outdated
Comment thread .github/workflows/build-failure-analysis-command.md Outdated
Comment thread .github/workflows/build-failure-analysis-command.md
Comment thread .github/workflows/build-failure-analysis-command.md Outdated
…d budget

Evangelink's review found the missing-leg check misidentifies legs, and the
compressed-download budget does not hold. Both reproduce on real builds.

Leg matching guessed the artifact name from the timeline job's display name,
but arcade names the artifact from `\$(Agent.Os)` and `\$(Agent.JobName)`, which
are not what the job is called. Every rule tried mis-scored real builds: a
`MacOS` job publishes `..._Darwin_...`, and jobs outside the arcade template
(`Detect changed paths`, the `msbuild_cache_seed` stage, `Monitor Helix Jobs`)
publish nothing at all. Replaying 40 recent builds, 10 produced a false
missing-leg report, and every build with a genuinely failed macOS leg reported
two healthy legs as missing -- the case the review predicted and the one the
prompt turns into a hedged analysis.

Ask arcade's `Publish logs` task record instead. It answers the question
directly, so no spelling is inferred, and a failed job with no such task is
correctly not a logs-publishing leg. `canceled` and `abandoned` legs now count
alongside `failed`; they also finish without logs.

The compressed budget was charged after the per-artifact skip, but the bytes
are already on the wire by then, so an oversized artifact cost a full
MAX_ZIP_BYTES and was never counted. Simulated, 40 oversized legs pulled 20 GB
against a stated 3 GB bound; charging on receipt holds it to 3.5 GB and leaves
healthy builds untouched. Likewise `unzip` exit 11 fell through to the
uncompressed accounting though nothing was extracted, so a large binlog-free
archive could push a useful later leg out of budget -- it now skips.

Job display names come from PR-branch YAML, so the missing-leg string is
stripped of control characters and length-bounded before it reaches
`\$GITHUB_OUTPUT`/`\$GITHUB_ENV`, and it is written first so it cannot override
a later key. The gate's actor gets the same shape check `PR_NUMBER` already
had, and reports the permission fallback rather than 'none'.
Copilot AI review requested due to automatic review settings July 31, 2026 18:34

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

Suppressed comments (2)

.github/workflows/build-failure-analysis-command.md:385

  • The advertised 3 GB compressed-download cap is checked only after the current artifact has been fully streamed with the 500 MB per-artifact limit. If 2.9 GB has already been consumed, this downloads roughly another 500 MB before breaking, so total network usage can reach about 3.5 GB. Bound head -c by the remaining cumulative allowance (and stop before curl when none remains), then charge the bytes, so the abuse/cost cap is enforced before transfer.
            TOTAL_ZIP_BYTES=$((TOTAL_ZIP_BYTES + ZIP_BYTES))
            if [ "${TOTAL_ZIP_BYTES}" -gt "${MAX_TOTAL_ZIP_BYTES}" ]; then
              echo "::warning::Cumulative compressed download budget ${MAX_TOTAL_ZIP_BYTES} reached at ${name}; stopping."; break

.github/workflows/build-failure-analysis.md:391

  • The advertised 3 GB compressed-download cap is checked only after the current artifact has been fully streamed with the 500 MB per-artifact limit. If 2.9 GB has already been consumed, this downloads roughly another 500 MB before breaking, so total network usage can reach about 3.5 GB. Bound head -c by the remaining cumulative allowance (and stop before curl when none remains), then charge the bytes, so the abuse/cost cap is enforced before transfer.
            TOTAL_ZIP_BYTES=$((TOTAL_ZIP_BYTES + ZIP_BYTES))
            if [ "${TOTAL_ZIP_BYTES}" -gt "${MAX_TOTAL_ZIP_BYTES}" ]; then
              echo "::warning::Cumulative compressed download budget ${MAX_TOTAL_ZIP_BYTES} reached at ${name}; stopping."; break

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants