Skip to content
Closed
Show file tree
Hide file tree
Changes from 4 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
81 changes: 47 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,56 @@ 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,
// so two outputs targeting the same repo result in a single checkout step.
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.

Doc comment mismatch: buildSafeOutputsCheckoutManager says the manager “deduplicates registrations that share the same repository”, but CheckoutManager deduplication is keyed on (repository, path). Updating the comment to reflect (repository, path) (and that safe_outputs registrations currently always use the workspace root path) would avoid confusion for future readers.

Suggested change
// CheckoutManager deduplicates registrations that share the same repository,
// so two outputs targeting the same repo result in a single checkout step.
// CheckoutManager deduplicates registrations that share the same (repository, path)
// pair, and safe_outputs currently always use the workspace root path as the
// checkout path. As a result, two outputs targeting the same repo result in a
// single checkout step.

Copilot uses AI. Check for mistakes.
//
// 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 +135,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 +157,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 +169,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 +188,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
61 changes: 61 additions & 0 deletions pkg/workflow/compiler_safe_outputs_steps_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -264,6 +264,67 @@ 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{
// Deduplication: same repo registered by both outputs → exactly one checkout
"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.
},
{
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
Loading