Skip to content

Commit 6c7fc6a

Browse files
bpamiribpamiri
authored
ci(bot): enable wheels-bot review on fork PRs via hardened pull_request_target (#2871)
1 parent eda5c90 commit 6c7fc6a

4 files changed

Lines changed: 359 additions & 17 deletions

File tree

‎.github/workflows/bot-review-a-fork.yml‎

Lines changed: 192 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,192 @@
1+
name: Wheels Bot — Reviewer A (fork PRs)
2+
3+
# Fork PRs cannot use the standard `pull_request` Reviewer A path: GitHub does
4+
# not pass `vars` OR `secrets` to `pull_request` runs from a forked repository,
5+
# so the bot's `vars.WHEELS_BOT_ENABLED == 'true'` gate fails closed (the var
6+
# reads as empty) and the App token / ANTHROPIC_API_KEY would be absent anyway.
7+
# This workflow runs the *initial* Reviewer A review for maintainer-labeled
8+
# fork PRs via `pull_request_target`, which executes in the BASE-repo context
9+
# where vars + secrets are available.
10+
#
11+
# SECURITY — pull_request_target hardening (load-bearing, do not weaken):
12+
# * We check out the BASE branch ONLY. We never check out the fork's head and
13+
# never run any fork-controlled code. The local composite action
14+
# `./.github/actions/wheels-bot-skip-check` therefore always resolves to
15+
# trusted base code. Checking out the fork ref first would let a malicious
16+
# fork swap that action's implementation and run arbitrary code with the
17+
# bot's write-capable App token + ANTHROPIC_API_KEY (the classic
18+
# "pwn-request").
19+
# * The fork's commit OBJECTS are fetched (refs/pull/<n>/head) so the review's
20+
# read-only `git log/diff/show` work, but the working tree stays on base —
21+
# fetching objects executes nothing.
22+
# * persist-credentials:false keeps no token in .git/config.
23+
# * Gated on a maintainer-applied `bot-review` label: only users with write
24+
# access can apply labels, so a human vets the fork diff before the bot runs.
25+
# * Reviewer A's tool surface is read-only (gh + read-only git + Read/Grep/Glob).
26+
#
27+
# Downstream: when Reviewer A submits its review here, the existing
28+
# `bot-review-b.yml` (pull_request_review) and the `bot-review-a.yml`
29+
# issue_comment convergence path take over — both hardened in the same PR to
30+
# check out base, never the fork ref.
31+
32+
on:
33+
pull_request_target:
34+
types: [labeled, synchronize]
35+
branches: [develop]
36+
37+
permissions:
38+
# Keep the default GITHUB_TOKEN read-only (mirrors bot-review-a.yml). All
39+
# writes — posting the review, dismissing bogus reviews — go through the
40+
# App token, not this token. Minimizing the default token is extra
41+
# defense-in-depth for the pull_request_target context.
42+
contents: read
43+
44+
concurrency:
45+
# Shared with bot-review-a.yml so a fork review and an internal review for the
46+
# same PR number can never overlap. A PR is either fork or internal, so in
47+
# practice only one of the two workflows ever matches.
48+
group: wheels-bot-review-a-${{ github.event.pull_request.number }}
49+
cancel-in-progress: false
50+
51+
jobs:
52+
review:
53+
# Distinct from bot-review-a.yml's "Reviewer A": both workflows trigger for
54+
# a labeled fork PR (the pull_request one skips on the absent vars gate,
55+
# this one runs), so distinct check names keep the UI unambiguous.
56+
name: Reviewer A (fork)
57+
runs-on: ubuntu-latest
58+
timeout-minutes: 20
59+
# Fork PRs only, and only once a maintainer has applied the `bot-review`
60+
# label. `synchronize` re-reviews on new pushes while the label is present;
61+
# the skip-check idempotency marker prevents duplicate reviews on a SHA
62+
# already reviewed, so re-firing on unrelated label events is a safe no-op.
63+
if: |
64+
vars.WHEELS_BOT_ENABLED == 'true'
65+
&& github.event.pull_request.head.repo.fork == true
66+
&& contains(github.event.pull_request.labels.*.name, 'bot-review')
67+
steps:
68+
- name: Checkout BASE branch (trusted — never the fork ref)
69+
uses: actions/checkout@v6
70+
with:
71+
ref: ${{ github.event.pull_request.base.ref }}
72+
persist-credentials: false
73+
fetch-depth: 0
74+
75+
- name: Generate App token
76+
id: app-token
77+
uses: actions/create-github-app-token@v2
78+
with:
79+
app-id: ${{ secrets.WHEELS_BOT_APP_ID }}
80+
private-key: ${{ secrets.WHEELS_BOT_PRIVATE_KEY }}
81+
82+
- name: Resolve PR info
83+
id: pr
84+
env:
85+
# Pass event values through env (never interpolate ${{ }} straight
86+
# into the script body). Both are GitHub-generated — number is an
87+
# integer, head.sha a 40-char hex — but env + validation is the
88+
# defense-in-depth pattern. head.sha is the commit the review marker
89+
# keys off (#2848).
90+
PR_NUM: ${{ github.event.pull_request.number }}
91+
HEAD_SHA: ${{ github.event.pull_request.head.sha }}
92+
run: |
93+
set -euo pipefail
94+
if ! [[ "$PR_NUM" =~ ^[0-9]+$ ]]; then
95+
echo "::error::PR number is not numeric: $PR_NUM"
96+
exit 1
97+
fi
98+
if ! [[ "$HEAD_SHA" =~ ^[0-9a-fA-F]{7,40}$ ]]; then
99+
echo "::error::head SHA is not a hex commit id: $HEAD_SHA"
100+
exit 1
101+
fi
102+
echo "pr_num=${PR_NUM}" >> "$GITHUB_OUTPUT"
103+
echo "sha=${HEAD_SHA}" >> "$GITHUB_OUTPUT"
104+
105+
- name: Fetch PR head commit objects (read-only; never checked out)
106+
env:
107+
PR_NUMBER: ${{ steps.pr.outputs.pr_num }}
108+
run: |
109+
set -euo pipefail
110+
# Bring the fork's commit OBJECTS into the local repo so the review's
111+
# read-only `git log/diff/show <base>..<head-sha>` resolve. The working
112+
# tree stays on the trusted base branch; no fork code is executed.
113+
git fetch --no-tags origin "refs/pull/${PR_NUMBER}/head"
114+
115+
- name: Skip check
116+
id: gate
117+
uses: ./.github/actions/wheels-bot-skip-check
118+
with:
119+
target-type: pr
120+
target-number: ${{ steps.pr.outputs.pr_num }}
121+
# Initial review marker: `wheels-bot:review-a:<pr>:<sha>` (no suffix).
122+
marker-pattern: 'wheels-bot:review-a:${{ steps.pr.outputs.pr_num }}:${{ steps.pr.outputs.sha }}'
123+
github-token: ${{ steps.app-token.outputs.token }}
124+
125+
- name: Run Reviewer A
126+
if: steps.gate.outputs.skip == 'false'
127+
uses: anthropics/claude-code-action@v1
128+
with:
129+
allowed_bots: 'wheels-bot[bot],github-actions[bot]'
130+
anthropic_api_key: ${{ secrets.ANTHROPIC_API_KEY }}
131+
github_token: ${{ steps.app-token.outputs.token }}
132+
prompt: |
133+
/review-pr ${{ steps.pr.outputs.pr_num }} ${{ steps.pr.outputs.sha }}
134+
claude_args: |
135+
--model claude-sonnet-4-6
136+
--max-turns 250
137+
--allowedTools "Bash(gh:*),Bash(git log:*),Bash(git diff:*),Bash(git show:*),Bash(git grep:*),Bash(git status),Read,Grep,Glob"
138+
139+
# Post-submission guard (issue #2558), mirrored from bot-review-a.yml.
140+
# Auto-dismisses any wheels-bot review on this SHA that is too short or
141+
# missing the canonical marker (e.g. a CLI-probe placeholder). Runs on
142+
# always() so it still fires if the Claude step failed mid-session.
143+
- name: Validate Reviewer A output
144+
if: always() && steps.gate.outputs.skip == 'false'
145+
env:
146+
GH_TOKEN: ${{ steps.app-token.outputs.token }}
147+
PR_NUMBER: ${{ steps.pr.outputs.pr_num }}
148+
HEAD_SHA: ${{ steps.pr.outputs.sha }}
149+
run: |
150+
set -euo pipefail
151+
152+
reviews=$(gh api "repos/${GITHUB_REPOSITORY}/pulls/${PR_NUMBER}/reviews" --paginate \
153+
| jq -c --arg sha "$HEAD_SHA" \
154+
'[.[] | select(.user.login == "wheels-bot[bot]") | select(.commit_id == $sha) | select(.state == "APPROVED" or .state == "CHANGES_REQUESTED")]')
155+
156+
count=$(echo "$reviews" | jq 'length')
157+
if [[ "$count" == "0" ]]; then
158+
echo "::notice::No active wheels-bot reviews on ${HEAD_SHA} to validate"
159+
exit 0
160+
fi
161+
162+
dismissed=0
163+
while IFS= read -r row; do
164+
id=$(echo "$row" | jq -r '.id')
165+
body=$(echo "$row" | jq -r '.body')
166+
body_len=${#body}
167+
168+
if [[ "$body_len" -lt 200 ]] || ! grep -q 'wheels-bot:review-a' <<<"$body"; then
169+
echo "::warning::Dismissing bogus Reviewer A review id=${id} len=${body_len}"
170+
gh api -X PUT \
171+
"repos/${GITHUB_REPOSITORY}/pulls/${PR_NUMBER}/reviews/${id}/dismissals" \
172+
-f message="Auto-dismissed by Reviewer A guard: body is shorter than 200 characters or missing the canonical \`wheels-bot:review-a\` marker. See wheels-dev/wheels#2558 for context."
173+
dismissed=$((dismissed + 1))
174+
fi
175+
done < <(echo "$reviews" | jq -c '.[]')
176+
177+
if [[ "$dismissed" -gt 0 ]]; then
178+
guard_marker="wheels-bot:review-a-guard:${PR_NUMBER}:${HEAD_SHA}"
179+
existing=$(gh api "repos/${GITHUB_REPOSITORY}/issues/${PR_NUMBER}/comments" --paginate \
180+
| jq -r --arg m "$guard_marker" '[.[] | select(.body | contains($m))] | length')
181+
182+
if [[ "$existing" == "0" ]]; then
183+
short_sha=${HEAD_SHA:0:7}
184+
gh pr comment "$PR_NUMBER" --body "## Wheels Bot — Reviewer A guard
185+
186+
Detected and dismissed ${dismissed} bogus Reviewer A review(s) on commit \`${short_sha}\`. Cause: review body shorter than 200 characters or missing the canonical \`wheels-bot:review-a\` marker. See [wheels-dev/wheels#2558](https://github.com/wheels-dev/wheels/issues/2558) for context.
187+
188+
<!-- ${guard_marker} -->"
189+
else
190+
echo "::notice::Guard comment already present for ${PR_NUMBER}@${HEAD_SHA}; skipping duplicate"
191+
fi
192+
fi

‎.github/workflows/bot-review-b.yml‎

Lines changed: 27 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -28,16 +28,22 @@ jobs:
2828
|| github.event.pull_request.draft == false
2929
)
3030
steps:
31-
- name: Checkout the reviewed commit
31+
# SECURITY: check out the BASE branch, never the reviewed commit. This
32+
# workflow runs on `pull_request_review`, which carries the base repo's
33+
# secrets + write token even for fork PRs. Checking out
34+
# `github.event.review.commit_id` (a fork commit on fork PRs) and then
35+
# running the local `./.github/actions/wheels-bot-skip-check` composite
36+
# action below would execute fork-controlled code with the bot's token
37+
# (the classic pwn-request). The reviewed commit's objects are fetched
38+
# read-only after the token step so the review's git commands still
39+
# resolve; nothing from the fork is ever executed. The marker still keys
40+
# off review.commit_id (passed via `with:`/prompt, not shell), so the gate
41+
# and emitted marker stay aligned with the commit A reviewed (#2848).
42+
- name: Checkout BASE branch (trusted — never the reviewed/fork commit)
3243
uses: actions/checkout@v6
3344
with:
34-
# Check out the commit Reviewer A's review was attached to — not the
35-
# PR's (possibly newer) head. B critiques A's review of THIS commit,
36-
# so it must read exactly what A read, and the marker it emits must
37-
# key off the same SHA the skip-check below gates on. commit_id is a
38-
# GitHub-generated SHA, immune to head drift from concurrent pushes
39-
# (issue #2848).
40-
ref: ${{ github.event.review.commit_id }}
45+
ref: ${{ github.event.pull_request.base.ref }}
46+
persist-credentials: false
4147
fetch-depth: 0
4248

4349
- name: Generate App token
@@ -47,6 +53,19 @@ jobs:
4753
app-id: ${{ secrets.WHEELS_BOT_APP_ID }}
4854
private-key: ${{ secrets.WHEELS_BOT_PRIVATE_KEY }}
4955

56+
- name: Fetch PR head commit objects (read-only; never checked out)
57+
env:
58+
PR_NUMBER: ${{ github.event.pull_request.number }}
59+
run: |
60+
set -euo pipefail
61+
if ! [[ "$PR_NUMBER" =~ ^[0-9]+$ ]]; then
62+
echo "::error::PR number is not numeric: $PR_NUMBER"
63+
exit 1
64+
fi
65+
# Objects only — the working tree stays on the trusted base branch.
66+
# Best-effort: B reviews A's review via gh, so a fetch miss is non-fatal.
67+
git fetch --no-tags origin "refs/pull/${PR_NUMBER}/head" || true
68+
5069
- name: Skip check
5170
id: gate
5271
uses: ./.github/actions/wheels-bot-skip-check

‎CHANGELOG.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@ All historical references to "CFWheels" in this changelog have been preserved fo
2828

2929
### Added
3030

31+
- wheels-bot can now review **fork PRs** (external / first-time contributors), which it previously could not. GitHub withholds both the `vars` context and `secrets` from `pull_request` runs triggered by a forked repository, so Reviewer A's `vars.WHEELS_BOT_ENABLED == 'true'` job gate read empty and the job skipped — and Reviewer B, which only fires after A submits a review, never ran. A new `bot-review-a-fork.yml` workflow runs the initial Reviewer A review via `pull_request_target` (which executes in the base-repo context, where vars + secrets are available) for fork PRs that a maintainer has tagged with the `bot-review` label. Hardened against the `pull_request_target` "pwn-request" class: it checks out the **base** branch only and reviews the fork's changes through `gh pr diff`, never checking out or executing fork-controlled code, so the local `./.github/actions/wheels-bot-skip-check` composite action always resolves to trusted base code (the fork's commit objects are fetched read-only via `refs/pull/<n>/head` so the review's git commands still resolve). `bot-review-b.yml` is hardened the same way — it previously checked out `github.event.review.commit_id` (a fork commit on fork PRs) and then ran that local composite action, a latent pwn-request that was unexploitable only because Reviewer A never started the loop on forks; it now checks out the base branch with `persist-credentials: false`. The `bot-review` label (appliable only by write-access users) is the human-in-the-loop vet of the fork diff, and Reviewer A's tool surface stays read-only (#2871)
3132
- `cli.lucli.services.ArgSpec` — a typed argument-spec builder for Wheels CLI subcommands. LuCLI hands every module function a structured argument map (positionals as `arg1, arg2, ...`; `--key=value` as `key=value`; `--no-key` normalized to `key=false`), but `Module.cfc::argsFromCollection()` has historically flattened that map back to argv so each of ~18 subcommands could re-parse it with a hand-rolled token loop. The flatten step was the root cause of #2855 (it silently dropped every `false` value, so `--no-sqlite`/`--no-routes`/`--no-test-db`/`--no-open-browser` never survived the round trip) and is structurally lossy — it cannot distinguish a genuine `--no-X` negation from an explicit `--X=false`. `ArgSpec` consumes the structured handoff directly: a command declares its positionals, flags, and options up front (`.positional(name, required, default, type)`, `.flag(name, default)`, `.option(name, default, type)`), then calls `.parse(arguments)` to receive a typed result struct — no flatten, no re-parse, no lossy `false` round trip. Designed for incremental adoption: `getArgs()` and `argsFromCollection()` remain in place as a deprecated shim until every call site is converted, and each command that adopts `ArgSpec` drops its hand-rolled token loop in the same change. Cross-engine clean (no closures, no struct-member collisions, no `application`-scope function storage, no `attributeCollection = arguments`); boolean coercion handles both the string `"false"` LuCLI normally emits and a literal `false` value, so Lucee/Adobe/BoxLang all agree on the parsed semantics. Required-positional violations throw `Wheels.CLI.MissingArgument` with the positional's declared name in the message. The cross-framework research that informed the API surface (Rails/Thor, Laravel/Artisan, Django/argparse, Phoenix/Mix, Spring/picocli, Symfony Console) is recorded on the issue (#2861)
3233
- A "Reserved scope names" section in the Controllers and Actions guide documenting identifiers (`client`, `url`, `form`, `session`, `cgi`, `request`, `application`, `cookie`, `server`, `arguments`, `variables`, `local`, `this`) that must not be used as local variable names in Wheels controllers (and CFML components generally). Specifically calls out `client` — the most confusing case — because Lucee 7 throws `"client scope is not enabled"` when `clientManagement` is off, making the error look like an application misconfiguration rather than a bad variable name (#2833)
3334
- RustCFML is now recognized as a first-class engine in the engine-adapter layer. Wheels detects it via `server.coldfusion.productName == "RustCFML"` (it exposes no `server.lucee`/`server.boxlang`), instantiates a `RustCFMLAdapter` (extends `Base`, whose defaults are Lucee-shaped, matching RustCFML's semantics) ordered before the Adobe ColdFusion fallback, and accepts any version in `$checkMinimumVersion` (RustCFML is pre-1.0 and rapidly evolving, so the usual minimum-version guard doesn't apply). Because RustCFML does not yet implement the `cfcache` built-in, the framework's cfcache-backed template/static cache degrades gracefully to a no-op when the adapter reports `supportsCfcache() = false`, so requests still render (cacheless-but-working). The new `supportsCfcache()` capability defaults to `true` on Lucee/Adobe/BoxLang, leaving their behavior unchanged. Support is best-effort: RustCFML is a young, JVM-free CFML interpreter and is not yet part of the CI matrix (#2837)

0 commit comments

Comments
 (0)