Skip to content

ci: stop putting a fork's tree on the runner at all - #49

Merged
yuting0624 merged 1 commit into
masterfrom
fix/external-review-no-fork-checkout
Aug 2, 2026
Merged

ci: stop putting a fork's tree on the runner at all#49
yuting0624 merged 1 commit into
masterfrom
fix/external-review-no-fork-checkout

Conversation

@yuting0624

Copy link
Copy Markdown
Owner

#26 was the first real exercise of this workflow, and it failed at checkout:

##[error]Refusing to check out fork pull request code from a 'pull_request_target'
workflow. ... To opt in ... set 'allow-unsafe-pr-checkout: true'

Not our logicauthorise passed, so the timeline lookup and the write-access check
both worked. actions/checkout added allow-unsafe-pr-checkout (default false) and now
refuses the pattern this workflow was built on. Reasonably: it's the shape behind most
"pwn request" reports, because the next person adds a build step.

Why not just opt in

It was available and defensible — --allowedTools here is reading and commenting only, so
there's no execution path today.

Declined, because "no execution path" is an invariant somebody has to keep re-establishing
on every future edit of that tools list, and not depending on that is precisely what the
guardrail is for. This repo produced three guards that were silent no-ops in the last two
days; I'd rather not add a fourth that depends on future me.

Removing the need turned out to be cheap

A fork PR's unchanged files are the base repo's — already checked out at the workspace
root, trusted, readable. gh pr diff carries the changed ones, and for a newly added file
the diff is the entire file
.

On #26 that's not a theoretical trade: 13 of 20 files are new — the .cmd wrappers, the .js
hooks, including hooks/validate-delegate-bash.js, a JavaScript reimplementation of the
security gate. The single highest-scrutiny file in the change is new, so it arrives in full.

What's actually lost: full content of modified files, where the reviewer now sees hunks
against a base it can read. Modest, and worth it.

Also

Both actions pinned to commits. This job holds a write-scoped token and an API credential and
was the only workflow left on floating tags — which is also how the behaviour changed under us
without a diff.

README updated: "checked out read-only into a subdirectory" → "never reaches the runner at
all". That sentence is a promise to contributors and it needed to stay true.

191 tests, validate passes. No plugin code touched.


@yuting0624 — separately, on #26 being likely wontfix: worth deciding that before re-labelling. This
runs a paid review on a 20-file change, and if the answer is already no, the useful reply is a
short one from you about scope, not a code review.

PR #26 was the first real exercise of this workflow and it failed at checkout:

  Refusing to check out fork pull request code from a 'pull_request_target'
  workflow ... set 'allow-unsafe-pr-checkout: true'

Not our logic -- authorise passed, timeline lookup and permission check both
worked. actions/checkout added that input (default false) and now refuses the
pattern this workflow was built on, which is the pattern behind most pwn-request
reports because the next person adds a build step.

Opting back in was available and defensible: tools here are restricted to
reading and commenting, so there is no execution path today. Declined. 'No
execution path' is an invariant someone has to keep re-establishing on every
future edit of allowedTools, and not depending on that is what the guardrail is
for. Today alone produced three guards that were silent no-ops.

Removing the need is cheap here. A fork PR's unchanged files ARE the base repo's,
which is already checked out at the root and readable, and gh pr diff carries the
changed ones -- for a newly added file the diff is the whole file. On #26, 13 of
20 files are new, including the JS reimplementation of the security gate, so the
diff contains essentially everything worth reviewing.

Also pinned both actions to commits. This job holds a write-scoped token and an
API credential; it was the only workflow left on floating tags.
@quorum-code-review

quorum-code-review Bot commented Aug 2, 2026

Copy link
Copy Markdown

Quorum review

gemini-3.6-flash, claude-sonnet-5 each read the diff without seeing the other's output (gemini-3.6-flash 0 and claude-sonnet-5 0), which merged to 0 distinct finding(s). 0 of those were reported by both models independently.
The remaining findings were each judged by the model that did not report them: 0 confirmed, 0 uncertain, 0 refuted.

Beyond the diff, the models made 12 read-only lookup(s) into the checkout at 841d911 and opened .github/workflows/claude-review-external.yml, .github/workflows/claude-review.yml, .github/workflows/quorum-review.yml.

No new issues found in this diff.


Usage
Model Region Calls Input Cached input Output
gemini-3.6-flash global 9 79,849 25,949 230
claude-sonnet-5 global 6 59,261 15,632 5,005

Reviewed 5770d52 · quorum-review 1.7.0 · models gemini-3.6-flash, claude-sonnet-5 · 73s

@claude

claude Bot commented Aug 2, 2026

Copy link
Copy Markdown

Review

What it does: Removes the fork-HEAD actions/checkout step from claude-review-external.yml entirely (previously checked out into pr-head/ via --add-dir, which actions/checkout now refuses under pull_request_target unless allow-unsafe-pr-checkout: true). The reviewing agent now sees only gh pr diff output plus the base-ref checkout at the workspace root for context. Both actions are pinned to full commit SHAs (with version comments). README updated to match.

Assessment: sound, and a real improvement, not just a workaround.

  • Removing the untrusted checkout instead of opting into allow-unsafe-pr-checkout is the right call — it eliminates the risk class instead of relying on "tools are read-only today" staying true across future edits to --allowedTools. Good instinct given this repo's own track record of "silent no-op guard" incidents (per the PR description).
  • SHA-pinning both actions closes the actual root cause noted in the PR body: this job was the last one on floating tags, which is how the checkout behavior changed without a diff to review. Correct fix for a pull_request_target job holding a write-scoped token.
  • The updated prompt explicitly warns the reviewer that the workspace-root checkout is the base (before) state, not the contributor's code — a real, non-obvious failure mode (an agent could otherwise mistake base-repo content for the submitted diff when reading a modified file). Good addition.
  • Checked for stale references to the old pattern (pr-head, --add-dir pr-head, "checked out read-only into a subdirectory") across .md/.yml files — none remain. README wording and workflow comments are consistent with the new behavior.
  • No plugin code touched, consistent with the PR description.

Not verified: I don't have network access in this sandbox, so I could not confirm that the two pinned SHAs (actions/checkout@11d5960a…, anthropics/claude-code-action@be7b93b1…) actually resolve to the v4/v1 tags the trailing comments claim. A mismatch here would fail loudly (wrong action behavior/missing inputs) rather than silently, so it's low risk, but worth a final sanity check (git ls-remote against both tags) before merge since it wasn't something I could check myself.

No other issues found.

@yuting0624 yuting0624 added the enhancement New feature or request label Aug 2, 2026
@yuting0624
yuting0624 merged commit d8c9e70 into master Aug 2, 2026
6 checks passed
@yuting0624
yuting0624 deleted the fix/external-review-no-fork-checkout branch August 2, 2026 04:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant