Address review feedback on the build-failure-analysis pre-gate

[YuliiaKovalova]

Follow-up to #17228. Resolves the five review threads @Evangelink left on that PR, plus a bug found while validating the change and one separate concurrency issue in the same files.

E1 — the role_name union was a security widening (thread on line 133)

You were right, and my code comment was self-contradictory: it claimed role_name "reports the precise role (so maintain and triage stay distinct)" and that permission alone would miss maintainers. Both cannot be true.

The REST docs for this endpoint say it plainly:

The permission attribute provides the legacy base roles of admin, write, read, and none, where the maintain role is mapped to write and the triage role is mapped to read. The role_name attribute provides the name of the assigned role, including custom roles.

So .permission in {admin, write} is exactly "has push access or better" — precisely the set roles: [admin, maintainer, write] describes, maintainers included. My stated rationale for adding role_name was factually wrong, and consulting it only added the risk you identified: a custom organization role merely named like a privileged one (a custom maintainer inheriting read) would clear the gate with no push access. Reverted to .permission only.

One nuance worth recording, since it cuts the other way from "exactly equivalent": gh-aw's own check (v0.83.1, actions/setup/js/check_permissions_utils.cjs) prefers role_name, normalizes maintainer → maintain, and falls back to inherited_role for custom roles. So .permission-only is a slightly stricter early-out than the authoritative pre_activation check that follows it. That is the safe direction for a pre-gate — it can cost a legitimate user a skipped download, never grant one access — so I've gone with your version.

Bug found while validating this. Your suggested snippet used gh api ... --jq '.permission'. Live-testing it showed it does not fail closed cleanly: on a non-2xx response gh prints the error document to stdout, and --jq does not filter it, so perm became a multi-line raw JSON blob that then got echoed into the workflow log. Kept the two-step shape instead, which yields "" for any error shape:

resp=$(gh api "repos/${GITHUB_REPOSITORY}/collaborators/${COMMENTER}/permission" 2>/dev/null)
perm=$(printf '%s' "${resp}" | jq -r '.permission // empty' 2>/dev/null)
case "${perm}" in admin|write) authorized=true ;; *) authorized=false ;; esac

Exercised against the live GitHub API: admin → allow; read → deny; github-actions[bot] → rejected before the call; empty login → rejected; foo/../../bar → rejected; nonexistent user → deny ('none').

E2 — contains() is a substring match (thread on line 93)

Documented, as you suggested. The comment now states that contains() matches anywhere in the body while the authoritative check_command_position requires a valid position, so a write-access user merely mentioning the command — or editing an old comment that quotes it, since types: includes edited — still starts the job and pays for the download before pre_activation discards the result. It stays a deliberate over-approximation: if: expressions have no regex, and startsWith would reject the leading whitespace/newlines gh-aw accepts. It can cost a runner; it can never grant access.

E3 — PR-only scoping is not compiler-enforced (thread on line 90)

Documented. github.event.issue.pull_request is what keeps plain issue comments out; gh-aw emits no such filter of its own despite events: [pull_request_comment] (verified in the generated lock). Noted that it degrades safely without it — repos/.../pulls/<issue#> 404s and no binlog is emitted — but it would pay for a runner first.

E4 — authorization restated in three places (thread on line 110)

Added KEEP IN SYNC with roles: notes to both hand-written gates, spelling out that editing roles: does not update them because only pre_activation is generated from the frontmatter. Agreed the underlying footgun is upstream — custom needs: jobs are scheduled ahead of pre_activation, so the only way to gate on cost is to restate the policy by hand.

E5 — COMMENTER unvalidated; missing shell: (thread on line 125)

COMMENTER now gets the same shape check PR_NUMBER and BUILD_ID already had (grep -qE '^[A-Za-z0-9-]+$'), rejecting bot logins like github-actions[bot] and empty values before the API call. shell: bash added to every hand-written run: step in both workflows.

Separate concern: colliding concurrency groups

Not from the review, but in the same files. Both workflows resolved their group to build-failure-analysis-<pr>. Groups are repository-global and cancel-in-progress is true, so an automatic analysis and an on-demand /analyze-build-failure for the same PR cancelled each other. Reproduced live in a fork: a command run had pre_activation/activation/agent/safe_outputs cancelled when an automatic run for the same PR number started. The command workflow now uses build-failure-analysis-cmd-<pr>; each still collapses its own repeat invocations.

Verification

• gh aw compile --strict with v0.77.5 (matching this repo's existing locks): 0 errors, 0 warnings
• bash -n clean on every generated run: block
• .gitattributes / actions-lock.json compile side-effects reverted
• the gate exercised against the live GitHub API (6 cases above)

The same two changes are going into microsoft/testfx#10401 and dotnet/sdk#55539, which carry ports of this workflow.

---

Second round: propagated from dotnet/sdk#55539 (commit 74664bb)

@Evangelink left six more comments on the dotnet/sdk port. Three of them apply to arcade, and two more are real here but cannot be expressed on the gh-aw version this repo pins, so they are documented in place instead.

Applied

The command-position check now runs before the download. This is E2 above, but actually fixed rather than only documented. The job-level if: can only use contains(), so a comment that merely mentions /analyze-build-failure — or an edited old comment quoting it — still reached fetch-binlog and paid for the ~600MB download before pre_activation threw the result away. The gate now reproduces gh-aw's rule in bash before the permission API call:

first_word=$(printf '%s' "${COMMENT_BODY}" | tr -d '\r' | awk 'NF {print $1; exit}')
if [ "${first_word}" != "/${COMMAND_NAME}" ]; then ... exit 0; fi

That mirrors actions/setup/js/slash_command_matcher.cjs, which runs /^\/([a-zA-Z0-9][a-zA-Z0-9._-]*)(?=$|\s)/ over the trimmed body and then compares the captured name. tr -d '\r' is required because JS .trim() and \s treat CR as whitespace while awk's default field splitting does not. I differential-tested the bash against the real regex on 20 inputs (leading whitespace, blank first lines, CRLF, /analyze-build-failure-now, mentions mid-body, ::-prefixed text): 20/20 identical verdicts. The reported token is sanitised with tr -cd 'A-Za-z0-9/._-' before being echoed, since the raw first token is attacker-controlled and ::-prefixed text is interpreted by the runner as a workflow command.

/analyze-build-failure now posts with target: "triggering". Both add-comment and create-pull-request-review-comment were target: "*", which lets the agent name its own destination. With min-integrity: none the agent reads binlogs produced from an unmerged, possibly external PR, so a prompt-injected agent could redirect its comment to any issue or PR in the repo. triggering pins the destination to the PR the comment was posted on. The automatic workflow keeps "*" deliberately: it is triggered by check_run, which is not an issue/PR context, and its PR number comes from the Azure DevOps build rather than from the event.

Removed a non-existent argument from the analyst agent. The instructions showed binlog_warnings { binlog_file: "<path>", top: 10 }. I installed Microsoft.AITools.BinlogMcp 2.0.1 and spoke JSON-RPC to it over stdio: binlog_warnings accepts exactly binlog_file (required), project and code, and no tool among the 40 accepts top. Calling with and without top returns byte-identical output, so this was misleading documentation rather than a failing call. Replaced with a pointer to the code/project filters that do exist.

Documented, because v0.77.5 cannot express them

The binlog-mcp container is not digest-pinned. The tag genuinely moves: it resolved to sha256:9f1e2c3e8281… from 2026-07-16 until 2026-08-03, when it became sha256:ee7b7e5c6e16… (confirmed against MCR). This container receives the binlogs of an unmerged PR and its output is what the agent reports, so the bare tag is a supply-chain decision made by whoever last pushed it. It cannot be fixed here: v0.77.5 validates container against ^[a-zA-Z0-9][a-zA-Z0-9/:_.-]*$, which has no @, so image@sha256:… is rejected at compile time, and there is no separate digest field. gh-aw >= v0.83.x resolves and pins the digest automatically — microsoft/testfx on v0.84.3 emits digest + pinned_image and pulls by @sha256:. Documented above the container: line in both workflows so the next compiler bump picks it up.

hide-older-comments cannot supersede across the two workflows. Hiding is scoped to the posting workflow's id (GH_AW_WORKFLOW_ID, the file stem — verified as build-failure-analysis and build-failure-analysis-command in the two locks), so each workflow only ever hides its own comments and a /analyze-build-failure re-run leaves the stale automatic analysis visible beside the fresh one. dotnet/sdk and microsoft/testfx fix this with the object form hide-older-comments: {enabled: true, match: [...]}, but the v0.77.5 schema types hide-older-comments as a plain templatable_boolean with no object variant and no match. Documented in both files.

Not applicable

The Azure DevOps attempt de-duplication from the sdk PR does not apply: this repo matches ^Logs_Build_, and real arcade builds publish no _Attempt<N> suffix on those names.

Verification for this commit

• gh aw compile --strict with v0.77.5: 0 errors, 0 warnings; .gitattributes / actions-lock.json / update-default-versions.lock.yml side-effects reverted
• bash -n clean on all 91 generated run: blocks
• compiled target reads "triggering" in the command lock and "*" in the automatic lock
• concurrency groups still distinct
• lock diff reviewed line by line: only the intended changes plus gh-aw's hash/heredoc-marker churn

[Copilot]

Pull request overview

This PR updates the Build Failure Analysis GitHub Actions workflows (and their gh-aw generated lock files) to incorporate prior review feedback, tighten authorization logic for the pre-download “cost gate”, and prevent cross-workflow cancellation via concurrency group collisions.

Changes:

• Tighten the command workflow’s pre-gate authorization to rely on the GitHub REST permission field only (avoiding a potential security widening via role_name) and validate COMMENTER before using it in API paths/logs.
• Add explicit shell: bash to hand-written run: steps to ensure consistent shell behavior.
• Split concurrency groups between automatic vs slash-command workflows to avoid them canceling each other for the same PR.

Reviewed changes

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

File	Description
.github/workflows/build-failure-analysis.md	Adds explicit shell: bash to hand-written steps for consistent execution.
.github/workflows/build-failure-analysis.lock.yml	Regenerates lock output reflecting updated shell settings and metadata.
.github/workflows/build-failure-analysis-command.md	Fixes pre-gate permission logic, validates commenter login, documents intentional over-approximation, and separates concurrency group.
.github/workflows/build-failure-analysis-command.lock.yml	Regenerates lock output reflecting updated concurrency, shell settings, and permission gate logic.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

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

Suppressed comments (1)

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

• The “KEEP IN SYNC” note references on.command.name “below”, but the workflow trigger is on.slash_command.name and it appears above this step. This comment is misleading when someone updates the command name later.

          # KEEP IN SYNC with `on.command.name` below.
          first_word=$(printf '%s' "${COMMENT_BODY}" | tr -d '\r' | awk 'NF {print $1; exit}')