Fixes an issue when a bot triggers a PR review

[derekmisler]

Summary

When a bot posts a /review comment on a PR, it was inadvertently triggering the manual-review workflow, causing an infinite loop or unintended review runs. This PR fixes that by filtering out bot-authored comments, switching from bare PR numbers to full PR URLs (which work correctly in detached HEAD checkouts), and guarding against artifact upload failures when no verbose log file was produced.

Changes

• .github/workflows/review-pr.yml: Added github.event.comment.user.type != 'Bot' guard to the manual-review job's if condition, preventing bot comments from triggering the /review workflow.
• review-pr/action.yml: Replaced bare $PR_NUMBER references with full $PR_URL (https://github.com/${REPO}/pull/${PR_NUMBER}) in all gh pr view and gh pr diff calls, fixing 403/resolution failures in detached HEAD (forked PR) checkouts. Also switches the diff pre-fetch to use github.token (which has contents:read) instead of the App token.
• action.yml: Added a steps.run-agent.outputs.verbose-log-file != '' check to the "Upload verbose agent log" step so it only runs when a log file was actually produced, avoiding spurious upload failures.
• review-pr/agents/pr-review.yaml: Updated agent instructions to reflect that the prompt now contains a full PR URL instead of a bare number, and to prefer pre-fetched pr_metadata.json/changed_files.txt before falling back to gh pr view.

How to Test

• Post a /review comment as a bot account on an open PR and confirm the manual-review workflow does not trigger.
• Post a /review comment as a human user on a forked PR and confirm the workflow runs successfully, pre-fetches the diff using the full URL, and posts a review without a 403 error.
• Trigger a workflow run where no verbose log is produced and confirm the "Upload verbose agent log" step is skipped cleanly without an error.

[derekmisler]

/describe

[docker-agent]

✅ PR description has been generated and updated!

[docker-agent]

Review Summary

✅ Approved - No issues found.

This PR successfully addresses the bot authorization issue by:

• Adding a bot filter to prevent bot-triggered reviews (github.event.comment.user.type != 'Bot')
• Improving token usage by separating read permissions (github.token) from review posting permissions (app token)
• Fixing detached HEAD issues by using full PR URLs instead of just PR numbers
• Adding proper conditional checks for artifact uploads

The changes follow GitHub Actions best practices and include clear documentation updates. All potential concerns were verified and found to be properly handled.

[derekmisler]

/review

[docker-agent]

Review Summary

✅ This PR effectively addresses the bot-triggered infinite review loop issue with well-structured changes. However, I've identified two notable concerns regarding the token handling and fallback mechanism implementation that should be addressed.

Key Changes Reviewed

• ✅ Bot filtering in workflow conditions is correct
• ⚠️ Token override approach needs refinement
• ⚠️ Fallback mechanism messaging could be more accurate
• ✅ Artifact upload guard is correctly implemented
• ✅ Documentation updates are clear and helpful

[docker-agent]

Token override may not work reliably due to inline environment variable shadowing

The inline GH_TOKEN="$READ_TOKEN" override, while technically functional in bash, creates potential issues:

1. The outer env block (lines 144-146) already sets GH_TOKEN to the App token, creating confusion about which token is active
2. Inline environment variable overrides are fragile and non-standard in GitHub Actions
3. If the command fails or doesn't properly use the overridden token, there's a risk of token leakage

Recommendation: Use a subshell or separate run block with explicit token scoping:

- name: Pre-fetch diff with read token
  env:
    GH_TOKEN: ${{ github.token }}
  run: |
    if gh pr diff "$PR_URL" > pr.diff 2>/dev/null; then
      echo "✅ Pre-fetched PR diff"
    fi

This makes the token scope explicit and prevents confusion.

[derekmisler]

VAR=value command is standard POSIX shell — it temporarily overrides the env var for exactly that one process, then reverts. This is well-defined bash behavior, not a GitHub Actions quirk. Both tokens are already in the environment (GH_TOKEN via the env: block, READ_TOKEN alongside it), so there's no "token leakage" risk. A separate step would split the logically-related "get PR information" work for no real benefit.

[docker-agent]

Fallback mechanism mentioned but not guaranteed to work

The warning message states "agent will use git diff as fallback", but this fallback is not validated by the orchestration layer:

1. The code removes pr.diff on failure but doesn't verify that the git-based fallback will succeed
2. If git diff $(git merge-base origin/main HEAD)...HEAD also fails (e.g., detached HEAD without proper refs, shallow clone issues), there's no error recovery
3. The warning message implies a guaranteed recovery mechanism, but it's actually best-effort

Recommendation: Either:

• Soften the message: "agent will attempt git diff as fallback"
• Or add validation: Test that the git fallback works before proceeding

else
  if git diff $(git merge-base origin/main HEAD 2>/dev/null || echo "main")...HEAD > /dev/null 2>&1; then
    echo "::warning::Failed to pre-fetch PR diff — agent will use git diff as fallback"
  else
    echo "::error::Cannot fetch PR diff and git fallback unavailable"
    exit 1
  fi
  rm -f pr.diff
fi
``` <!-- cagent-review -->

[derekmisler]

The suggestion to test the git fallback and exit 1 on failure is harmful — we don't want to block the entire review because a pre-fetch failed. The agent has its own fallback chain and should be given a chance to run.

[gtardif]

Ah, I better understand what might have happened:
I didn't have the issue until this morning on https://github.com/docker/pinata/pull/39588, after I hit /help to get some pinata PR commands.
(and I did not hit /review ).
as the output of /help the pinata bot displayed all commands, and that triggered the condition contains(github.event.comment.body, '/review').
So filtering out bot-authored comments should guard from this indeed.
(could check if there's a better way testing for ^/review at line start, but otherwise this is fine)