diff --git a/.github/workflows/smoke-update-cross-repo-pr.lock.yml b/.github/workflows/smoke-update-cross-repo-pr.lock.yml index 9abc4358bab..753e87d26a8 100644 --- a/.github/workflows/smoke-update-cross-repo-pr.lock.yml +++ b/.github/workflows/smoke-update-cross-repo-pr.lock.yml @@ -1210,6 +1210,7 @@ jobs: if: (!cancelled()) && needs.agent.result != 'skipped' && contains(needs.agent.outputs.output_types, 'push_to_pull_request_branch') uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 with: + repository: githubnext/gh-aw-side-repo ref: ${{ github.base_ref || github.event.pull_request.base.ref || github.ref_name || github.event.repository.default_branch }} token: ${{ secrets.GH_AW_SIDE_REPO_PAT }} persist-credentials: false @@ -1217,7 +1218,7 @@ jobs: - name: Configure Git credentials if: (!cancelled()) && needs.agent.result != 'skipped' && contains(needs.agent.outputs.output_types, 'push_to_pull_request_branch') env: - REPO_NAME: ${{ github.repository }} + REPO_NAME: "githubnext/gh-aw-side-repo" SERVER_URL: ${{ github.server_url }} GIT_TOKEN: ${{ secrets.GH_AW_SIDE_REPO_PAT }} run: | diff --git a/pkg/workflow/checkout_manager.go b/pkg/workflow/checkout_manager.go index cf0ee97562e..f25824fe4a3 100644 --- a/pkg/workflow/checkout_manager.go +++ b/pkg/workflow/checkout_manager.go @@ -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 { diff --git a/pkg/workflow/compiler_safe_outputs_steps.go b/pkg/workflow/compiler_safe_outputs_steps.go index 3af1939da47..495431ea9a5 100644 --- a/pkg/workflow/compiler_safe_outputs_steps.go +++ b/pkg/workflow/compiler_safe_outputs_steps.go @@ -68,6 +68,43 @@ 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. @@ -75,10 +112,13 @@ 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 @@ -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), @@ -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 @@ -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{ @@ -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", diff --git a/pkg/workflow/compiler_safe_outputs_steps_test.go b/pkg/workflow/compiler_safe_outputs_steps_test.go index 946964bc819..3a42a7516ca 100644 --- a/pkg/workflow/compiler_safe_outputs_steps_test.go +++ b/pkg/workflow/compiler_safe_outputs_steps_test.go @@ -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", @@ -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"`, + }, + 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{ @@ -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) + } }) } }