fix(compiler): honor push-to-pull-request-branch target-repo in shared PR checkout steps#30474
Conversation
…est target-repo in shared PR checkout steps Agent-Logs-Url: https://github.com/github/gh-aw/sessions/3d130500-35bf-48b0-84d9-538e382c4714 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR updates the safe-outputs compiler so shared PR checkout steps can derive the checkout repository from additional safe-output configs, not just create-pull-request.
Changes:
- Extended
buildSharedPRCheckoutStepstarget-repo resolution to fall back fromcreate-pull-requesttopush-to-pull-request-branch, thenupdate-pull-request, then trial/default repos. - Added unit tests covering push-only target-repo resolution, update-pull-request fallback behavior, and one precedence case where
create-pull-requestwins. - Keeps cross-repo checkout behavior aligned with the safer base-ref expression that avoids
github.ref_name.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/compiler_safe_outputs_steps.go |
Expands target-repo selection logic for shared PR checkout/git setup. |
pkg/workflow/compiler_safe_outputs_steps_test.go |
Adds regression tests for new target-repo fallback and precedence behavior. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 2
| } else if data.SafeOutputs.UpdatePullRequests != nil && data.SafeOutputs.UpdatePullRequests.TargetRepoSlug != "" { | ||
| targetRepoSlug = data.SafeOutputs.UpdatePullRequests.TargetRepoSlug | ||
| consolidatedSafeOutputsStepsLog.Printf("Using target-repo from update-pull-request: %s", targetRepoSlug) |
| { | ||
| name: "update-pull-request with target-repo and no create-pull-request", | ||
| safeOutputs: &SafeOutputsConfig{ | ||
| UpdatePullRequests: &UpdatePullRequestsConfig{ | ||
| UpdateEntityConfig: UpdateEntityConfig{ | ||
| SafeOutputTargetConfig: SafeOutputTargetConfig{TargetRepoSlug: "microsoft/vscode"}, | ||
| }, | ||
| }, | ||
| PushToPullRequestBranch: &PushToPullRequestBranchConfig{}, |
|
@copilot review all comments |
…fallback chain; add precedence test Agent-Logs-Url: https://github.com/github/gh-aw/sessions/4e6a7f4b-10b3-4990-8cff-0d1b2fbfb4e1 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Addressed both reviewer comments in cd5ecbe: Comment 1 (lines 87-89): Removed Comment 2 (test lines 247-255): Replaced the incorrect fallback test with two cases that cover the real semantics:
Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
buildSharedPRCheckoutStepsonly consultedCreatePullRequests.TargetRepoSlugwhen resolving which repo to check out in thesafe_outputsjob. Workflows that setpush-to-pull-request-branch.target-repowithoutcreate-pull-requestgot a checkout against the workflow repo, causinggit ls-remote --exit-code --heads origin <branch>to exit 2.update-pull-requestis an API-only operation (no git push), so itstarget-repois intentionally excluded from the git checkout resolution — including it would silently retargetoriginforpush-to-pull-request-branchwhen the push config has notarget-repoof its own, causing a repo/token mismatch.Changes
compiler_safe_outputs_steps.go— extend target repo resolution inbuildSharedPRCheckoutStepsto fall through toPushToPullRequestBranch.TargetRepoSlugwhencreate-pull-requestis absent. Priority order:create-pull-request.target-repopush-to-pull-request-branch.target-repocompiler_safe_outputs_steps_test.go— new cases covering:push-to-pull-request-branchwithtarget-repoonly (the core fix)update-pull-request.target-repodoes not affect shared git checkout (API-only operation)push-to-pull-request-branch.target-repotakes precedence overupdate-pull-request.target-repocreate-pull-request.target-repotakes precedence when all are set