[sergo] Sergo Report: errorf-anti-pattern-revisit-and-map-set-standardization - 2026-04-03

[github-actions[bot]]

Date: 2026-04-03
Strategy: errorf-anti-pattern-revisit-and-map-set-standardization
Run ID: §23960763553
Success Score: 7/10

Executive Summary

Today's Sergo run combined a revisit of the previously-identified fmt.Errorf("%s", str) anti-pattern (cached, 50%) with a new exploration of set-map standardization issues across the codebase (new, 50%). The fmt.Errorf anti-pattern has grown from 6 to 10 locations since the last audit (2026-04-02), indicating new code is adopting the pattern without correction. On the new side, a pervasive map[T]bool-as-set pattern (202 instances) versus only 7 idiomatic map[T]struct{} instances reveals a deep inconsistency in Go idiom usage — including in exported API types. A bonus finding: the sliceutil.Contains wrapper function has become redundant now that the codebase overwhelmingly uses slices.Contains directly.

🛠️ Serena Tools Update

Tools Snapshot

• Total Tools Available: 21
• New Tools Since Last Run: None
• Removed Tools: None
• Modified Tools: None

Note: Serena MCP experienced intermittent connection resets during this session (EOF errors on search_for_pattern and activate_project calls). Analysis fell back to bash/grep for pattern searches.

Tool Capabilities Used Today

• search_for_pattern — regex search across Go files (partial; connection issues)
• bash/grep/python — pattern analysis, brace-depth tracking, cross-file correlation

📊 Strategy Selection

Cached Reuse Component (50%)

Previous Strategy Adapted: errors-new-anti-pattern-and-interface-assertions (2026-04-02, score: 8/10)

• Why Reused: The fmt.Errorf("%s", str) anti-pattern was well-defined, actionable, and — crucially — unfixed. Revisiting it revealed the issue grew.
• Modifications: Extended search scope to catch new occurrences introduced since last audit. Added run_workflow_validation.go (new file) and template_injection_validation.go to coverage.
• Outcome: Found 10 occurrences (4 new since April 2 run): run_workflow_validation.go:269, template_injection_validation.go:305, dangerous_permissions_validation.go:96, workflow_errors.go:261.

New Exploration Component (50%)

Novel Approach: Set-map idiom audit + stdlib redundancy analysis

• Tools Employed: bash python analysis (brace-depth map traversal), grep pattern analysis
• Hypothesis: The codebase would show inconsistent use of map[T]bool vs map[T]struct{} — confirmed with 202:7 ratio.
• Target Areas: pkg/cli/, pkg/workflow/, pkg/parser/, pkg/agentdrain/
• Bonus finding: sliceutil.Contains — 7 remaining uses vs 67 direct slices.Contains calls

Combined Strategy Rationale

The two components complement each other by targeting consistency gaps: one at the error-creation level (wrong constructor), one at the data-structure level (wrong type for sets). Both are low-risk, high-readability improvements that share a common root cause: patterns established before idiomatic alternatives were clear, now propagating as copy-paste without correction.

🔍 Analysis Execution

Codebase Context

• Total Go Files (non-test): 664
• Packages Analyzed: 25 (full codebase scan)
• LOC Analyzed: ~153,609 total
• Focus Areas: pkg/workflow/, pkg/cli/, pkg/parser/, pkg/agentdrain/

Findings Summary

• Total Issues Found: 3
• Critical: 0
• High: 1
• Medium: 1
• Low: 1

📋 Detailed Findings

High Priority: fmt.Errorf("%s", str) anti-pattern — grew from 6 → 10 locations

The fmt.Errorf("%s", str) pattern is an anti-pattern for constructing errors from pre-built strings. It should be errors.New(str) because:

1. fmt.Errorf parses the format string at runtime — unnecessary work when there's no interpolation
2. errors.New is semantically clear: "create a new error from this message"
3. Go linters (errorlint, staticcheck) flag this as fmt.Errorf with no %w and no substitution

All 10 locations (4 new since last audit):

File	Line	Status
pkg/cli/run_workflow_execution.go	210	Known (prev. run)
pkg/workflow/tools_validation.go	358	Known (prev. run)
pkg/workflow/engine_validation.go	79	Known (prev. run)
pkg/workflow/runtime_validation.go	91	Known (prev. run)
pkg/workflow/permissions_validation.go	346	Known (prev. run)
pkg/workflow/engine_definition.go	280	Known (prev. run)
pkg/cli/run_workflow_validation.go	269	NEW
pkg/workflow/template_injection_validation.go	305	NEW
pkg/workflow/dangerous_permissions_validation.go	96	NEW
pkg/workflow/workflow_errors.go	261	NEW

Medium Priority: map[T]bool used as sets — 202 instances vs 7 idiomatic map[T]struct{}

The codebase systematically uses map[T]bool where only = true is ever assigned — the semantic of a set, not a boolean flag map. The idiomatic Go set type is map[T]struct{} because:

1. struct{} is zero-size — no memory allocation per value (saves 1 byte per entry)
2. Signals intent: "this is a presence check, not a flag"
3. Prevents accidental writes of = false (which would be meaningless in a set)

Key locations by priority:

Exported API types (highest impact):

• pkg/cli/engine_secrets.go:47 — ExistingSecrets map[string]bool in EngineSecretConfig struct
• pkg/cli/tool_graph.go:32 — Tools map[string]bool in ToolGraph struct

Internal types:

• pkg/workflow/compiler_types.go:78,423 — actionPinWarnings map[string]bool
• pkg/agentdrain/mask.go:53 — excluded local variable

Local variables (pure sets):

• pkg/cli/tokens_bootstrap.go:86, pkg/cli/trial_runner.go:146, pkg/cli/add_interactive_orchestrator.go:42
• pkg/parser/include_expander.go:19
• pkg/cli/compile_workflow_processor.go:162, pkg/cli/gateway_logs.go:921
• pkg/cli/compile_batch_operations.go:134, pkg/cli/generate_action_metadata_command.go:203,235,265

All 202 map[T]bool locations (partial view)

There are 202 map[T]bool instances in production Go files. The grep shows them distributed across pkg/cli/ (majority), pkg/workflow/, and pkg/agentdrain/. Migrating all at once is impractical; the task focuses on the exported types first.

Low Priority: sliceutil.Contains — redundant wrapper over slices.Contains

// pkg/sliceutil/sliceutil.go
func Contains(slice []string, item string) bool {
    return slices.Contains(slice, item)  // literally just this
}

Usage: 7 calls to sliceutil.Contains vs 67 direct calls to slices.Contains. The wrapper adds zero value and creates decision overhead: "should I use sliceutil or slices?" The answer has been slices in 90% of new code since Go 1.21+. The wrapper predates slices being in stdlib.

Locations using the wrapper:

• pkg/cli/mcp_permissions.go:92
• pkg/cli/logs_github_api.go:286
• pkg/workflow/compiler_activation_job.go:111,114,438,554,557

✅ Improvement Tasks Generated

---

Task 1: Replace fmt.Errorf("%s", str) with errors.New(str) across 10 locations

Issue Type: Error Construction Anti-Pattern

Problem: Ten locations in pkg/cli/ and pkg/workflow/ use fmt.Errorf("%s", str) to wrap a pre-built string into an error. This is semantically equivalent to errors.New(str) but adds unnecessary format-string parsing overhead and is flagged by Go linters. The issue grew from 6 → 10 locations between 2026-04-02 and today, suggesting new code is copying the pattern without correction.

Location(s):

• pkg/cli/run_workflow_execution.go:210
• pkg/cli/run_workflow_validation.go:269 (new)
• pkg/workflow/tools_validation.go:358
• pkg/workflow/template_injection_validation.go:305 (new)
• pkg/workflow/engine_validation.go:79
• pkg/workflow/runtime_validation.go:91
• pkg/workflow/dangerous_permissions_validation.go:96 (new)
• pkg/workflow/permissions_validation.go:346
• pkg/workflow/workflow_errors.go:261 (new)
• pkg/workflow/engine_definition.go:280

Impact:

• Severity: High (linter violation; propagating anti-pattern)
• Affected Files: 10 files in pkg/cli/ and pkg/workflow/
• Risk: None — this is a pure semantic equivalence replacement

Before:

return fmt.Errorf("%s", errMsg)
return fmt.Errorf("%s", errMsg.String())
return fmt.Errorf("%s", strings.Join(lines, "\n"))

After:

return errors.New(errMsg)
return errors.New(errMsg.String())
return errors.New(strings.Join(lines, "\n"))

Validation:

• Run existing tests — all should pass unchanged (behavior is identical)
• Verify import "errors" is present in each file (add if missing; remove "fmt" if it becomes unused)
• Run go vet ./... to confirm no linter regressions

Estimated Effort: Small (10 one-line changes, possibly 1-2 import updates per file)

---

Task 2: Migrate exported map[T]bool sets to map[T]struct{}

Issue Type: Type Idiom Inconsistency

Problem: Two exported struct fields use map[string]bool as sets (keys only ever set to true, value never used as a real boolean). Using map[T]struct{} is the Go idiom for sets: the zero-size value makes intent explicit and prevents the = false footgun. With 202 map[T]bool vs 7 map[T]struct{} in the codebase, the exported types are the highest-priority fix because they form the public API and influence how consumers write code.

Location(s):

• pkg/cli/engine_secrets.go:47 — ExistingSecrets map[string]bool in EngineSecretConfig
• pkg/cli/tool_graph.go:32 — Tools map[string]bool in ToolGraph
• pkg/workflow/compiler_types.go:78 — actionPinWarnings map[string]bool
• pkg/workflow/compiler_types.go:423 — ActionPinWarnings map[string]bool

Impact:

• Severity: Medium (API clarity, minor memory optimization, correctness signal)
• Affected Files: 4 primary + callers that write to these maps
• Risk: Low — callers need map[k] = struct{}{} instead of map[k] = true, and presence checks _, ok := map[k] instead of if map[k]

Before:

ExistingSecrets map[string]bool
// ...
existingSecrets[name] = true
// ...
if existingSecrets[name] { ... }

After:

ExistingSecrets map[string]struct{}
// ...
existingSecrets[name] = struct{}{}
// ...
if _, ok := existingSecrets[name]; ok { ... }

Validation:

• Search for all write sites: ExistingSecrets[...] = true → ExistingSecrets[...] = struct{}{}
• Search for all read sites: if ExistingSecrets[k] → if _, ok := ExistingSecrets[k]; ok
• Run existing tests (no behavior change expected)
• Consider using a type alias type StringSet = map[string]struct{} if the pattern repeats enough

Estimated Effort: Medium (4 type changes + 10-20 call-site updates)

---

Task 3: Deprecate and inline sliceutil.Contains wrapper

Issue Type: Redundant Abstraction

Problem: sliceutil.Contains(slice, item) is a one-line wrapper over slices.Contains(slice, item) from the Go standard library. It predates Go 1.21's slices package. Today, 90% of Contains calls (67/74) use slices.Contains directly; only 7 use the wrapper. The wrapper creates unnecessary API surface and forces callers to decide which to import. Removing it standardizes the codebase on the stdlib function.

Location(s):

• pkg/sliceutil/sliceutil.go — Remove the Contains function
• pkg/cli/mcp_permissions.go:92 — Update import
• pkg/cli/logs_github_api.go:286 — Update import
• pkg/workflow/compiler_activation_job.go:111,114,438,554,557 — Update import (5 call sites in one file)

Impact:

• Severity: Low (DX improvement, API simplification)
• Affected Files: 3 files (1 source + 2 callers with 7 call sites)
• Risk: Very low — purely mechanical replacement; behavior is identical

Before:

import "github.com/github/gh-aw/pkg/sliceutil"
// ...
if sliceutil.Contains(agenticWorkflowNames, workflowName) { ... }

After:

import "slices"
// ...
if slices.Contains(agenticWorkflowNames, workflowName) { ... }

Validation:

• Replace all 7 call sites with slices.Contains
• Remove Contains function from pkg/sliceutil/sliceutil.go
• Remove "slices" import from sliceutil.go if no other functions use it
• Run go build ./... and existing tests

Estimated Effort: Small (7 one-line replacements + 1 function deletion)

---

📈 Success Metrics

This Run

• Findings Generated: 3
• Tasks Created: 3
• Files Analyzed: 664 Go files
• Success Score: 7/10

Reasoning for Score

• Findings Quality (3/4): High-quality actionable findings. The anti-pattern recurrence tracking (6→10 occurrences) adds genuine value. Docked 1 point because no critical/security issues were found.
• Coverage (2/3): Full codebase scan via bash/grep. Docked 1 point because Serena MCP was intermittently unavailable, limiting LSP-based semantic analysis.
• Task Generation (2/3): 3 well-specified tasks with before/after examples. Docked 1 point because Task 3 is low-impact.

📊 Historical Context

Strategy Performance

Date	Strategy	Score	Findings	Tasks
2026-03-29	error-patterns-first-run	7/10	3	3
2026-03-30	multi-return-struct-refactoring	8/10	5	3
2026-03-30	close-error-propagation-and-context-depth	8/10	5	3
2026-03-31	mutex-safety-and-compile-time-checks	8/10	4	3
2026-04-01	regex-duplication-and-goroutine-context	9/10	5	3
2026-04-02	errors-new-anti-pattern-and-interface-assertions	8/10	3	3
2026-04-03	errorf-anti-pattern-revisit-and-map-set-standardization	7/10	3	3

Cumulative Statistics

• Total Runs: 7
• Total Findings: 28
• Total Tasks Generated: 21
• Average Success Score: 7.9/10
• Most Successful Strategy: regex-duplication-and-goroutine-context (9/10)

🎯 Recommendations

Immediate Actions

1. [High] Fix fmt.Errorf("%s", str) → errors.New(str) — 10 locations, pure find-replace, zero risk. The fact this grew from 6→10 suggests a comment in a validation file might be triggering the copy.
2. [Medium] Migrate exported set maps to map[T]struct{} — Start with EngineSecretConfig.ExistingSecrets and ToolGraph.Tools as they affect the public API.
3. [Low] Remove sliceutil.Contains wrapper — 7 easy replacements to eliminate an unnecessary wrapper.

Long-term Improvements

• Recurring finding: fmt.Errorf anti-patterns — Three of seven Sergo runs have surfaced error-construction issues. Consider adding a custom linter rule (e.g., golangci-lint with perfsprint or a custom go/analysis pass) to block fmt.Errorf("%s", ...) at PR time.
• map[T]bool prevalence — With 202 instances, a codebase-wide migration would be substantial. Consider a go vet custom check or staticcheck rule to flag new map[T]bool declarations in PR reviews.

🔄 Next Run Preview

Suggested Focus Areas

• Unacted findings: Compile-time interface assertions (var _ Interface = (*Struct)(nil)) were identified in the 2026-03-31 and 2026-04-02 runs but not yet implemented. Revisiting this could verify whether they were added.
• New territory: HTTP client configuration audit — check for http.Client instances without explicit Timeout fields, Transport settings, or context propagation in the pkg/parser/remote_fetch.go (898 LOC, largest parser file).

Strategy Evolution

Today's run confirmed that revisiting unfixed cached findings has value (finding grew from 6→10). Future runs should include a "regression check" component: verify whether previously-reported high-priority findings were addressed between runs, and re-report with updated scope if not.

---

References:

• §23960763553

Generated by Sergo - Serena Go Expert · ● 475.5K · ◷

• expires on Apr 4, 2026, 8:30 PM UTC

[github-actions[bot]]

This discussion has been marked as outdated by Sergo - Serena Go Expert.

A newer discussion is available at Discussion #24572.