Repository navigation
fix(dispatch)!: move the shim from pull_request_target to pull_request - #9230
fullsend-ai-coder[bot] wants to merge 2 commits into
Conversation
|
/fs-fix-stop |
|
Fix agent disabled for this PR. Remove the |
|
🤖 Finished Review · Commit: Runtime: pi · Model: sonnet → claude-sonnet-5-5 · Effort: high · Cost: $0.16 This review was superseded by a newer commit. Check the current pull request HEAD's review status before merging. |
waynesun09
changed the title
|
🤖 Review · Commit: This review was superseded by a newer commit. Check the current pull request HEAD's review status before merging. |
Site previewPreview: https://d82cddbc-site.fullsend-ai.workers.dev Commit: |
50dedcf to
9f96820
Compare
|
🤖 Finished Review · Commit: Runtime: pi · Model: sonnet → claude-sonnet-5-5 · Effort: high · Cost: $1.73 This review was superseded by a newer commit. Check the current pull request HEAD's review status before merging. |
9f96820 to
dbeee1f
Compare
|
🤖 Finished Review · Commit: Runtime: pi · Model: sonnet → claude-sonnet-5-5 · Effort: high · Cost: $2.63 This review was superseded by a newer commit. Check the current pull request HEAD's review status before merging. |
dbeee1f to
4d9d683
Compare
|
🤖 Finished Review · ✅ Success · Started 9:21 PM UTC · Completed 9:38 PM UTC Commit: Runtime: pi · Model: sonnet → claude-sonnet-5-5 · Effort: high · Cost: $3.28 |
Live validation (scratch repo
|
| Case | Event | Result | Run |
|---|---|---|---|
| Same-repo PR (new shim) | pull_request |
Routed to stage: review, no warning |
42701344439 |
| Same-repo PR (old shim) | pull_request_target |
Routed to stage: review + upgrade warning |
42701345178 |
| Same-repo PR closed | pull_request / pull_request_target |
Routed to stage: retro on both |
42697610876, 42697611997 |
| Fork PR, shim guard intact | pull_request |
dispatch job skipped by the shim guard |
42699631298 |
| Fork PR, fork deleted the shim guard | pull_request |
Dispatch refuses: Fork pull request on pull_request: not routed |
42701356574 |
| Fork PR (old shim) | pull_request_target |
Still routes, with the upgrade warning | 42701352392 |
/fs-review on the fork PR |
issue_comment |
Routed to stage: review |
42701659373 |
Route checkout on pull_request |
— | git checkout ... 23dc2312 = PR base SHA (head was 987d2751) |
42697126408 |
Local reusable workflow on pull_request |
— | job.workflow_sha = github.sha = merge commit 9055d649 (base 7a362a45), which is why vendored reads now use base.sha |
42699602779 |
Notes:
- The first two rows are at the current head
4d9d68331. The other rows ran at the earlier heads50dedcf9f/dbeee1ff9; routing has not changed since. - Not run live:
- Dependabot skip: a scratch repo can't produce Dependabot PRs. It is covered by
TestDetermineStagePullRequest. - Agent-job checkouts: those jobs need the mint and secrets. They are covered by
TestEveryCheckoutHasTrustedRefandTestWorkflowSHAUsesAreTrusted.
- Dependabot skip: a scratch repo can't produce Dependabot PRs. It is covered by
|
Risk Assessment: elevated (3/5) DetailsLarge blast radius across 32 files, modifications to 3 protected CI workflow files, high regression history and change coupling in dispatch workflows, and unflagged behavioral changes to dispatch routing elevate the composite risk to elevated. Previous runRisk Assessment: elevated (3/5) DetailsElevated risk due to large blast radius modifying protected CI workflows across 28 files, high historical churn and regression frequency on dispatch files, and linked security issue context with no feature-flag rollback gate. Previous run (2)Risk Assessment: elevated (3/5) DetailsElevated risk due to large blast radius modifying protected CI workflows across 23 files, high historical churn and regression frequency on dispatch files, and linked security issue context with no feature-flag rollback gate. Previous run (3)Risk Assessment: elevated (3/5) DetailsElevated risk due to large blast radius modifying protected CI workflows across 23 files, high historical churn and regression frequency on dispatch files, and linked security issue context with no feature-flag rollback gate. |
Per-repo shims are moving from pull_request_target to pull_request for same-repo PRs (#9221). IsLifecycleCloseAuthorized only admitted a pull_request_target close, so a retro on a pull_request close by a read-only author, triager or role-less App bot would be denied by `fullsend dispatch --route retro`. It now admits both event names; every other condition (GitHub, change proposal, closed or merged, harness role retro) is unchanged, and pull_request_target keeps working for shims that have not upgraded. ghaevent.go already maps both names through mapPREvent and keeps source.raw_type as the exact event name. A search of internal/ and cmd/ found no other non-test check of the literal. Tests add pull_request rows next to the pull_request_target ones: lifecycle-close auth, gha-event mapping parity, review and retro route parity, retro routing and bot-role dispatch. The normative authorization (v1, v2) and normalized-event docs name both events; the v1 lifecycle-close anchor is kept. Note: pre-commit could not fetch its remote hook repos (TLS error). gofmt, go vet, lint-docs-links, lychee and lint-broken-symlinks ran directly and passed; semgrep could not download its ruleset. Some existing fetch/SSRF tests fail in this sandbox on main too (DNS resolves to 198.18.0.0/15). Closes #9224 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
4d9d683 to
e04dce8
Compare
|
🤖 Finished Review · ✅ Success · Started 10:56 PM UTC · Completed 11:07 PM UTC Commit: Runtime: pi · Model: sonnet → claude-sonnet-5-5 · Effort: high · Cost: $2.76 |
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
e04dce8 to
082a890
Compare
|
🤖 Review · Commit: This review was superseded by a newer commit. Check the current pull request HEAD's review status before merging. |
…ll_request GitHub blocks pull_request_target by default in public repositories from 2026-12-02 (#9221). Follow github/gh-aw: the shim triggers on pull_request for same-repo PRs only (head.repo.id == github.repository_id); fork PRs are reviewed when a maintainer comments /fs-review. - shim template and fullsend's own fullsend.yaml: pull_request trigger plus the same-repo guard. - reusable-dispatch.yml: - route pull_request like pull_request_target; warn when a caller still uses pull_request_target, which keeps working; - never route a fork or Dependabot PR on pull_request, in bash, CEL and the harness jobs. A pull_request run uses the PR's own copy of the shim, so a fork can drop the shim guard; Dependabot runs get only Dependabot secrets; - base side for every checkout on pull_request: base.sha where a ref is pinned, the default branch for the target-repo checkouts that used the event default (github.ref was the default branch on pull_request_target, it is the merge commit on pull_request); - job.workflow_sha reads use base.sha on pull_request when the reusable workflow lives in the caller repository (vendored installs), where job.workflow_sha would be the PR merge commit. - reusable-prioritize.yml: same checkout and workflow_sha rules. - scaffold tests: shim trigger and guard, every checkout has a trusted ref, every job.workflow_sha read is trusted, harness guard, routing of same-repo, fork, Dependabot and pull_request_target events. - behaviour tests: fork PR scenarios now expect no dispatch from fork PR events (open, label, push) and dispatch from a maintainer comment on the fork PR (new step "the <actor> posts ... on the fork pull request"); the outsider fork scenario checks the outsider's comment is denied; the GitHub Actions BT driver treats action_required runs (held for approval) as runs that ran no job; fork step trigger times are truncated. - docs: required-upgrade section (merge-conflict PRs, forks, Dependabot, private-repo fork secrets setting), troubleshooting entry, user guides. BREAKING CHANGE: after upgrading, fork and Dependabot PRs are no longer reviewed or retro'd automatically; a maintainer comments /fs-review instead. Assisted-by: Claude (fix), DeepSeek (review) Signed-off-by: Wayne Sun <gsun@redhat.com>
082a890 to
7acfcf1
Compare
|
🤖 Finished Review · ✅ Success · Started 1:02 AM UTC · Completed 1:14 AM UTC Commit: Runtime: pi · Model: sonnet → claude-sonnet-5-5 · Effort: high · Cost: $3.33 |
GitHub blocks pull_request_target by default on public repos from 2026-12-02, and a blocked run ends in startup_failure, which cannot be re-run. This moves the four workflows that still use it, following the gh-aw model (pull_request with a same-repo guard; fork PRs through a maintainer slash command; writes in jobs that run no PR code): - e2e.yml: pull_request for same-repo PRs, gated as before. Fork PRs get no secrets, so the gate and suites skip. A workflow_call with pr_number and head_sha runs the suites on a /ok-to-test-authorized sha, and each suite reports its job status as an output. The redaction script comes from the base sha, since pull_request's github.sha is the merge commit. - e2e-ok-to-test.yml: issue_comment /ok-to-test instead of the ok-to-test label. The authorize job creates check runs named behaviour and playback on the authorized head sha and the report job completes them, so the required checks on main are met (the old workflow_call route reported "run / behaviour"). - vouch-check.yml: a 10-minute schedule running scripts/vouch-sweep.sh. First-time contributors' pull_request runs wait for approval, and fork runs get a read-only token, so a PR event cannot enforce the gate. - notify-adr-slack.yml: push to main with the ADR paths filter, passing the pushed commits to scripts/notify-adr-merge.sh. hack/workflow_triggers_test.py fails when any workflow's `on:` declares pull_request_target; fullsend.yaml stays allowed until #9230 moves it. Assisted-by: Claude Signed-off-by: Wayne Sun <gsun@redhat.com>
GitHub blocks pull_request_target by default on public repos from 2026-12-02, and a blocked run ends in startup_failure, which cannot be re-run. This moves the four workflows that still use it, following the gh-aw model (pull_request with a same-repo guard; fork PRs through a maintainer slash command; writes in jobs that run no PR code): - e2e.yml: pull_request for same-repo PRs, gated as before. Fork PRs get no secrets, so the gate and suites skip. A workflow_call with pr_number and head_sha runs the suites on a /ok-to-test-authorized sha, and each suite reports its job status as an output. On that path the relevance check diffs the authorized sha, not the live PR head. The redaction script comes from the base sha, since pull_request's github.sha is the merge commit (ci-workflows.md updated to match). - e2e-ok-to-test.yml: issue_comment /ok-to-test on an open PR instead of the ok-to-test label. The authorize job creates check runs named behaviour and playback on the authorized head sha and the report job completes them, so the required checks on main are met (the old workflow_call route reported "run / behaviour"). - vouch-check.yml: a 10-minute schedule running scripts/vouch-sweep.sh. First-time contributors' pull_request runs wait for approval, and fork runs get a read-only token, so a PR event cannot enforce the gate. - notify-adr-slack.yml: push to main with the ADR paths filter, passing the pushed commits to scripts/notify-adr-merge.sh. hack/workflow_triggers_test.py fails when any workflow's `on:` declares pull_request_target; fullsend.yaml stays allowed until #9230 moves it. Assisted-by: Claude (fix), Claude (review), DeepSeek (review) Signed-off-by: Wayne Sun <gsun@redhat.com>
Summary
GitHub blocks
pull_request_targetby default in public repositories from December 2, 2026 (brownout again on November 16). This PR moves the fullsend shim topull_requestfor same-repo PRs, following github/gh-aw, and keeps every dispatch path that still receivespull_request_targetworking until each repository upgrades.Closes #9224. Part of #9221 (approach: #9221 (comment)).
What changes for users
pull_request_targetpull_request/fs-review; push branches to the repository to keep automatic review/fs-reviewpull_requestrun until the conflict is resolved;/fs-reviewmeanwhilefullsend repos installThe shim update is a required upgrade, marked
!even though #9224 said no!. The dispatch inputs are unchanged and old shims keep working, but upgrading drops automatic fork and Dependabot review, and the release notes must say so:fullsend repos installrewrites the trigger and the pin in one commit. After the upgrade, GitHub PR events carrysource.raw_type(and the harnessevent_type)pull_requestinstead ofpull_request_target. Custom CEL triggers or harness filters that match onraw_type == "pull_request_target"must match both values, or useevent.entity.kind/event.transition.kind. fullsend's built-in triggers do not readraw_type.Docs: new section "Required upgrade: shim moves from
pull_request_targettopull_request" and a troubleshooting entry indocs/guides/getting-started/repo-management.md.Changes
Workflows (takeover commit)
internal/scaffold/fullsend-repo/templates/shim-per-repo.yamland fullsend's own.github/workflows/fullsend.yaml:pull_request(same types as before) and the gh-aw same-repo guardgithub.event.pull_request.head.repo.id == github.repository_id.reusable-dispatch.yml:pull_requestexactly likepull_request_target(bashcaseand the CEL opt-in step).pull_request: bash router, CEL opt-in step and both harness jobs. Apull_requestrun uses the PR's own copy of the shim, so a fork could delete the shim guard.pull_request. The 10 checkouts that pin a ref now usebase.shaforpull_requesttoo; onpull_request,github.shais the PR merge commit. The 5Checkout target repositorysteps that set no ref (triage, code, review, retro, harness) now check out the default branch onpull_request, which is whatgithub.refwas onpull_request_target../reusable workflow onpull_requestruns from the merge commit, sojob.workflow_shais the merge commit. Everyjob.workflow_sharead (checkouts, CLI build,version, cache keys) now usesbase.shawhenjob.workflow_sha == github.sha. Remote@refcalls are unchanged.::warning::when a caller still usespull_request_target.reusable-prioritize.yml: same checkout andjob.workflow_sharules.Go (fs code agent commit)
IsLifecycleCloseAuthorizedaccepts apull_requestclose as well as apull_request_targetclose (retro role only).ghaevent.goalready mapped both names; a new test pinsraw_type.internal/andcmd/for non-test"pull_request_target"checks: onlyauth.go(fixed),ghaevent.go(already maps both), and comments inpoll/events.goandcli/run.go.Security notes
Testing
internal/scaffold/pr_trigger_test.go:TestEveryCheckoutHasTrustedRef: everyactions/checkoutstep names a ref that is trusted onpull_request;TestWorkflowSHAUsesAreTrusted: everyjob.workflow_sharead goes through the base-SHA rule;Determine stageon same-repo, fork, deleted-head, Dependabot, human-push-to-Dependabot, bot-opened andpull_request_targetevents, including the warning.fork-bash-routing,fork-dispatchand the outsider fork scenario inactor-authorizationnow expect no dispatch from fork PR open, label or push, and dispatch from a maintainer comment on the fork PR (new step: "the posts ... on the fork pull request").action_requiredruns (held for first-time-contributor approval) as runs that ran no job.@skip:gitlab(GitLab BT is not wired in CI).cel_route_test.go:pull_requestrows for the opt-in event gate and the bash/CEL agreement test; the opt-inif:assertion covers the fork guard.pull_request(bot commit).go test ./internal/scaffold/ ./internal/cli/ ./internal/harnessdispatch/... ./internal/poll/passes, exceptTestInstallFullsendCLIScript_ReleasePinDoesNotClearSettings, which fails on macOS arm64 only (unsupported architecture: arm64) and also fails onmain.make lintpasses.Follow-ups (not in this PR)
pull_request_targetworkflows (vouch-check.yml,notify-adr-slack.yml,e2e.yml,e2e-ok-to-test.yml): tracked in ci: move fullsend's own workflows off pull_request_target before the Dec 2 default block #9245.release.ymlonly mentions it in a comment.repos installor the dispatch) when "send secrets" or "send write tokens" to fork PR workflows is on, or add a fork/ref check in the mint. With write tokens on, a vendored fork run could getid-token: writewithout any repo secret (raised on docs(adr): add ADR 0164, run the shim on pull_request for same-repo PRs #9227).docs/architecture.mdanddocs/contributing/ci-workflows.mdonce docs(adr): add ADR 0164, run the shim on pull_request for same-repo PRs #9227 merges (plain text for now, so the link checker passes).🤖 Generated with Claude Code