Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
175 changes: 175 additions & 0 deletions .github/workflows/pr-review-approach.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,175 @@
name: PR Approach Review

on:
pull_request_target:

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ Info: Running approach reviews only on opened (not synchronize or reopened) makes sense—the overall approach typically doesn't change between commits. The code review workflow handles subsequent pushes.

types: [opened]

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium: The PR description states "Remove legacy workflow files" but no files were actually deleted in this PR. The pr-review-axrun.yml was renamed to pr-review-code.yml, not removed. Consider updating the PR description to accurately reflect the changes:

  • Rename pr-review-axrun.yml to pr-review-code.yml
  • Add pr-review-approach.yml
  • Remove legacy workflow files (this didn't happen)

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium: This approach review only triggers on . If you expect it to re-run when the PR changes, include / (like the code review workflow). If the intent is 'one-time approach review', this is fine.

workflow_dispatch:
inputs:
pr_number:
description: "PR number to review"
required: true
type: number

env:
AXRUN_ALLOW: "read,write,glob,grep,bash:*"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Critical: Same security vulnerability as noted in pr-review-code.yml. bash:* allows execution of any command, and pull_request_target provides the secrets. This combination allows malicious PRs to exfiltrate secrets via prompt injection.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Critical: Same issue here: is fully permissive (). In combination with + checking out PR head and reading untrusted PR files, this materially increases the risk of prompt-injection leading to secret exfiltration. The comment 'safe in GitHub Actions sandboxed environment' is misleading when secrets are present.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 High: Security Risk: Granting bash:* permission in a pull_request_target workflow that checks out the PR head (ref: ${{ steps.pr.outputs.head_sha }}) is dangerous.

If the agent decides to execute a script from the PR (e.g., for "exploration" or "reproduction"), it runs in the context of the base repo with access to secrets (AXVAULT, etc.). A malicious PR could include a script that exfiltrates these secrets.

Recommendation:

  1. Remove bash:* if not strictly needed for an "approach" review.
  2. If execution is needed, do not use pull_request_target with an unsafe checkout. See GitHub Security Lab: Preventing pwn requests.


jobs:
review:
if: github.event_name == 'workflow_dispatch' || github.event.pull_request.head.repo.fork == false
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 High: bypasses the fork safety check for manual runs. A maintainer can accidentally this workflow against a fork PR number, which would then run with secrets and check out untrusted fork code/content. Suggest adding an explicit guard after resolving PR info (e.g., or ) and when it’s a fork/cross-repo PR.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium: Logic: This condition restricts the workflow to internal PRs only (fork == false).

If the goal of using pull_request_target was to support external contributors (forks) with access to secrets, this disables it.
If the goal is strictly internal reviews, pull_request (non-target) might be safer and sufficient, as internal branches typically have access to secrets anyway.

Recommendation: Verify if external contributors are intended to be supported. If so, this check prevents it (but fixing the security issue above is a prerequisite for enabling forks).

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Critical: The job-level if allows workflow_dispatch runs unconditionally (github.event_name == 'workflow_dispatch' || ...fork == false, line 18). In the dispatch path, the workflow resolves an arbitrary PR number and then checks out that PR’s head SHA (lines 47-60). This means a maintainer can accidentally run the workflow (with secrets enabled) against a fork PR, defeating the fork guard and executing untrusted code with secret access. This is the exact risk class called out for pull_request_target workflows: https://securitylab.github.com/resources/github-actions-preventing-pwn-requests/.

Fix: after resolving PR_NUMBER, query PR metadata (e.g., gh pr view $PR_NUMBER --json headRepository --jq '.headRepository.isFork') and fail/exit for forks (or run a “no-secrets” mode for fork PRs).

runs-on: ubuntu-latest
permissions:
contents: read
pull-requests: write
actions: read
strategy:
fail-fast: false
matrix:
include:
- agent: claude
display_name: Claude
model: opus
vault_credential: ci-oauth-token
- agent: gemini
display_name: Gemini
model: gemini-3-pro-preview
vault_credential: ci-oauth-credentials
- agent: copilot
display_name: Copilot
model: gpt-5.2
vault_credential: ci-oauth-token
name: approach (${{ matrix.agent }}, ${{ matrix.model }})
steps:
- name: Resolve PR info
id: pr
env:
GH_TOKEN: ${{ github.token }}
run: |
if [ "${{ github.event_name }}" = "workflow_dispatch" ]; then
PR_NUMBER="${{ inputs.pr_number }}"
else
PR_NUMBER="${{ github.event.pull_request.number }}"
fi
echo "number=$PR_NUMBER" >> $GITHUB_OUTPUT
HEAD_SHA=$(gh pr view $PR_NUMBER --repo ${{ github.repository }} --json headRefOid --jq '.headRefOid')
echo "head_sha=$HEAD_SHA" >> $GITHUB_OUTPUT

- name: Checkout PR head
uses: actions/checkout@v6
with:
ref: ${{ steps.pr.outputs.head_sha }}
fetch-depth: 10
Comment on lines +56 to +60

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Handle forked PRs when workflow_dispatch is used

Because the job if allows workflow_dispatch regardless of fork status, maintainers can manually run this workflow for forked PRs. In that case gh pr view (lines 47–54) will return a head_sha that exists only in the fork, but actions/checkout here uses the base repository by default with just ref, so the checkout will fail with an unknown ref when the commit is not in the base repo. If manual runs are intended to support forked PRs, also capture headRepository from gh pr view and pass it as repository: (or restrict workflow_dispatch to non‑forks).

Useful? React with 👍 / 👎.


- name: Setup Node.js
Comment on lines +56 to +62

Check failure

Code scanning / CodeQL

Checkout of untrusted code in trusted context High

Potential execution of untrusted code on a privileged workflow (
pull_request_target
)
uses: actions/setup-node@v6
with:
node-version: "22"

- name: Install ${{ matrix.agent }}
run: >-
NPM_CONFIG_REGISTRY=https://registry.npmjs.org
npm_config_userconfig=/dev/null
npm_config_strict_ssl=true
npm_config_proxy=
npm_config_https_proxy=
npx -y axinstall@latest ${{ matrix.agent }} --with npm

- name: Run ${{ matrix.display_name }} Approach Review
env:
GH_TOKEN: ${{ github.token }}
AXVAULT: ${{ secrets.AXVAULT }}
PERPLEXITY_API_KEY: ${{ secrets.PERPLEXITY_API_KEY }}
PR_NUMBER: ${{ steps.pr.outputs.number }}
run: |
cat > /tmp/prompt.md << 'PROMPT_EOF'
# Approach Review

You review PRs at a high level. Your only job: **Is there a better way to solve this problem?**

## What You Do

1. Understand what problem the PR solves (read PR description, explore the codebase)
2. Consider if there's a fundamentally better approach
3. If yes, explain the alternative. If no, say "approach looks good"

## What You Don't Do

- Find bugs (there's another review for that)
- Suggest small improvements or optimizations
- Comment on code style, naming, or formatting
- Nitpick implementation details

## When to Suggest an Alternative

Only suggest a different approach when:
- A standard library or existing codebase utility already does this
- The solution is significantly more complex than necessary
- There's a well-known pattern that fits better
- The approach will cause obvious scaling or maintenance issues

Don't suggest alternatives when:
- It's just a different way to do the same thing with similar trade-offs

## Context

- **Repository**: ${{ github.repository }}
- **PR Number**: __PR_NUMBER__

## How to Post Your Review

1. Get commit SHA:
```bash
gh pr view __PR_NUMBER__ --json headRefOid --jq '.headRefOid'
```

2. Write review to `/tmp/review.json`:
```bash
cat > /tmp/review.json << 'REVIEWJSON'
{
"commit_id": "COMMIT_SHA_HERE",
"event": "COMMENT",
"body": "**Approach Review:** [Your assessment]\n\n---\n\n_Approach review by ${{ matrix.display_name }} (${{ matrix.model }})_",
"comments": []
}
REVIEWJSON
```

If suggesting an alternative approach, add it to the `comments` array attached to the most relevant changed line:
```json
"comments": [
{"path": "src/file.ts", "line": 10, "side": "RIGHT", "body": "💡 **Alternative approach:** [Your suggestion]"}
]
```

3. Post once:
```bash
gh api repos/${{ github.repository }}/pulls/__PR_NUMBER__/reviews --method POST --input /tmp/review.json
```

## Examples

**Good feedback:**
- "This reimplements `lodash.debounce` - consider using the existing dependency"
- "This polling approach could be replaced with the existing WebSocket connection"
- "The codebase already has a `BaseValidator` class that handles this pattern"
- "Consider using TypeScript instead of JavaScript - it would catch the type errors this PR is trying to fix at compile time"

**Not helpful (don't say these):**
- "You could use a Map instead of an object here" (minor implementation detail)
- "Consider extracting this into a separate function" (refactoring preference)
- "This variable name could be clearer" (detail, not approach)
PROMPT_EOF

# Substitute PR number placeholder
sed -i "s/__PR_NUMBER__/$PR_NUMBER/g" /tmp/prompt.md

NPM_CONFIG_REGISTRY=https://registry.npmjs.org \
npm_config_userconfig=/dev/null \
npm_config_strict_ssl=true \
npm_config_proxy= \
npm_config_https_proxy= \
npx -y axrun@latest --agent ${{ matrix.agent }} \
${{ matrix.provider && format('--provider "{0}"', matrix.provider) || '' }} \
--model ${{ matrix.model }} \
--vault-credential ${{ matrix.vault_credential }} \
--allow '${{ env.AXRUN_ALLOW }}' \
--prompt "$(cat /tmp/prompt.md)"
Loading
Loading