Disable checkout by default for pull_request_target workflows

docs/adr/46771-disable-checkout-by-default-for-pull-request-target.md

@@ -0,0 +1,50 @@
+# ADR-46771: Disable PR Branch Checkout by Default for pull_request_target Workflows
+
+**Date**: 2026-07-20
+**Status**: Draft
+**Deciders**: pelikhan (via Copilot SWE agent, PR #46771)
+
+---
+
+### Context
+
+The `pull_request_target` GitHub Actions trigger runs in the context of the base branch, which is a security boundary that enables accessing repository secrets from fork PRs. The workflow compiler previously generated a "Checkout PR branch" step by default whenever the workflow had `contents: read` permission, regardless of the trigger type.
+
+For `pull_request_target` workflows, the "Checkout PR branch" step hard-fails in two common scenarios: (1) when the event type is `closed`, because the head branch is deleted on merge; and (2) on any fork PR, because the fork's head branch is inaccessible from the base repository. A pre-existing bug compounded this: the `checkout: false` frontmatter key was not suppressing the PR checkout step — it only skipped the default `actions/checkout` — so users who set `checkout: false` still saw the step generated and failing.
+
+### Decision
+
+We will auto-disable the "Checkout PR branch" step for `pull_request_target` workflows at compile time when no explicit `checkout:` key is present in the frontmatter and no checkout configurations are set. Users who require PR branch checkout for `pull_request_target` can opt in by providing an explicit checkout mapping (e.g., pinning to `base.sha`). We will also fix the `ShouldGeneratePRCheckoutStep` function to respect the `CheckoutDisabled` flag, so `checkout: false` in frontmatter correctly suppresses the step.
+
+### Alternatives Considered
+
+#### Alternative 1: Make the Checkout Step Resilient with `continue-on-error`
+
+Add `continue-on-error: true` to the generated "Checkout PR branch" step, or add explicit error handling in the step script, so that failures on merged/fork PRs do not fail the whole workflow.
+
+This was not chosen because it silently hides failures — the step would report as successful even when checkout did not happen, making downstream steps that depend on the checked-out state behave unpredictably. It also does not address the security concern of attempting to access fork branch content in a `pull_request_target` context.
+
+#### Alternative 2: Runtime Detection and Dynamic Skip
+
+Detect at runtime (inside the step) whether the head branch is accessible — checking if the PR is merged or from a fork — and emit a skip-with-warning rather than executing checkout.
+
+This was not chosen because it adds runtime complexity and latency (the check requires an API call), and the failure condition can be determined statically from the trigger type. Compile-time disabling is simpler, more predictable, and avoids unnecessary step execution in the generated workflow YAML entirely.
+
+### Consequences
+
+#### Positive
+- `pull_request_target` workflows no longer hard-fail on `closed` events or fork PRs due to the checkout step attempting to access a deleted or inaccessible branch.
+- The pre-existing bug where `checkout: false` did not suppress the "Checkout PR branch" step is fixed; `CheckoutDisabled` is now respected by `ShouldGeneratePRCheckoutStep`.
+- Reduces the attack surface from insecure checkouts in `pull_request_target` contexts, since checking out fork code in this trigger is a known security anti-pattern.
+
+#### Negative
+- Existing `pull_request_target` workflows that previously relied on the implicit checkout step (without an explicit `checkout:` key) will have the step removed after recompile. In practice these workflows were already failing on closed/fork events, so the blast radius is low.
+- Users who intentionally need checkout in a `pull_request_target` context must now explicitly configure it, adding a small onboarding friction for new workflows.
+
+#### Neutral
+- The `ai-moderator.lock.yml` compiled workflow is updated to reflect the removal of the checkout step and its associated `checkout_pr_success` output variable and downstream references.
+- Validation test expectations for `pull_request_target` are updated: cases that previously emitted an "insecure checkout" error/warning now only emit the "dangerous-trigger" warning, since checkout is no longer auto-generated.
+
+---
+
+*ADR created by [adr-writer agent]. Review and finalize before changing status from Draft to Accepted.*

docs/src/content/docs/reference/checkout.md

@@ -7,7 +7,7 @@ sidebar:
 
 The `checkout:` frontmatter field controls how `actions/checkout` is invoked in the agent job. Configure custom checkout settings, check out multiple repositories, or disable checkout entirely.
 
-By default, the agent checks out the repository where the workflow is running with a shallow fetch (`fetch-depth: 1`). If triggered by a pull request event, it also checks out the PR head ref. For most workflows, this default checkout is sufficient and no `checkout:` configuration is necessary.
+By default, the agent checks out the repository where the workflow is running with a shallow fetch (`fetch-depth: 1`). If triggered by a `pull_request` event, it also checks out the PR head ref. For `pull_request_target` events, checkout of the PR head branch is **disabled by default** — the head branch may be deleted (merged/closed PRs) or inaccessible (fork PRs), causing the step to hard-fail. For most workflows, this default checkout is sufficient and no `checkout:` configuration is necessary.
 
 Use `checkout:` when you need to check out additional branches, check out multiple repositories, or to disable checkout entirely for workflows that don't need to access code or can access code dynamically through the GitHub Tools.
 
@@ -112,14 +112,31 @@ Fetch everything the workflow needs at checkout time using `fetch-depth` and [`f
 
 ## Disabling Checkout (`checkout: false`)
 
-Set `checkout: false` to suppress the default `actions/checkout` step entirely. Use this for workflows that access repositories through MCP servers or other mechanisms that do not require a local clone:
+Set `checkout: false` to suppress both the default `actions/checkout` step and the PR-specific "Checkout PR branch" step entirely. Use this for workflows that access repositories through MCP servers or other mechanisms that do not require a local clone:
 
 ```yaml wrap
 checkout: false
 ```
 
 This is equivalent to omitting the checkout step from the agent job. Custom dev-mode steps (such as "Checkout actions folder") are unaffected.
 
+## `pull_request_target` Checkout
+
+For `pull_request_target` workflows, the "Checkout PR branch" step is auto-disabled when no `checkout:` key is present in frontmatter. This prevents hard failures when the PR head branch is deleted (merged/closed events) or inaccessible (fork PRs).
+
+To check out a trusted ref such as the base SHA, opt in explicitly with a `checkout:` mapping:
+
+```yaml wrap
+on:
+  pull_request_target:
+    types: [opened, synchronize]
+checkout:
+  repository: ${{ github.repository }}
+  ref: ${{ github.event.pull_request.base.sha }}
+```
+
+This produces the "Checkout PR branch" step pointing at the safe base ref. Do **not** use `github.event.pull_request.head.sha` or `refs/pull/.../head` in a privileged `pull_request_target` job — that would check out untrusted code in a context with write permissions.
+
 ## Marking a Primary Repository (`current: true`)
 
 When a workflow running from a central repository targets a different repository, use `current: true` to tell the agent which repository to treat as its primary working target. The agent uses this as the default for all GitHub operations (creating issues, opening PRs, reading content) unless the prompt instructs otherwise. When omitted, the agent defaults to the repository where the workflow is running.

pkg/workflow/pr.go

@@ -10,8 +10,16 @@ import (
 var prLog = logger.New("workflow:pr")
 
 // ShouldGeneratePRCheckoutStep returns true if the checkout-pr step should be generated
-// based on the workflow permissions. The step requires contents read access.
+// based on the workflow permissions and checkout configuration.
+// The step requires contents read access and checkout must not be disabled.
+// For pull_request_target-only workflows (where pull_request is not also a trigger),
+// the step is always suppressed regardless of checkout configuration:
+// checkout_pr_branch.cjs fetches refs/pull/<n>/head (the untrusted PR head),
+// which replaces any safe base-SHA checkout and is inaccessible for fork PRs.
 func ShouldGeneratePRCheckoutStep(data *WorkflowData) bool {
+	if data.CheckoutDisabled || data.IsPullRequestTarget {
+		return false
+	}
 	if data.CachedPermissions != nil {
 		return data.CachedPermissions.HasContentsReadAccess()
 	}

pkg/workflow/pr_ready_for_review_checkout_test.go

@@ -224,3 +224,133 @@ Test workflow with pull_request triggers.
 		}
 	}
 }
+
+// TestPullRequestTargetCheckoutDisabledByDefault verifies that pull_request_target workflows
+// do not generate a "Checkout PR branch" step when checkout is not explicitly configured.
+// This prevents the step from hard-failing when head branches are deleted (merged PRs) or
+// inaccessible (fork PRs).
+func TestPullRequestTargetCheckoutDisabledByDefault(t *testing.T) {
+	tests := []struct {
+		name             string
+		workflowContent  string
+		expectPRCheckout bool
+		description      string
+	}{
+		{
+			name: "pull_request_target with no checkout key - no PR checkout step",
+			workflowContent: `---
+on:
+  pull_request_target:
+    types: [closed]
+permissions:
+  contents: read
+  pull-requests: read
+engine: claude
+strict: false
+---
+
+# Thank you note workflow
+Workflow triggered when a PR is closed.
+`,
+			expectPRCheckout: false,
+			description:      "pull_request_target without explicit checkout key should not generate 'Checkout PR branch' step",
+		},
+		{
+			name: "pull_request_target with checkout: false - no PR checkout step",
+			workflowContent: `---
+on:
+  pull_request_target:
+    types: [closed]
+permissions:
+  contents: read
+  pull-requests: read
+engine: claude
+strict: false
+checkout: false
+---
+
+# Thank you note workflow
+Workflow triggered when a PR is closed.
+`,
+			expectPRCheckout: false,
+			description:      "pull_request_target with explicit checkout: false should not generate 'Checkout PR branch' step",
+		},
+		{
+			name: "pull_request_target with trusted checkout mapping - no PR checkout step",
+			workflowContent: `---
+on:
+  pull_request_target:
+    types: [opened]
+permissions:
+  contents: read
+  pull-requests: read
+engine: claude
+checkout:
+  repository: ${{ github.repository }}
+  ref: ${{ github.event.pull_request.base.sha }}
+---
+
+# PR review workflow
+Workflow triggered when a PR is opened with a trusted base checkout.
+`,
+			expectPRCheckout: false,
+			description:      "pull_request_target with explicit checkout mapping should NOT generate 'Checkout PR branch' step (checkout_pr_branch.cjs fetches refs/pull/<n>/head, not the trusted base SHA)",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			// Create temporary directory for test
+			tempDir, err := os.MkdirTemp("", "prt-checkout-test")
+			if err != nil {
+				t.Fatalf("Failed to create temp dir: %v", err)
+			}
+			defer os.RemoveAll(tempDir)
+
+			// Create workflows directory
+			workflowsDir := filepath.Join(tempDir, ".github", "workflows")
+			if err := os.MkdirAll(workflowsDir, 0755); err != nil {
+				t.Fatalf("Failed to create workflows directory: %v", err)
+			}
+
+			// Write test workflow file
+			workflowPath := filepath.Join(workflowsDir, "test-prt-workflow.md")
+			if err := os.WriteFile(workflowPath, []byte(tt.workflowContent), 0644); err != nil {
+				t.Fatalf("Failed to write workflow file: %v", err)
+			}
+
+			// Compile workflow
+			compiler := NewCompiler()
+			compiler.SetActionMode(ActionModeDev)
+			if err := compiler.CompileWorkflow(workflowPath); err != nil {
+				t.Fatalf("Failed to compile workflow: %v", err)
+			}
+
+			// Read generated lock file
+			lockPath := filepath.Join(workflowsDir, "test-prt-workflow.lock.yml")
+			lockContent, err := os.ReadFile(lockPath)
+			if err != nil {
+				t.Fatalf("Failed to read lock file: %v", err)
+			}
+			lockStr := string(lockContent)
+
+			// Check for PR checkout step
+			hasPRCheckout := strings.Contains(lockStr, "Checkout PR branch")
+			if hasPRCheckout != tt.expectPRCheckout {
+				t.Errorf("%s: expected PR checkout step: %v, got: %v", tt.description, tt.expectPRCheckout, hasPRCheckout)
+			}
+
+			// When a PR checkout step IS present, verify it does not fetch the insecure
+			// PR-head ref (refs/pull/<n>/head). The trusted-base opt-in must use a safe ref
+			// such as the base SHA — checkout_pr_branch.cjs always fetches refs/pull/N/head.
+			// We check only for "refs/pull" because that is the specific pattern emitted by
+			// checkout_pr_branch.cjs; broader substrings (e.g. "head_ref") could appear in
+			// unrelated step names or comments and would produce false positives.
+			if hasPRCheckout {
+				if strings.Contains(lockStr, "refs/pull") {
+					t.Errorf("%s: expected trusted base checkout but lock file contains insecure pattern \"refs/pull\"", tt.description)
+				}
+			}
+		})
+	}
+}

pkg/workflow/pr_test.go

@@ -76,6 +76,36 @@ pull-requests: read`,
 	}
 }
 
+func TestShouldGeneratePRCheckoutStep_CheckoutDisabled(t *testing.T) {
+	t.Run("returns false when CheckoutDisabled is true even with contents read", func(t *testing.T) {
+		data := &WorkflowData{
+			Permissions:      "contents: read",
+			CheckoutDisabled: true,
+		}
+		result := ShouldGeneratePRCheckoutStep(data)
+		assert.False(t, result, "ShouldGeneratePRCheckoutStep() should return false when CheckoutDisabled is true")
+	})
+
+	t.Run("returns true when CheckoutDisabled is false with contents read", func(t *testing.T) {
+		data := &WorkflowData{
+			Permissions:      "contents: read",
+			CheckoutDisabled: false,
+		}
+		result := ShouldGeneratePRCheckoutStep(data)
+		assert.True(t, result, "ShouldGeneratePRCheckoutStep() should return true when CheckoutDisabled is false and permissions allow")
+	})
+
+	t.Run("returns false when IsPullRequestTarget is true even with contents read and explicit checkout", func(t *testing.T) {
+		data := &WorkflowData{
+			Permissions:         "contents: read",
+			CheckoutDisabled:    false,
+			IsPullRequestTarget: true,
+		}
+		result := ShouldGeneratePRCheckoutStep(data)
+		assert.False(t, result, "ShouldGeneratePRCheckoutStep() should return false for pull_request_target workflows to prevent refs/pull/<n>/head checkout")
+	})
+}
+
 func TestGeneratePRReadyForReviewCheckout_IncludesWorkflowDispatchIssueCommentContext(t *testing.T) {
 	compiler := NewCompiler()
 	var yaml strings.Builder
@@ -92,3 +122,32 @@ func TestGeneratePRReadyForReviewCheckout_IncludesWorkflowDispatchIssueCommentCo
 	assert.Contains(t, rendered, "fromJSON(github.event.inputs.aw_context || '{}').item_type == 'pull_request'")
 	assert.NotContains(t, rendered, "fromJSON(github.event.inputs.aw_context || '{}').event_type")
 }
+
+func TestFrontmatterHasTrigger(t *testing.T) {
+	tests := []struct {
+		name     string
+		onVal    any
+		trigger  string
+		expected bool
+	}{
+		// scalar string form: on: pull_request_target
+		{name: "scalar matches", onVal: "pull_request_target", trigger: "pull_request_target", expected: true},
+		{name: "scalar no match", onVal: "push", trigger: "pull_request_target", expected: false},
+		// sequence form: on: [pull_request_target, push]
+		{name: "slice matches first", onVal: []any{"pull_request_target", "push"}, trigger: "pull_request_target", expected: true},
+		{name: "slice matches second", onVal: []any{"push", "pull_request_target"}, trigger: "pull_request_target", expected: true},
+		{name: "slice no match", onVal: []any{"push", "schedule"}, trigger: "pull_request_target", expected: false},
+		// mapping form: on:\n  pull_request_target:\n    types: [closed]
+		{name: "map matches", onVal: map[string]any{"pull_request_target": map[string]any{"types": []any{"closed"}}}, trigger: "pull_request_target", expected: true},
+		{name: "map no match", onVal: map[string]any{"push": nil}, trigger: "pull_request_target", expected: false},
+		// nil / unknown
+		{name: "nil returns false", onVal: nil, trigger: "pull_request_target", expected: false},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got := frontmatterHasTrigger(tt.onVal, tt.trigger)
+			assert.Equal(t, tt.expected, got)
+		})
+	}
+}