Disable checkout by default for pull_request_target workflows#46771
Conversation
- ShouldGeneratePRCheckoutStep now respects CheckoutDisabled flag - Auto-disable checkout for pull_request_target when not explicitly configured - Update tests to reflect new default behavior - Recompile ai-moderator workflow (checkout: false now correctly removes PR checkout step)" Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Updates checkout generation for pull_request_target workflows to avoid unavailable PR-head refs.
Changes:
- Auto-disables implicit checkout for
pull_request_target. - Makes
checkout: falsesuppress PR-specific checkout. - Adds tests and recompiles the affected workflow.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/workflow_builder.go |
Adds automatic checkout disabling. |
pkg/workflow/pr.go |
Honors disabled checkout. |
pkg/workflow/pr_test.go |
Tests checkout suppression. |
pkg/workflow/pr_ready_for_review_checkout_test.go |
Tests compiled checkout behavior. |
pkg/workflow/pull_request_target_validation_test.go |
Updates validation expectations. |
.github/workflows/ai-moderator.lock.yml |
Removes generated PR checkout steps. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (2)
pkg/workflow/workflow_builder.go:112
- This check runs before imported checkout settings are merged, so a
pull_request_targetworkflow that inheritscheckout:from a shared workflow is marked disabled and line 129 then drops the inherited configuration entirely. That breaks the established checkout-import contract (checkout_import_test.go:14-68). Treat a non-emptyMergedCheckoutas an explicit configuration, or move auto-disablement after import merging.
if len(workflowData.CheckoutConfigs) == 0 {
pkg/workflow/workflow_builder.go:116
- The repository accepts
onas a scalar or list (event_validation.go:116-130), but this map-only assertion leaveson: pull_request_targetandon: [pull_request_target]with checkout enabled. Those valid forms therefore retain the failing PR-head checkout and bypass the new safe default. Reuse the existing event-presence helper, which handles all supported forms.
if onVal, ok := result.Frontmatter["on"]; ok {
if onMap, ok := onVal.(map[string]any); ok {
if _, hasPRT := onMap["pull_request_target"]; hasPRT {
- Files reviewed: 6/6 changed files
- Comments generated: 2
- Review effort level: Medium
| expectPRCheckout: true, | ||
| description: "pull_request_target with explicit checkout mapping should generate 'Checkout PR branch' step", |
There was a problem hiding this comment.
Addressed in f534297 and 37bc639. The original code only checked CheckoutDisabled, so an explicit checkout: mapping on a pull_request_target workflow still emitted checkout_pr_branch.cjs. The fix adds an IsPullRequestTarget field (set in workflow_builder.go for all pull_request_target-only workflows) and checks it first in ShouldGeneratePRCheckoutStep — so the PR-head checkout step is always suppressed for pull_request_target, regardless of whether an explicit actions/checkout config was provided. The test expectation was corrected to expectPRCheckout: false.
| // Auto-disable checkout for pull_request_target workflows when not explicitly configured. | ||
| // For pull_request_target events, the head branch is often deleted (closed/merged PRs) | ||
| // or inaccessible (fork PRs), causing the "Checkout PR branch" step to fail. | ||
| // Users who need checkout can explicitly set a checkout configuration in frontmatter. |
There was a problem hiding this comment.
Updated in commit 36427c9 and expanded further in f534297. docs/src/content/docs/reference/checkout.md now has an explicit ## pull_request_target Checkout section describing: (1) the PR head checkout is auto-disabled by default for pull_request_target workflows, (2) opt-in with an explicit checkout: mapping for a trusted ref (e.g. base SHA), and a warning against using head.sha or refs/pull/.../head in a privileged job. The checkout: false description was also updated to say it suppresses both actions/checkout and the PR-specific step.
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
✅ Test Quality Sentinel completed test quality analysis. Test message |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: Disable checkout by default for pull_request_target workflows
The fix correctly addresses the hard-fail on deleted/fork head branches and the pre-existing bug where checkout: false was ignored. One blocking issue found:
Mixed-trigger regression (see inline comment on workflow_builder.go:116): a workflow with both pull_request and pull_request_target triggers has checkout incorrectly auto-disabled for all events. The guard must verify pull_request (and other safe triggers) are absent before setting CheckoutDisabled = true.
Non-blocking observations
- The test suite covers the three key scenarios well (no-checkout-key, explicit false, explicit mapping).
ai-moderator.lock.ymlrecompile looks correct — the removed steps align with the disabled checkout.- The
ShouldGeneratePRCheckoutStepearly-exit placement is clean and correct.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 28.3 AIC · ⌖ 4.44 AIC · ⊞ 5K
| if _, checkoutExplicitlySet := result.Frontmatter["checkout"]; !checkoutExplicitlySet { | ||
| if onVal, ok := result.Frontmatter["on"]; ok { | ||
| if onMap, ok := onVal.(map[string]any); ok { | ||
| if _, hasPRT := onMap["pull_request_target"]; hasPRT { |
There was a problem hiding this comment.
Mixed-trigger regression: if a workflow uses both pull_request and pull_request_target triggers, checkout is auto-disabled for all events — including pull_request events where head branches are accessible. The guard should skip auto-disabling when pull_request (or other normal checkout-compatible triggers) co-exist.
_, hasPR := onMap["pull_request"]
if hasPRT && !hasPR {
workflowData.CheckoutDisabled = true
}@copilot please address this.
There was a problem hiding this comment.
Fixed in f534297. The auto-disable block now guards with !hasPR (using the new frontmatterHasTrigger helper) so workflows combining pull_request and pull_request_target triggers are not affected.
There was a problem hiding this comment.
REQUEST_CHANGES — 3 correctness bugs in the auto-disable logic
The core idea is sound but the implementation has three bugs that will silently misfire in real-world usage.
Blocking findings
1. (Critical) on: scalar/list form bypasses auto-disable — on: pull_request_target or on: [push, pull_request_target] silently skips the type assertion and checkout is never disabled. This is the most common YAML form in many workflows. See comment on line 115.
2. (High) Guard evaluates before imported checkout configs are merged — A workflow importing a shared workflow that provides checkout configs will have CheckoutConfigs empty at the guard, trigger auto-disable, and permanently lose the shared checkout. See comment on line 112.
3. (Medium) Multi-trigger workflows have checkout globally killed — Any workflow with pull_request_target alongside another trigger (e.g. workflow_dispatch, push) will have CheckoutDisabled = true for all triggers, not just the pull_request_target path. See comment on line 116.
All three bugs share the same root: the guard is doing trigger detection + checkout-config awareness in a fragile, ad-hoc way. A small refactor to (a) use a proper YAML-form-aware hasTrigger helper, (b) move the block after import merging, and (c) require single-trigger exclusivity would close all three.
🔎 Code quality review by PR Code Quality Reviewer · 36.5 AIC · ⌖ 4.79 AIC · ⊞ 5.6K
Comment /review to run again
| if len(workflowData.CheckoutConfigs) == 0 { | ||
| if _, checkoutExplicitlySet := result.Frontmatter["checkout"]; !checkoutExplicitlySet { | ||
| if onVal, ok := result.Frontmatter["on"]; ok { | ||
| if onMap, ok := onVal.(map[string]any); ok { |
There was a problem hiding this comment.
on: as scalar or list silently bypasses the auto-disable logic — if a user writes on: pull_request_target (scalar) or on: [push, pull_request_target] (sequence), the inner onMap type assertion fails and CheckoutDisabled is never set, defeating the safety mechanism.
💡 Suggested fix
Extract a helper that handles all three YAML on: forms:
func hasTrigger(onVal any, trigger string) bool {
switch v := onVal.(type) {
case string:
return v == trigger
case []any:
for _, item := range v {
if s, ok := item.(string); ok && s == trigger {
return true
}
}
case map[string]any:
_, ok := v[trigger]
return ok
}
return false
}Then replace the nested map assertion with hasTrigger(onVal, "pull_request_target"). The same blind spot exists at lines 159 and 615 in this file.
There was a problem hiding this comment.
Fixed in f534297. Added frontmatterHasTrigger(onVal, trigger string) bool in workflow_builder.go that handles all three YAML on: forms: string scalar (on: pull_request_target), sequence (on: [push, pull_request_target]), and mapping (on:\n pull_request_target:\n types: [closed]). Both hasPRT and hasPR are detected using this helper, so on: pull_request_target (scalar) and on: [pull_request_target] (sequence) correctly trigger auto-disable.
| // For pull_request_target events, the head branch is often deleted (closed/merged PRs) | ||
| // or inaccessible (fork PRs), causing the "Checkout PR branch" step to fail. | ||
| // Users who need checkout can explicitly set a checkout configuration in frontmatter. | ||
| if len(workflowData.CheckoutConfigs) == 0 { |
There was a problem hiding this comment.
Auto-disable guard fires before imported checkout configs are merged, so a workflow that imports a shared workflow providing checkout configs will have CheckoutConfigs empty at this check, trigger auto-disable, and lose the shared checkout — which is the opposite of the intended behavior.
💡 Suggested fix
Move the auto-disable block to after the import-merge block (lines 125–144), or alternatively re-evaluate after merging:
// ... merge imported checkout configs first ...
// Auto-disable for pull_request_target only when nothing was configured at all
if len(workflowData.CheckoutConfigs) == 0 && !workflowData.CheckoutDisabled {
if hasPullRequestTarget(result.Frontmatter) {
workflowData.CheckoutDisabled = true
}
}Placing it after the merge ensures that an explicitly imported checkout config prevents auto-disable.
There was a problem hiding this comment.
Fixed in f534297. The auto-disable block was moved to after the import-merge block. Imports are merged at lines 112–129; auto-disable runs at lines 131–152 (after). The guard checks len(workflowData.CheckoutConfigs) == 0 at that point, so an imported checkout config (merged in at 127) prevents auto-disable. The comment in the code also explicitly notes this ordering: "This block runs after import merging so that imported checkout configs prevent auto-disable."
| if _, checkoutExplicitlySet := result.Frontmatter["checkout"]; !checkoutExplicitlySet { | ||
| if onVal, ok := result.Frontmatter["on"]; ok { | ||
| if onMap, ok := onVal.(map[string]any); ok { | ||
| if _, hasPRT := onMap["pull_request_target"]; hasPRT { |
There was a problem hiding this comment.
Multi-trigger workflows have checkout globally disabled — if a workflow uses both push and pull_request_target in its on: map, this block sets CheckoutDisabled = true for all triggers, breaking the push path which has no such restriction.
💡 Suggested fix
This is inherently tricky because CheckoutDisabled is a single boolean. The auto-disable should only apply when pull_request_target is the only (or at least the primary) trigger. Either restrict auto-disable to single-trigger pull_request_target workflows, or introduce per-trigger checkout configuration rather than a global flag.
As a minimal safe approach, skip auto-disable when other triggers coexist:
if onMap, ok := onVal.(map[string]any); ok {
_, hasPRT := onMap["pull_request_target"]
onlyPRT := hasPRT && len(onMap) == 1
if onlyPRT {
workflowData.CheckoutDisabled = true
}
}There was a problem hiding this comment.
Fixed in f534297. Auto-disable only fires when hasPRT && !hasPR — i.e., when pull_request_target is present but pull_request is not. Workflows combining pull_request_target with other triggers like push or workflow_dispatch do still get IsPullRequestTarget = true (to suppress checkout_pr_branch.cjs), but CheckoutDisabled is only auto-set when no pull_request trigger co-exists and no explicit checkout config is present.
…_request_target Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (169 new lines in 📄 Draft ADR committed:
📋 What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. ❓ Why ADRs Matter
ADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
🧪 Test Quality Sentinel Report✅ Test Quality Score: 100/100 — Excellent
Coverage: All 5 tests directly verify the feature invariant: pull_request_target workflows auto-disable checkout when not explicitly configured. Tests Added:
Quality Metrics:
Verdict: ✅ Passed. Tests provide high-confidence behavioral coverage with no coding guideline violations. Recommend approval. Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — requesting changes on two correctness issues and one test-coverage gap.
📋 Key Themes & Highlights
Key Themes
- Incomplete trigger-format coverage (
/diagnosing-bugs): The auto-disable logic only handleson:as a YAML map. The string shorthand (on: pull_request_target) and list form (on: [pull_request_target, push]) are silently skipped, leaving those workflows still generating the failing checkout step. - Redundant state write (
/diagnosing-bugs): The outerCheckoutConfigs == 0guard does not also check!CheckoutDisabled, so acheckout: falseworkflow re-sets the flag unnecessarily — fragile if the flag gains new semantics. - Shallow integration test (
/tdd): The trusted base-SHA opt-in test only asserts presence of"Checkout PR branch", not the absence of insecure head-ref patterns, so it wouldn't catch a future regression that checks out the wrong ref.
Positive Highlights
- ✅ Clean, minimal fix in
pr.go—CheckoutDisabledguard placed at the top ofShouldGeneratePRCheckoutStepbefore any other logic. - ✅ Comprehensive test matrix covering no-checkout-key, explicit false, and explicit mapping cases.
- ✅ Correctly threads the two bugs together: the pre-existing
checkout: falsebug and the new auto-disable behaviour are fixed in a single coherent change. - ✅ Lock file recompiled to remove the previously-broken steps.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 47.7 AIC · ⌖ 4.83 AIC · ⊞ 6.7K
Comment /matt to run again
| if len(workflowData.CheckoutConfigs) == 0 { | ||
| if _, checkoutExplicitlySet := result.Frontmatter["checkout"]; !checkoutExplicitlySet { | ||
| if onVal, ok := result.Frontmatter["on"]; ok { | ||
| if onMap, ok := onVal.(map[string]any); ok { |
There was a problem hiding this comment.
[/diagnosing-bugs] The map[string]any assertion silently skips auto-disable when on: is a string or list — e.g. on: pull_request_target or on: [pull_request_target, push] — leaving those workflows exposed to the original failure.
💡 Suggested fix
After the existing map-path check, also handle the string form:
if onStr, ok := onVal.(string); ok && onStr == "pull_request_target" {
workflowBuilderLog.Print("Auto-disabling checkout for pull_request_target workflow")
workflowData.CheckoutDisabled = true
}And for the list form ([]any), iterate and check each element. Without this, a workflow authored with on: pull_request_target (the shorthand YAML syntax) will not receive the auto-disable.
@copilot please address this.
There was a problem hiding this comment.
Fixed in f534297. Added frontmatterHasTrigger(onVal, trigger) that handles all three YAML on: forms: string scalar (on: pull_request_target), sequence (on: [pull_request_target, push]), and mapping. Both the auto-disable check and the IsPullRequestTarget flag now use this helper.
| // For pull_request_target events, the head branch is often deleted (closed/merged PRs) | ||
| // or inaccessible (fork PRs), causing the "Checkout PR branch" step to fail. | ||
| // Users who need checkout can explicitly set a checkout configuration in frontmatter. | ||
| if len(workflowData.CheckoutConfigs) == 0 { |
There was a problem hiding this comment.
[/diagnosing-bugs] The outer guard len(workflowData.CheckoutConfigs) == 0 is checked before checkoutExplicitlySet, so a workflow that sets checkout: false first populates CheckoutDisabled = true (line 102) but also happens to have empty CheckoutConfigs, making it pass this outer condition — and then re-sets CheckoutDisabled = true unnecessarily for any pull_request_target workflow. This is harmless today, but it is logically redundant and fragile: if CheckoutDisabled is used as a sentinel for something in the future the double-write may mask a real state.
💡 Suggestion
Add an early exit for the already-explicitly-disabled case:
if !workflowData.CheckoutDisabled && len(workflowData.CheckoutConfigs) == 0 {
if _, checkoutExplicitlySet := result.Frontmatter["checkout"]; !checkoutExplicitlySet {
...
}
}This makes the intent explicit: only auto-disable when nothing has been configured.
@copilot please address this.
There was a problem hiding this comment.
Fixed in f534297. The outer guard now reads if !workflowData.CheckoutDisabled && len(workflowData.CheckoutConfigs) == 0 { so a workflow with checkout: false (which already sets CheckoutDisabled = true) bypasses the block entirely and avoids the redundant write.
| Workflow triggered when a PR is opened with a trusted base checkout. | ||
| `, | ||
| expectPRCheckout: true, | ||
| description: "pull_request_target with explicit checkout mapping should generate 'Checkout PR branch' step", |
There was a problem hiding this comment.
[/tdd] The test asserts expectPRCheckout: true for the trusted base-SHA opt-in path but only checks for the presence of "Checkout PR branch" — it doesn't verify that an insecure head ref (e.g. refs/pull/.../head) is absent. A future regression in checkout_pr_branch.cjs that resolves to the head ref would still pass this test while silently breaking the security guarantee.
💡 Suggested assertion
After the existing hasPRCheckout check, add:
if hasPRCheckout {
for _, insecure := range []string{"refs/pull", "head.ref", "head_ref"} {
if strings.Contains(lockStr, insecure) {
t.Errorf("%s: expected trusted base checkout but lock file contains insecure pattern %q", tt.description, insecure)
}
}
}This makes the integration test a genuine security regression guard, not just a presence check.
@copilot please address this.
There was a problem hiding this comment.
Addressed in f534297 and 37bc639. The root cause was that ShouldGeneratePRCheckoutStep did not suppress checkout_pr_branch.cjs for pull_request_target workflows — so adding a security assertion on an expectPRCheckout: true case would always fail. The fix: ShouldGeneratePRCheckoutStep now returns false for pull_request_target-only workflows (via the new IsPullRequestTarget field), and the test case is corrected to expectPRCheckout: false. A refs/pull absence assertion is still present as a regression guard for any future test case where a PR checkout step IS expected.
|
@copilot run pr-finisher skill |
- Add frontmatterHasTrigger() helper supporting string/[]any/map forms of the YAML `on:` field (fixes silent bypass when on: is scalar/list) - Add IsPullRequestTarget field to WorkflowData; set it when pull_request_target trigger is present without pull_request co-trigger - ShouldGeneratePRCheckoutStep now returns false for IsPullRequestTarget to prevent checkout_pr_branch.cjs from fetching refs/pull/<n>/head in a privileged context (the trusted base-SHA checkout still happens) - Move auto-disable block after import-merge so imported checkout configs prevent auto-disable - Guard auto-disable with !CheckoutDisabled to avoid redundant writes - Fix mixed-trigger regression: only auto-disable when pull_request trigger is absent - Update integration test: trusted base-SHA checkout case no longer expects the PR checkout step (expectPRCheckout: false) - Add security assertion: PR checkout step must not contain refs/pull - Add unit tests for frontmatterHasTrigger and IsPullRequestTarget - Update checkout.md docs to describe pull_request_target defaults and that checkout: false suppresses both steps Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…ratePRCheckoutStep - Remove head.ref/head_ref from security assertion (only check refs/pull which is the specific pattern emitted by checkout_pr_branch.cjs) - Clarify pr.go comment: suppression is for pull_request_target-only workflows (not all pull_request_target workflows) Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
[pr-sous-chef] @copilot please run the Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "pi.dev"See Network Configuration for more information.
|
|
🎉 This pull request is included in a new release. Release: |
The "Checkout PR branch" step hard-fails on
pull_request_target: closedruns because the head branch is deleted on merge, and on any fork PR since fork branches are inaccessible in the base repo. Additionally,checkout: falsein frontmatter was not suppressing this step — only the defaultactions/checkoutwas skipped.Changes
pkg/workflow/pr.go—ShouldGeneratePRCheckoutStepnow returnsfalsewhenCheckoutDisabledistrue. This fixes the pre-existing bug wherecheckout: falsestill generated the "Checkout PR branch" step (confirmed byai-moderator.lock.ymlwhich hadcheckout: falsebut the step was present).pkg/workflow/workflow_builder.go— Auto-setsCheckoutDisabled = trueforpull_request_targettriggers when nocheckoutkey is present in frontmatter and no checkout configs are set. Users who need checkout can opt in explicitly:pkg/workflow/pull_request_target_validation_test.go— Updated test expectations: with checkout auto-disabled, there's no insecure-checkout warning/error for unconfiguredpull_request_targetworkflows.pkg/workflow/pr_test.go/pr_ready_for_review_checkout_test.go— Added unit and integration tests covering:CheckoutDisabledsuppresses the step,pull_request_targetwith no checkout key produces no "Checkout PR branch" step, explicit checkout mapping still produces the step..github/workflows/ai-moderator.lock.yml— Recompiled; the existingcheckout: falsenow correctly removes the step it was failing to suppress.pull_request_target: closedruns (deleted head branch) #46768run: https://github.com/github/gh-aw/actions/runs/29735453773
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
pi.devSee Network Configuration for more information.