Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion .github/workflows/smoke-update-cross-repo-pr.lock.yml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

46 changes: 46 additions & 0 deletions pkg/workflow/checkout_manager.go
Original file line number Diff line number Diff line change
Expand Up @@ -265,6 +265,52 @@ func (cm *CheckoutManager) add(cfg *CheckoutConfig) {
}
}

// GetPrimaryTargetRepo returns the repository of the first checkout entry in the manager,
// or an empty string if no checkouts are configured.
//
// Used by the safe_outputs job to determine the git remote target for PR operations
// when a cross-repo target is configured (e.g. via push-to-pull-request-branch or
// create-pull-request target-repo settings).
func (cm *CheckoutManager) GetPrimaryTargetRepo() string {
if len(cm.ordered) == 0 {
return ""
}
return cm.ordered[0].key.repository
}

// GenerateSafeOutputsCheckoutStep generates an actions/checkout YAML step for PR
// operations in the safe_outputs job.
//
// Parameters:
// - condition: optional step condition (if nil the if: line is omitted)
// - ref: Git ref expression to checkout (e.g. base branch expression)
// - token: GitHub token for authentication
// - getActionPin: resolves action references to pinned SHA form
//
// The step checks out GetPrimaryTargetRepo() when a target repository is configured,
// or the current repository when none is set.
// Returns a slice of YAML lines (each ending with \n).
func (cm *CheckoutManager) GenerateSafeOutputsCheckoutStep(condition ConditionNode, ref, token string, getActionPin func(string) string) []string {
targetRepo := cm.GetPrimaryTargetRepo()
checkoutManagerLog.Printf("Generating safe-outputs checkout step: targetRepo=%q ref=%q", targetRepo, ref)

var steps []string
steps = append(steps, " - name: Checkout repository\n")
if condition != nil {
steps = append(steps, fmt.Sprintf(" if: %s\n", RenderCondition(condition)))
}
steps = append(steps, fmt.Sprintf(" uses: %s\n", getActionPin("actions/checkout")))
steps = append(steps, " with:\n")
if targetRepo != "" {
steps = append(steps, fmt.Sprintf(" repository: %s\n", targetRepo))
}
steps = append(steps, fmt.Sprintf(" ref: %s\n", ref))
steps = append(steps, fmt.Sprintf(" token: %s\n", token))
steps = append(steps, " persist-credentials: false\n")
steps = append(steps, " fetch-depth: 1\n")
return steps
}

// GetDefaultCheckoutOverride returns the resolved checkout for the default workspace
// (empty path, empty repository). Returns nil if the user did not configure one.
func (cm *CheckoutManager) GetDefaultCheckoutOverride() *resolvedCheckout {
Expand Down
82 changes: 48 additions & 34 deletions pkg/workflow/compiler_safe_outputs_steps.go
Original file line number Diff line number Diff line change
Expand Up @@ -68,17 +68,57 @@ func (c *Compiler) buildConsolidatedSafeOutputStep(data *WorkflowData, config Sa
return steps
}

// buildSafeOutputsCheckoutManager creates a CheckoutManager by having each
// enabled PR safe output independently register its checkout requirement.
// CheckoutManager deduplicates registrations that share the same (repository, path)
// pair, and safe_outputs always use the workspace root path. As a result, two
// outputs targeting the same repo result in a single checkout step.
//
// Registration order determines which repo GenerateSafeOutputsCheckoutStep
// uses when multiple unique repos are present (create-pull-request registers
// before push-to-pull-request-branch).
//
// The trial-mode override is applied only when no safe output registered a
// cross-repo checkout, preserving default-repo checkout behaviour for the
// common same-repo case.
func (c *Compiler) buildSafeOutputsCheckoutManager(safeOutputs *SafeOutputsConfig) *CheckoutManager {
var configs []*CheckoutConfig

// Each safe output independently registers the checkout it needs.
// CheckoutManager deduplicates entries with the same (repository, path) key,
// so two outputs targeting the same cross-repo destination produce one step.
if safeOutputs.CreatePullRequests != nil && safeOutputs.CreatePullRequests.TargetRepoSlug != "" {
configs = append(configs, &CheckoutConfig{Repository: safeOutputs.CreatePullRequests.TargetRepoSlug})
consolidatedSafeOutputsStepsLog.Printf("buildSafeOutputsCheckoutManager: create-pull-request registered checkout for %s", safeOutputs.CreatePullRequests.TargetRepoSlug)
}
if safeOutputs.PushToPullRequestBranch != nil && safeOutputs.PushToPullRequestBranch.TargetRepoSlug != "" {
configs = append(configs, &CheckoutConfig{Repository: safeOutputs.PushToPullRequestBranch.TargetRepoSlug})
consolidatedSafeOutputsStepsLog.Printf("buildSafeOutputsCheckoutManager: push-to-pull-request-branch registered checkout for %s", safeOutputs.PushToPullRequestBranch.TargetRepoSlug)
}

// Trial mode: apply override only when no safe output registered a cross-repo checkout.
if len(configs) == 0 && c.trialMode && c.trialLogicalRepoSlug != "" {
configs = append(configs, &CheckoutConfig{Repository: c.trialLogicalRepoSlug})
consolidatedSafeOutputsStepsLog.Printf("buildSafeOutputsCheckoutManager: trial mode registered checkout for %s", c.trialLogicalRepoSlug)
}

return NewCheckoutManager(configs)
}

// buildSharedPRCheckoutSteps builds checkout and git configuration steps that are shared
// between create-pull-request and push-to-pull-request-branch operations.
// These steps are added once with a combined condition to avoid duplication.
func (c *Compiler) buildSharedPRCheckoutSteps(data *WorkflowData) []string {
consolidatedSafeOutputsStepsLog.Print("Building shared PR checkout steps")
var steps []string

// Build a CheckoutManager from safe-output configurations to determine
// the target repository for PR operations.
cm := c.buildSafeOutputsCheckoutManager(data.SafeOutputs)

// Determine which token to use for checkout
// Uses computeEffectivePRCheckoutToken for consistent token resolution (GitHub App or PAT chain)
checkoutToken, _ := computeEffectivePRCheckoutToken(data.SafeOutputs)
gitRemoteToken := checkoutToken

// Build combined condition: execute if either create_pull_request or push_to_pull_request_branch will run
var condition ConditionNode
Expand All @@ -96,17 +136,6 @@ func (c *Compiler) buildSharedPRCheckoutSteps(data *WorkflowData) []string {
condition = BuildSafeOutputType("push_to_pull_request_branch")
}

// Determine target repository for checkout and git config
// Priority: create-pull-request target-repo > trialLogicalRepoSlug > default (source repo)
var targetRepoSlug string
if data.SafeOutputs.CreatePullRequests != nil && data.SafeOutputs.CreatePullRequests.TargetRepoSlug != "" {
targetRepoSlug = data.SafeOutputs.CreatePullRequests.TargetRepoSlug
consolidatedSafeOutputsStepsLog.Printf("Using target-repo from create-pull-request: %s", targetRepoSlug)
} else if c.trialMode && c.trialLogicalRepoSlug != "" {
targetRepoSlug = c.trialLogicalRepoSlug
consolidatedSafeOutputsStepsLog.Printf("Using trialLogicalRepoSlug: %s", targetRepoSlug)
}

// Determine the ref (branch) to checkout
// Priority: create-pull-request base-branch > fallback expression
// This is critical: we must checkout the base branch, not github.sha (the triggering commit),
Expand All @@ -129,7 +158,7 @@ func (c *Compiler) buildSharedPRCheckoutSteps(data *WorkflowData) []string {
// * event trigger context
// * ideally repository context too
// So safe outputs are "self-describing" and already know which base branch, repository etc. they're
// targeting.  Then a lot of this gnarly event code will be only on the "front end" (prepping the
// targeting. Then a lot of this gnarly event code will be only on the "front end" (prepping the
// coding agent) not the "backend" (applying the safe outputs)
const baseBranchFallbackExpr = "${{ github.base_ref || github.event.pull_request.base.ref || github.ref_name || github.event.repository.default_branch }}"
var checkoutRef string
Expand All @@ -141,32 +170,17 @@ func (c *Compiler) buildSharedPRCheckoutSteps(data *WorkflowData) []string {
consolidatedSafeOutputsStepsLog.Printf("Using fallback base branch expression for checkout ref")
}

// Step 1: Checkout repository with conditional execution
steps = append(steps, " - name: Checkout repository\n")
steps = append(steps, fmt.Sprintf(" if: %s\n", RenderCondition(condition)))
steps = append(steps, fmt.Sprintf(" uses: %s\n", GetActionPin("actions/checkout")))
steps = append(steps, " with:\n")

// Set repository parameter if checking out a different repository
if targetRepoSlug != "" {
steps = append(steps, fmt.Sprintf(" repository: %s\n", targetRepoSlug))
consolidatedSafeOutputsStepsLog.Printf("Added repository parameter: %s", targetRepoSlug)
}

// Set ref to checkout the base branch, not github.sha
steps = append(steps, fmt.Sprintf(" ref: %s\n", checkoutRef))
steps = append(steps, fmt.Sprintf(" token: %s\n", checkoutToken))
steps = append(steps, " persist-credentials: false\n")
steps = append(steps, " fetch-depth: 1\n")
// Step 1: Checkout repository with conditional execution (delegated to CheckoutManager)
steps = append(steps, cm.GenerateSafeOutputsCheckoutStep(condition, checkoutRef, checkoutToken, GetActionPin)...)

// Step 2: Configure Git credentials with conditional execution
// Security: Pass GitHub token through environment variable to prevent template injection

// Determine REPO_NAME value based on target repository
repoNameValue := "${{ github.repository }}"
if targetRepoSlug != "" {
repoNameValue = fmt.Sprintf("%q", targetRepoSlug)
consolidatedSafeOutputsStepsLog.Printf("Using target repo for REPO_NAME: %s", targetRepoSlug)
if targetRepo := cm.GetPrimaryTargetRepo(); targetRepo != "" {
repoNameValue = fmt.Sprintf("%q", targetRepo)
consolidatedSafeOutputsStepsLog.Printf("Using target repo for REPO_NAME: %s", targetRepo)
}

gitConfigSteps := []string{
Expand All @@ -175,7 +189,7 @@ func (c *Compiler) buildSharedPRCheckoutSteps(data *WorkflowData) []string {
" env:\n",
fmt.Sprintf(" REPO_NAME: %s\n", repoNameValue),
" SERVER_URL: ${{ github.server_url }}\n",
fmt.Sprintf(" GIT_TOKEN: %s\n", gitRemoteToken),
fmt.Sprintf(" GIT_TOKEN: %s\n", checkoutToken),
" run: |\n",
" git config --global user.email \"github-actions[bot]@users.noreply.github.com\"\n",
" git config --global user.name \"github-actions[bot]\"\n",
Expand Down
73 changes: 73 additions & 0 deletions pkg/workflow/compiler_safe_outputs_steps_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,9 @@ func TestBuildSharedPRCheckoutSteps(t *testing.T) {
trialMode bool
trialRepo string
checkContains []string
// checkCounts maps a substring to the exact number of times it must
// appear in the generated YAML. Use this to verify deduplication.
checkCounts map[string]int
}{
{
name: "create pull request only",
Expand Down Expand Up @@ -264,6 +267,71 @@ func TestBuildSharedPRCheckoutSteps(t *testing.T) {
"GIT_TOKEN: ${{ secrets.PUSH_BRANCH_PAT }}",
},
},
{
name: "push-to-pull-request-branch cross-repo uses target-repo for checkout",
safeOutputs: &SafeOutputsConfig{
PushToPullRequestBranch: &PushToPullRequestBranchConfig{
BaseSafeOutputConfig: BaseSafeOutputConfig{
GitHubToken: "${{ secrets.GH_AW_SIDE_REPO_PAT }}",
},
TargetRepoSlug: "githubnext/gh-aw-side-repo",
Target: "1",
},
},
checkContains: []string{
"repository: githubnext/gh-aw-side-repo",
"token: ${{ secrets.GH_AW_SIDE_REPO_PAT }}",
"GIT_TOKEN: ${{ secrets.GH_AW_SIDE_REPO_PAT }}",
`REPO_NAME: "githubnext/gh-aw-side-repo"`,
},
},
{
name: "both safe outputs with same target-repo are deduplicated to one checkout",
safeOutputs: &SafeOutputsConfig{
CreatePullRequests: &CreatePullRequestsConfig{
BaseSafeOutputConfig: BaseSafeOutputConfig{
GitHubToken: "${{ secrets.CROSS_REPO_PAT }}",
},
TargetRepoSlug: "githubnext/gh-aw-side-repo",
},
PushToPullRequestBranch: &PushToPullRequestBranchConfig{
BaseSafeOutputConfig: BaseSafeOutputConfig{
GitHubToken: "${{ secrets.CROSS_REPO_PAT }}",
},
TargetRepoSlug: "githubnext/gh-aw-side-repo",
},
},
checkContains: []string{
"repository: githubnext/gh-aw-side-repo",
`REPO_NAME: "githubnext/gh-aw-side-repo"`,
},
Comment on lines +304 to +307
Copy link

Copilot AI Mar 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new "deduplicated to one checkout" case only asserts that the generated YAML contains the repository/REPO_NAME strings, but it doesn’t verify that only one checkout step (or only one repository: line) was emitted. As written, it would still pass if two identical checkout steps were generated. Consider asserting the count (e.g., strings.Count(stepsContent, "- name: Checkout repository") == 1 or counting repository: githubnext/gh-aw-side-repo).

Copilot uses AI. Check for mistakes.
checkCounts: map[string]int{
// Both outputs register the same repo → CheckoutManager deduplicates
// to exactly one checkout step.
"repository: githubnext/gh-aw-side-repo": 1,
},
},
{
name: "both safe outputs with different target-repos: create-pr repo is used (registered first)",
safeOutputs: &SafeOutputsConfig{
CreatePullRequests: &CreatePullRequestsConfig{
BaseSafeOutputConfig: BaseSafeOutputConfig{
GitHubToken: "${{ secrets.CREATE_PR_PAT }}",
},
TargetRepoSlug: "org/create-pr-repo",
},
PushToPullRequestBranch: &PushToPullRequestBranchConfig{
BaseSafeOutputConfig: BaseSafeOutputConfig{
GitHubToken: "${{ secrets.PUSH_BRANCH_PAT }}",
},
TargetRepoSlug: "org/push-branch-repo",
},
},
checkContains: []string{
"repository: org/create-pr-repo",
`REPO_NAME: "org/create-pr-repo"`,
},
},
{
name: "both operations with create-pr token takes precedence",
safeOutputs: &SafeOutputsConfig{
Expand Down Expand Up @@ -340,6 +408,11 @@ func TestBuildSharedPRCheckoutSteps(t *testing.T) {
for _, expected := range tt.checkContains {
assert.Contains(t, stepsContent, expected, "Expected to find: "+expected)
}

for substr, wantCount := range tt.checkCounts {
gotCount := strings.Count(stepsContent, substr)
assert.Equal(t, wantCount, gotCount, "Expected %q to appear %d time(s)", substr, wantCount)
}
})
}
}
Expand Down