Skip to content
Merged
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
2 changes: 0 additions & 2 deletions .github/workflows/issue-triage-agent.lock.yml

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

2 changes: 0 additions & 2 deletions .github/workflows/weekly-issue-summary.lock.yml

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

11 changes: 8 additions & 3 deletions pkg/workflow/compiler_activation_jobs.go
Original file line number Diff line number Diff line change
Expand Up @@ -738,11 +738,16 @@ func (c *Compiler) buildMainJob(data *WorkflowData, activationJobCreated bool) (
outputs["has_patch"] = "${{ steps.collect_output.outputs.has_patch }}"
}

// Add checkout_pr_success output to track PR checkout status
// 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)")
}
Comment on lines +741 to +750
Copy link

Copilot AI Feb 7, 2026

Choose a reason for hiding this comment

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

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.

Copilot uses AI. Check for mistakes.

// Build job-level environment variables for safe outputs
var env map[string]string
Expand Down
5 changes: 4 additions & 1 deletion pkg/workflow/notify_comment.go
Original file line number Diff line number Diff line change
Expand Up @@ -163,7 +163,10 @@ func (c *Compiler) buildConclusionJob(data *WorkflowData, mainJobName string, sa
}

// 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))
}
Comment on lines 165 to +169
Copy link

Copilot AI Feb 7, 2026

Choose a reason for hiding this comment

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

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.

Copilot uses AI. Check for mistakes.

// Pass assignment error outputs from safe_outputs job if assign-to-agent is configured
if data.SafeOutputs != nil && data.SafeOutputs.AssignToAgent != nil {
Expand Down
10 changes: 8 additions & 2 deletions pkg/workflow/pr.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,12 +9,18 @@ import (

var prLog = logger.New("workflow:pr")

// ShouldGeneratePRCheckoutStep returns true if the checkout-pr step should be generated
// based on the workflow permissions. The step requires contents read access.
func ShouldGeneratePRCheckoutStep(data *WorkflowData) bool {
permParser := NewPermissionsParser(data.Permissions)
return permParser.HasContentsReadAccess()
}

// generatePRReadyForReviewCheckout generates a step to checkout the PR branch when PR context is available
func (c *Compiler) generatePRReadyForReviewCheckout(yaml *strings.Builder, data *WorkflowData) {
prLog.Print("Generating PR checkout step")
// Check that permissions allow contents read access
permParser := NewPermissionsParser(data.Permissions)
if !permParser.HasContentsReadAccess() {
if !ShouldGeneratePRCheckoutStep(data) {
prLog.Print("Skipping PR checkout step: no contents read access")
return // No contents read access, cannot checkout
}
Expand Down
76 changes: 76 additions & 0 deletions pkg/workflow/pr_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
//go:build !integration

package workflow

import (
"testing"

"github.com/stretchr/testify/assert"
)

func TestShouldGeneratePRCheckoutStep(t *testing.T) {
tests := []struct {
name string
permissions string
expected bool
}{
{
name: "with contents read permission",
permissions: "contents: read",
expected: true,
},
{
name: "with contents write permission",
permissions: "contents: write",
expected: true,
},
{
name: "without contents permission",
permissions: "issues: read",
expected: false,
},
{
name: "with read-all shorthand",
permissions: "read-all",
expected: true,
},
{
name: "with write-all shorthand",
permissions: "write-all",
expected: true,
},
{
name: "with none shorthand",
permissions: "none",
expected: false,
},
{
name: "with all: read",
permissions: `all: read`,
expected: true,
},
{
name: "multiple permissions including contents",
permissions: `contents: read
issues: write
pull-requests: read`,
expected: true,
},
{
name: "multiple permissions without contents",
permissions: `issues: write
pull-requests: read`,
expected: false,
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
data := &WorkflowData{
Permissions: tt.permissions,
}
result := ShouldGeneratePRCheckoutStep(data)
assert.Equal(t, tt.expected, result, "ShouldGeneratePRCheckoutStep() result mismatch")
})
}
}
Loading