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.

[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.