[sergo] Sergo Report: filepath-walk-consolidation-and-error-suppression - 2026-04-11

[github-actions[bot]]

Executive Summary

Today's Sergo run combined two complementary lenses: extending the error-suppression sweep from the previous run, and a new exploration into utility-function adoption. The most impactful discovery is that the cli package already contains a findFileInDir helper (defined in copilot_events_jsonl.go:131) that perfectly encapsulates the "walk a directory tree to find the first file matching a name" pattern — yet 9 other sites in the same package reimplement the identical pattern inline instead of calling it. A secondary finding confirms that silent os.Setenv discards in engine_secrets.go remain unaddressed from the previous run, and a new medium-severity issue was found in mcp_playwright_config.go where both return values of ExtractExpressions are discarded, silently swallowing extraction errors.

Overall code quality is good; these are refinement-level issues rather than architectural problems. The codebase continues to show careful error handling in most places, making these outlier suppressions more notable.

---

Serena Tools Update

Tools Snapshot

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

Tool Capabilities Used Today

Tool	Usage
search_for_pattern	Searched for _ = filepath.Walk, _ = \w+\.\w+\(, go func(), context.WithTimeout, sync.WaitGroup patterns
find_symbol	Verified ExtractExpressions signature in expression_extraction.go
find_referencing_symbols	Traced callers of findFileInDir
get_symbols_overview	Reviewed file structure of mcp_playwright_config.go
list_dir / read_file	Deep-read docker_images.go, update_check.go, pr_command.go, git.go, safe_outputs_actions.go

---

Strategy Selection

Cached Reuse Component (50%)

Previous Strategy Adapted: context-sleep-extension-and-error-suppression (2026-04-10, score: 8/10)

• Original Success Score: 8/10
• Last Used: 2026-04-10
• Why Reused: The error-suppression scan from the previous run narrowed to _ = os.Setenv() in engine_secrets.go. That finding is still open. Today the same _ = func() lens was widened to all non-test production Go files, revealing a larger cluster of suppressed errors in filepath.Walk and exec.Command callsites.
• Modifications: Broadened from os.Setenv only → all _ = method() and _, _ = method() patterns in pkg/, filtered to the most impactful (non-obvious, non-cleanup) suppressions.

New Exploration Component (50%)

Novel Approach: Utility function adoption analysis — does the codebase define helpers that are then ignored in favor of inline duplication?

• Tools Employed: search_for_pattern, find_referencing_symbols, read_file
• Hypothesis: Large codebases often grow utility functions organically in one file that are never adopted elsewhere. Finding such orphaned utilities is high-value because the fix (use the existing function) is trivially safe.
• Target Areas: pkg/cli — the largest package (30+ files, heavy I/O operations)

Combined Strategy Rationale

The cached component found where errors are suppressed. The new component found why the same pattern keeps appearing: the utility that should replace these inline implementations already exists but isn't widely known/used within the package. Together they tell a complete story: the findFileInDir helper was written to prevent this kind of duplication, but the duplication happened anyway — likely because the function is buried inside copilot_events_jsonl.go with no indication it's a general-purpose utility.

---

Analysis Execution

Codebase Context

• Total Go Files (non-test): 675
• Packages Analyzed: pkg/cli, pkg/workflow
• Focus Areas: Error suppression patterns (_ = func()), goroutine management, utility function adoption

Goroutine Safety Check (New: Ruled Out)

All go func() spawns in pkg/cli non-test code were inspected:

• spinner.go:140 — WaitGroup-coordinated ✅
• update_check.go:242 — fire-and-forget with context check + panic recovery ✅
• docker_images.go:132 — context-aware with select ctx.Done() + exponential backoff ✅
• mcp_inspect_inspector.go:139 — WaitGroup-coordinated with timeout ✅

No goroutine leaks found. Goroutine management is solid.

Context Cancellation in Loops (New: Ruled Out)

safe_outputs_actions.go:297–300 uses manual cancel() inside a for range loop rather than defer cancel(). This is actually correct — defer would delay cancellation until the function returns, not the loop iteration. The inline cancel() before continue is the right pattern here.

Findings Summary

Severity	Count	Description
High	2	findFileInDir duplication (9 sites), os.Setenv silent failure (3 sites)
Medium	1	ExtractExpressions double-discard in mcp_playwright_config.go

---

Detailed Findings

Finding 1 — findFileInDir Utility Ignored by Same-Package Code (High)

Location: pkg/cli/copilot_events_jsonl.go:131 (definition), plus 9 call sites that should use it

A generic file-search utility already exists:

// findFileInDir searches for a file by name within dir (recursively).
// Returns the first matching path, or empty string if not found.
func findFileInDir(dir, name string) string {
    var found string
    _ = filepath.Walk(dir, func(path string, info os.FileInfo, err error) error {
        if err != nil || info == nil {
            return nil
        }
        if !info.IsDir() && info.Name() == name && found == "" {
            found = path
            return errors.New("stop") // sentinel to stop walking
        }
        return nil
    })
    return found
}

The following 9 sites in the same cli package duplicate this logic inline instead of calling findFileInDir:

File	Line	Searching For
copilot_events_jsonl.go	108–119	events.jsonl (duplicate in defining file!)
copilot_agent.go	109–119	agent-specific log patterns
redacted_domains.go	122–130	redacted-urls.log
token_usage.go	195–205	token-usage.jsonl
logs_parsing_core.go	113–120	AgentOutputArtifactName
logs_parsing_core.go	148–158	first file in agentOutputDir
logs_parsing_core.go	178–200	events.jsonl + *.log
logs_parsing_core.go	207–220	events.jsonl + session/process logs
logs_parsing_core.go	240–250	agent-stdio.log
logs_download.go	858–870	file count in outputDir
logs_github_rate_limit_usage.go	86–95	rate-limit usage file

Impact: 10 independent reimplementations of the same pattern. Changes to the Walk logic (e.g., handling symlinks, adding logging) must be replicated everywhere. The defining file (copilot_events_jsonl.go) even has a duplicate next to its own function definition.

Note: logs_parsing_core.go has more complex variants (multi-file searches) that can't be naively replaced with findFileInDir, but lines 113–120 and 240–250 are direct substitutions.

---

Finding 2 — Silent os.Setenv Failure in engine_secrets.go (High, Carried from 2026-04-10)

Location: pkg/cli/engine_secrets.go:329, 377, 430

Three prompt functions all silently discard os.Setenv errors:

// Line 329 (promptForCopilotTokenUnified)
_ = os.Setenv(req.Name, token)
fmt.Fprintln(os.Stderr, console.FormatSuccessMessage("Valid fine-grained Copilot token received"))

// Line 377 (promptForSystemTokenUnified)
_ = os.Setenv(req.Name, token)
fmt.Fprintln(os.Stderr, console.FormatSuccessMessage(req.Name+" token received"))

// Line 430 (promptForGenericAPIKeyUnified)
_ = os.Setenv(req.Name, apiKey)
fmt.Fprintln(os.Stderr, console.FormatSuccessMessage(label+" API key received"))

os.Setenv only fails when the variable name contains = or a null byte — but req.Name is user-controlled (from workflow frontmatter). If a malformed engine secret name reaches these functions, the env var is never set, the workflow engine never gets the secret, but the user sees a success message. The failure is completely invisible.

---

Finding 3 — ExtractExpressions Double-Discard in mcp_playwright_config.go (Medium)

Location: pkg/workflow/mcp_playwright_config.go:53

func replaceExpressionsInPlaywrightArgs(args []string, expressions map[string]string) []string {
    combined := strings.Join(args, "\n")
    extractor := NewExpressionExtractor()
    _, _ = extractor.ExtractExpressions(combined)   // ← both returns discarded
    replaced := extractor.ReplaceExpressionsWithEnvVars(combined)
    return strings.Split(replaced, "\n")
}

ExtractExpressions signature (from expression_extraction.go:50):

func (e *ExpressionExtractor) ExtractExpressions(markdown string) ([]*ExpressionMapping, error)

ExtractExpressions is called for its side effect of populating the extractor's internal expression map. The []*ExpressionMapping result and error are both thrown away. If ExtractExpressions returns an error (e.g., on a malformed $\{\{ }} expression), ReplaceExpressionsWithEnvVars will execute against an incomplete map, producing incorrect/unreplaced expressions in Playwright args — silently.

---

Improvement Tasks Generated

Task 1: Promote findFileInDir to a Package-Level Utility and Eliminate Duplicates

Issue Type: Code Duplication / Utility Adoption

Problem: findFileInDir is buried in copilot_events_jsonl.go with no indication it's a general-purpose helper. Nine other sites in the same cli package reimplement the same _ = filepath.Walk() file-search pattern.

Location(s):

• Definition: pkg/cli/copilot_events_jsonl.go:129–142
• Duplicates: copilot_events_jsonl.go:108, copilot_agent.go:109, redacted_domains.go:122, token_usage.go:195, logs_parsing_core.go:113,240, logs_download.go:858, logs_github_rate_limit_usage.go:86

Impact:

• Severity: High
• Affected Files: 6 files, 10 sites
• Risk: Logic drift — any fix or improvement to the Walk logic must be applied in 10 places

Recommendation: Move findFileInDir to a dedicated utility file (e.g., pkg/cli/file_search.go) with a clear doc comment indicating it's a general helper. Then replace the 5+ direct-substitution duplicates:

Before (repeated pattern):

var foundPath string
_ = filepath.Walk(logDir, func(path string, info os.FileInfo, err error) error {
    if err != nil || info == nil {
        return nil
    }
    if !info.IsDir() && info.Name() == "agent-stdio.log" {
        foundPath = path
        return errors.New("stop")
    }
    return nil
})

After:

foundPath := findFileInDir(logDir, "agent-stdio.log")

Validation:

• Run existing tests — all log-parsing tests should pass unchanged
• Verify findFileInDir handles the errors.New("stop") sentinel correctly (it already does)
• Check that multi-file search variants in logs_parsing_core.go are handled separately if they can't be simplified

Estimated Effort: Small (mechanical substitution)

---

Task 2: Propagate os.Setenv Errors in engine_secrets.go

Issue Type: Silent Error Suppression / Incorrect User Feedback

Problem: Three token-prompting functions discard os.Setenv errors and print a success message regardless. If req.Name contains an invalid character (= or null byte), the secret is never set but the user is told it was received successfully.

Location(s):

• pkg/cli/engine_secrets.go:329 — promptForCopilotTokenUnified
• pkg/cli/engine_secrets.go:377 — promptForSystemTokenUnified
• pkg/cli/engine_secrets.go:430 — promptForGenericAPIKeyUnified

Impact:

• Severity: High
• Affected Files: 1 file, 3 functions
• Risk: Misleading UX — user believes authentication succeeded; downstream engine fails with a confusing "missing secret" error

Recommendation: Check os.Setenv return value and surface errors before printing success:

Before:

_ = os.Setenv(req.Name, token)
fmt.Fprintln(os.Stderr, console.FormatSuccessMessage("Valid fine-grained Copilot token received"))

After:

if err := os.Setenv(req.Name, token); err != nil {
    return fmt.Errorf("failed to store token in environment: %w", err)
}
fmt.Fprintln(os.Stderr, console.FormatSuccessMessage("Valid fine-grained Copilot token received"))

Validation:

• Verify the three functions return error (check callers to ensure error propagation chain)
• Add a test with a deliberately invalid env var name (e.g., "INVALID=NAME") to confirm the error is now surfaced
• Confirm success message only prints after successful Setenv

Estimated Effort: Small

---

Task 3: Handle ExtractExpressions Error in mcp_playwright_config.go

Issue Type: Silent Error Discard / Incorrect Output

Problem: replaceExpressionsInPlaywrightArgs calls ExtractExpressions for its side effect but discards the error. An extraction failure would cause ReplaceExpressionsWithEnvVars to silently produce incorrect Playwright configuration.

Location(s):

• pkg/workflow/mcp_playwright_config.go:53
• pkg/workflow/expression_extraction.go:50 (signature reference)

Impact:

• Severity: Medium
• Affected Files: 1 file, 1 function
• Risk: Silent misconfiguration of Playwright MCP server arguments; $\{\{ }} expressions may be passed un-replaced to the process

Recommendation: Return the error from replaceExpressionsInPlaywrightArgs and propagate it:

Before:

func replaceExpressionsInPlaywrightArgs(args []string, expressions map[string]string) []string {
    combined := strings.Join(args, "\n")
    extractor := NewExpressionExtractor()
    _, _ = extractor.ExtractExpressions(combined)
    replaced := extractor.ReplaceExpressionsWithEnvVars(combined)
    return strings.Split(replaced, "\n")
}

After:

func replaceExpressionsInPlaywrightArgs(args []string, expressions map[string]string) ([]string, error) {
    combined := strings.Join(args, "\n")
    extractor := NewExpressionExtractor()
    if _, err := extractor.ExtractExpressions(combined); err != nil {
        return nil, fmt.Errorf("extracting expressions from playwright args: %w", err)
    }
    replaced := extractor.ReplaceExpressionsWithEnvVars(combined)
    return strings.Split(replaced, "\n"), nil
}

Validation:

• Update callers of replaceExpressionsInPlaywrightArgs to handle the new error return
• Verify ExpressionExtractor.ExtractExpressions actually returns meaningful errors (check test cases)
• Add a test with a malformed expression to confirm error propagation

Estimated Effort: Small-Medium (signature change requires updating callers)

---

Success Metrics

This Run

• Findings Generated: 3
• Tasks Created: 3
• Files Analyzed: ~30 non-test Go files across pkg/cli and pkg/workflow
• Success Score: 8/10

Reasoning for Score

• Findings Quality (3/4): All three findings are real and actionable. Finding 1 (utility duplication) is the most impactful — 10 sites, mechanical fix. Finding 2 (os.Setenv) carries forward from last run and remains unaddressed. Finding 3 is medium but concrete.
• Coverage (2.5/3): Good coverage of pkg/cli error suppression and pkg/workflow expression handling. Goroutine analysis was thorough and correctly ruled out false positives.
• Task Generation (3/3): 3 well-scoped, immediately actionable tasks with clear before/after code.

---

Historical Context

Strategy Performance

Date	Strategy	Score	Findings
2026-04-10	context-sleep-extension-and-error-suppression	8	4
2026-04-09	io-writer-render-and-double-timenow	9	3
2026-04-08	stdout-io-writer-and-vestigial-init-continuation	8	3
2026-04-11	filepath-walk-consolidation-and-error-suppression	8	3

Cumulative Statistics

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

---

Recommendations

Immediate Actions

1. [High] Move findFileInDir to pkg/cli/file_search.go and replace the 5+ direct-substitution duplicates in copilot_agent.go, redacted_domains.go, token_usage.go, logs_parsing_core.go:113, logs_parsing_core.go:240, logs_github_rate_limit_usage.go
2. [High] Propagate os.Setenv errors in engine_secrets.go:329,377,430 — check callers accept an error return and thread it upward
3. [Medium] Change replaceExpressionsInPlaywrightArgs signature to return ([]string, error) and update its callers

Long-term Improvements

• Document package-internal utilities: The findFileInDir duplication suggests that utility functions defined in domain-specific files (e.g., copilot_events_jsonl.go) are not discoverable. Consider creating a pkg/cli/util.go (or pkg/cli/file_search.go) as a canonical home for general-purpose helpers, with a comment indicating they are package-wide utilities.
• Lint rule for _ = os.Setenv: Consider adding an errcheck linter exception list that explicitly excludes this pattern, making remaining suppressions intentional and visible.

---

Next Run Preview

Suggested Focus Areas

• Open issues: vestigial init() bodies (js.go:14, scripts.go:20, expression_patterns.go:67) — 3 runs without resolution
• New area: strings.Builder adoption — check for += string concatenation in loops, which Go's compiler handles but strings.Builder makes explicit and measurable
• Follow-up: Verify that replaceExpressionsInPlaywrightArgs callers are updated if Task 3 is completed

Strategy Evolution

The "utility adoption" lens proved productive today (new approach). Consider pairing it with the "duplicate code detection" strategy next cycle — look for other internal helpers that are defined once but ignored in favor of inline reimplementation.

---

References:

• §24290724834

Run ID: 24290724834 · Strategy: filepath-walk-consolidation-and-error-suppression

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

• expires on Apr 12, 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 #25934.