Let the build-failure analyst push fixes it cannot suggest inline

[YuliiaKovalova]

Why

GitHub only accepts inline suggestion blocks on lines that are part of a PR's diff. Dependency-flow PRs break exactly that assumption: their diff is nothing but version files, so when a flowed package changes an API and previously-untouched call sites stop compiling, the build-failure analyst can only describe the fix and ask a human to commit it.

Concrete example that motivated this: #17348 (comment) — the analysis correctly identified two CS1503s in PublishBuildToMaestro.cs (a flowed Microsoft.DotNet.ProductConstructionService.Client inserted a parameter before CancellationToken) but had to end with "the fix needs to be committed separately", because that file is not in the dependency-bump diff.

What

Enable the push-to-pull-request-branch safe output on the automatic build-failure-analysis workflow (the /analyze-build-failure command workflow is deliberately left comment-only), so the agent can append the fix commit when — and only when — a suggestion structurally cannot carry it.

Guardrails

• The push target is not the model's to choose. With target: "*" gh-aw takes the pull request number out of the agent's own tool call and only then checks whether that pull request is a fork, so an injected agent could redirect the push at an unrelated same-repo PR. The target is instead bound to check_run.pull_requests[0].number, which GitHub fills in itself and which never passes through the model.
• Fork PRs can never be pushed to. GitHub leaves check_run.pull_requests empty for fork-originated check runs, so a fork PR has no push target at all (verified against live check runs in all three repos: same-repo PRs report exactly one entry, fork ones report none) — and gh-aw's handler refuses fork branches regardless. That bounds roles: all to same-repo branches: dependency flow plus people who already have write access. Pushes are append-only; force-push is impossible. The comment-only analysis still runs on fork PRs.
• allowed-files is an exclusive allowlist, so build infrastructure (eng/, global.json, NuGet.config, .github/) is out of reach no matter what the agent produces; protected-files keeps its default blocked policy on top.
• max: 1 plus a loop guard enforced entirely in trusted code. commit-title-suffix makes gh-aw's push handler stamp [build-failure-analysis] onto the commit title as it applies the patch, so the marker is written by the handler and never by the model. This requires patch-format: am: the default bundle transport never rewrites commit titles, which would have made the marker — and therefore the guard — a silent no-op. Before anything else runs, the fetch job reads the PR's head commit; if that tip is already such a commit and the build still fails, it publishes push-blocked, and the workflow's job-level if: skips the activation and agent jobs — gh-aw's safe_outputs job is itself conditioned on the agent not being skipped, so no push code path remains. The check fails closed (an unreadable commit blocks) and is scoped to the branch tip, so any later commit by anyone restores full analysis rather than abandoning the PR after one attempt. The push uses GITHUB_TOKEN, which does not re-trigger GitHub Actions — but the Azure DevOps GitHub app does rebuild, so this guard is the real brake on a fail → push → rebuild → fail loop.
• Step 6b of the analyst playbook additionally requires the fix to be outside the diff, mechanical, and provable from the compiler error itself. Anything needing a design decision, changing behavior, or suppressing an analyzer stays a comment.
• fallback-as-pull-request: false so a diverged branch cannot silently become a surprise PR.

Supporting changes

• PR-head checkout. Authoring the commit needs the PR's tree — push-to-pull-request-branch ships file contents, so a fix authored against main would silently revert whatever else changed in that file. The fetch job now resolves pr-checkout-ref: the head branch name for same-repo PRs (gh-aw derives the push target from git rev-parse --abbrev-ref HEAD, so a detached SHA checkout would break bundle generation), falling back to refs/pull/<n>/head for forks.
• Prompt-injection mitigation. gh-aw's own base-branch config restore is gated on its built-in PR-checkout step, which never fires for check_run (that event carries no pull_request payload). Checking out the PR head therefore puts PR-controlled agent-config content in the workspace — where the analyst reads its own playbook. A second sparse checkout of the base branch plus a pre-agent-steps step restores it before the agent starts, replaying gh-aw's inline sub-agent/skill restores afterwards. That checkout uses the PR's own base ref (resolved from the GitHub API by the fetch job), not the repository default branch, so a release-branch PR is analysed with the playbook that branch actually carries. The restore covers gh-aw's full protected set — folders .agents .antigravity .claude .codex .crush .gemini .github .opencode .pi and root files .crush.json .mcp.json AGENTS.md ANTIGRAVITY.md CLAUDE.md GEMINI.md PI.md opencode.jsonc — including paths that do not exist on the base branch, which are deleted rather than left as PR-authored content.
• No new secret is required. GH_AW_CI_TRIGGER_TOKEN appears in the lock because gh-aw wires that magic secret into every workflow using this safe output; it only exists to add an empty commit under a PAT so GitHub Actions CI re-runs on the pushed commit. It is deliberately left unset — our CI is Azure DevOps, which rebuilds on its own — and when unset the value is empty and gh-aw simply skips that step.
• No PR code is built or executed. The workflow's bash allowlist gains only scoped git status/diff/log/rev-parse/add/commit — no interpreters, package managers or build tools. Note that gh-aw itself appends git branch/checkout/merge/rm/switch to the generated --allow-tool list whenever push-to-pull-request-branch is enabled; that cannot be narrowed from the workflow config, so the analyst playbook forbids those commands explicitly. git push is not injected — the agent can never write to the remote, and the push is performed by the safe_outputs job from a bundle of the agent's local commits.

Validation

• Compiled with the pinned gh-aw compiler version, --strict, clean.
• Lock diff reviewed: no action pin changes; contents: write is added only to the safe_outputs and conclusion jobs, the agent job stays least-privilege.
• Verified git bundle of the incremental commit range works from the shallow agent checkout.

Residual risks

• Azure DevOps rebuilds on the pushed commit; the marker guard bounds it to one automated attempt.
• Fix commits land on the PR branch as verified bot commits and still require human review — the agent is required to say so in its summary comment.

[Copilot]

Pull request overview

This PR enhances the automatic build-failure-analysis workflow so the build-failure analyst can append a mechanical fix commit to the PR branch when an inline GitHub suggestion is structurally impossible (because the fix is in a file not present in the PR diff), while adding guardrails and PR-head checkout to ensure fixes are authored against the correct revision and to mitigate prompt-injection risks from PR-controlled agent configuration.

Changes:

• Enable push-to-pull-request-branch as a safe output for the automatic build-failure analysis workflow (command workflow remains comment-only).
• Update checkout behavior to use the PR head (when applicable) and restore trusted agent configuration from the base branch before the agent runs.
• Extend shared workflow guidance and the build-failure analyst playbook with a “Step 6b” path for when a fix must be delivered as a commit.

Reviewed changes

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

Show a summary per file

File	Description
.github/workflows/shared/build-failure-analysis-shared.md	Documents the new escape hatch (push_to_pull_request_branch) and when to use it.
.github/workflows/build-failure-analysis.md	Implements PR-head checkout, base-branch agent-config restore, and enables the push safe output + git authoring tooling.
.github/workflows/build-failure-analysis.lock.yml	Updates the compiled lockfile to reflect the new checkout/pre-agent steps and push safe output wiring.
.github/workflows/build-failure-analysis-command.lock.yml	Updates the compiled lockfile for the command workflow (kept comment-only).
.github/agents/build-failure-analyst.agent.md	Updates the analyst playbook with Step 6b instructions and guardrails for committing/pushing mechanical fixes.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

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:192

• pre-agent-steps intentionally overwrites .github/ / .agents/ with base-branch copies, leaving the worktree dirty. Because the agent is allowed to run git add/git commit, an accidental broad stage (e.g. git add -A) would include these restored files and then get rejected by allowed-files: src/**, wasting the run. Consider marking the restored config paths as skip-worktree to make accidental staging much harder.

      # The restored files differ from the PR head, so leave them staged-free and
      # let git see them as modifications: the agent only ever commits the single
      # source file it fixes, and gh-aw builds its patch from commits, never from
      # the dirty worktree. Fail loudly if that assumption ever breaks.
      git -c core.fileMode=false status --porcelain -- .github .agents AGENTS.md | head -n 20 || true

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

• The agent bash allowlist includes git log:*, but the playbook explicitly says not to use git log for the loop guard (shallow checkout) and it isn't needed for staging/committing a mechanical fix. Keeping the allowlist minimal reduces the chance of the agent relying on misleading local history and narrows the command surface.

    - "git status:*"
    - "git diff:*"
    - "git log:*"
    - "git rev-parse:*"
    - "git add:*"

[Copilot]

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:660

• The outputs block writes two echo statements on a single line, which will concatenate push-blocked and ado-build-id into one malformed output value and likely break downstream needs.fetch-binlog.outputs.* consumers. Split these into separate lines so each output key is written correctly.

            echo "pr-checkout-ref=${CHECKOUT_REF}"
            echo "push-blocked=${PUSH_BLOCKED}"            echo "ado-build-id=${BUILD_ID}"
            echo "ado-build-url=${ADO_BUILD_UI}?buildId=${BUILD_ID}"

.github/workflows/build-failure-analysis.lock.yml:1865

• This generated lock has the same issue as the source workflow: push-blocked and ado-build-id are emitted by two echos on a single line, which will produce a malformed $GITHUB_OUTPUT entry at runtime. Regenerate the lock after fixing the source workflow, or at minimum split these into separate lines here as well.

            echo "pr-checkout-ref=${CHECKOUT_REF}"
            echo "push-blocked=${PUSH_BLOCKED}"            echo "ado-build-id=${BUILD_ID}"
            echo "ado-build-url=${ADO_BUILD_UI}?buildId=${BUILD_ID}"

[YuliiaKovalova]

Addressed the latest review round (including the suppressed comments, which contained the most important findings).

The loop guard did not actually work, so it was rebuilt in trusted code.
The pre-commit hook lived in ${RUNNER_TEMP}/gh-aw-refuse-commits, but the agent firewall only mounts ${RUNNER_TEMP}/gh-aw and the workspace — git saw a nonexistent core.hooksPath and would have committed anyway, and even mounted, the allowed git commit:* permits --no-verify. Anything installed inside the agent sandbox is a suggestion, not a guarantee. The hook is gone. Instead:

• The fetch job publishes push-blocked and the workflow's job-level if: consumes it. When it is true, the activation and agent jobs never run, and gh-aw's safe_outputs job — conditioned on the agent not being skipped — is skipped with them, so no push code path remains. The /analyze-build-failure command workflow is comment-only and ignores the output.
• The [build-failure-analysis] marker is now stamped by commit-title-suffix, i.e. by gh-aw's push handler while it applies the patch. The guard no longer depends on the model writing (or correctly spelling) its own marker, and the playbook explicitly tells the agent not to add one.

The check fails closed now. It previously ran gh api … 2>/dev/null | grep -q, so a transient API error silently produced "not blocked". It defaults to blocked and clears only after the tip commit was read successfully. It also resolves the tip from the PR's head.sha instead of the ambient HEAD_SHA, which can hold the check run's merge commit — that alone would have made the guard never fire. Fork PRs are exempt, since gh-aw refuses to push to them and the guard must not suppress their comment-only analysis.

Scoped to the branch tip, not the whole history, so one automated attempt does not disable analysis on that PR forever: any later commit by anyone restores it.

Real bug fixed. Two echos had been joined onto one line in the fetch job's outputs block, which would have written a malformed $GITHUB_OUTPUT entry and lost ado-build-id entirely. Good catch.

No duplicate summary comment. Step 5 posts exactly one; it is now posted after the push is requested and describes the commit as requested and pending rather than completed, since the push runs in a later job and can still be rejected.

[Copilot]

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:138

• The workflow claims to restore agent config from the PR’s base branch, but the second checkout (and BASE_BRANCH env) are wired to github.event.repository.default_branch. For PRs targeting release/*, this can restore the wrong version of .github/ / playbook files (from main), changing the analyst instructions and mitigation behavior compared to the actual base branch.

Consider checking out the PR base ref resolved from PR_JSON (already computed as BASE_REF) and using that ref for .gh-aw-base-config + logging, with a fallback to the default branch if resolution fails.

checkout:
  - ref: ${{ needs.fetch-binlog.outputs.pr-checkout-ref }}
  - ref: ${{ github.event.repository.default_branch }}
    path: .gh-aw-base-config
    fetch-depth: 1

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

• The comment says “Fail loudly if that assumption ever breaks”, but the command is explicitly made non-fatal with || true and only prints status output. This makes the comment misleading about what the step enforces.

Either remove the “Fail loudly” wording, or actually enforce a failure condition (e.g., detect staged changes outside the intended file(s)).

      # The restored files differ from the PR head, so leave them staged-free and
      # let git see them as modifications: the agent only ever commits the single
      # source file it fixes, and gh-aw builds its patch from commits, never from
      # the dirty worktree. Fail loudly if that assumption ever breaks.
      git -c core.fileMode=false status --porcelain -- .github .agents AGENTS.md | head -n 20 || true

[YuliiaKovalova]

Round 4 of review feedback, applied to all three repos (arcade #17353, testfx #10607, sdk #55807).

1. The push target was model-controlled. With target: "*", gh-aw's handler resolves the pull request number supplied in the agent's own tool call and only then checks whether that pull request is a fork — so an injection could have aimed the push at an unrelated same-repo PR, escaping both the fork boundary and the loop guard. The target is now bound to check_run.pull_requests[0].number, straight from GitHub's webhook payload.

Because GitHub leaves that field empty for fork-originated check runs, this removes the push target entirely on fork PRs while leaving the comment-only analysis untouched. Verified against live check runs in all three repos: same-repo PRs report exactly one entry, fork PRs report none.

2. commit-title-suffix was a silent no-op. patch-format defaults to bundle, and the handler only rewrites commit titles on the git am path. The [build-failure-analysis] marker would never have been stamped, so the loop guard added in round 3 would never have fired. Pinned to patch-format: am, verified as "patch_format":"am" in the regenerated locks.

3. The agent-config restore used the wrong branch. It checked out github.event.repository.default_branch, so a release-branch PR was analysed with main's playbook and agent instructions. It now uses the PR's own base ref, which the fetch job already resolves from the GitHub API, falling back to the default branch if that lookup returns nothing.

4. Target/guard consistency. The fetch job prefers the ADO build's source branch when resolving the PR number. If that ever disagreed with the check payload, the guard and the push would be scoped to different branches, so the run is now refused outright in that case.

5. Reworded a comment that claimed a deliberately non-fatal diagnostic listing would "fail loudly".

All locks recompiled --strict with 0 errors and 0 warnings — including the target: "*" warning gh-aw used to emit, which the bound target removes.

[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.md:439

• The loop guard treats any commit subject containing [build-failure-analysis] as an automated fix. Because the marker is intended to be an appended suffix from the handler, a substring match can false-positive on unrelated commits (and will then skip all analysis via push-blocked). Tighten the check to only match the expected suffix at end-of-subject.

            if printf '%s' "${TIP_SUBJECT}" | grep -qF '[build-failure-analysis]'; then

[YuliiaKovalova]

Round 5. One finding this time (arcade, suppressed): the loop guard matched [build-failure-analysis] anywhere in the tip commit subject, which can false-positive on an unrelated commit that quotes the marker. The suggested fix was to anchor the match to the end of the subject.

Tightened, but deliberately not anchored — anchoring would have introduced a fail-open bug.

gh-aw appends the suffix by rewriting the first Subject: line of a git format-patch mbox, and git folds subjects longer than ~72 characters onto continuation lines. git am then reassembles the title with the marker in the middle. Verified end to end, using real git format-patch output rewritten with the handler's own regex:

SUBJECT: Fix CS1503 after [build-failure-analysis] Microsoft.DotNet.ProductConstructionService.Client bump changed GetBuildAsync signature at both call sites
ends with marker? False
contains " [marker]"? True

So an end-of-subject anchor would silently miss exactly the commits the guard exists to catch, and the fail → push → rebuild → fail loop would run unbounded. The asymmetry matters: a false positive skips one analysis (and any later commit on the branch restores it), while a false negative removes the brake entirely.

What did change:

• The match now requires the leading space the handler always inserts, so a subject that merely opens with the marker is no longer treated as an automated fix. That removes the plausible false positive without giving up the substring semantics.
• The reasoning is recorded next to the check, so it is not "simplified" into an anchored match later.
• The analyst playbook now asks for commit titles of 60 characters or fewer, so the marker lands at the end in practice and the title stays readable.

Also confirmed there is nothing outstanding elsewhere: the two sdk threads (git checkout/git merge in the generated lock, and GH_AW_CI_TRIGGER_TOKEN) were addressed in earlier rounds and are resolved, and testfx has no comments newer than my round-4 replies.

All locks recompiled --strict: 0 errors, 0 warnings. testfx pin audit passes (2181 references across 47 files).

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[Copilot]

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:780

• The bash allowlist includes git log:*, but the playbook explicitly says not to use git log for the loop guard (shallow checkout) and the commit-authoring flow doesn’t require git log. Dropping it reduces unnecessary capability surface and keeps the allowlist aligned with the documented workflow.

    - "git status:*"
    - "git diff:*"
    - "git log:*"
    - "git rev-parse:*"

.github/agents/build-failure-analyst.agent.md:222

• The guidance to keep the commit subject to “60 characters or fewer” doesn’t actually prevent folding once commit-title-suffix: " [build-failure-analysis]" is appended (the suffix itself is ~25 chars). If the goal is to avoid subject folding at ~72 chars, the pre-suffix subject limit should be ~47 chars (or lower).

   Do **not** add a `[build-failure-analysis]` marker yourself. The workflow configures `commit-title-suffix`, so the safe-outputs job appends the marker to the commit title as it applies the patch. That is deliberate: the loop guard must not depend on the model remembering — or correctly spelling — a marker. Keep the first line to 60 characters or fewer: the marker is appended to it, and git folds longer subject lines when the commit travels as a patch, which strands the marker in the middle of the title.

[YuliiaKovalova]

Round 6 — all six suppressed findings triaged; five applied, one respectfully disputed with evidence.

Applied

1. Tip-scoped guard vs. "no earlier commit on the branch" (3 findings on Engineering documentation for epic hand-off (done done done) #10607, same root cause). Correct and the most serious of the batch. The guard reads only PR_JSON.head.sha's subject, so a human commit on top legitimately re-enables the escape hatch — but the shared playbook, the analyst instructions and a workflow comment all still said "no earlier [build-failure-analysis] commit on the branch" / "no previous automated attempt". Since those files are injected into the prompt, that phrasing could have taught the analyst to refuse every push after the first, permanently disabling the feature. All three now describe the tip-only check and tell the agent it does not need to check history at all.

2. Token comment (#55807). Correct. The push runs with secrets.GH_AW_GITHUB_TOKEN || secrets.GITHUB_TOKEN — verified in the generated lock — so a repo that sets GH_AW_GITHUB_TOKEN to a PAT/App token does get Actions re-runs. The comment now states both cases and notes this is a further reason the guard lives in a trusted job.

3. git log:* allowlist entry (Let the build-failure analyst push fixes it cannot suggest inline #17353). Correct — dropped. The checkout is depth-1 so it could only ever show the tip, and the playbook already forbade using it to reconstruct the guard.

Disputed — the 60-character commit-title guidance

The suggestion was to lower the limit to ~47 because the ~25-char suffix is appended. That would be right if the suffix were part of the commit before git format-patch ran, but it is not. From actions/setup/js/push_to_pull_request_branch.cjs at the pinned v0.77.5, the suffix is applied to the already-generated patch text:

patchContent.replace(/^Subject: (?:\[PATCH\] )?(.*)$/gm,
  (match, title) => `Subject: [PATCH] ${title}${commitTitleSuffix}`);

So folding is decided by format-patch on the bare title; the suffix never competes for that budget. Measured threshold, not estimated:

title length	Subject: line	folded?
60	77	no
61	78	no
62	—	yes

61 is the exact limit (Subject: [PATCH] = 17 chars of overhead against a 78-column header), so 60 is correct with one character of margin; 47 would have cost usable title space for no benefit.

That said, the original sentence was ambiguous enough to invite exactly this reading, so I rewrote it to say explicitly that the budget is for the title alone, that the marker is appended afterwards to the generated patch, and that the real constraint is the Subject: [PATCH] prefix folding at 62.

Note this is also why the round-5 guard match is deliberately unanchored: if a title does fold, git am reassembles it with the marker mid-string, so anchoring to end-of-subject would fail open.

All three repos recompiled with their pinned compilers — 0 errors, 0 warnings. testfx pin audit passes (2274 refs / 49 files).

[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/shared/build-failure-analysis-shared.md:70

• The new guidance around push_to_pull_request_branch doesn’t explicitly state that the PR number should not be supplied in the tool call. Since this workflow intentionally binds the push target from the trusted check_run payload (to avoid a model-chosen target), it’d be clearer to tell the analyst to omit any PR-number parameter when using the push tool.

     lines). If — and only if — `push_to_pull_request_branch` is available to
     you, follow **Step 6b** of the playbook to append a mechanical fix commit
     to the PR branch. Every one of its conditions must hold (same-repo PR,
     `src/` only, provable from the compiler error); otherwise just describe