[sergo] Sergo Report: defer-pattern-and-logging-inconsistency - 2026-04-22

[github-actions[bot]]

Date: 2026-04-22
Strategy: defer-pattern-and-logging-inconsistency (first run — no prior cache)
Success Score: 7/10
Run ID: §24801003579

---

Executive Summary

This is the inaugural Sergo run for this repository. With no strategy cache available, analysis focused entirely on new exploration across the largest and most complex files in pkg/workflow/. Three distinct code quality issues were identified: a bug-prone pattern of manual state restore in the compiler's engine setup function, a logging namespace inconsistency spread across many files, and a silent error suppression in MCP scripts timeout parsing. Two existing open Sergo issues (#27663, #27665) were found and avoided to prevent duplication. Three new GitHub issues were created.

---

Serena Tools Snapshot

• Total Tools Available: 18
• New Tools Since Last Run: N/A (first run)
• Removed Tools: N/A
• Tool Capabilities Used Today: activate_project, check_onboarding_performed, onboarding, search_for_pattern, get_symbols_overview, find_symbol, list_dir, write_memory

---

Strategy Selection

Cached Reuse Component (50%)

No prior cache available — this component was replaced with a second new exploration thread.

New Exploration Component 1: State Restore Anti-Pattern Detection

• Hypothesis: Large compiler functions with multiple error return paths often contain repeated manual field-restore code that should use defer
• Tools: search_for_pattern for c.strictMode = initial, get_symbols_overview on compiler.go
• Target: pkg/workflow/compiler_orchestrator_engine.go — the largest orchestration file at 360+ lines

New Exploration Component 2: Logging Namespace Audit

• Hypothesis: A package with 150+ files each defining their own logger will have some files accidentally using the shared package-level logger instead
• Tools: search_for_pattern for log.Printf + Log = logger.New cross-reference
• Target: All non-test .go files in pkg/workflow/

Combined Strategy Rationale

Both threads are structural/correctness-oriented rather than feature-focused. They complement each other: one targets a single critical function (depth), the other scans the entire package (breadth).

---

Codebase Context

• Total Go Files (non-test): 730
• Packages Analyzed: pkg/workflow, pkg/parser, pkg/cli
• LOC Analyzed: ~166,000 (all non-test Go files)
• Focus Areas: compiler_orchestrator_engine.go, mcp_scripts_parser.go, logging patterns across pkg/workflow/

---

Findings Summary

Severity	Count
High	1
Medium	1
Low	1
Total	3

---

Detailed Findings

Finding 1 (High): Repeated manual c.strictMode restore — should use defer

File: pkg/workflow/compiler_orchestrator_engine.go
Lines: 67–107 (Phase 1: 5 restores + 1 success restore) and 303–349 (Phase 2: 5 restores + 1 success restore)

The function setupEngineAndImports temporarily overrides c.strictMode to the effective strict mode, runs a battery of validations, and must restore the original value on every exit path. The current implementation does this 11 times manually:

initialStrictMode := c.strictMode
c.strictMode = c.effectiveStrictMode(result.Frontmatter)

if err := c.validateStrictMode(...); err != nil {
    c.strictMode = initialStrictMode  // manual #1
    return nil, err
}
if err := c.validateEnvSecrets(...); err != nil {
    c.strictMode = initialStrictMode  // manual #2
    return nil, err
}
// ... 3 more identical blocks ...
c.strictMode = initialStrictMode  // on success

The same pattern repeats for a second phase of firewall/network validations. Any future developer adding a new validation check in either phase must remember to add the restore — a classic "easy to forget" maintenance trap.

Recommendation: Extract each phase into a helper using defer func() { c.strictMode = saved }().

Finding 2 (Medium): pkg/workflow files use global log (tagged "workflow:compiler") instead of file-specific loggers

Files affected (no own logger): prompts.go, templatables.go, runtime_deduplication.go, mcp_playwright_config.go, project_config_parsing.go
Files affected (have own logger but also use global log): validation_helpers.go, config_helpers.go, create_discussion.go, update_entity_helpers.go

compiler.go defines:

var log = logger.New("workflow:compiler")

Files that don't define their own logger variable use this shared log via Go's package-level scoping. Their log messages appear under the workflow:compiler tag even though they have nothing to do with the compiler entry point. This makes GH_AW_LOG=workflow:compiler unexpectedly noisy and makes filtering for compiler-specific output unreliable.

Recommendation: Add var xLog = logger.New("workflow:x") to each affected file and update all log.Printf calls.

Finding 3 (Low): Silent timeout string parse failures in mcp_scripts_parser.go

File: pkg/workflow/mcp_scripts_parser.go
Lines: 184 and 355

case string:
    // Try to parse string as integer
    _, _ = fmt.Sscanf(t, "%d", &toolConfig.Timeout)

When a user provides a timeout like "30s" (common for duration strings), Sscanf fails silently and toolConfig.Timeout stays at zero. The user's MCP tool gets no timeout instead of the intended 30-second timeout. No warning or log message is emitted.

Recommendation: Add a log message via mcpScriptsLog.Printf when the parse fails.

Previously Open Sergo Issues (not duplicated)

• Replace fmt.Errorf("%s", ...) with errors.New(...) in pkg/workflow (9 sites) #27663: Replace fmt.Errorf("%s", ...) with errors.New(...) in pkg/workflow (9 sites) — already tracked
• Inconsistent logging guard in error constructors causes unconditional log allocations #27665: Inconsistent logging guard in error constructors — NewValidationError and NewConfigurationError call logger unconditionally — already tracked

---

Improvement Tasks Generated

Task 1: Introduce withEffectiveStrictMode helper to eliminate 9 manual restores

Severity: High | Effort: Small

Problem: setupEngineAndImports in compiler_orchestrator_engine.go saves/restores c.strictMode 11 times across 11 explicit assignments.

Before:

initialStrictMode := c.strictMode
c.strictMode = c.effectiveStrictMode(result.Frontmatter)
if err := c.validateStrictMode(...); err != nil {
    c.strictMode = initialStrictMode
    return nil, err
}

After:

func (c *Compiler) withEffectiveStrictMode(fm map[string]any, fn func() error) error {
    saved := c.strictMode
    c.strictMode = c.effectiveStrictMode(fm)
    defer func() { c.strictMode = saved }()
    return fn()
}

Validation:

• go test ./pkg/workflow/... passes
• strict_mode_validation_test.go and related tests pass

---

Task 2: Fix logging namespace for ~9 affected pkg/workflow files

Severity: Medium | Effort: Small

Problem: Log output from prompts.go, templatables.go, etc. is attributed to "workflow:compiler".

Fix: Add var xLog = logger.New("workflow:x") to each file; update all log.Printf calls.

Validation:

• go build ./pkg/workflow/ passes
• GH_AW_LOG=workflow:prompts shows prompts-related log output

---

Task 3: Log diagnostic for invalid MCP tool timeout strings

Severity: Low | Effort: Trivial

Problem: _, _ = fmt.Sscanf(t, "%d", &toolConfig.Timeout) at lines 184 and 355 silently discards parse errors.

Fix:

if n, err := fmt.Sscanf(t, "%d", &toolConfig.Timeout); err != nil || n == 0 {
    mcpScriptsLog.Printf("timeout value %q for tool %q is not a plain integer; using default", t, toolName)
}

Validation:

• go test ./pkg/workflow/... passes

---

Success Metrics

This Run

• Findings Generated: 3
• Tasks Created: 3
• Files Analyzed: 730 (full pkg/workflow)
• Issues Created: 3 (all new, no duplicates)
• Success Score: 7/10

Reasoning for Score

• Findings Quality (3/4): Two medium-high quality findings (defer pattern, logging), one low (silent parse). No critical/security issues found.
• Coverage (2/3): Full pkg/workflow scanned; cmd/ and internal/ not analyzed.
• Task Generation (2/3): Three concrete, well-scoped tasks created; could have been more actionable with code-ready patches.

---

Historical Context

Cumulative Statistics (after this run)

• Total Runs: 1
• Total Findings: 3
• Total Tasks Generated: 3
• Average Success Score: 7.0/10
• Most Successful Strategy: defer-pattern-and-logging-inconsistency

---

Recommendations

Immediate Actions

1. [High] Refactor setupEngineAndImports to use withEffectiveStrictMode helper — eliminates 9 error-prone restores
2. [Medium] Add file-specific loggers to prompts.go, templatables.go, runtime_deduplication.go, mcp_playwright_config.go, project_config_parsing.go and fix mixed logger usage in validation_helpers.go, config_helpers.go, create_discussion.go, update_entity_helpers.go
3. [Low] Add diagnostic log for unrecognized timeout string format in mcp_scripts_parser.go:184 and :355

Long-term Improvements

• Consider a custom linter rule that detects manual save/restore patterns on struct fields (candidates for defer)
• Add a check in CI that verifies all files in pkg/workflow/ define their own file-specific logger

---

Next Run Preview

Suggested Focus Areas

• pkg/workflow/compiler_jobs.go (983 lines) and compiler_yaml.go (917 lines) — large files likely to contain complex function logic
• pkg/parser/ — remote fetch (946 lines) for error handling patterns
• Interface implementation completeness checks across engine types (Claude, Gemini, Copilot, Codex, OpenCode, Crush)

Strategy Evolution

• Cache is now populated; next run should sample 50% from defer-pattern-and-logging-inconsistency strategy (adapted to new file targets) and 50% new exploration of type/interface patterns

---

References:

• §24801003579

Generated by Sergo - The Serena Go Expert
Strategy: defer-pattern-and-logging-inconsistency

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

• expires on Apr 23, 2026, 8:44 PM UTC

[github-actions[bot]]

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

A newer discussion is available at Discussion #28159.