From 21e2dda624540d8c5a9c3fdc3f5ea4ec99ae60c8 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 24 Aug 2026 18:58:33 +0000 Subject: [PATCH 1/6] Initial plan From 65f1a09d0fc46456dc9869ef9fe6f46865d1409c Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 24 Aug 2026 19:11:28 +0000 Subject: [PATCH 2/6] Guard allocation capacity calculations Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/cli/allocation_helpers.go | 17 +++++++++++++ pkg/cli/allocation_helpers_test.go | 29 +++++++++++++++++++++++ pkg/cli/experiments_analyze_statistics.go | 2 +- pkg/workflow/tools.go | 10 ++++---- 4 files changed, 52 insertions(+), 6 deletions(-) create mode 100644 pkg/cli/allocation_helpers.go create mode 100644 pkg/cli/allocation_helpers_test.go diff --git a/pkg/cli/allocation_helpers.go b/pkg/cli/allocation_helpers.go new file mode 100644 index 00000000000..47435a11b6b --- /dev/null +++ b/pkg/cli/allocation_helpers.go @@ -0,0 +1,17 @@ +package cli + +import "math" + +// safeAllocationCapacity returns the summed capacity hint when it fits in int. +// When the total would overflow, it falls back to 0 so callers can skip +// preallocation without changing correctness. +func safeAllocationCapacity(parts ...int) int { + total := 0 + for _, part := range parts { + if part < 0 || total > math.MaxInt-part { + return 0 + } + total += part + } + return total +} diff --git a/pkg/cli/allocation_helpers_test.go b/pkg/cli/allocation_helpers_test.go new file mode 100644 index 00000000000..bd894a98496 --- /dev/null +++ b/pkg/cli/allocation_helpers_test.go @@ -0,0 +1,29 @@ +//go:build !integration + +package cli + +import ( + "math" + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestSafeAllocationCapacity(t *testing.T) { + t.Parallel() + + t.Run("sums sizes when the result fits in int", func(t *testing.T) { + assert.Equal(t, 5, safeAllocationCapacity(2, 3)) + assert.Equal(t, math.MaxInt, safeAllocationCapacity(math.MaxInt-1, 1)) + }) + + t.Run("returns zero when the sum would overflow int", func(t *testing.T) { + assert.Zero(t, safeAllocationCapacity(math.MaxInt, 1)) + assert.Zero(t, safeAllocationCapacity(math.MaxInt-1, 2)) + }) + + t.Run("returns zero for negative parts", func(t *testing.T) { + assert.Zero(t, safeAllocationCapacity(-1)) + assert.Zero(t, safeAllocationCapacity(2, -1)) + }) +} diff --git a/pkg/cli/experiments_analyze_statistics.go b/pkg/cli/experiments_analyze_statistics.go index 229086a0ca0..c8e8db182e3 100644 --- a/pkg/cli/experiments_analyze_statistics.go +++ b/pkg/cli/experiments_analyze_statistics.go @@ -428,7 +428,7 @@ func experimentVariantCounts(exp ExperimentVariantStats, cfg *workflow.Experimen if !includeDeclared || cfg == nil { return exp.Variants } - counts := make(map[string]int, len(exp.Variants)+len(cfg.Variants)) + counts := make(map[string]int, safeAllocationCapacity(len(exp.Variants), len(cfg.Variants))) maps.Copy(counts, exp.Variants) for _, name := range cfg.Variants { if _, ok := counts[name]; !ok { diff --git a/pkg/workflow/tools.go b/pkg/workflow/tools.go index 53f5a25fafd..e0a09dbd3c7 100644 --- a/pkg/workflow/tools.go +++ b/pkg/workflow/tools.go @@ -262,11 +262,11 @@ func (c *Compiler) buildCommandTriggerEventsMap(data *WorkflowData) (map[string] if existingMap, ok := existingAny.(map[string]any); ok { switch t := existingMap["types"].(type) { case []string: - newTypes := make([]any, len(t)+1) - for i, s := range t { - newTypes[i] = s + newTypes := make([]any, 0, safeAllocationCapacity(len(t), 1)) + for _, s := range t { + newTypes = append(newTypes, s) } - newTypes[len(t)] = "labeled" + newTypes = append(newTypes, "labeled") existingMap["types"] = newTypes case []any: existingMap["types"] = append(t, "labeled") @@ -759,7 +759,7 @@ func ensureGitHubAllowedTool(githubConfig map[string]any, tool string) { } func appendStringAny(values []string, value string) []any { - result := make([]any, 0, len(values)+1) + result := make([]any, 0, safeAllocationCapacity(len(values), 1)) for _, existing := range values { result = append(result, existing) } From 223b133a1bebec811af093a9ef914d44ea2cbbc6 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 24 Aug 2026 20:30:29 +0000 Subject: [PATCH 3/6] Move allocation capacity helper to typeutil Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/cli/experiments_analyze_statistics.go | 3 +- .../experiments_analyze_statistics_test.go | 15 ++++++++ .../allocation.go} | 6 +-- .../allocation_helpers_test.go | 14 +++---- pkg/workflow/allocation_helpers.go | 24 ------------ pkg/workflow/allocation_helpers_test.go | 37 ------------------- pkg/workflow/awf_helpers.go | 3 +- pkg/workflow/compiler_activation_job.go | 6 +-- pkg/workflow/compiler_aw_context.go | 3 +- .../compiler_builtin_job_augmentation.go | 3 +- pkg/workflow/compiler_job_step_helpers.go | 5 ++- pkg/workflow/concurrency.go | 5 ++- pkg/workflow/domains.go | 5 ++- pkg/workflow/known_action_credentials.go | 6 +-- pkg/workflow/mcp_setup_safe_outputs.go | 3 +- pkg/workflow/network_firewall_validation.go | 3 +- pkg/workflow/observability_otlp.go | 3 +- pkg/workflow/permissions.go | 3 +- pkg/workflow/permissions_validation.go | 3 +- pkg/workflow/run_step_sanitizer.go | 6 +-- pkg/workflow/safe_jobs_needs_validation.go | 3 +- pkg/workflow/safe_output_handlers.go | 3 +- pkg/workflow/threat_detection_steps.go | 3 +- pkg/workflow/tools.go | 7 ++-- pkg/workflow/workflow_import_merge.go | 3 +- 25 files changed, 73 insertions(+), 102 deletions(-) rename pkg/{cli/allocation_helpers.go => typeutil/allocation.go} (69%) rename pkg/{cli => typeutil}/allocation_helpers_test.go (53%) delete mode 100644 pkg/workflow/allocation_helpers.go delete mode 100644 pkg/workflow/allocation_helpers_test.go diff --git a/pkg/cli/experiments_analyze_statistics.go b/pkg/cli/experiments_analyze_statistics.go index c8e8db182e3..6022c9b3d4a 100644 --- a/pkg/cli/experiments_analyze_statistics.go +++ b/pkg/cli/experiments_analyze_statistics.go @@ -11,6 +11,7 @@ import ( "github.com/github/gh-aw/pkg/console" "github.com/github/gh-aw/pkg/logger" "github.com/github/gh-aw/pkg/sliceutil" + "github.com/github/gh-aw/pkg/typeutil" "github.com/github/gh-aw/pkg/workflow" ) @@ -428,7 +429,7 @@ func experimentVariantCounts(exp ExperimentVariantStats, cfg *workflow.Experimen if !includeDeclared || cfg == nil { return exp.Variants } - counts := make(map[string]int, safeAllocationCapacity(len(exp.Variants), len(cfg.Variants))) + counts := make(map[string]int, typeutil.SafeAllocationCapacity(len(exp.Variants), len(cfg.Variants))) maps.Copy(counts, exp.Variants) for _, name := range cfg.Variants { if _, ok := counts[name]; !ok { diff --git a/pkg/cli/experiments_analyze_statistics_test.go b/pkg/cli/experiments_analyze_statistics_test.go index c0815e9855e..765cb64812d 100644 --- a/pkg/cli/experiments_analyze_statistics_test.go +++ b/pkg/cli/experiments_analyze_statistics_test.go @@ -174,6 +174,21 @@ func TestExpectedProportions(t *testing.T) { }) } +func TestExperimentVariantCounts(t *testing.T) { + t.Parallel() + + exp := ExperimentVariantStats{ + Variants: map[string]int{"control": 3}, + } + cfg := &workflow.ExperimentConfig{ + Variants: []string{"control", "candidate"}, + } + + got := experimentVariantCounts(exp, cfg, true) + + assert.Equal(t, map[string]int{"control": 3, "candidate": 0}, got) +} + // TestComputeExperimentAnalysis verifies the end-to-end statistical computation. func TestComputeExperimentAnalysis(t *testing.T) { t.Parallel() diff --git a/pkg/cli/allocation_helpers.go b/pkg/typeutil/allocation.go similarity index 69% rename from pkg/cli/allocation_helpers.go rename to pkg/typeutil/allocation.go index 47435a11b6b..4a0a8ce649a 100644 --- a/pkg/cli/allocation_helpers.go +++ b/pkg/typeutil/allocation.go @@ -1,11 +1,11 @@ -package cli +package typeutil import "math" -// safeAllocationCapacity returns the summed capacity hint when it fits in int. +// SafeAllocationCapacity returns the summed capacity hint when it fits in int. // When the total would overflow, it falls back to 0 so callers can skip // preallocation without changing correctness. -func safeAllocationCapacity(parts ...int) int { +func SafeAllocationCapacity(parts ...int) int { total := 0 for _, part := range parts { if part < 0 || total > math.MaxInt-part { diff --git a/pkg/cli/allocation_helpers_test.go b/pkg/typeutil/allocation_helpers_test.go similarity index 53% rename from pkg/cli/allocation_helpers_test.go rename to pkg/typeutil/allocation_helpers_test.go index bd894a98496..19932137458 100644 --- a/pkg/cli/allocation_helpers_test.go +++ b/pkg/typeutil/allocation_helpers_test.go @@ -1,6 +1,6 @@ //go:build !integration -package cli +package typeutil import ( "math" @@ -13,17 +13,17 @@ func TestSafeAllocationCapacity(t *testing.T) { t.Parallel() t.Run("sums sizes when the result fits in int", func(t *testing.T) { - assert.Equal(t, 5, safeAllocationCapacity(2, 3)) - assert.Equal(t, math.MaxInt, safeAllocationCapacity(math.MaxInt-1, 1)) + assert.Equal(t, 5, SafeAllocationCapacity(2, 3)) + assert.Equal(t, math.MaxInt, SafeAllocationCapacity(math.MaxInt-1, 1)) }) t.Run("returns zero when the sum would overflow int", func(t *testing.T) { - assert.Zero(t, safeAllocationCapacity(math.MaxInt, 1)) - assert.Zero(t, safeAllocationCapacity(math.MaxInt-1, 2)) + assert.Zero(t, SafeAllocationCapacity(math.MaxInt, 1)) + assert.Zero(t, SafeAllocationCapacity(math.MaxInt-1, 2)) }) t.Run("returns zero for negative parts", func(t *testing.T) { - assert.Zero(t, safeAllocationCapacity(-1)) - assert.Zero(t, safeAllocationCapacity(2, -1)) + assert.Zero(t, SafeAllocationCapacity(-1)) + assert.Zero(t, SafeAllocationCapacity(2, -1)) }) } diff --git a/pkg/workflow/allocation_helpers.go b/pkg/workflow/allocation_helpers.go deleted file mode 100644 index 7121be6188d..00000000000 --- a/pkg/workflow/allocation_helpers.go +++ /dev/null @@ -1,24 +0,0 @@ -package workflow - -import ( - "math" - - "github.com/github/gh-aw/pkg/logger" -) - -var allocationLog = logger.New("workflow:allocation_helpers") - -// safeAllocationCapacity returns the summed capacity hint when it fits in int. -// When the total would overflow, it falls back to 0 so callers can skip -// preallocation without changing correctness. -func safeAllocationCapacity(parts ...int) int { - total := 0 - for _, part := range parts { - if part < 0 || total > math.MaxInt-part { - allocationLog.Printf("Capacity hint overflow or negative part (part=%d, running total=%d), falling back to 0", part, total) - return 0 - } - total += part - } - return total -} diff --git a/pkg/workflow/allocation_helpers_test.go b/pkg/workflow/allocation_helpers_test.go deleted file mode 100644 index 1a0f109d2e4..00000000000 --- a/pkg/workflow/allocation_helpers_test.go +++ /dev/null @@ -1,37 +0,0 @@ -//go:build !integration - -package workflow - -import ( - "math" - "testing" - - "github.com/stretchr/testify/assert" -) - -func TestSafeAllocationCapacity(t *testing.T) { - t.Run("handles zero inputs", func(t *testing.T) { - assert.Zero(t, safeAllocationCapacity()) - assert.Zero(t, safeAllocationCapacity(0, 0)) - assert.Equal(t, 5, safeAllocationCapacity(0, 5)) - assert.Equal(t, 5, safeAllocationCapacity(5, 0)) - }) - - t.Run("sums sizes when the result fits in int", func(t *testing.T) { - assert.Equal(t, 5, safeAllocationCapacity(2, 3)) - assert.Equal(t, 6000, safeAllocationCapacity(1000, 5000)) - assert.Equal(t, math.MaxInt, safeAllocationCapacity(math.MaxInt-1, 1)) - assert.Equal(t, math.MaxInt, safeAllocationCapacity(math.MaxInt-2, 1, 1)) - }) - - t.Run("returns zero when the sum would overflow int", func(t *testing.T) { - assert.Zero(t, safeAllocationCapacity(math.MaxInt, 1)) - assert.Zero(t, safeAllocationCapacity(math.MaxInt-1, 2)) - assert.Zero(t, safeAllocationCapacity(math.MaxInt-2, 2, 1)) - }) - - t.Run("returns zero for negative parts", func(t *testing.T) { - assert.Zero(t, safeAllocationCapacity(-1)) - assert.Zero(t, safeAllocationCapacity(2, -1)) - }) -} diff --git a/pkg/workflow/awf_helpers.go b/pkg/workflow/awf_helpers.go index f49a4b8c9e3..03eeeafe031 100644 --- a/pkg/workflow/awf_helpers.go +++ b/pkg/workflow/awf_helpers.go @@ -12,6 +12,7 @@ import ( "fmt" "github.com/github/gh-aw/pkg/logger" + "github.com/github/gh-aw/pkg/typeutil" ) var awfHelpersLog = logger.New("workflow:awf_helpers") @@ -120,7 +121,7 @@ func buildModelsJSONPathExportScript(isArcDind bool) string { func buildWorkflowCallNetworkAllowedUpdateScript() (string, error) { ecosystemDomains := getLoadedEcosystemDomains() awfHelpersLog.Printf("buildWorkflowCallNetworkAllowedUpdateScript: ecosystems=%d, compoundEcosystems=%d", len(ecosystemDomains), len(compoundEcosystems)) - ecosystemMap := make(map[string][]string, safeAllocationCapacity(len(ecosystemDomains), len(compoundEcosystems))) + ecosystemMap := make(map[string][]string, typeutil.SafeAllocationCapacity(len(ecosystemDomains), len(compoundEcosystems))) for ecosystem := range ecosystemDomains { ecosystemMap[ecosystem] = getEcosystemDomains(ecosystem) } diff --git a/pkg/workflow/compiler_activation_job.go b/pkg/workflow/compiler_activation_job.go index 67d6e35061b..b1473b3c72a 100644 --- a/pkg/workflow/compiler_activation_job.go +++ b/pkg/workflow/compiler_activation_job.go @@ -6,11 +6,11 @@ import ( "path/filepath" "strings" - "github.com/goccy/go-yaml" - "github.com/github/gh-aw/pkg/constants" "github.com/github/gh-aw/pkg/logger" "github.com/github/gh-aw/pkg/setutil" + "github.com/github/gh-aw/pkg/typeutil" + "github.com/goccy/go-yaml" ) var compilerActivationJobLog = logger.New("workflow:compiler_activation_job") @@ -610,7 +610,7 @@ func injectIfConditionAfterName(step, condition string) string { fieldIndent = nameIndent + " " } - newLines := make([]string, 0, safeAllocationCapacity(len(lines), 1)) + newLines := make([]string, 0, typeutil.SafeAllocationCapacity(len(lines), 1)) newLines = append(newLines, lines[:nameLineIdx+1]...) newLines = append(newLines, fieldIndent+"if: "+condition) newLines = append(newLines, lines[nameLineIdx+1:]...) diff --git a/pkg/workflow/compiler_aw_context.go b/pkg/workflow/compiler_aw_context.go index 0e57b7f7c46..ebc17cc9431 100644 --- a/pkg/workflow/compiler_aw_context.go +++ b/pkg/workflow/compiler_aw_context.go @@ -5,6 +5,7 @@ import ( "strings" "github.com/github/gh-aw/pkg/logger" + "github.com/github/gh-aw/pkg/typeutil" ) var awContextLog = logger.New("workflow:compiler_aw_context") @@ -118,7 +119,7 @@ func injectInputIntoTrigger(onSection string, triggerName string, inputName stri inputLines := buildInputLines(triggerIndent) - result := make([]string, 0, safeAllocationCapacity(len(lines), len(inputLines), 1)) + result := make([]string, 0, typeutil.SafeAllocationCapacity(len(lines), len(inputLines), 1)) for i, line := range lines { // When the trigger line contains an explicit null/~ value, // replace it with a bare trigger so sub-keys can follow. diff --git a/pkg/workflow/compiler_builtin_job_augmentation.go b/pkg/workflow/compiler_builtin_job_augmentation.go index 46c7c7f852e..4bc2fe49d05 100644 --- a/pkg/workflow/compiler_builtin_job_augmentation.go +++ b/pkg/workflow/compiler_builtin_job_augmentation.go @@ -6,6 +6,7 @@ import ( "strings" "github.com/github/gh-aw/pkg/constants" + "github.com/github/gh-aw/pkg/typeutil" ) func (c *Compiler) applyBuiltinJobPreSteps(data *WorkflowData) error { @@ -97,7 +98,7 @@ func insertActivationStepsBeforeArtifactStaging(jobName string, steps []string, } } - result := make([]string, 0, safeAllocationCapacity(len(steps), len(activationSteps))) + result := make([]string, 0, typeutil.SafeAllocationCapacity(len(steps), len(activationSteps))) result = append(result, steps[:insertIdx]...) result = append(result, activationSteps...) result = append(result, steps[insertIdx:]...) diff --git a/pkg/workflow/compiler_job_step_helpers.go b/pkg/workflow/compiler_job_step_helpers.go index b6ddf929063..fe5ef251f5f 100644 --- a/pkg/workflow/compiler_job_step_helpers.go +++ b/pkg/workflow/compiler_job_step_helpers.go @@ -7,6 +7,7 @@ import ( "strings" "github.com/github/gh-aw/pkg/constants" + "github.com/github/gh-aw/pkg/typeutil" ) var exactSetupStepIDPattern = regexp.MustCompile(`(?m)^\s*id:\s*setup\s*$`) @@ -71,7 +72,7 @@ func insertSetupStepsAtStart(steps []string, setupSteps []string) []string { return steps } - result := make([]string, 0, safeAllocationCapacity(len(steps), len(setupSteps))) + result := make([]string, 0, typeutil.SafeAllocationCapacity(len(steps), len(setupSteps))) result = append(result, setupSteps...) result = append(result, steps...) return result @@ -143,7 +144,7 @@ func insertPreStepsAtEarliestBoundary(steps []string, preSteps []string) []strin insertIdx = len(steps) } - result := make([]string, 0, safeAllocationCapacity(len(steps), len(preSteps))) + result := make([]string, 0, typeutil.SafeAllocationCapacity(len(steps), len(preSteps))) result = append(result, steps[:insertIdx]...) result = append(result, preSteps...) result = append(result, steps[insertIdx:]...) diff --git a/pkg/workflow/concurrency.go b/pkg/workflow/concurrency.go index 3cf471e6961..a9b8329ac82 100644 --- a/pkg/workflow/concurrency.go +++ b/pkg/workflow/concurrency.go @@ -5,6 +5,7 @@ import ( "strings" "github.com/github/gh-aw/pkg/logger" + "github.com/github/gh-aw/pkg/typeutil" ) var concurrencyLog = logger.New("workflow:concurrency") @@ -182,7 +183,7 @@ func isSlashCommandWorkflow(on string) bool { // inserted between the primary identifiers and the tail, providing a stable per-item // key for manual workflow_dispatch runs triggered via the label trigger shorthand. func entityConcurrencyKey(primaryParts []string, tailParts []string, hasItemNumber bool) string { - parts := make([]string, 0, safeAllocationCapacity(len(primaryParts), len(tailParts), 1)) + parts := make([]string, 0, typeutil.SafeAllocationCapacity(len(primaryParts), len(tailParts), 1)) parts = append(parts, primaryParts...) if hasItemNumber { parts = append(parts, "inputs.item_number") @@ -197,7 +198,7 @@ func entityConcurrencyKey(primaryParts []string, tailParts []string, hasItemNumb // When contains(github.actor, '[bot]') is true, the expression short-circuits to // github.run_id so that bot-triggered runs never share a group with human runs. func botIsolatedConcurrencyKey(primaryParts []string, tailParts []string, hasItemNumber bool) string { - parts := make([]string, 0, safeAllocationCapacity(len(primaryParts), len(tailParts), 2)) + parts := make([]string, 0, typeutil.SafeAllocationCapacity(len(primaryParts), len(tailParts), 2)) // Prepend the bot-actor isolation check: bot runs always get a unique key parts = append(parts, "contains(github.actor, '[bot]') && github.run_id") parts = append(parts, primaryParts...) diff --git a/pkg/workflow/domains.go b/pkg/workflow/domains.go index 1baa9c3cab0..7e399f9655f 100644 --- a/pkg/workflow/domains.go +++ b/pkg/workflow/domains.go @@ -4,6 +4,7 @@ import ( _ "embed" "encoding/json" "fmt" + "github.com/github/gh-aw/pkg/typeutil" "sort" "strings" "sync" @@ -220,7 +221,7 @@ func getPiDefaultDomains(model string) ([]string, error) { if err != nil { return nil, err } - domains := make([]string, 0, safeAllocationCapacity(len(PiBaseDefaultDomains), 1)) + domains := make([]string, 0, typeutil.SafeAllocationCapacity(len(PiBaseDefaultDomains), 1)) domains = append(domains, PiBaseDefaultDomains...) if domain, ok := piProviderDomains[provider]; ok { @@ -658,7 +659,7 @@ func resolveEngineNetworkDomains(network *EngineNetworkDefinition, model string) if provider == "" { provider = network.DefaultProvider } - domains := make([]string, 0, safeAllocationCapacity(len(network.Defaults), 1)) + domains := make([]string, 0, typeutil.SafeAllocationCapacity(len(network.Defaults), 1)) domains = append(domains, network.Defaults...) if domain, ok := network.ProviderDomains[provider]; ok { domains = append(domains, domain) diff --git a/pkg/workflow/known_action_credentials.go b/pkg/workflow/known_action_credentials.go index 054ccb62e0b..039ae356fdb 100644 --- a/pkg/workflow/known_action_credentials.go +++ b/pkg/workflow/known_action_credentials.go @@ -30,9 +30,9 @@ import ( "maps" "strings" - "github.com/goccy/go-yaml" - "github.com/github/gh-aw/pkg/logger" + "github.com/github/gh-aw/pkg/typeutil" + "github.com/goccy/go-yaml" ) var knownActionCredentialsLog = logger.New("workflow:known_action_credentials") @@ -199,7 +199,7 @@ func mergeKnownActionEnvVars(a, b map[string]struct { return nil } merged := make(map[string]struct { - }, safeAllocationCapacity(len(a), len(b))) + }, typeutil.SafeAllocationCapacity(len(a), len(b))) maps.Copy(merged, a) maps.Copy(merged, b) return merged diff --git a/pkg/workflow/mcp_setup_safe_outputs.go b/pkg/workflow/mcp_setup_safe_outputs.go index 1bdaa3b45c5..d63b3cdd64c 100644 --- a/pkg/workflow/mcp_setup_safe_outputs.go +++ b/pkg/workflow/mcp_setup_safe_outputs.go @@ -8,6 +8,7 @@ import ( "github.com/github/gh-aw/pkg/constants" "github.com/github/gh-aw/pkg/logger" "github.com/github/gh-aw/pkg/sliceutil" + "github.com/github/gh-aw/pkg/typeutil" ) var safeOutputsSetupLog = logger.New("workflow:mcp_setup_safe_outputs") @@ -131,7 +132,7 @@ func buildSafeOutputsConfigRuntimeEnvVars(safeOutputConfig string) ([]string, ma configSecrets := ExtractSecretsFromValue(safeOutputConfig) configContextVars := ExtractGitHubContextExpressionsFromValue(safeOutputConfig) configWorkflowInputs := ExtractWorkflowInputExpressionsFromValue(safeOutputConfig) - envValues := make(map[string]string, safeAllocationCapacity(len(configSecrets), len(configContextVars), len(configWorkflowInputs))) + envValues := make(map[string]string, typeutil.SafeAllocationCapacity(len(configSecrets), len(configContextVars), len(configWorkflowInputs))) addEnvValue := func(key, value string) { envValues[key] = value } diff --git a/pkg/workflow/network_firewall_validation.go b/pkg/workflow/network_firewall_validation.go index ced6c14c8b4..20ed64774a4 100644 --- a/pkg/workflow/network_firewall_validation.go +++ b/pkg/workflow/network_firewall_validation.go @@ -20,6 +20,7 @@ import ( "github.com/github/gh-aw/pkg/constants" "github.com/github/gh-aw/pkg/logger" + "github.com/github/gh-aw/pkg/typeutil" ) var networkFirewallValidationLog = logger.New("workflow:network_firewall_validation") @@ -156,7 +157,7 @@ func isKnownEcosystemIdentifier(id string) bool { // including both the base identifiers from ecosystemDomains and compound identifiers. func getValidEcosystemIdentifiers() []string { ecosystemDomains := getLoadedEcosystemDomains() - ids := make([]string, 0, safeAllocationCapacity(len(ecosystemDomains), len(compoundEcosystems))) + ids := make([]string, 0, typeutil.SafeAllocationCapacity(len(ecosystemDomains), len(compoundEcosystems))) for id := range ecosystemDomains { ids = append(ids, id) } diff --git a/pkg/workflow/observability_otlp.go b/pkg/workflow/observability_otlp.go index e8bd432e452..de1eb482edd 100644 --- a/pkg/workflow/observability_otlp.go +++ b/pkg/workflow/observability_otlp.go @@ -11,6 +11,7 @@ import ( "github.com/github/gh-aw/pkg/constants" "github.com/github/gh-aw/pkg/logger" "github.com/github/gh-aw/pkg/sliceutil" + "github.com/github/gh-aw/pkg/typeutil" ) var otlpLog = logger.New("workflow:observability_otlp") @@ -539,7 +540,7 @@ func mergeOTLPStringMaps(base, override map[string]string) map[string]string { if len(base) == 0 && len(override) == 0 { return nil } - merged := make(map[string]string, safeAllocationCapacity(len(base), len(override))) + merged := make(map[string]string, typeutil.SafeAllocationCapacity(len(base), len(override))) maps.Copy(merged, override) // base takes precedence maps.Copy(merged, base) diff --git a/pkg/workflow/permissions.go b/pkg/workflow/permissions.go index 7896f79c18b..35d4c76e2e2 100644 --- a/pkg/workflow/permissions.go +++ b/pkg/workflow/permissions.go @@ -4,6 +4,7 @@ import ( "slices" "github.com/github/gh-aw/pkg/logger" + "github.com/github/gh-aw/pkg/typeutil" ) var permissionsLog = logger.New("workflow:permissions") @@ -12,7 +13,7 @@ var validPermissionScopes = func() map[string]struct{} { scopes := GetAllPermissionScopes() appOnlyScopes := GetAllGitHubAppOnlyScopes() - m := make(map[string]struct{}, safeAllocationCapacity(len(scopes), len(appOnlyScopes), 1)) + m := make(map[string]struct{}, typeutil.SafeAllocationCapacity(len(scopes), len(appOnlyScopes), 1)) for _, scope := range scopes { m[string(scope)] = struct{}{} } diff --git a/pkg/workflow/permissions_validation.go b/pkg/workflow/permissions_validation.go index 9b312d8a71c..aa785b846a0 100644 --- a/pkg/workflow/permissions_validation.go +++ b/pkg/workflow/permissions_validation.go @@ -14,6 +14,7 @@ import ( "github.com/github/gh-aw/pkg/setutil" "github.com/github/gh-aw/pkg/sliceutil" "github.com/github/gh-aw/pkg/stringutil" + "github.com/github/gh-aw/pkg/typeutil" "github.com/goccy/go-yaml" ) @@ -21,7 +22,7 @@ var allPermissionScopeNames = sync.OnceValue(func() []string { ghTokenScopes := GetAllPermissionScopes() appOnlyScopes := GetAllGitHubAppOnlyScopes() // +1 for copilot-requests which is not in GetAllPermissionScopes - all := make([]string, 0, safeAllocationCapacity(len(ghTokenScopes), len(appOnlyScopes), 1)) + all := make([]string, 0, typeutil.SafeAllocationCapacity(len(ghTokenScopes), len(appOnlyScopes), 1)) for _, scope := range ghTokenScopes { all = append(all, string(scope)) } diff --git a/pkg/workflow/run_step_sanitizer.go b/pkg/workflow/run_step_sanitizer.go index 645ab1a3cf2..1742efe21f2 100644 --- a/pkg/workflow/run_step_sanitizer.go +++ b/pkg/workflow/run_step_sanitizer.go @@ -50,10 +50,10 @@ import ( "slices" "strings" - "github.com/goccy/go-yaml" - "github.com/github/gh-aw/pkg/logger" "github.com/github/gh-aw/pkg/setutil" + "github.com/github/gh-aw/pkg/typeutil" + "github.com/goccy/go-yaml" ) var runStepSanitizerLog = logger.New("workflow:run_step_sanitizer") @@ -147,7 +147,7 @@ func sanitizeRunStepExpressions(step map[string]any) (map[string]any, []string, // - If it exists with a different value → pick an alternate name by appending // a numeric suffix (_2, _3, …) so the original user-defined value is preserved. existingEnv, _ := step["env"].(map[string]any) - newEnv := make(map[string]any, safeAllocationCapacity(len(existingEnv), len(ordered))) + newEnv := make(map[string]any, typeutil.SafeAllocationCapacity(len(existingEnv), len(ordered))) maps.Copy(newEnv, existingEnv) for i := range ordered { diff --git a/pkg/workflow/safe_jobs_needs_validation.go b/pkg/workflow/safe_jobs_needs_validation.go index f5e827eb30c..5e8f66618ee 100644 --- a/pkg/workflow/safe_jobs_needs_validation.go +++ b/pkg/workflow/safe_jobs_needs_validation.go @@ -9,6 +9,7 @@ import ( "github.com/github/gh-aw/pkg/logger" "github.com/github/gh-aw/pkg/setutil" "github.com/github/gh-aw/pkg/stringutil" + "github.com/github/gh-aw/pkg/typeutil" ) var safeJobsNeedsValidationLog = logger.New("workflow:safe_jobs_needs_validation") @@ -195,7 +196,7 @@ func detectSafeJobCycles(jobs map[string]*SafeJobConfig) error { } if state[node] == visiting { // Build the cycle description using original names where available - cycleNodes := make([]string, 0, safeAllocationCapacity(len(path), 1)) + cycleNodes := make([]string, 0, typeutil.SafeAllocationCapacity(len(path), 1)) for _, p := range path { if orig, ok := originalNames[p]; ok { cycleNodes = append(cycleNodes, orig) diff --git a/pkg/workflow/safe_output_handlers.go b/pkg/workflow/safe_output_handlers.go index 44d05276155..18b90ae6982 100644 --- a/pkg/workflow/safe_output_handlers.go +++ b/pkg/workflow/safe_output_handlers.go @@ -5,6 +5,7 @@ import ( "strings" "github.com/github/gh-aw/pkg/logger" + "github.com/github/gh-aw/pkg/typeutil" ) var safeOutputHandlerLog = logger.New("workflow:safe_output_handlers") @@ -621,7 +622,7 @@ var safeOutputHandlers = []safeOutputHandlerDescriptor{ var safeOutputHandlersByKey = buildSafeOutputHandlersByKey() func buildSafeOutputHandlersByKey() map[string]safeOutputHandlerDescriptor { - result := make(map[string]safeOutputHandlerDescriptor, safeAllocationCapacity(len(safeOutputHandlers), 1)) + result := make(map[string]safeOutputHandlerDescriptor, typeutil.SafeAllocationCapacity(len(safeOutputHandlers), 1)) for _, handler := range safeOutputHandlers { if handler.Key != "" { result[handler.Key] = handler diff --git a/pkg/workflow/threat_detection_steps.go b/pkg/workflow/threat_detection_steps.go index 7a1ca51fd36..d6e4ce645f4 100644 --- a/pkg/workflow/threat_detection_steps.go +++ b/pkg/workflow/threat_detection_steps.go @@ -7,6 +7,7 @@ import ( "strconv" "github.com/github/gh-aw/pkg/constants" + "github.com/github/gh-aw/pkg/typeutil" ) // buildDetectionJobSteps builds the threat detection steps to be run in the separate detection job. @@ -483,7 +484,7 @@ func (c *Compiler) buildCustomThreatDetectionSteps(steps []any) []string { // Inject the detection guard condition unless the user already provided an if: condition. if _, hasIf := stepMap["if"]; !hasIf { // Clone the map to avoid mutating the original config. - injected := make(map[string]any, safeAllocationCapacity(len(stepMap), 1)) + injected := make(map[string]any, typeutil.SafeAllocationCapacity(len(stepMap), 1)) maps.Copy(injected, stepMap) injected["if"] = detectionStepCondition stepMap = injected diff --git a/pkg/workflow/tools.go b/pkg/workflow/tools.go index e0a09dbd3c7..280ce308134 100644 --- a/pkg/workflow/tools.go +++ b/pkg/workflow/tools.go @@ -11,6 +11,7 @@ import ( "github.com/github/gh-aw/pkg/constants" "github.com/github/gh-aw/pkg/logger" "github.com/github/gh-aw/pkg/parser" + "github.com/github/gh-aw/pkg/typeutil" "github.com/github/gh-aw/pkg/workflow/compilerenv" "github.com/goccy/go-yaml" ) @@ -262,7 +263,7 @@ func (c *Compiler) buildCommandTriggerEventsMap(data *WorkflowData) (map[string] if existingMap, ok := existingAny.(map[string]any); ok { switch t := existingMap["types"].(type) { case []string: - newTypes := make([]any, 0, safeAllocationCapacity(len(t), 1)) + newTypes := make([]any, 0, typeutil.SafeAllocationCapacity(len(t), 1)) for _, s := range t { newTypes = append(newTypes, s) } @@ -348,7 +349,7 @@ func mergeLabelCommandOtherEvents(labelEventsMap map[string]any, otherEvents map if existingMap != nil && userMap != nil { existingTypes, _ := existingMap["types"].([]any) userTypes, _ := userMap["types"].([]any) - merged := make([]any, 0, safeAllocationCapacity(len(existingTypes), len(userTypes))) + merged := make([]any, 0, typeutil.SafeAllocationCapacity(len(existingTypes), len(userTypes))) merged = append(merged, existingTypes...) merged = append(merged, userTypes...) existingMap["types"] = merged @@ -759,7 +760,7 @@ func ensureGitHubAllowedTool(githubConfig map[string]any, tool string) { } func appendStringAny(values []string, value string) []any { - result := make([]any, 0, safeAllocationCapacity(len(values), 1)) + result := make([]any, 0, typeutil.SafeAllocationCapacity(len(values), 1)) for _, existing := range values { result = append(result, existing) } diff --git a/pkg/workflow/workflow_import_merge.go b/pkg/workflow/workflow_import_merge.go index cb6d885fd7c..e3dfcec87bd 100644 --- a/pkg/workflow/workflow_import_merge.go +++ b/pkg/workflow/workflow_import_merge.go @@ -11,6 +11,7 @@ import ( "github.com/github/gh-aw/pkg/constants" "github.com/github/gh-aw/pkg/logger" "github.com/github/gh-aw/pkg/parser" + "github.com/github/gh-aw/pkg/typeutil" "github.com/goccy/go-yaml" ) @@ -208,7 +209,7 @@ func mergeJobStepField(mainJob map[string]any, importedJob map[string]any, field return append([]any(nil), mainSteps...), true } - mergedSteps := make([]any, 0, safeAllocationCapacity(len(importedSteps), len(mainSteps))) + mergedSteps := make([]any, 0, typeutil.SafeAllocationCapacity(len(importedSteps), len(mainSteps))) mergedSteps = append(mergedSteps, importedSteps...) mergedSteps = append(mergedSteps, mainSteps...) return mergedSteps, true From de8aa1f033d4c16cc1575e619812466fcfdd13da Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 24 Aug 2026 20:32:01 +0000 Subject: [PATCH 4/6] Retain allocation helper edge tests Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/typeutil/allocation_helpers_test.go | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/pkg/typeutil/allocation_helpers_test.go b/pkg/typeutil/allocation_helpers_test.go index 19932137458..9921ed5aff3 100644 --- a/pkg/typeutil/allocation_helpers_test.go +++ b/pkg/typeutil/allocation_helpers_test.go @@ -12,14 +12,24 @@ import ( func TestSafeAllocationCapacity(t *testing.T) { t.Parallel() + t.Run("handles zero inputs", func(t *testing.T) { + assert.Zero(t, SafeAllocationCapacity()) + assert.Zero(t, SafeAllocationCapacity(0, 0)) + assert.Equal(t, 5, SafeAllocationCapacity(0, 5)) + assert.Equal(t, 5, SafeAllocationCapacity(5, 0)) + }) + t.Run("sums sizes when the result fits in int", func(t *testing.T) { assert.Equal(t, 5, SafeAllocationCapacity(2, 3)) + assert.Equal(t, 6000, SafeAllocationCapacity(1000, 5000)) assert.Equal(t, math.MaxInt, SafeAllocationCapacity(math.MaxInt-1, 1)) + assert.Equal(t, math.MaxInt, SafeAllocationCapacity(math.MaxInt-2, 1, 1)) }) t.Run("returns zero when the sum would overflow int", func(t *testing.T) { assert.Zero(t, SafeAllocationCapacity(math.MaxInt, 1)) assert.Zero(t, SafeAllocationCapacity(math.MaxInt-1, 2)) + assert.Zero(t, SafeAllocationCapacity(math.MaxInt-2, 2, 1)) }) t.Run("returns zero for negative parts", func(t *testing.T) { From 3d2f4a89b7fe5713ca0c51844bf02d4b38e700fd Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 24 Aug 2026 22:19:10 +0000 Subject: [PATCH 5/6] Address PR follow-up checks Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com> --- ...80-safe-allocation-capacity-in-typeutil.md | 54 +++++++++++++++++++ pkg/workflow/domains.go | 2 +- 2 files changed, 55 insertions(+), 1 deletion(-) create mode 100644 docs/adr/55480-safe-allocation-capacity-in-typeutil.md diff --git a/docs/adr/55480-safe-allocation-capacity-in-typeutil.md b/docs/adr/55480-safe-allocation-capacity-in-typeutil.md new file mode 100644 index 00000000000..ff0f2b46f6c --- /dev/null +++ b/docs/adr/55480-safe-allocation-capacity-in-typeutil.md @@ -0,0 +1,54 @@ +# ADR-55480: Centralize Safe Allocation Capacity Calculation in typeutil + +**Date**: 2026-08-24 +**Status**: Accepted +**Deciders**: Copilot + +--- + +### Context + +CodeQL flagged several `go/allocation-size-overflow` paths where summed `len(...)` values were passed directly as allocation capacity hints. If extremely large or malformed inputs caused those sums to overflow `int`, the resulting capacity could become negative or otherwise unsafe before reaching `make`. + +The affected call sites were in multiple packages, including `pkg/workflow` and `pkg/cli`, so a package-private helper would either duplicate the overflow logic or leave future call sites without a shared convention. + +### Decision + +We will provide `typeutil.SafeAllocationCapacity(parts ...int) int` as the shared helper for allocation capacity hints built from multiple integer parts. The helper returns the summed capacity when every part is non-negative and the addition does not overflow; otherwise it returns zero so callers still allocate correctly without unsafe preallocation. + +Call sites that previously used direct additive capacity expressions will use this shared helper when summing length-derived allocation hints across packages. + +### Alternatives Considered + +#### Alternative 1: Keep package-local helpers + +Keeping separate helpers in `pkg/workflow` and `pkg/cli` avoids a new shared API, but it duplicates security-sensitive overflow handling and lets behavior drift between packages. + +#### Alternative 2: Inline overflow checks at each allocation site + +Inlining checks keeps each call site self-contained, but it makes the overflow policy harder to audit and increases the chance that a future allocation hint misses one of the required checks. + +#### Alternative 3: Remove capacity hints entirely + +Removing all summed capacity hints would also avoid overflow, but it discards useful preallocation for normal inputs and obscures the intended size relationship between the source collections and the destination allocation. + +### Consequences + +#### Positive + +- CodeQL-flagged allocation capacity calculations now use a single overflow-safe helper. +- The zero-capacity fallback preserves correctness while avoiding unsafe preallocation on overflow or negative input. +- Future callers have one reusable helper for length-derived allocation hints. + +#### Negative + +- `pkg/typeutil` gains a small public API that should keep its current overflow semantics stable. + +#### Neutral + +- Valid inputs preserve the existing capacity hint behavior. +- Overflow and negative inputs may allocate with default growth instead of the original precomputed capacity. + +--- + +*ADR finalized after implementation and CodeQL review.* diff --git a/pkg/workflow/domains.go b/pkg/workflow/domains.go index 5864b33effa..44afa571ec8 100644 --- a/pkg/workflow/domains.go +++ b/pkg/workflow/domains.go @@ -4,7 +4,6 @@ import ( _ "embed" "encoding/json" "fmt" - "github.com/github/gh-aw/pkg/typeutil" "sort" "strings" "sync" @@ -13,6 +12,7 @@ import ( "github.com/github/gh-aw/pkg/logger" "github.com/github/gh-aw/pkg/sliceutil" "github.com/github/gh-aw/pkg/stringutil" + "github.com/github/gh-aw/pkg/typeutil" ) var domainsLog = logger.New("workflow:domains") From 38b0a59389e80b3565af1399f3b085e99e794abc Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 24 Aug 2026 22:29:54 +0000 Subject: [PATCH 6/6] Address non-blocking review feedback Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com> --- .../experiments_analyze_statistics_test.go | 41 +++++++++++++++---- pkg/typeutil/allocation.go | 3 +- ...ion_helpers_test.go => allocation_test.go} | 0 3 files changed, 35 insertions(+), 9 deletions(-) rename pkg/typeutil/{allocation_helpers_test.go => allocation_test.go} (100%) diff --git a/pkg/cli/experiments_analyze_statistics_test.go b/pkg/cli/experiments_analyze_statistics_test.go index 765cb64812d..5b7209c918f 100644 --- a/pkg/cli/experiments_analyze_statistics_test.go +++ b/pkg/cli/experiments_analyze_statistics_test.go @@ -177,16 +177,41 @@ func TestExpectedProportions(t *testing.T) { func TestExperimentVariantCounts(t *testing.T) { t.Parallel() - exp := ExperimentVariantStats{ - Variants: map[string]int{"control": 3}, - } - cfg := &workflow.ExperimentConfig{ - Variants: []string{"control", "candidate"}, - } + t.Run("includes declared variants with zero counts", func(t *testing.T) { + exp := ExperimentVariantStats{ + Variants: map[string]int{"control": 3}, + } + cfg := &workflow.ExperimentConfig{ + Variants: []string{"control", "candidate"}, + } + + got := experimentVariantCounts(exp, cfg, true) - got := experimentVariantCounts(exp, cfg, true) + assert.Equal(t, map[string]int{"control": 3, "candidate": 0}, got) + }) - assert.Equal(t, map[string]int{"control": 3, "candidate": 0}, got) + t.Run("returns observed variants when declared variants are excluded", func(t *testing.T) { + exp := ExperimentVariantStats{ + Variants: map[string]int{"control": 3}, + } + cfg := &workflow.ExperimentConfig{ + Variants: []string{"control", "candidate"}, + } + + got := experimentVariantCounts(exp, cfg, false) + + assert.Equal(t, exp.Variants, got) + }) + + t.Run("returns observed variants when config is nil", func(t *testing.T) { + exp := ExperimentVariantStats{ + Variants: map[string]int{"control": 3}, + } + + got := experimentVariantCounts(exp, nil, true) + + assert.Equal(t, exp.Variants, got) + }) } // TestComputeExperimentAnalysis verifies the end-to-end statistical computation. diff --git a/pkg/typeutil/allocation.go b/pkg/typeutil/allocation.go index 4a0a8ce649a..1edd5992150 100644 --- a/pkg/typeutil/allocation.go +++ b/pkg/typeutil/allocation.go @@ -4,7 +4,8 @@ import "math" // SafeAllocationCapacity returns the summed capacity hint when it fits in int. // When the total would overflow, it falls back to 0 so callers can skip -// preallocation without changing correctness. +// preallocation without changing correctness. The helper is intentionally +// side-effect free so utility callers do not inherit logging dependencies. func SafeAllocationCapacity(parts ...int) int { total := 0 for _, part := range parts { diff --git a/pkg/typeutil/allocation_helpers_test.go b/pkg/typeutil/allocation_test.go similarity index 100% rename from pkg/typeutil/allocation_helpers_test.go rename to pkg/typeutil/allocation_test.go