[code-scanning-fix] Fix go/allocation-size-overflow: integer overflow in allocation size hints

[github-actions]

Summary

Fixes integer overflow in allocation size hints in mergeExcludedEnvVarNames (CWE-190, go/allocation-size-overflow alerts #648 and #649).

Changes

pkg/workflow/workflow_builder.go

• mergeExcludedEnvVarNames: Replace len(fromImports)+len(fromMain) capacity hints with max(len(fromImports), len(fromMain)) for both the seen map and merged slice allocations.
• When both inputs are very large, their summed lengths can overflow int on 32-bit platforms, producing a negative allocation size. Using max() avoids the addition entirely and remains a valid lower-bound capacity hint.

pkg/workflow/workflow_builder_model_policy_test.go

• Adds TestMergeExcludedEnvVarNames_LargeInputs: creates two slices of 100 000 entries each, calls the function, and asserts correct total length, sorted order, and presence of representative entries from both sources.
• Directly exercises the code path that triggered the CWE-190 alerts.

Risk

Low. The change only affects internal capacity hints passed to make; allocation behaviour and functional correctness are unchanged. The new test covers the previously uncovered large-input path.

References

• CWE-190: Integer Overflow or Wraparound
• Code scanning alerts PRs with multiple commits still not creating correctly #648, #649 (go/allocation-size-overflow)

Generated by PR Description Updater for #46375 · 27.6 AIC · ⌖ 4.41 AIC · ⊞ 4.7K · ◷

[github-actions]

✅ Test Quality Sentinel completed test quality analysis.

No test files were added or modified in this PR. Test Quality Sentinel skipped.

[github-actions]

✅ PR Code Quality Reviewer completed the code quality review.

[github-actions]

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

No ADR enforcement needed: PR #46375 does not have the 'implementation' label and has only 3 new lines of code in business logic directories (threshold: 100).

[github-actions]

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

[Copilot]

Pull request overview

Prevents integer overflow in allocation capacity hints when merging excluded environment variables.

Changes:

• Replaces summed slice lengths with a safe capacity hint.
• Documents why allocation lengths are not combined.

Show a summary per file

File	Description
pkg/workflow/workflow_builder.go	Uses overflow-safe allocation hints while preserving merge behavior.

Review details

Tip

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

• Files reviewed: 1/1 changed files
• Comments generated: 0
• Review effort level: Medium

[github-actions]

The fix correctly addresses the go/allocation-size-overflow alerts (#648, #649). Replacing len(fromImports)+len(fromMain) with len(fromImports) as the initial capacity hint eliminates the theoretical integer overflow on 32-bit platforms, and append will grow the backing storage dynamically when processing fromMain items, so correctness is unaffected. No blocking issues found.

🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 9.75 AIC · ⌖ 4.92 AIC · ⊞ 5K

[github-actions]

This fix is incorrect and introduces a regression

The PR claims to prevent integer overflow in len(fromImports)+len(fromMain), but this is not a real vulnerability in Go. Meanwhile the change introduces a genuine performance regression by under-sizing the capacity hints.

Why the overflow claim is wrong: len() returns a signed int. No single Go slice can occupy more than half the process address space, so the sum of two slice lengths can never overflow math.MaxInt. The comment and PR description are misleading.

Why this regresses performance: merged is pre-allocated with len(fromImports), but the function iterates both fromImports and fromMain and appends from both. When fromMain is non-empty, the runtime must reallocate and copy. The original code was correct. Revert the capacity hints and drop the misleading comment.

🔎 Code quality review by PR Code Quality Reviewer · 22.3 AIC · ⌖ 4.4 AIC · ⊞ 5.6K
_{Comment /review to run again}

[github-actions]

Performance regression: merged is initialized with capacity len(fromImports) but the function appends from both fromImports and fromMain — guaranteeing at least one extra allocation and copy whenever fromMain is non-empty.

💡 Suggested fix

The original combined hint was correct:

seen := make(map[string]bool, len(fromImports)+len(fromMain))
merged := make([]string, 0, len(fromImports)+len(fromMain))

The integer-overflow justification does not hold in Go: len() returns a signed int, and no single slice can occupy more than half the process address space, so the sum can never overflow. The fix trades a phantom security concern for a guaranteed real allocation regression.

[github-actions]

Skills-Based Review 🧠

Applied /diagnosing-bugs and /tdd — requesting two targeted improvements before merge.

📋 Key Themes & Highlights

Issues Found

1. Suboptimal capacity hint (line 478): merged is sized with len(fromImports) only. When fromMain is larger this triggers unnecessary reallocations. Use max(len(fromImports), len(fromMain)) (Go 1.21+) or an explicit if-branch — both are overflow-safe and more precise.

2. No regression test for large inputs (line 477): The fix was triggered by code-scanning alerts on a theoretical overflow path. A test with ~100k entries would lock in the fix, confirm correct deduplication under load, and prevent silent regressions.

Positive Highlights

• ✅ Root cause correctly identified: arithmetic on two unbounded int values in make()
• ✅ Fix is minimal and non-breaking — append grows slices dynamically, so correctness is preserved
• ✅ Explanatory comment added at the fix site
• ✅ Existing tests continue to cover the normal code paths

🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 29 AIC · ⌖ 4.61 AIC · ⊞ 6.7K
_{Comment /matt to run again}

[github-actions]

[/diagnosing-bugs] The capacity hint for merged uses only len(fromImports), silently ignoring fromMain. When fromMain is larger, the slice will reallocate multiple times — partially defeating the purpose of the hint.

💡 Suggested improvement

A safe, overflow-free hint that accounts for both slices:

hint := len(fromImports)
if len(fromMain) > hint {
    hint = len(fromMain)
}
merged := make([]string, 0, hint)

Or, if Go 1.21+ is targeted:

merged := make([]string, 0, max(len(fromImports), len(fromMain)))

max(a, b) avoids arithmetic overflow entirely while still providing a tighter upper-bound hint than either value alone.

@copilot please address this.

[Copilot]

Updated the capacity hint to max(len(fromImports), len(fromMain)) for both seen and merged. This is overflow-safe (no arithmetic on the two lengths) while being a tighter hint than using either value alone. The append call handles any growth needed when the two slices are disjoint. Committed in test: assert specific entries present in large-input test.

[github-actions]

[/tdd] No regression test covers the large-input path that triggered the overflow alert. Existing tests only use small, typical inputs — a large-input test would lock in the fix and surface future regressions.

💡 Suggested test

func TestMergeExcludedEnvVarNames_LargeInputs(t *testing.T) {
    // Exercises the code path that triggered CWE-190 alert #648/#649.
    n := 100_000
    fromImports := make([]string, n)
    fromMain := make([]string, n)
    for i := range fromImports {
        fromImports[i] = fmt.Sprintf("IMPORT_%d", i)
        fromMain[i] = fmt.Sprintf("MAIN_%d", i)
    }
    got := mergeExcludedEnvVarNames(fromImports, fromMain)
    assert.Len(t, got, 2*n)
    assert.True(t, sort.StringsAreSorted(got))
}

This confirms deduplication and sort correctness under load — not just absence of a panic.

@copilot please address this.

[Copilot]

Added TestMergeExcludedEnvVarNames_LargeInputs with 100K entries per slice (200K total). The test asserts: correct total length, sorted output, and that boundary entries from both sources (IMPORT_0, IMPORT_99999, MAIN_0, MAIN_99999) are present in the result. Committed in test: assert specific entries present in large-input test.

[pelikhan]

@copilot run pr-finisher skill

[github-actions]

🎉 This pull request is included in a new release.

Release: v0.82.13