Skip to content

[lint-monster] Migrate unsafe sort.Slice calls to type-safe slices.SortFunc #38014

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 2 commits into from
Jun 9, 2026
Merged

[lint-monster] Migrate unsafe sort.Slice calls to type-safe slices.SortFunc #38014

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
756fb94
Initial plan
Copilot Jun 9, 2026
220bd03
refactor: migrate unsafe sort.Slice calls to slices.SortFunc
Copilot Jun 9, 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
18 changes: 14 additions & 4 deletions pkg/cli/audit_comparison.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -268,11 +268,21 @@ func selectAuditComparisonBaseline(current ProcessedRun, candidates []auditCompa
scoreAuditComparisonCandidate(current, &candidates[index])
}

sort.SliceStable(candidates, func(left, right int) bool {
if candidates[left].Score != candidates[right].Score {
return candidates[left].Score > candidates[right].Score
slices.SortStableFunc(candidates, func(left, right auditComparisonCandidate) int {

@github-actions github-actions Bot Jun 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[/tdd] candidates[0] is returned immediately after this sort — it is the audit comparison baseline. A sign inversion in either the Score or CreatedAt comparisons would silently select the lowest-scoring or oldest run as the baseline for every audit.

No test was added to verify: (1) higher Score wins, (2) for equal scores, more recent CreatedAt wins, (3) stability is preserved.

💡 Minimal regression test shape 
func TestSelectAuditComparisonBaseline_Ordering(t *testing.T) {
	// Verify higher score wins
	// Verify equal-score tiebreak by CreatedAt descending
}

if left.Score != right.Score {
if left.Score > right.Score {
return -1
}
return 1
}
switch {
case left.Run.CreatedAt.After(right.Run.CreatedAt):
return -1
case right.Run.CreatedAt.After(left.Run.CreatedAt):
return 1
default:
return 0
}
return candidates[left].Run.CreatedAt.After(candidates[right].Run.CreatedAt)
})

return &candidates[0]
Expand Down
39 changes: 31 additions & 8 deletions pkg/cli/audit_expanded.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
"fmt"
"os"
"path/filepath"
"slices"
"sort"
"strings"
"time"
Expand Down Expand Up @@ -402,11 +403,21 @@ func buildSafeOutputSummary(items []CreatedItemReport, chainMetrics SafeOutputCh
Count: count,
})
}
sort.Slice(summary.TypeDetails, func(i, j int) bool {
if summary.TypeDetails[i].Count == summary.TypeDetails[j].Count {
return summary.TypeDetails[i].Type < summary.TypeDetails[j].Type
slices.SortFunc(summary.TypeDetails, func(a, b SafeOutputTypeDetail) int {
if a.Count == b.Count {
switch {
case a.Type < b.Type:
return -1
case a.Type > b.Type:
return 1
default:
return 0
}
}
if a.Count > b.Count {
return -1
}
return summary.TypeDetails[i].Count > summary.TypeDetails[j].Count
return 1
})

// Build human-readable summary string
Expand Down Expand Up @@ -537,8 +548,14 @@ func buildMCPServerHealth(mcpToolUsage *MCPToolUsageData, mcpFailures []MCPFailu
}

// Sort servers by request count (highest first)
sort.Slice(health.Servers, func(i, j int) bool {
return health.Servers[i].RequestCount > health.Servers[j].RequestCount
slices.SortFunc(health.Servers, func(a, b MCPServerHealthDetail) int {
if a.RequestCount > b.RequestCount {
return -1
}
if a.RequestCount < b.RequestCount {
return 1
}
return 0
})

// Build summary string
Expand Down Expand Up @@ -581,8 +598,14 @@ func buildSlowestToolCalls(calls []MCPToolCall, topN int) []MCPSlowestToolCall {
}

// Sort by duration descending
sort.Slice(withDuration, func(i, j int) bool {
return withDuration[i].duration > withDuration[j].duration
slices.SortFunc(withDuration, func(a, b callWithDuration) int {
if a.duration > b.duration {
return -1
}
if a.duration < b.duration {
return 1
}
return 0
})

// Take top N
Expand Down
12 changes: 9 additions & 3 deletions pkg/cli/compile_stats.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import (
"fmt"
"os"
"path/filepath"
"sort"
"slices"
"strconv"

"github.com/github/gh-aw/pkg/console"
Expand Down Expand Up @@ -252,8 +252,14 @@ func displayStatsTable(statsList []*WorkflowStats) {
compileStatsLog.Printf("Displaying stats table: workflow_count=%d", len(statsList))

// Sort by file size (descending)
sort.Slice(statsList, func(i, j int) bool {
return statsList[i].FileSize > statsList[j].FileSize
slices.SortFunc(statsList, func(a, b *WorkflowStats) int {
if a.FileSize > b.FileSize {
return -1
}
if a.FileSize < b.FileSize {
return 1
}
return 0
})

// Calculate totals
Expand Down
13 changes: 10 additions & 3 deletions pkg/cli/deps_outdated.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import (
"io"
"net/http"
"os"
"sort"
"slices"
"strings"
"time"

Expand Down Expand Up @@ -100,8 +100,15 @@ func DisplayOutdatedDependencies(outdated []OutdatedDependency, totalDeps int) {
fmt.Fprintln(os.Stderr, "")

// Sort by module name
sort.Slice(outdated, func(i, j int) bool {
return outdated[i].Module < outdated[j].Module
slices.SortFunc(outdated, func(a, b OutdatedDependency) int {

@github-actions github-actions Bot Jun 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[/improve-codebase-architecture] This 7-line switch block reinvents cmp.Compare, which the codebase already uses for identical patterns in logs_report_mcp.go, logs_report_tools.go, audit_agentic_analysis.go, and others. About 20 of the 31 changed files have the same pattern.

💡 Suggested simplification 
slices.SortFunc(outdated, func(a, b OutdatedDependency) int {
	return cmp.Compare(a.Module, b.Module)
})

Applying cmp.Compare (and cmp.Or for multi-field comparators) across the PR would cut ~150 lines of added code and stay consistent with the established pattern. See pkg/actionpins/actionpins.go for the multi-field cmp.Or version.

switch {
case a.Module < b.Module:
return -1
case a.Module > b.Module:
return 1
default:
return 0
}
})

// Display table
Expand Down
14 changes: 11 additions & 3 deletions pkg/cli/deps_security.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import (
"io"
"net/http"
"os"
"sort"
"slices"
"strings"

"github.com/github/gh-aw/pkg/console"
Expand Down Expand Up @@ -100,8 +100,16 @@ func DisplaySecurityAdvisories(advisories []SecurityAdvisory) {
fmt.Fprintln(os.Stderr, "")

// Sort by severity (critical first)
sort.Slice(advisories, func(i, j int) bool {
return severityWeight(advisories[i].Severity) > severityWeight(advisories[j].Severity)
slices.SortFunc(advisories, func(a, b SecurityAdvisory) int {
aw := severityWeight(a.Severity)
bw := severityWeight(b.Severity)
if aw > bw {
return -1
}
if aw < bw {
return 1
}
return 0
})

// Display each advisory
Expand Down
23 changes: 20 additions & 3 deletions pkg/cli/experiments_command.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (
"os"
"os/exec"
"path/filepath"
"slices"
"sort"
"strings"

Expand Down Expand Up @@ -685,8 +686,15 @@ func experimentDetailsFromState(workflowID, branchName string, state *Experiment
Total: total,
})
}
sort.Slice(experiments, func(i, j int) bool {
return experiments[i].Name < experiments[j].Name
slices.SortFunc(experiments, func(a, b ExperimentVariantStats) int {
switch {
case a.Name < b.Name:
return -1
case a.Name > b.Name:
return 1
default:
return 0
}
})

recentRuns := state.Runs
Expand Down Expand Up @@ -771,7 +779,16 @@ func printExperimentDetails(d *ExperimentDetails) {
for k, v := range exp.Variants {
pairs = append(pairs, kv{k, v})
}
sort.Slice(pairs, func(i, j int) bool { return pairs[i].k < pairs[j].k })
slices.SortFunc(pairs, func(a, b kv) int {
switch {
case a.k < b.k:
return -1
case a.k > b.k:
return 1
default:
return 0
}
})
for _, p := range pairs {
pct := 0
if exp.Total > 0 {
Expand Down
11 changes: 8 additions & 3 deletions pkg/cli/firewall_policy.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ import (
"path/filepath"
"regexp"
"slices"
"sort"
"strings"

"github.com/github/gh-aw/pkg/logger"
Expand Down Expand Up @@ -99,8 +98,14 @@ func loadPolicyManifest(manifestPath string) (*PolicyManifest, error) {
}

// Sort rules by order for deterministic matching
sort.Slice(manifest.Rules, func(i, j int) bool {
return manifest.Rules[i].Order < manifest.Rules[j].Order
slices.SortFunc(manifest.Rules, func(a, b PolicyRule) int {
if a.Order < b.Order {
return -1
}
if a.Order > b.Order {
return 1
}
return 0
})

firewallPolicyLog.Printf("Loaded policy manifest: version=%d, rules=%d, ssl_bump=%v, dlp=%v",
Expand Down
55 changes: 39 additions & 16 deletions pkg/cli/forecast.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import (
"os"
"os/signal"
"path/filepath"
"slices"
"sort"
"strings"
"time"
Expand Down Expand Up @@ -329,16 +330,22 @@ func RunForecast(config ForecastConfig) error {
}

// Sort results by Monte Carlo P50 (or point estimate when MC unavailable) descending.

@github-actions github-actions Bot Jun 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[/tdd] This sort controls which workflow ranks first in forecast output — a sign flip would silently display the highest-cost workflow as the best option. No test was added to verify the comparator sign or the nil-guard branch on MonteCarlo.

💡 Minimal regression test shape 
func TestForecastResultSortOrder(t *testing.T) {
	results := []ForecastWorkflowResult{
		{WorkflowID: "low",  ProjectedAIC: 10},
		{WorkflowID: "high", ProjectedAIC: 100},
		{WorkflowID: "mc",   MonteCarlo: &MonteCarloResult{P50ProjectedAIC: 50}},
	}
	slices.SortFunc(results, compareForecastResults) // extracted helper
	assert.Equal(t, "high", results[0].WorkflowID)
	assert.Equal(t, "mc",   results[1].WorkflowID)
}

Extract the comparator to compareForecastResults (see duplicate sort in emitPartialForecastResults) so a single test covers both call sites.

sort.Slice(results, func(i, j int) bool {
pi := results[i].ProjectedAIC
if mc := results[i].MonteCarlo; mc != nil {
slices.SortFunc(results, func(a, b ForecastWorkflowResult) int {
pi := a.ProjectedAIC
if mc := a.MonteCarlo; mc != nil {
pi = mc.P50ProjectedAIC
}
pj := results[j].ProjectedAIC
if mc := results[j].MonteCarlo; mc != nil {
pj := b.ProjectedAIC
if mc := b.MonteCarlo; mc != nil {
pj = mc.P50ProjectedAIC
}
return pi > pj
if pi > pj {
return -1
}
if pi < pj {
return 1
}
return 0
})

output := ForecastResult{
Expand Down Expand Up @@ -837,11 +844,21 @@ func extractExperimentVariantStubs(cfg *workflow.FrontmatterConfig) []ForecastVa
})
}
}
sort.Slice(stubs, func(i, j int) bool {
if stubs[i].ExperimentName != stubs[j].ExperimentName {
return stubs[i].ExperimentName < stubs[j].ExperimentName
slices.SortFunc(stubs, func(a, b ForecastVariantResult) int {
if a.ExperimentName != b.ExperimentName {
if a.ExperimentName < b.ExperimentName {
return -1
}
return 1
}
switch {
case a.Variant < b.Variant:
return -1
case a.Variant > b.Variant:
return 1
default:
return 0
}
return stubs[i].Variant < stubs[j].Variant
})
return stubs
}
Expand Down Expand Up @@ -1032,16 +1049,22 @@ func emitPartialForecastResults(results []ForecastWorkflowResult, config Forecas
fmt.Sprintf("Forecast interrupted; emitting partial results for %d workflow(s) processed so far.", len(results))))

// Sort partial results by Monte Carlo P50 descending (mirrors the full-results sort).
sort.Slice(results, func(i, j int) bool {
pi := results[i].ProjectedAIC
if mc := results[i].MonteCarlo; mc != nil {
slices.SortFunc(results, func(a, b ForecastWorkflowResult) int {

@github-actions github-actions Bot Jun 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Duplicate Monte Carlo P50 comparator will diverge — this exact 12-line block also appears at line 333 in RunForecast. When one changes (e.g., adding a NaN guard, switching to P90), the other will not.

💡 Suggested fix

Extract to a package-level helper and call it from both sites:

func compareForecastByP50(a, b ForecastWorkflowResult) int {
    pa := a.ProjectedAIC
    if mc := a.MonteCarlo; mc != nil {
        pa = mc.P50ProjectedAIC
    }
    pb := b.ProjectedAIC
    if mc := b.MonteCarlo; mc != nil {
        pb = mc.P50ProjectedAIC
    }
    return cmp.Compare(pb, pa) // descending
}

// at both call sites:
slices.SortFunc(results, compareForecastByP50)

With cmp.Compare(pb, pa) the descending direction is immediately readable; with the current if pi > pj { return -1 } pattern it takes a moment to confirm.

@github-actions github-actions Bot Jun 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[/improve-codebase-architecture] This comparator is byte-for-byte identical to the one in RunForecast (line ~332). If the sort order ever needs to change, it will need to be updated in both places.

💡 Extract to a shared helper 
// compareForecastResults orders results by Monte Carlo P50 (or ProjectedAIC fallback), descending.
func compareForecastResults(a, b ForecastWorkflowResult) int {
	pi := a.ProjectedAIC
	if mc := a.MonteCarlo; mc != nil {
		pi = mc.P50ProjectedAIC
	}
	pj := b.ProjectedAIC
	if mc := b.MonteCarlo; mc != nil {
		pj = mc.P50ProjectedAIC
	}
	return cmp.Compare(pj, pi) // descending: higher cost first
}

Both sites become slices.SortFunc(results, compareForecastResults).

pi := a.ProjectedAIC
if mc := a.MonteCarlo; mc != nil {
pi = mc.P50ProjectedAIC
}
pj := results[j].ProjectedAIC
if mc := results[j].MonteCarlo; mc != nil {
pj := b.ProjectedAIC
if mc := b.MonteCarlo; mc != nil {
pj = mc.P50ProjectedAIC
}
return pi > pj
if pi > pj {
return -1
}
if pi < pj {
return 1
}
return 0
})

output := ForecastResult{
Expand Down
31 changes: 24 additions & 7 deletions pkg/cli/gateway_logs_mcp.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import (
"fmt"
"os"
"path/filepath"
"sort"
"slices"
"strings"
"time"

Expand Down Expand Up @@ -234,15 +234,32 @@ func buildMCPSummaryStats(gatewayMetrics *GatewayMetrics, mcpData *MCPToolUsageD
}

// Sort summaries by server name, then tool name
sort.Slice(mcpData.Summary, func(i, j int) bool {
if mcpData.Summary[i].ServerName != mcpData.Summary[j].ServerName {
return mcpData.Summary[i].ServerName < mcpData.Summary[j].ServerName
slices.SortFunc(mcpData.Summary, func(a, b MCPToolSummary) int {

@github-actions github-actions Bot Jun 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[/improve-codebase-architecture] This two-field sort (ServerName, then ToolName) is a perfect candidate for cmp.Or. Note that logs_report_mcp.go — in the same package — already uses exactly this pattern for the equivalent summaries sort.

💡 Match the existing pattern 
slices.SortFunc(mcpData.Summary, func(a, b MCPToolSummary) int {
	return cmp.Or(
		cmp.Compare(a.ServerName, b.ServerName),
		cmp.Compare(a.ToolName, b.ToolName),
	)
})

This keeps the two MCP-summary sort implementations consistent.

if a.ServerName != b.ServerName {
if a.ServerName < b.ServerName {
return -1
}
return 1
}
switch {
case a.ToolName < b.ToolName:
return -1
case a.ToolName > b.ToolName:
return 1
default:
return 0
}
return mcpData.Summary[i].ToolName < mcpData.Summary[j].ToolName
})

// Sort servers by name
sort.Slice(mcpData.Servers, func(i, j int) bool {
return mcpData.Servers[i].ServerName < mcpData.Servers[j].ServerName
slices.SortFunc(mcpData.Servers, func(a, b MCPServerStats) int {
switch {
case a.ServerName < b.ServerName:
return -1
case a.ServerName > b.ServerName:
return 1
default:
return 0
}
})
}
Loading
Loading