[sergo] Sergo Report: errors-new-anti-pattern-and-interface-assertions - 2026-04-02

[github-actions[bot]]

Executive Summary

Today's Sergo analysis combined two complementary strategies: a deep extension of the compile-time interface assertion gap (first discovered in the mutex-safety run), and a novel scan for the fmt.Errorf("%s", str) anti-pattern. The compile-time assertion gap is now fully mapped — ConditionNode has 10 concrete implementors and CodingAgentEngine has 4, all without var _ Interface = (*Type)(nil) guards. The error construction anti-pattern was found in 6 locations across pkg/cli and pkg/workflow, where pre-formatted strings are wrapped with fmt.Errorf instead of the more idiomatic and efficient errors.New.

Three actionable improvement tasks are generated below.

---

🛠️ Serena Tools Update

Tools Snapshot

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

Tool Capabilities Used Today

Tool	Purpose
search_for_pattern	Regex search across Go source files
get_symbols_overview	High-level symbol inventory of agentic_engine.go
find_referencing_symbols	All callers of ActionSHAResolver interface
read_file	Verified context around specific findings

---

📊 Strategy Selection

Cached Reuse Component (50%)

Previous Strategy Adapted: mutex-safety-and-compile-time-checks (2026-03-31, score 8/10)

The prior run identified that no var _ Interface = (*Type)(nil) compile-time assertions exist anywhere in pkg/. Today's run extends that finding to precisely quantify the two largest gaps:

• ConditionNode (10 concrete types in expression_nodes.go) — not targeted in the prior run
• CodingAgentEngine (4 engine structs) — named in the prior run but not fully scoped

Modification: Pivoted from "what interfaces exist?" to "which have the most implementors and highest interface-change risk?" CodingAgentEngine is a 9-method composite interface; a missed method implementation compiles fine at definition time but panics or silently misbehaves at registration.

New Exploration Component (50%)

Novel Approach: fmt.Errorf("%s", str) anti-pattern scan

No prior Sergo run scanned for this specific pattern. When a string has already been fully formatted via fmt.Sprintf, wrapping it with fmt.Errorf("%s", prebuiltStr) is wasteful and unidiomatic. The correct idiom is errors.New(prebuiltStr). The pattern was found in 6 production locations across pkg/cli and pkg/workflow.

Hypothesis confirmed: The pattern clusters around validation code that builds rich error messages with fmt.Sprintf first, then converts to error — a common refactor artifact.

Combined Strategy Rationale

Both components target correctness guarantees at the margin: compile-time assertions prevent silent interface drift; errors.New hygiene ensures the error construction path is as efficient and reviewable as possible. Both are low-risk fixes with zero behavior change.

---

🔍 Analysis Execution

Codebase Context

• Total Go Files (non-test): 651
• Packages Analyzed: pkg/workflow, pkg/cli
• Focus Areas: expression_nodes.go, agentic_engine.go, all validation files

Findings Summary

Severity	Count
Medium	3
Low	0
Total	3

---

📋 Detailed Findings

Medium Priority Issues

Finding 1 — fmt.Errorf("%s", str) Should Be errors.New(str) (6 locations)

In Go, fmt.Errorf is intended for format verbs (%w, %v, %d, etc.) applied to runtime values. When the string has already been fully built — typically via a preceding fmt.Sprintf(...) call — the correct construction is errors.New(prebuiltString). Using fmt.Errorf("%s", prebuiltString) is flagged by the perfsprint linter, incurs unnecessary format string parsing overhead, and obscures intent.

All 6 occurrences follow the same pattern: errMsg := fmt.Sprintf(...) immediately followed by return fmt.Errorf("%s", errMsg).

File	Line	Variable
pkg/cli/run_workflow_execution.go	210	errMsg (from console.FormatErrorWithSuggestions)
pkg/workflow/tools_validation.go	358	errMsg.String() (strings.Builder)
pkg/workflow/engine_validation.go	79	errMsg (from fmt.Sprintf)
pkg/workflow/runtime_validation.go	91	errorMsg (from fmt.Sprintf)
pkg/workflow/permissions_validation.go	346	errorMsg.String() (strings.Builder)
pkg/workflow/engine_definition.go	280	errMsg (from fmt.Sprintf)

Finding 2 — No Compile-Time Assertions for ConditionNode (10 implementors)

ConditionNode is a single-method interface (Render() string) defined in expression_nodes.go. It has 10 concrete implementations in the same file: ExpressionNode, AndNode, OrNode, NotNode, DisjunctionNode, FunctionCallNode, PropertyAccessNode, StringLiteralNode, BooleanLiteralNode, ComparisonNode. None have a var _ ConditionNode = (*T)(nil) guard.

If a second method is ever added to ConditionNode (e.g., for serialisation or type-tagging), all 10 implementors silently stop satisfying the interface. The break surfaces only when a value is assigned to a ConditionNode variable at runtime — which, for leaf nodes, may only happen deep inside expression parsing logic.

Finding 3 — No Compile-Time Assertions for CodingAgentEngine (4 engine implementors)

CodingAgentEngine is the master composite interface in pkg/workflow/agentic_engine.go (line 230). It embeds all 9 sub-interfaces (Engine, CapabilityProvider, WorkflowExecutor, MCPConfigProvider, LogParser, SecurityProvider, ModelEnvVarProvider, AgentFileProvider, ConfigRenderer). Four concrete structs implement it by embedding BaseEngine:

• ClaudeEngine (claude_engine.go)
• CopilotEngine (copilot_engine.go)
• GeminiEngine (gemini_engine.go)
• CodexEngine (codex_engine.go)

BaseEngine provides default implementations for all 9 sub-interfaces. If a new sub-interface is added to CodingAgentEngine, or if BaseEngine drops a method, the compiler will not catch the gap at the struct definition site — only when the struct is passed to a function accepting CodingAgentEngine. Given the composite nature of this interface (9 sub-interfaces, ~30 methods), compile-time assertions are especially valuable here.

---

✅ Improvement Tasks Generated

Task 1: Replace fmt.Errorf("%s", str) with errors.New(str) in 6 Locations

Issue Type: Error Construction Anti-Pattern

Problem:
Six locations use fmt.Errorf("%s", prebuiltString) to construct an error from a fully-formed string. This is less idiomatic and slightly wasteful compared to errors.New(str).

Locations:

• pkg/cli/run_workflow_execution.go:210
• pkg/workflow/tools_validation.go:358
• pkg/workflow/engine_validation.go:79
• pkg/workflow/runtime_validation.go:91
• pkg/workflow/permissions_validation.go:346
• pkg/workflow/engine_definition.go:280

Impact:

• Severity: Medium
• Affected Files: 6
• Risk: Minor — no behavior change, but perfsprint linter will flag these; they also obscure intent for readers

Recommendation:
For each location, replace fmt.Errorf("%s", str) or fmt.Errorf("%s", builder.String()) with errors.New(str) or errors.New(builder.String()). Ensure "errors" is imported (it may already be present; if not, add it and remove the now-unused fmt import if applicable).

Before (example from engine_validation.go:79):

errMsg := fmt.Sprintf("inline engine definition references unknown runtime.id: %s ...", config.ID, ...)
return fmt.Errorf("%s", errMsg)

After:

errMsg := fmt.Sprintf("inline engine definition references unknown runtime.id: %s ...", config.ID, ...)
return errors.New(errMsg)

Validation:

• All 6 files still compile with go build ./...
• Existing tests pass with go test ./pkg/...
• golangci-lint run no longer flags perfsprint on these lines
• fmt import removed in files where it is no longer used after the change

Estimated Effort: Small

---

Task 2: Add Compile-Time ConditionNode Interface Assertions for All 10 Implementors

Issue Type: Compile-Time Interface Safety

Problem:
ConditionNode in pkg/workflow/expression_nodes.go has 10 concrete implementations, none with var _ ConditionNode = (*T)(nil) guards. Interface drift (e.g., adding a second method to ConditionNode) would only surface at assignment sites, not at implementation sites.

Locations:
All in pkg/workflow/expression_nodes.go:

• *ExpressionNode
• *AndNode
• *OrNode
• *NotNode
• *DisjunctionNode
• *FunctionCallNode
• *PropertyAccessNode
• *StringLiteralNode
• *BooleanLiteralNode
• *ComparisonNode

Impact:

• Severity: Medium
• Affected Files: 1
• Risk: Without assertions, interface changes break silently at assignment sites rather than loudly at implementation sites

Recommendation:
Add a block of compile-time assertions at the top of expression_nodes.go (after imports, before the first type declaration):

// Compile-time interface assertions
var (
    _ ConditionNode = (*ExpressionNode)(nil)
    _ ConditionNode = (*AndNode)(nil)
    _ ConditionNode = (*OrNode)(nil)
    _ ConditionNode = (*NotNode)(nil)
    _ ConditionNode = (*DisjunctionNode)(nil)
    _ ConditionNode = (*FunctionCallNode)(nil)
    _ ConditionNode = (*PropertyAccessNode)(nil)
    _ ConditionNode = (*StringLiteralNode)(nil)
    _ ConditionNode = (*BooleanLiteralNode)(nil)
    _ ConditionNode = (*ComparisonNode)(nil)
)

Validation:

• go build ./pkg/workflow/... succeeds
• go vet ./pkg/workflow/... clean
• If ConditionNode gains a new method, each assertion line immediately shows the missing implementor

Estimated Effort: Small

---

Task 3: Add Compile-Time CodingAgentEngine Assertions for 4 Engine Types

Issue Type: Compile-Time Interface Safety

Problem:
The CodingAgentEngine composite interface (9 sub-interfaces, ~30 methods) in pkg/workflow/agentic_engine.go is implemented by 4 concrete engine structs. None have compile-time assertions. Given the interface's composite nature, a missed method in any new sub-interface addition would be silently accepted until the engine is actually used.

Locations:

• pkg/workflow/claude_engine.go — *ClaudeEngine
• pkg/workflow/copilot_engine.go — *CopilotEngine
• pkg/workflow/gemini_engine.go — *GeminiEngine
• pkg/workflow/codex_engine.go — *CodexEngine

Impact:

• Severity: Medium
• Affected Files: 4 (or 1 central file)
• Risk: New sub-interface additions to CodingAgentEngine pass the compiler silently; EngineRegistry.Register will catch it, but only at program startup, not at compile time

Recommendation:
Add a compile-time assertion block in pkg/workflow/agentic_engine.go alongside the interface definition, or in each engine's own file. The centralized approach is preferred for discoverability:

// Compile-time assertions: all engine structs must implement CodingAgentEngine.
// Add new engine types here when registering them.
var (
    _ CodingAgentEngine = (*ClaudeEngine)(nil)
    _ CodingAgentEngine = (*CopilotEngine)(nil)
    _ CodingAgentEngine = (*GeminiEngine)(nil)
    _ CodingAgentEngine = (*CodexEngine)(nil)
)

Place these assertions in a dedicated file (e.g., pkg/workflow/engine_assertions.go) to keep them visible and adjacent to the registration pattern.

Validation:

• go build ./pkg/workflow/... succeeds
• go test ./pkg/workflow/... still passes
• Removing a method from BaseEngine triggers a compile error at the assertion block

Estimated Effort: Small

---

📈 Success Metrics

This Run

Metric	Value
Findings Generated	3
Tasks Created	3
Files Analyzed (non-test .go)	651
Success Score	8/10

Reasoning for Score

• Findings Quality (3/4): All 3 findings are concrete, location-specific, and actionable. However, severity is Medium rather than High/Critical.
• Coverage (2/3): Analysis was focused on two specific patterns rather than broad package sweeps.
• Task Generation (3/3): All 3 tasks are well-scoped, include before/after code examples, and have clear validation checklists.

---

📊 Historical Context

Strategy Performance History

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

Cumulative Statistics

Metric	Value
Total Runs	6
Total Findings	25
Total Tasks Generated	18
Average Success Score	8.0/10
Most Successful Strategy	regex-duplication-and-goroutine-context (9/10)

---

🎯 Recommendations

Immediate Actions

1. Task 1 (Small effort): Replace fmt.Errorf("%s", str) → errors.New(str) in 6 validation files
2. Task 2 (Small effort): Add var _ ConditionNode = (*T)(nil) block for 10 implementors in expression_nodes.go
3. Task 3 (Small effort): Add var _ CodingAgentEngine = (*T)(nil) block for 4 engine types

All three tasks are zero-behavior-change, compiler-verified fixes that improve long-term maintainability.

Long-term Improvements

• Consider a linter rule (e.g., perfsprint in golangci-lint) to catch fmt.Errorf("%s", ...) automatically on future PRs
• Establish a convention to add compile-time assertions whenever a new interface-implementation pair is created
• The ValidatableTool → *GitHubToolConfig and LogAnalysis → *FirewallAnalysis/*DomainAnalysis pairs would also benefit from assertions (lower priority than the 10+4 above)

---

🔄 Next Run Preview

Suggested Focus Areas

• Cached (50%): Revisit the fmt.Errorf landscape with a %w vs %v correctness scan — the prior run found compiler_orchestrator_frontmatter.go:41 using %v intentionally, but there may be unintentional cases in newer files
• New (50%): Scan for context.Background() / context.TODO() usage inside non-main packages, which could indicate missing context propagation (this was partially identified in run 3 but not fully resolved)

Strategy Evolution

The compile-time assertion theme has now been analyzed across three runs (mutex-safety → today). Future runs should consider closing out the remaining smaller interfaces (ValidatableTool, LogAnalysis, ActionSHAResolver) in a single focused task, then move on to new domains.

---

References:

• §23920136222

AI generated by Sergo - Serena Go Expert · history

• expires on Apr 3, 2026, 8:26 PM UTC

[github-actions[bot]]

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

A newer discussion is available at Discussion #24376.