Skip to content

Apply checkout.fetch-depth to safe_outputs checkout steps #33746

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
pelikhan merged 9 commits into from
May 21, 2026
Merged

Apply checkout.fetch-depth to safe_outputs checkout steps #33746

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
f098cb4
Initial plan
Copilot May 21, 2026
46c3ede
Plan safe_outputs checkout override fix
Copilot May 21, 2026
0b7c311
Apply checkout fetch-depth override to safe_outputs job checkout steps
Copilot May 21, 2026
0e6d226
chore: run make fmt
pelikhan May 21, 2026
283ddb4
Merge remote-tracking branch 'origin/main' into copilot/bug-fix-check…
pelikhan May 21, 2026
05d7024
chore: post-merge cleanup
pelikhan May 21, 2026
6abc601
Merge branch 'main' into copilot/bug-fix-checkout-settings
pelikhan May 21, 2026
e20cd08
Validate checkout.fetch-depth is non-negative
Copilot May 21, 2026
5f64dd6
Merge branch 'main' into copilot/bug-fix-checkout-settings
pelikhan May 21, 2026
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
4 changes: 2 additions & 2 deletions .github/workflows/glossary-maintainer.lock.yml
View file
Open in desktop

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

4 changes: 2 additions & 2 deletions .github/workflows/pr-sous-chef.lock.yml
View file
Open in desktop

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

3 changes: 3 additions & 0 deletions pkg/workflow/checkout_config_parser.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,9 @@ func checkoutConfigFromMap(m map[string]any) (*CheckoutConfig, error) {
default:
return nil, errors.New("checkout.fetch-depth must be an integer")
}
if cfg.FetchDepth != nil && *cfg.FetchDepth < 0 {
return nil, errors.New("checkout.fetch-depth must be >= 0")
}
}

if v, ok := m["sparse-checkout"]; ok {
Expand Down
11 changes: 11 additions & 0 deletions pkg/workflow/checkout_manager_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -281,6 +281,17 @@ func TestParseCheckoutConfigs(t *testing.T) {
assert.Equal(t, 0, *configs[0].FetchDepth, "fetch-depth should be 0")
})

t.Run("negative fetch-depth returns error", func(t *testing.T) {
for _, depth := range []float64{-1, -999999} {
raw := map[string]any{
"fetch-depth": depth,
}
_, err := ParseCheckoutConfigs(raw)
require.Error(t, err)
assert.Contains(t, err.Error(), "checkout.fetch-depth must be >= 0")
}
})

t.Run("backward compat: token key still works", func(t *testing.T) {
raw := map[string]any{
"token": "${{ secrets.MY_TOKEN }}",
Expand Down
10 changes: 8 additions & 2 deletions pkg/workflow/compiler_safe_outputs_steps.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,12 @@ func buildExtractBaseBranchStep() []string {
func (c *Compiler) buildSharedPRCheckoutSteps(data *WorkflowData) []string {
consolidatedSafeOutputsStepsLog.Print("Building shared PR checkout steps")
var steps []string
fetchDepth := 1
Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot May 21, 2026

Choose a reason for hiding this comment

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

[/diagnose] Potential inefficiency: NewCheckoutManager(data.CheckoutConfigs) is called inline within the if condition.

If CheckoutConfigs is large or NewCheckoutManager has non-trivial initialization logic, this could be optimized:

fetchDepth := 1
checkoutManager := NewCheckoutManager(data.CheckoutConfigs)
if defaultCheckout := checkoutManager.GetDefaultCheckoutOverride(); defaultCheckout != nil && defaultCheckout.fetchDepth != nil {
    fetchDepth = *defaultCheckout.fetchDepth
    consolidatedSafeOutputsStepsLog.Printf("Using custom checkout fetch-depth for safe_outputs: %d", fetchDepth)
}

This makes the code more readable and avoids potential repeated instantiation if the manager is used elsewhere in this function.


if defaultCheckout := NewCheckoutManager(data.CheckoutConfigs).GetDefaultCheckoutOverride(); defaultCheckout != nil && defaultCheckout.fetchDepth != nil {
Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot May 21, 2026

Choose a reason for hiding this comment

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

[/diagnose] Good defensive programming: initializing fetchDepth to 1 maintains backward compatibility if the lookup fails.

Consider adding a debug log when using the default:

fetchDepth := 1
if defaultCheckout := NewCheckoutManager(data.CheckoutConfigs).GetDefaultCheckoutOverride(); defaultCheckout != nil && defaultCheckout.fetchDepth != nil {
    fetchDepth = *defaultCheckout.fetchDepth
    consolidatedSafeOutputsStepsLog.Printf("Using custom checkout fetch-depth for safe_outputs: %d", fetchDepth)
} else {
    consolidatedSafeOutputsStepsLog.Print("Using default fetch-depth: 1")
}

This helps with debugging when users expect depth=0 but it silently falls back to depth=1.

fetchDepth = *defaultCheckout.fetchDepth
consolidatedSafeOutputsStepsLog.Printf("Using custom checkout fetch-depth for safe_outputs: %d", fetchDepth)
Comment on lines +59 to +60
}

// Determine which token to use for checkout
// Uses resolvePRCheckoutToken for consistent token resolution (GitHub App or PAT chain)
Expand Down Expand Up @@ -149,7 +155,7 @@ func (c *Compiler) buildSharedPRCheckoutSteps(data *WorkflowData) []string {
steps = append(steps, " ref: ${{ github.event.repository.default_branch }}\n")
steps = append(steps, fmt.Sprintf(" token: %s\n", checkoutToken))
steps = append(steps, " persist-credentials: false\n")
steps = append(steps, " fetch-depth: 1\n")
steps = append(steps, fmt.Sprintf(" fetch-depth: %d\n", fetchDepth))
}

// Step 1b: Checkout repository with conditional execution
Expand All @@ -172,7 +178,7 @@ func (c *Compiler) buildSharedPRCheckoutSteps(data *WorkflowData) []string {
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")
steps = append(steps, fmt.Sprintf(" fetch-depth: %d\n", fetchDepth))

// Step 2: Configure Git credentials with conditional execution
// Security: Pass GitHub token through environment variable to prevent template injection
Expand Down
23 changes: 21 additions & 2 deletions pkg/workflow/compiler_safe_outputs_steps_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,12 @@ import (

// TestBuildSharedPRCheckoutSteps tests shared PR checkout step generation
func TestBuildSharedPRCheckoutSteps(t *testing.T) {
fetchDepthZero := 0

tests := []struct {
name string
safeOutputs *SafeOutputsConfig
checkoutConfigs []*CheckoutConfig
trialMode bool
trialRepo string
checkContains []string
Expand All @@ -39,6 +42,21 @@ func TestBuildSharedPRCheckoutSteps(t *testing.T) {
"github-actions[bot]@users.noreply.github.com",
},
},
{
name: "uses custom checkout fetch-depth",
safeOutputs: &SafeOutputsConfig{
CreatePullRequests: &CreatePullRequestsConfig{},
},
checkoutConfigs: []*CheckoutConfig{
{FetchDepth: &fetchDepthZero},
},
checkContains: []string{
"fetch-depth: 0",
Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot May 21, 2026

Choose a reason for hiding this comment

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

[/tdd] This test case validates the happy path (custom fetch-depth is applied), but it's missing edge case coverage:

  1. What happens when CheckoutConfigs is nil?
  2. What happens when CheckoutConfigs is empty ([])?
  3. What happens when FetchDepth pointer is nil?

Consider adding test cases for these scenarios to ensure the default fallback logic (fetchDepth := 1) is exercised:

{
    name: "uses default fetch-depth when CheckoutConfigs is nil",
    safeOutputs: &SafeOutputsConfig{
        CreatePullRequests: &CreatePullRequestsConfig{},
    },
    checkoutConfigs: nil,
    checkContains: []string{
        "fetch-depth: 1",
    },
},
{
    name: "uses default fetch-depth when CheckoutConfigs is empty",
    safeOutputs: &SafeOutputsConfig{
        CreatePullRequests: &CreatePullRequestsConfig{},
    },
    checkoutConfigs: []*CheckoutConfig{},
    checkContains: []string{
        "fetch-depth: 1",
    },
},

These edge cases protect against regressions if the NewCheckoutManager logic changes.

},
checkNotContains: []string{
"fetch-depth: 1",
},
},
{
name: "push to PR branch only",
safeOutputs: &SafeOutputsConfig{
Expand Down Expand Up @@ -317,8 +335,9 @@ func TestBuildSharedPRCheckoutSteps(t *testing.T) {
}

workflowData := &WorkflowData{
Name: "Test Workflow",
SafeOutputs: tt.safeOutputs,
Name: "Test Workflow",
SafeOutputs: tt.safeOutputs,
CheckoutConfigs: tt.checkoutConfigs,
}

steps := compiler.buildSharedPRCheckoutSteps(workflowData)
Expand Down
Loading