Fix invalid checkout-pr output references in workflows without contents permission#14286
Fix invalid checkout-pr output references in workflows without contents permission#14286
Conversation
- Add ShouldGeneratePRCheckoutStep() helper to check if checkout-pr step will be generated - Make checkout_pr_success output conditional based on contents read permission - Update notify_comment.go to conditionally add GH_AW_CHECKOUT_PR_SUCCESS env var - Add comprehensive unit tests for ShouldGeneratePRCheckoutStep() - Recompile all workflows - fixes issue-triage-agent and weekly-issue-summary Fixes #<issue_number> Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes invalid GitHub Actions expressions produced by the workflow compiler when the checkout-pr step is skipped due to missing contents: read permissions, by making downstream references conditional on whether the step will be generated.
Changes:
- Added
ShouldGeneratePRCheckoutStep()to centralize the “has contents read access” check for PR checkout generation. - Made
checkout_pr_successjob output andGH_AW_CHECKOUT_PR_SUCCESSenv var conditional on that permission check. - Updated the affected locked workflows to remove invalid
steps.checkout-pr.outputs.*/needs.*.outputs.*references whencontentspermission isn’t present.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/pr_test.go | Adds unit tests covering permission inputs for ShouldGeneratePRCheckoutStep(). |
| pkg/workflow/pr.go | Introduces ShouldGeneratePRCheckoutStep() and reuses it for PR checkout step generation gating. |
| pkg/workflow/notify_comment.go | Conditionally injects GH_AW_CHECKOUT_PR_SUCCESS into conclusion/failure handling env vars. |
| pkg/workflow/compiler_activation_jobs.go | Conditionally publishes checkout_pr_success job output only when checkout-pr step will exist. |
| .github/workflows/weekly-issue-summary.lock.yml | Removes invalid checkout_pr_success output and env var references from the lockfile. |
| .github/workflows/issue-triage-agent.lock.yml | Removes invalid checkout_pr_success output and env var references from the lockfile. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Add checkout_pr_success output to track PR checkout status only if the checkout-pr step will be generated | ||
| // This is used by the conclusion job to skip failure handling when checkout fails | ||
| // (e.g., when PR is merged and branch is deleted) | ||
| outputs["checkout_pr_success"] = "${{ steps.checkout-pr.outputs.checkout_pr_success || 'true' }}" | ||
| compilerActivationJobsLog.Print("Added checkout_pr_success output") | ||
| // The checkout-pr step is only generated when the workflow has contents read permission | ||
| if ShouldGeneratePRCheckoutStep(data) { | ||
| outputs["checkout_pr_success"] = "${{ steps.checkout-pr.outputs.checkout_pr_success || 'true' }}" | ||
| compilerActivationJobsLog.Print("Added checkout_pr_success output (workflow has contents read access)") | ||
| } else { | ||
| compilerActivationJobsLog.Print("Skipped checkout_pr_success output (workflow lacks contents read access)") | ||
| } |
There was a problem hiding this comment.
The new conditional addition of checkout_pr_success output isn’t covered by unit tests. Since pkg/workflow/compiler_activation_jobs_test.go already exercises buildMainJob, add assertions for both cases (permissions with contents read/write vs. without contents) to ensure the output key is present/absent as expected and prevent regressions to invalid steps.checkout-pr.outputs.* references.
| // Add checkout_pr_success to detect PR checkout failures (e.g., PR merged and branch deleted) | ||
| agentFailureEnvVars = append(agentFailureEnvVars, fmt.Sprintf(" GH_AW_CHECKOUT_PR_SUCCESS: ${{ needs.%s.outputs.checkout_pr_success }}\n", mainJobName)) | ||
| // Only add if the checkout-pr step will be generated (requires contents read access) | ||
| if ShouldGeneratePRCheckoutStep(data) { | ||
| agentFailureEnvVars = append(agentFailureEnvVars, fmt.Sprintf(" GH_AW_CHECKOUT_PR_SUCCESS: ${{ needs.%s.outputs.checkout_pr_success }}\n", mainJobName)) | ||
| } |
There was a problem hiding this comment.
GH_AW_CHECKOUT_PR_SUCCESS is now conditionally added based on permissions, but pkg/workflow/notify_comment_test.go doesn’t assert either the presence or absence of this env var. Add a test (or extend an existing one) covering both branches so workflows without contents: read don’t accidentally reintroduce invalid needs.<job>.outputs.checkout_pr_success references.
Two workflows (
issue-triage-agent,weekly-issue-summary) referencesteps.checkout-pr.outputs.checkout_pr_successbut lackcontents: readpermission, causing the step to be skipped. The compiler unconditionally added the output reference, creating invalid workflow expressions.Changes
ShouldGeneratePRCheckoutStep()helper to check contents read permissioncheckout_pr_successoutput conditional incompiler_activation_jobs.go(only added when step will be generated)GH_AW_CHECKOUT_PR_SUCCESSenv var conditional innotify_comment.go(conclusion job)Pattern
Follows existing pattern for conditional outputs (e.g.,
secret_verification_result):Workflows with
permissions: { issues: read }no longer generate the invalid reference. Workflows withcontents: readpreserve existing behavior.Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.