Consolidate ~25 duplicate helper implementations#39720
Conversation
…pers - Replaced 10 inline percentage calculations with safePercent() - Fixed missing zero-guard at health_metrics.go:178 - Replaced 9 inline truncations with stringutil.Truncate() - All changes build successfully Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
- Removed appendIfMissing from claude_tools.go, replaced with sliceutil.MergeUnique (11 sites) - Removed addUniqueWorkflow from logs_report_errors.go, replaced with sliceutil.MergeUnique (3 sites) - Removed uniqueSorted from central_slash_command_workflow.go, replaced with sliceutil.Deduplicate + sort (2 sites) - All changes build successfully Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
P2.1: Collapsed byte-identical templatable env-var builders - Extracted buildTemplatableEnvVar from buildTemplatableBoolEnvVar and buildTemplatableIntEnvVar - Kept original function names as thin wrappers for readability P2.2: Extracted anomaly-marker formatting (8 sites) - Added formatAnomalyTag(bool) string to audit_math_helpers.go - Added formatAnomalyNote(bool, string) string to audit_math_helpers.go - Replaced all 8 inline anomaly formatting blocks in audit_diff_render.go P2.3: Used sortedMapKeys at 3 map[string]string sites - safe_outputs_config_helpers.go - safe_scripts.go - safe_outputs_app_config.go All changes build successfully Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
- Updated logs_report_test.go to use sliceutil.MergeUnique instead of removed addUniqueWorkflow - All tests passing - make agent-report-progress passes Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR consolidates several duplicated inline helper implementations across pkg/cli and pkg/workflow to canonical utilities, improving consistency and reducing repeated logic across the codebase.
Changes:
- Replace ad-hoc percentage calculations with
safePercent(adds a consistent zero-guard behavior). - Replace repeated string truncation blocks with
stringutil.Truncate. - Replace local slice dedupe helpers with
sliceutil.MergeUnique/sliceutil.Deduplicate, and centralize map key sorting viasortedMapKeys. - Extract shared templatable env-var formatting into
buildTemplatableEnvVar, keeping existing wrapper names for compatibility. - Consolidate anomaly marker formatting into
formatAnomalyTag/formatAnomalyNote.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/templatables.go | Extracts shared env-var YAML formatting logic into a single helper with thin wrappers. |
| pkg/workflow/safe_scripts.go | Switches deterministic key ordering to sortedMapKeys. |
| pkg/workflow/safe_outputs_config_helpers.go | Switches deterministic key ordering to sortedMapKeys for stable JSON output. |
| pkg/workflow/safe_outputs_app_config.go | Switches permission field ordering to sortedMapKeys for deterministic step generation. |
| pkg/workflow/claude_tools.go | Replaces local “append if missing” helper with sliceutil dedupe helpers. |
| pkg/workflow/central_slash_command_workflow.go | Replaces local uniqueSorted helper with sliceutil.Deduplicate + explicit sort. |
| pkg/cli/mcp_registry_list.go | Uses stringutil.Truncate for table-safe descriptions. |
| pkg/cli/mcp_list.go | Uses stringutil.Truncate for table-safe columns and list descriptions. |
| pkg/cli/mcp_inspect_mcp.go | Uses stringutil.Truncate for capability descriptions. |
| pkg/cli/logs_report_test.go | Updates tests to use sliceutil.MergeUnique for workflow list de-duplication behavior. |
| pkg/cli/logs_report_mcp.go | Uses sliceutil.MergeUnique while aggregating unique workflow names per summary. |
| pkg/cli/logs_report_errors.go | Uses sliceutil.MergeUnique while aggregating unique workflow names per summary. |
| pkg/cli/health_metrics.go | Replaces percent calculations with safePercent. |
| pkg/cli/experiments_analyze_statistics.go | Replaces percent calculations with safePercent. |
| pkg/cli/deps_report.go | Replaces percent calculations with safePercent. |
| pkg/cli/audit_report.go | Uses stringutil.Truncate to cap extracted error messages deterministically. |
| pkg/cli/audit_report_analysis.go | Uses stringutil.Truncate for key-findings error message summaries. |
| pkg/cli/audit_math_helpers.go | Adds anomaly formatting helpers and provides shared safePercent. |
| pkg/cli/audit_expanded.go | Replaces error-rate calculations with safePercent. |
| pkg/cli/audit_diff_render.go | Consolidates anomaly markers via new audit math helpers. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 20/20 changed files
- Comments generated: 6
| func(summary *MissingToolSummary, tool MissingToolReport) { | ||
| summary.Count++ | ||
| summary.Workflows = addUniqueWorkflow(summary.Workflows, tool.WorkflowName) | ||
| summary.Workflows = sliceutil.MergeUnique(summary.Workflows, tool.WorkflowName) |
| func(summary *MissingDataSummary, data MissingDataReport) { | ||
| summary.Count++ | ||
| summary.Workflows = addUniqueWorkflow(summary.Workflows, data.WorkflowName) | ||
| summary.Workflows = sliceutil.MergeUnique(summary.Workflows, data.WorkflowName) |
| func(summary *MCPFailureSummary, failure MCPFailureReport) { | ||
| summary.Count++ | ||
| summary.Workflows = addUniqueWorkflow(summary.Workflows, failure.WorkflowName) | ||
| summary.Workflows = sliceutil.MergeUnique(summary.Workflows, failure.WorkflowName) |
| allowedTools = sliceutil.MergeUnique(allowedTools, fmt.Sprintf("Read(%s)", cacheDirPattern)) | ||
| allowedTools = sliceutil.MergeUnique(allowedTools, fmt.Sprintf("Write(%s)", cacheDirPattern)) | ||
| allowedTools = sliceutil.MergeUnique(allowedTools, fmt.Sprintf("Edit(%s)", cacheDirPattern)) | ||
| allowedTools = sliceutil.MergeUnique(allowedTools, fmt.Sprintf("MultiEdit(%s)", cacheDirPattern)) |
| for _, bashTool := range bashCacheTools { | ||
| allowedTools = appendIfMissing(allowedTools, bashTool) | ||
| allowedTools = sliceutil.MergeUnique(allowedTools, bashTool) | ||
| } | ||
| allowedTools = appendIfMissing(allowedTools, "BashOutput") | ||
| allowedTools = appendIfMissing(allowedTools, "KillBash") | ||
| allowedTools = sliceutil.MergeUnique(allowedTools, "BashOutput") | ||
| allowedTools = sliceutil.MergeUnique(allowedTools, "KillBash") |
| allowedTools = sliceutil.MergeUnique(allowedTools, fmt.Sprintf("Read(%s)", pattern)) | ||
| allowedTools = sliceutil.MergeUnique(allowedTools, fmt.Sprintf("Write(%s)", pattern)) | ||
| allowedTools = sliceutil.MergeUnique(allowedTools, fmt.Sprintf("Edit(%s)", pattern)) | ||
| allowedTools = sliceutil.MergeUnique(allowedTools, fmt.Sprintf("MultiEdit(%s)", pattern)) |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #39720 does not have the 'implementation' label (has_implementation_label=false) and has only 92 new lines of code in business logic directories, which is at or below the 100-line threshold (requires_adr_by_default_volume=false). |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
🧪 Test Quality Sentinel Report✅ Test Quality Score: 85/100 — Excellent
📊 Metrics & Test Classification (2 tests analyzed)
Go: 1 ( Score breakdown:
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /zoom-out, /improve-codebase-architecture, and /tdd — approving. Net −80 lines of clean, consistent consolidation across 20 files with all behaviour correctly preserved.
📋 Key Themes & Highlights
Observations (non-blocking)
- Missed migration:
outdatedPercentageindeps_report.gostill uses the verbose zero-guard pattern immediately above the newly migrated block - Stale test name:
TestAddUniqueWorkflowreferences the deletedaddUniqueWorkflowfunction - File boundary:
formatAnomalyTag/formatAnomalyNoteare string-formatting helpers landing in a math helpers file - Missing regression test: The only real correctness fix (NaN→0 in
calculateSuccessRate) has no accompanying test - Wrapper intent:
buildTemplatableBoolEnvVarwrapper comment does not explain why Bool and Int are now identical
Positive Highlights
- ✅ All 25 consolidation sites correctly preserve original behaviour
- ✅
buildTemplatableBoolEnvVarandbuildTemplatableIntEnvVarkept as thin wrappers — good API stability call - ✅ PR description clearly catalogs every substitution category with file-level detail
- ✅ Tests updated in lockstep with code changes
- ✅
safePercentcorrectly fixes a real NaN risk athealth_metrics.go:178
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer
| } | ||
|
|
||
| return float64(successCount) / float64(len(runs)) * 100 | ||
| return safePercent(successCount, len(runs)) |
There was a problem hiding this comment.
[/tdd] The NaN→0 behaviour change in calculateSuccessRate is the PR's only real correctness fix, but there is no regression test pinning the empty-runs case. Before this PR the function returned NaN for an empty slice; after the fix it returns 0.0. A missing test means this could silently regress.
💡 Suggested test
func TestCalculateSuccessRateEmpty(t *testing.T) {
got := calculateSuccessRate(nil)
if got != 0.0 {
t.Errorf("expected 0.0 for empty runs, got %v", got)
}
}The fix is correct and important — zero-guarding before the division prevents NaN propagation into health summary structs. Anchoring it with a test makes the intent explicit and prevents future regression.
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| result := addUniqueWorkflow(tt.workflows, tt.workflow) | ||
| result := sliceutil.MergeUnique(tt.workflows, tt.workflow) |
There was a problem hiding this comment.
[/zoom-out] TestAddUniqueWorkflow references a function that no longer exists. The test now exercises sliceutil.MergeUnique, which presumably has its own unit tests in pkg/sliceutil. Consider renaming to TestSliceutil_MergeUnique_Deduplication or removing it entirely if the behaviour is already covered downstream — stale names in tests mislead future contributors searching for coverage.
💡 Suggested fix
Option A — rename in place:
// TestMergeUniqueDeduplicatesWorkflows verifies that MergeUnique correctly
// deduplicates workflow names as previously guaranteed by addUniqueWorkflow.
func TestMergeUniqueDeduplicatesWorkflows(t *testing.T) {Option B — remove it and rely on pkg/sliceutil tests directly.
| return fmt.Sprintf("%.3f", delta) | ||
| } | ||
|
|
||
| // formatAnomalyTag returns a warning emoji suffix for markdown rendering |
There was a problem hiding this comment.
[/improve-codebase-architecture] formatAnomalyTag and formatAnomalyNote are string-formatting helpers, not arithmetic — placing them in audit_math_helpers.go stretches the file beyond its conceptual boundary. The math helpers file already contains safePercent, formatPercent, formatVolumeChange, and formatFloatDelta; the anomaly helpers belong in a separate audit_format_helpers.go alongside renderFirewallDiff* concerns, or at least noted as a deliberate placement choice.
💡 Suggestion
Move both functions to pkg/cli/audit_format_helpers.go (new file) so that audit_math_helpers.go stays focused on numeric calculations and the anomaly formatting has a natural home near the render functions that use it.
| @@ -201,14 +192,9 @@ func DisplayDependencyReportJSON(report *DependencyReport) error { | |||
| outdatedPercentage = float64(len(report.Outdated)) / float64(report.DirectDeps) * 100 | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
[/zoom-out] outdatedPercentage immediately above the migrated block still uses the verbose zero-guard pattern that this PR eliminates elsewhere. safePercent(len(report.Outdated), report.DirectDeps) would make the function internally consistent with the three lines directly below it.
💡 Suggested change
// Before (unchanged by this PR, but inconsistent):
outdatedPercentage := 0.0
if report.DirectDeps > 0 {
outdatedPercentage = float64(len(report.Outdated)) / float64(report.DirectDeps) * 100
}
// After:
outdatedPercentage := safePercent(len(report.Outdated), report.DirectDeps)The same pattern also applies to the outdatedPercentage guard in DisplayDependencyReport (not in this diff).
| // templatable boolean field. If value is a GitHub Actions expression it is | ||
| // embedded unquoted so that GitHub Actions can evaluate it at runtime; | ||
| // otherwise the literal string is quoted. Returns nil if value is nil. | ||
| func buildTemplatableBoolEnvVar(envVarName string, value *string) []string { |
There was a problem hiding this comment.
[/zoom-out] The buildTemplatableBoolEnvVar wrapper is correct and keeping the original name is good for backward compat, but the comment does not explain why Bool and Int now share the same base. A future reader who diffs the two wrappers might wonder if the convergence was intentional or an accidental loss of type-specific behaviour.
💡 Suggested comment addition
// buildTemplatableBoolEnvVar is a thin wrapper around buildTemplatableEnvVar.
// Boolean and integer fields render identically in YAML (both use quoted-string
// form for literals and unquoted for GitHub Actions expressions), so the
// implementations were identical — this wrapper exists only to preserve the
// original API surface.
func buildTemplatableBoolEnvVar(envVarName string, value *string) []string {
return buildTemplatableEnvVar(envVarName, value)
}There was a problem hiding this comment.
No blocking issues found. Two non-blocking observations are inline.
Review summary
What was checked
All 20 changed files were reviewed line-by-line, covering:
- Semantic equivalence of every helper substitution (
safePercent,stringutil.Truncate,sliceutil.MergeUnique/Deduplicate,formatAnomalyTag/Note,sortedMapKeys,buildTemplatableEnvVar) - Correctness of the
calculateSuccessRatezero-guard fix - Behavioral preservation in
uniqueSorted→Deduplicate + sort.Strings - Import consistency after helper removals
What is correct
- All semantic equivalences hold. The
mcp_*truncation sites (max 40/50/80) already usedmaxLen-3 + "..."soTruncate(s, maxLen)is byte-identical. TheuniqueSortedreplacement correctly preserves the sort-after-deduplicate behaviour. Thetemplatables.goextraction is valid (both functions were byte-identical before). - The
calculateSuccessRatezero-guard fix is a genuine improvement; callers are currently protected by thelen(runs) < 4guard incalculateTrend, but the defensive change is still right. sortimport correctly remains incentral_slash_command_workflow.gosincesort.Stringsis now called at the call site.
Not flagged (already covered)
The sliceutil.MergeUnique allocation pattern (allocates new slice + map on every call vs. in-place appendIfMissing) has been flagged in detail by the existing six inline comments. Adding more comments on that theme would be redundant.
🔎 Code quality review by PR Code Quality Reviewer
Comments that could not be inline-anchored
pkg/cli/logs_report_test.go:227
Stale test name/comment after removing addUniqueWorkflow: the test and its doc comment still reference the deleted helper, making coverage attribution confusing.
<details>
<summary>💡 Suggested fix</summary>
Rename the function and update the comment:
// TestMergeUniqueWorkflow tests that sliceutil.MergeUnique correctly deduplicates a workflow list.
func TestMergeUniqueWorkflow(t *testing.T) {Anyone searching for test coverage of the removed addUniqueWorkflow will land he…
pkg/cli/audit_report.go:691
Silent truncation length change: the old inline code kept maxMessageLen content chars then appended "..." (1503 chars total); stringutil.Truncate(message, maxMessageLen) caps the total at maxMessageLen (1500 chars, only 1497 of actual content).
<details>
<summary>💡 Impact and options</summary>
The difference is 3 chars — cosmetically negligible — but it is an undocumented behaviour change that could surprise anyone who relied on the previous output length. The analogous call in…
pkg/workflow/central_slash_command_workflow.go:471
Atomic uniqueSorted split into two non-obvious steps: the old helper guaranteed deduplication and sorting as a single unit; the replacement is two separate statements with an implicit dependency between them.
<details>
<summary>💡 Why this matters</summary>
sliceutil.Deduplicate is order-preserving, not sorted. If a future reader sees the two lines and incorrectly assumes Deduplicate already produces sorted output (or removes the sort.Strings call thinking it is redundant), the …
Analysis found multiple inline reimplementations of existing utility functions scattered across
pkg/cliandpkg/workflow. Consolidates to canonical helpers for consistency and maintainability.Changes
Percentage calculations (10 sites →
safePercent)health_metrics.go,audit_expanded.go,experiments_analyze_statistics.go,deps_report.gohealth_metrics.go:178String truncation (9 sites →
stringutil.Truncate)mcp_list.go,mcp_registry_list.go,mcp_inspect_mcp.go,audit_report_analysis.go,audit_report.goSlice deduplication (17 sites →
sliceutil.MergeUnique/Deduplicate)appendIfMissing,addUniqueWorkflow,uniqueSortedclaude_tools.go(11 sites),logs_report_errors.go(3 sites),logs_report_mcp.go(1 site),central_slash_command_workflow.go(2 sites)Templatable env-var builders (byte-identical implementations)
buildTemplatableEnvVarfrombuildTemplatableBoolEnvVar/buildTemplatableIntEnvVarAnomaly formatting (8 sites →
formatAnomalyTag/formatAnomalyNote)audit_math_helpers.goaudit_diff_render.goinline blocksMap key sorting (3 sites →
sortedMapKeys)safe_outputs_config_helpers.go,safe_scripts.go,safe_outputs_app_config.goExample
Before:
After: