[sergo] Sergo Report: Error Patterns Analysis - 2026-03-29

[github-actions[bot]]

Executive Summary

Today's Sergo analysis focused on error handling patterns in the gh-aw Go codebase (629 non-test .go files, ~149K LOC). Using Serena's LSP-powered semantic search and pattern matching, three actionable issues were found, with the most impactful being timestamps embedded in validation error strings — a pattern that affects 35+ call sites and can cause test non-determinism. Two additional medium-priority issues were identified: idiomatic misuse of fmt.Errorf("%s", ...) (12+ locations) and silently discarded os.Setenv errors in secret-handling code.

This was the first Sergo run on this repository, establishing a baseline for future analysis.

---

🛠️ Serena Tools Update

Tools Snapshot

• Total Tools Available: 23
• New Tools Since Last Run: N/A (first run — baseline established)
• Removed Tools: N/A
• Modified Tools: N/A

Tool Capabilities Used Today

Tool	Purpose
activate_project	Activated Go LSP for this workspace
get_symbols_overview	Inspected struct/method structure of error types
find_symbol	Located ConvertToInt, NewValidationError bodies
find_referencing_symbols	Traced NewValidationError call sites
search_for_pattern	Found fmt.Errorf("%s",...), _ = os.Setenv, panic(, context.Background() patterns

---

📊 Strategy Selection

Cached Reuse Component (50%)

No prior cache — this run establishes the baseline. The "reuse" component was fulfilled by using well-known Go static analysis heuristics (error handling anti-patterns, ignored errors, panic in init code) that consistently yield high-value findings in Go codebases.

New Exploration Component (50%)

Novel approach: Deep structural inspection of the error type hierarchy using Serena's symbol overview and referencing tools. Rather than just pattern-searching for strings, this explored the architecture of error types to find design-level issues (missing Unwrap(), timestamp coupling, inconsistency across error structs).

Combined Strategy Rationale

Pattern search (broad) + symbol navigation (deep) is a complementary pairing: patterns surface the volume of an issue across the codebase, while symbol navigation reveals why the pattern is problematic and how widely it's used.

---

🔍 Analysis Execution

Codebase Context

• Total Go files (non-test): 629
• Packages analyzed: pkg/workflow, pkg/cli, pkg/semverutil, pkg/parser
• LOC analyzed: ~149K
• Focus areas: pkg/workflow/workflow_errors.go, pkg/cli/engine_secrets.go, error pattern sites

Findings Summary

Severity	Count
High	1
Medium	1
Low	1
Total	3

---

📋 Detailed Findings

High Priority

Finding 1: Timestamps embedded in error message strings

WorkflowValidationError, OperationError, and ConfigurationError in pkg/workflow/workflow_errors.go all embed time.Now() RFC3339 timestamps directly in their Error() string output:

// pkg/workflow/workflow_errors.go:43-44
fmt.Fprintf(&b, "[%s] Validation failed for field '%s'",
    e.Timestamp.Format(time.RFC3339), e.Field)

This pattern is repeated for all three error types (lines 43–44, ~91, 145–146). The timestamp is computed at construction time via time.Now() and baked into the error string.

Impact:

• Tests comparing error strings will be time-dependent and potentially flaky
• Log aggregation tools cannot deduplicate these errors (every occurrence looks unique)
• User-facing validation messages include machine-readable timestamps that reduce readability
• NewValidationError has 35+ call sites across pkg/workflow/safe_outputs_validation.go, expression_syntax_validation.go, sandbox_validation.go, npm_validation.go, and many others

Medium Priority

Finding 2: fmt.Errorf("%s", ...) used instead of errors.New()

12+ locations use fmt.Errorf("%s", someString) to construct errors from already-formatted strings. This is semantically misleading — it looks like error wrapping (which uses %w) but isn't. The correct idiomatic form is errors.New(someString).

Locations include:

• pkg/workflow/workflow_errors.go:266
• pkg/workflow/template_injection_validation.go:305
• pkg/workflow/dangerous_permissions_validation.go:96
• pkg/workflow/permissions_validation.go:346
• pkg/workflow/engine_validation.go:79
• pkg/workflow/runtime_validation.go:91
• pkg/workflow/tools_validation.go:349
• pkg/workflow/frontmatter_error.go:80
• pkg/cli/run_workflow_validation.go:269
• pkg/cli/run_workflow_execution.go:210
• pkg/parser/import_error.go:68,117
• pkg/workflow/engine_definition.go:280

Example pattern

// Current (12+ locations):
return fmt.Errorf("%s", sb.String())

// Idiomatic Go:
return errors.New(sb.String())

The fmt.Errorf("%s", ...) form also triggers the govet linter's printf checker in some configurations, since %w is the expected verb for wrapping.

Low Priority

Finding 3: Silently discarded os.Setenv return values in secret handling

In pkg/cli/engine_secrets.go, three calls to os.Setenv() discard the error:

// Lines 329, 377, 430
_ = os.Setenv(req.Name, token)

On Linux, os.Setenv rarely fails, but if it does (e.g., if the environment variable name contains =), the secret won't be set in the process environment. The calling code then proceeds to upload the secret to the repository without validating that the local env was updated. A log warning at minimum would make failures debuggable.

Context

These are in promptForCopilotTokenUnified, promptForSystemTokenUnified, and promptForGenericAPIKeyUnified. Each function prompts the user for a secret via a TUI form, then calls os.Setenv to store it locally before optionally uploading it. The discarded error means a rare OS-level failure would cause a silent, hard-to-diagnose bug.

---

✅ Improvement Tasks Generated

Task 1: Remove timestamps from error message strings

Issue Type: Error Type Design

Problem:
WorkflowValidationError, OperationError, and ConfigurationError embed RFC3339 timestamps in their Error() string output. Timestamps in error messages (not structured fields) break test determinism, prevent log deduplication, and add noise to user-facing output. The timestamp field is useful for structured logging — it should be kept as a field but not included in the Error() string.

Location(s):

• pkg/workflow/workflow_errors.go:43-44 — WorkflowValidationError.Error()
• pkg/workflow/workflow_errors.go:90-91 — OperationError.Error()
• pkg/workflow/workflow_errors.go:145-146 — ConfigurationError.Error()

Impact:

• Severity: High
• Affected Files: 1 primary + all test files that assert on error messages (35+ indirect call sites)
• Risk: Test flakiness; log deduplication failures; inconsistent user experience

Recommendation:
Remove the timestamp prefix from each Error() method. Retain the Timestamp field for structured logging or metrics use.

Before (WorkflowValidationError.Error()):

fmt.Fprintf(&b, "[%s] Validation failed for field '%s'",
    e.Timestamp.Format(time.RFC3339), e.Field)

After:

fmt.Fprintf(&b, "Validation failed for field '%s'", e.Field)

Validation:

• Run go test ./pkg/workflow/... — look for newly-passing tests that previously required time-insensitive matching
• Search for any test code using strings.Contains(err.Error(), "[20 to find assertions that relied on the timestamp prefix
• Apply same change to OperationError and ConfigurationError

Estimated Effort: Small

---

Task 2: Replace fmt.Errorf("%s", ...) with errors.New()

Issue Type: Code Idiom / Linting

Problem:
12+ call sites use fmt.Errorf("%s", someString) to construct new errors from formatted strings. This is:

1. Semantically misleading — fmt.Errorf with %w is for wrapping; %s does not wrap
2. Slightly less efficient (unnecessary fmt formatting overhead)
3. Potentially flagged by go vet / staticcheck in stricter configurations

Location(s):

• pkg/workflow/workflow_errors.go:266
• pkg/workflow/template_injection_validation.go:305
• pkg/workflow/dangerous_permissions_validation.go:96
• pkg/workflow/permissions_validation.go:346
• pkg/workflow/engine_validation.go:79
• pkg/workflow/runtime_validation.go:91
• pkg/workflow/tools_validation.go:349
• pkg/workflow/frontmatter_error.go:80
• pkg/cli/run_workflow_validation.go:269
• pkg/cli/run_workflow_execution.go:210
• pkg/parser/import_error.go:68,117
• pkg/workflow/engine_definition.go:280

Impact:

• Severity: Medium
• Affected Files: 12 files
• Risk: Low — purely a style/idiom issue; no behavioral change

Recommendation:
Replace each fmt.Errorf("%s", expr) with errors.New(expr). Ensure errors is imported in files where it isn't already.

Before:

return fmt.Errorf("%s", sb.String())

After:

return errors.New(sb.String())

Validation:

• Run go build ./... to confirm no breakage
• Run go test ./...
• Run go vet ./...

Estimated Effort: Small (mechanical replacement)

---

Task 3: Log or return os.Setenv errors in secret-handling code

Issue Type: Error Handling / Silent Failure

Problem:
Three calls to os.Setenv in pkg/cli/engine_secrets.go (lines 329, 377, 430) silently discard errors. If the environment variable cannot be set (e.g., due to a name containing = or an OS restriction), the secret will appear to be stored but won't be accessible to subsequent code that reads it from the environment.

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: Low
• Affected Files: 1
• Risk: Silent failure of secret injection; hard to debug in the field

Recommendation:
Log a warning (or return an error) if os.Setenv fails. Given that these functions already print status messages to stderr, a warning log is appropriate:

Before:

_ = os.Setenv(req.Name, token)

After:

if err := os.Setenv(req.Name, token); err != nil {
    engineSecretsLog.Printf("Warning: failed to set env var %s: %v", req.Name, err)
}

Validation:

• Run go build ./pkg/cli/...
• Run go test ./pkg/cli/...

Estimated Effort: Small

---

📈 Success Metrics

This Run

Metric	Value
Findings Generated	3
Tasks Created	3
Files Analyzed	~629 non-test Go files
Success Score	7/10

Reasoning for Score

• Findings Quality (5/7): All three findings are real, actionable issues with concrete locations. The timestamp finding is architecturally significant (35+ call sites). Deducted for not finding a critical/security-level issue.
• Coverage (2/3): Covered pkg/workflow and pkg/cli well; pkg/parser, utility packages had less depth.
• Task Generation (3/3): Three well-scoped, immediately actionable tasks with before/after examples.

---

📊 Historical Context

Cumulative Statistics

Metric	Value
Total Runs	1
Total Findings	3
Total Tasks Generated	3
Average Success Score	7.0/10
Most Successful Strategy	error-patterns-first-run

---

🎯 Recommendations

Immediate Actions

1. [High] Remove timestamps from error message strings — affects 35+ call sites; straightforward fix in workflow_errors.go
2. [Medium] Replace fmt.Errorf("%s", ...) with errors.New() — mechanical replacement across 12 files; zero behavioral change
3. [Low] Log os.Setenv failures — single-file change in engine_secrets.go

Long-term Improvements

• Consider adding a go vet or staticcheck rule to the CI pipeline to catch fmt.Errorf("%s", ...) in future
• The WorkflowValidationError and ConfigurationError types currently lack Unwrap() methods (unlike OperationError). If these errors are ever wrapped and then inspected with errors.As(), add Unwrap() for consistency

---

🔄 Next Run Preview

Suggested Focus Areas

• Interface compliance checks: Are all expected interfaces fully implemented? Use find_symbol with kind=Interface to enumerate interfaces and verify implementations
• Context propagation: Several context.Background() calls in pkg/cli (e.g., resources.go:79, add_command.go:363) may be ignoring request contexts passed from callers — worth a focused pass
• Dead code: With DEADCODE.md present in the repo, Serena's symbol navigation could cross-reference declared dead code against actual references

Strategy Evolution

Next run should reuse today's error-patterns approach (now cached) and pair it with a new interface-compliance exploration strategy.

---

References:

• §23718124131

AI generated by Sergo - Serena Go Expert · history

• expires on Mar 30, 2026, 8:25 PM UTC

[github-actions[bot]]

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

A newer discussion is available at Discussion #23504.