Skip to content

review-responder edits the default branch, not the PR branch, under workflow_dispatch #1047

@microsasa

Description

@microsasa

Summary

Under workflow_dispatch, the review-responder agent operates on the default branch (main), not the PR's head branch. It generates patches against main's file contents, which the post-agent job then pushes to the PR branch. If the PR branch has diverged from main for the file being edited, the edit is wrong (based on stale content) and may land cleanly on the PR but produce incorrect code — or fail to apply.

How it was found

While auditing checkout: config for security issue #92 (C5), I traced how the compiled lock file handles PR checkout. In .github/workflows/review-responder.lock.yml around the agent job:

- name: Checkout PR branch
  if: |
    github.event.pull_request || github.event.issue.pull_request
  uses: actions/github-script@...
  script: |
    const { main } = require('${{ runner.temp }}/gh-aw/actions/checkout_pr_branch.cjs');
    await main();

The if: guard evaluates against github.event fields populated only when the triggering event is pull_request or issue_comment. Our responder is dispatched via workflow_dispatch from pipeline-orchestrator.yml (gh workflow run review-responder.lock.yml -f pr_number="$PR"), so:

  • github.event.pull_request → undefined
  • github.event.issue.pull_request → undefined
  • The condition is false → step is skipped

Consequence: the initial actions/checkout (with no ref: override) pulls the default branch, and nothing later switches to the PR's head branch. The agent's working tree is main throughout.

Why it often looks like it works

Most review comments target files that haven't diverged between the PR branch and main (especially for young PRs or small diffs). Our aw-labeled PRs also tend to be short-lived and frequently rebased. So patches generated against main often apply cleanly to the PR branch and happen to contain the right content. This is coincidence, not correctness.

Failure modes

  1. Stale edits: responder "fixes" the file based on main's content; the PR branch has a different version of that file; the push lands a commit that partially reverts the PR's changes or misses context.
  2. File not on main: responder tries to edit a file introduced by the PR. On main it doesn't exist → the agent may hallucinate content or skip the edit with an error that gets swallowed.
  3. Hunk conflicts if the post-agent push_to_pull_request_branch handler tries to apply the patch on a ref where the surrounding lines no longer match.

Suggested fix (for discussion)

Option A — pass PR ref into the checkout explicitly:

# review-responder.md
checkout:
  ref: refs/pull/${{ inputs.pr_number }}/head
  fetch-depth: 1

Option B — add a manual git fetch + git checkout step driven by inputs.pr_number before the agent runs (pre-agent step).

Option C — upstream gh-aw: broaden the Checkout PR branch step's if: to also handle workflow_dispatch with a pr_number input convention.

Option A is the smallest local change.

Related

Severity

Medium. Currently a latent correctness bug, not a security issue. Most responder runs appear to produce reasonable output, but the mechanism is wrong and will fail silently on longer-lived PRs or files new to the PR.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions