Apply checkout.fetch-depth to safe_outputs checkout steps

[Copilot]

checkout overrides were only applied in the agent job, while safe_outputs always checked out with fetch-depth: 1. This broke create_pull_request flows that require commit history when users configure full checkout (fetch-depth: 0).

• Scope of fix: safe_outputs shared PR checkout generation

  • Updated buildSharedPRCheckoutSteps to read the default checkout override from CheckoutConfigs.
  • safe_outputs checkout steps now use configured fetch-depth when present, otherwise keep the default (1).
  • Applied consistently to both checkout paths in that job:
    • trusted default-branch checkout for comment-triggered events
    • standard checkout path for non-comment events

• Behavioral impact

  • checkout.fetch-depth is now honored across both the agent job and safe_outputs, removing the need to post-process compiled lock workflows.

• Targeted coverage

  • Added a focused test case asserting that a configured default checkout (fetch-depth: 0) propagates into generated safe_outputs checkout YAML.

fetchDepth := 1
if defaultCheckout := NewCheckoutManager(data.CheckoutConfigs).GetDefaultCheckoutOverride(); defaultCheckout != nil && defaultCheckout.fetchDepth != nil {
    fetchDepth = *defaultCheckout.fetchDepth
}

// ...
steps = append(steps, fmt.Sprintf("          fetch-depth: %d\n", fetchDepth))

[Copilot]

Pull request overview

This PR fixes safe_outputs checkout step generation so it honors the user-configured default checkout.fetch-depth (e.g., 0 for full history), matching behavior already supported in the agent job and unblocking create_pull_request flows that require commit history.

Changes:

• Updated buildSharedPRCheckoutSteps to derive fetch-depth from the default checkout override in CheckoutConfigs (falling back to 1).
• Applied the configured fetch-depth to both safe_outputs PR checkout paths (comment-triggered trusted default-branch checkout and the standard non-comment checkout).
• Added a unit test asserting fetch-depth: 0 is emitted when configured, and updated lock workflows accordingly.

Show a summary per file

File	Description
pkg/workflow/compiler_safe_outputs_steps.go	Reads default checkout override to propagate configured fetch-depth into safe_outputs checkout steps.
pkg/workflow/compiler_safe_outputs_steps_test.go	Adds coverage ensuring configured fetch-depth (0) is reflected in generated safe_outputs YAML.
.github/workflows/pr-sous-chef.lock.yml	Updates generated safe_outputs checkout steps to use fetch-depth: 0 per config.
.github/workflows/glossary-maintainer.lock.yml	Updates generated safe_outputs checkout steps to use fetch-depth: 0 per config.

Copilot's findings

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

• Files reviewed: 4/4 changed files
• Comments generated: 1

[github-actions]

🧪 Test Quality Sentinel completed test quality analysis.

[github-actions]

🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅

[github-actions]

✅ Design Decision Gate 🏗️ completed the design decision gate check.

No ADR enforcement needed for PR #33746: missing the implementation label and only 29 new lines in business logic directories (threshold: 100).

[github-actions]

✅ PR Code Quality Reviewer completed the code quality review.

[github-actions]

Code Quality Review Summary

Verdict: ✅ Approved

Analysis: This is a clean, focused bug fix that applies checkout.fetch-depth configuration to safe_outputs checkout steps. The implementation follows good practices:

• Proper nil safety: Correct nil checks before dereferencing pointers
• Consistent application: fetch-depth applied to both checkout paths (default branch + standard)
• Appropriate logging: Debug output for visibility during compilation
• Targeted test coverage: Focused test validates the core behavior

Note: One existing review comment correctly identifies the need for fetch-depth validation (negative values). This is the primary code quality concern and has already been raised.

No additional code quality issues found. The implementation is solid and the changes are appropriate for the bug being fixed.

🔎 Code quality review by PR Code Quality Reviewer · ● 523.2K

[github-actions]

Skills-Based Review 🧠

Applied /diagnose and /tdd based on the bug fix nature of this PR.

Key Themes

• Root cause addressed: The fix correctly propagates checkout.fetch-depth from frontmatter config to safe_outputs checkout steps, solving the immediate problem.
• Test coverage: Added a focused test for the happy path (fetch-depth: 0), but missing edge case tests for nil/empty CheckoutConfigs scenarios.
• Observability: Good use of debug logging when custom depth is applied, but missing log for default fallback case.

Positive Highlights

• ✅ Backward compatible: Defaults to fetch-depth: 1 when no override is configured
• ✅ Consistent application: Both checkout paths (comment-triggered and standard) use the same depth logic
• ✅ Clear PR description: Excellent summary of the problem, scope, and behavioral impact
• ✅ Minimal changeset: Surgical fix that doesn't touch unrelated code

Suggestions

1. [/tdd] Add edge case tests for nil/empty CheckoutConfigs to validate the fallback behavior
2. [/diagnose] Add debug log when using default fetch-depth: 1 (helps troubleshoot config issues)
3. [/diagnose] Consider extracting NewCheckoutManager call to a variable for readability

Verdict

This is a solid bug fix with good test coverage for the main scenario. The suggestions are minor improvements that would strengthen the regression protection and observability. Approving with optional improvements noted inline.

🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · ● 557.9K

[github-actions]

[/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.

[github-actions]

[/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.

[github-actions]

[/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.

[github-actions]

🧪 Test Quality Sentinel Report

Test Quality Score: 60/100

⚠️ Acceptable with suggestions

Metric	Value
New/modified tests analyzed	1
✅ Design tests (behavioral contracts)	1 (100%)
⚠️ Implementation tests (low value)	0 (0%)
Tests with error/edge cases	0 (0%)
Duplicate test clusters	0
Test inflation detected	Yes (3.17:1 ratio)
🚨 Coding-guideline violations	0

---

Test Classification Details

Test	File	Classification	Issues Detected
TestBuildSharedPRCheckoutSteps → "uses custom checkout fetch-depth"	pkg/workflow/compiler_safe_outputs_steps_test.go:46	✅ Design	No edge cases; test inflation (3.17:1)

---

📊 Detailed Analysis

Test Case: "uses custom checkout fetch-depth"

What this test verifies:

• When a user configures checkout.fetch-depth: 0 in workflow frontmatter
• The configuration is properly propagated to generated safe_outputs checkout steps
• The output YAML contains fetch-depth: 0 (not the default fetch-depth: 1)

Assertions (3 total):

• ✅ require.NotEmpty(t, steps) - validates steps were generated
• ✅ assert.Contains(t, stepsContent, "fetch-depth: 0") - verifies custom config appears
• ✅ assert.NotContains(t, stepsContent, "fetch-depth: 1") - verifies default doesn't appear

Behavioral contract tested:
"If the user sets checkout.fetch-depth in config, it must appear in the compiled workflow YAML for both the agent job and safe_outputs job."

Why this is a design test (high value):

• Tests observable output (generated YAML content)
• Verifies a user-facing behavioral contract
• Would catch a real regression (PR description indicates this fixes a bug where fetch-depth was ignored in safe_outputs)
• Remains valid after internal refactoring

Test structure:

• ✅ Table-driven test (follows project convention)
• ✅ No mock libraries used (follows project guideline)
• ✅ Build tag present (//go:build !integration)
• ✅ Assertion messages are descriptive

---

⚠️ Issues Detected

1. Test Inflation (3.17:1 ratio)

Issue: Test file grew 3.17x faster than production code

• Production code: +6 net lines (8 added, 2 removed)
• Test code: +19 net lines (21 added, 2 removed)
• Ratio: 19/6 = 3.17:1 (exceeds 2:1 threshold)

Analysis: While the ratio exceeds the guideline, this is partially justified:

• ✅ Table-driven tests naturally require more setup code
• ✅ The change adds a new field to the test struct (checkoutConfigs) used by all test cases
• ✅ Both positive and negative assertions are included (checkContains + checkNotContains)
• ⚠️ However, the ratio still suggests room for consolidation

Severity: Medium

---

2. No Edge Case Coverage

Issue: The new test case only covers the happy path (custom fetch-depth configured)

Missing coverage:

1. Nil checkoutConfigs: What happens when no checkout config is provided? (Should default to fetch-depth: 1)
2. Nil FetchDepth field: What happens when checkoutConfigs exists but FetchDepth is nil? (Should default to fetch-depth: 1)
3. Other values: Are arbitrary fetch-depth values (e.g., 50) handled correctly?

Production code has nil checks but tests don't verify fallback behavior:

if defaultCheckout := NewCheckoutManager(data.CheckoutConfigs).GetDefaultCheckoutOverride(); 
   defaultCheckout != nil && defaultCheckout.fetchDepth != nil {
    fetchDepth = *defaultCheckout.fetchDepth
}

Severity: Low (the existing test suite may already cover defaults)

Suggested additional test case:

{
    name: "uses default fetch-depth when config is nil",
    safeOutputs: &SafeOutputsConfig{
        CreatePullRequests: &CreatePullRequestsConfig{},
    },
    checkoutConfigs: nil, // explicit nil
    checkContains: []string{
        "fetch-depth: 1",
    },
    checkNotContains: []string{
        "fetch-depth: 0",
    },
},

---

Verdict

✅ Check passed. 0% of new tests are implementation tests (threshold: 30%).

This test adds valuable coverage for the bug fix described in the PR. The test verifies a genuine behavioral contract (fetch-depth configuration must propagate to safe_outputs steps). While edge case coverage could be improved, the test is well-structured and follows project conventions.

Score breakdown:

• ✅ 40/40 pts: Behavioral coverage (100% design tests)
• ❌ 0/30 pts: No error/edge case coverage
• ✅ 20/20 pts: No duplicate test patterns
• ❌ 0/10 pts: Test inflation exceeds 2:1 ratio

Total: 60/100 - Acceptable with suggestions

---

📖 Understanding Test Classifications

Design Tests (High Value) verify what the system does:

• Assert on observable outputs, return values, or state changes
• Cover error paths and boundary conditions
• Would catch a behavioral regression if deleted
• Remain valid even after internal refactoring

Implementation Tests (Low Value) verify how the system does it:

• Assert on internal function calls (mocking internals)
• Only test the happy path with typical inputs
• Break during legitimate refactoring even when behavior is correct
• Give false assurance: they pass even when the system is wrong

Goal: Shift toward tests that describe the system's behavioral contract — the promises it makes to its users and collaborators.

🧪 Test quality analysis by Test Quality Sentinel · ● 1.3M · ◷

[github-actions]

✅ Test Quality Sentinel: 60/100. Test quality is acceptable — 0% of new tests are implementation tests (threshold: 30%). The test verifies a behavioral contract and would catch regressions in fetch-depth configuration propagation.

[pelikhan]

@copilot review all comments, lint go

[Copilot]

@copilot review all comments, lint go

Done. I reviewed the comments and addressed the fetch-depth validation gap by rejecting negative checkout.fetch-depth values during checkout config parsing, plus added focused tests (including -1 and -999999).

Included in commit b567860.