Date: 2026-02-24
Strategy: error-wrapping-plus-context-propagation
Success Score: 8/10
Run ID: §22372431560

Executive Summary

Today's Sergo run combined a proven error-patterns strategy with a novel sync.Once reset audit. The cached component searched for fmt.Errorf calls using %s/%v to format error values instead of the %w verb — the pattern that breaks Go's errors.Is/errors.As chain inspection. Most uses proved legitimate (formatting non-error strings), but one real error chain breakage was found in the workflow validation path.

The new exploration component uncovered a more significant finding: two cache-clearing functions (ClearCurrentRepoSlugCache and ClearRepositoryFeaturesCache) reset sync.Once values via direct struct assignment (= sync.Once{}), which violates the Go memory model and constitutes a data race if any concurrent goroutine is still inside or has returned from Do(). A third finding — checkActorPermission creating its own context.Background()-derived context while its callers already hold a live MCP request context — rounds out the run.

Three actionable improvement tasks are generated below, covering a data race fix, a context propagation improvement, and an error chain repair.

Serena Tools Update

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

Strategy Selection

Cached Reuse Component (50%)

Previous Strategy Adapted: symbol-analysis-plus-error-patterns (Run 1, 2026-02-21, score 8/10)

• Why Reused: Run 1 successfully identified error wrapping anti-patterns. The approach is broadly applicable and yields findings across many packages.
• Modifications: Narrowed focus to fmt.Errorf with %s/%v formatting an err variable specifically — the subset of usages that silently break errors.Is/errors.As chains. Previous run checked broader patterns; today's search was more precise.
• Target: pkg/cli/ and pkg/workflow/ non-test Go files.

New Exploration Component (50%)

Novel Approach: sync.Once reset audit + context propagation gap analysis

• Hypothesis: Global sync.Once caches that need to be "reset" (e.g., for testing) are often reset by direct struct assignment — a pattern Go's sync package explicitly prohibits after first use.
• Tools Employed: search_for_pattern for sync\.Once\{\} and context\.Background\(\) with surrounding context lines; find_referencing_symbols to understand call sites.
• Target Areas: All files declaring sync.Once globals with associated clear/reset functions; MCP server handler chain for context propagation.

Combined Strategy Rationale

Error wrapping and sync.Once patterns both relate to correctness in concurrent Go code — one silently hides failure modes from callers, the other introduces undefined behavior under concurrent access. Together they provide both depth (specific error chain issue) and breadth (systemic data race pattern across two packages).

Analysis Execution

Codebase Context

• Total non-test Go files: 567
• Total LOC: ~132,784
• Packages Analyzed: pkg/cli, pkg/workflow
• Focus Areas: Cache-clear functions, MCP tool handler chain, workflow validation error paths

Findings Summary

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

Detailed Findings

High Priority Issues

H1 — sync.Once reset via direct struct assignment (data race)

Two cache-clear functions reset sync.Once by writing = sync.Once{}, which is a data race per the Go memory model if any goroutine is concurrently inside Do() or reading the once's internal state.

Both functions also unsynchronised-write their companion result/error variables alongside the sync.Once reset — a second data race avenue if GetCurrentRepoSlug() is running concurrently.

The Go sync package documentation states: "A Once must not be copied after first use." Direct struct assignment is semantically equivalent to a copy and clobbers the internal done uint32 atomic counter and embedded sync.Mutex without going through any synchronization primitive.

Currently both callers are test functions (sequential in practice), so no crash has been observed — but:

1. Any test marked t.Parallel() that calls the clear function creates a real race.
2. The pattern can be copy-pasted into production paths.
3. Go's race detector (-race) will flag this when triggered concurrently.

Medium Priority Issues

M1 — checkActorPermission ignores available MCP request context

checkActorPermission in pkg/cli/mcp_server_helpers.go creates its own independent context:

// mcp_server_helpers.go:205
ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
defer cancel()
permission, err := queryActorRole(ctx, actor, repo)

But both of its callers — registerLogsTool and registerAuditTool in mcp_tools_privileged.go — are MCP handler closures that already receive a live request ctx:

// mcp_tools_privileged.go:72
}, func(ctx context.Context, req *mcp.CallToolRequest, args logsArgs) (*mcp.CallToolResult, any, error) {
    if err := checkActorPermission(actor, validateActor, "logs"); err != nil { // ctx not passed

If the MCP client disconnects or the server shuts down, the parent ctx will be cancelled — but checkActorPermission will continue the GitHub API call for up to 5 seconds ignoring that cancellation signal.

M2 — Error chain broken after errors.As type assertion in run_workflow_validation.go

// pkg/cli/run_workflow_validation.go:286-290
var exitError *exec.ExitError
if errors.As(err, &exitError) {
    return fmt.Errorf("failed to list workflows in repository '%s': %s",
        repoOverride, string(exitError.Stderr))  // %s drops the original error
}
return fmt.Errorf("failed to list workflows in repository '%s': %w", repoOverride, err)

The ExitError branch uses %s with exitError.Stderr (a []byte), discarding err entirely. Callers cannot use errors.Is(err, someErr) or errors.As(err, &exec.ExitError{}) on the returned error. The non-ExitError branch immediately below correctly uses %w. The two branches are inconsistent and the ExitError case — the more specific and informative one — is the one that loses the chain.

Improvement Tasks Generated

Task 1: Replace sync.Once reset-by-assignment with a thread-safe resettable cache pattern

Issue Type: Concurrency / Go Memory Model

Problem: ClearCurrentRepoSlugCache() in pkg/cli/repo.go and ClearRepositoryFeaturesCache() in pkg/workflow/repository_features_validation.go reset sync.Once values by direct struct assignment (= sync.Once{}). This is a data race under concurrent access and violates the Go sync package contract.

Locations:

• pkg/cli/repo.go:17-29 — getCurrentRepoSlugOnce + ClearCurrentRepoSlugCache
• pkg/workflow/repository_features_validation.go:64-93 — getCurrentRepositoryOnce + ClearRepositoryFeaturesCache

Impact:

• Severity: High
• Affected Files: 2 production files + corresponding test files
• Risk: Data race flagged by -race detector; undefined behavior if clear is called concurrently with cache read

Recommendation: Replace the sync.Once + companion variables pattern with a sync.RWMutex-guarded struct or atomic.Pointer holding a nullable result. A minimal fix:

// Before (data race on concurrent clear + read)
var (
    getCurrentRepoSlugOnce sync.Once
    currentRepoSlugResult  string
    currentRepoSlugError   error
)

func ClearCurrentRepoSlugCache() {
    getCurrentRepoSlugOnce = sync.Once{} // DATA RACE
    currentRepoSlugResult = ""
    currentRepoSlugError = nil
}

// After (thread-safe with RWMutex)
type repoSlugCache struct {
    mu     sync.RWMutex
    result string
    err    error
    valid  bool
}

var repoSlugCacheState repoSlugCache

func ClearCurrentRepoSlugCache() {
    repoSlugCacheState.mu.Lock()
    repoSlugCacheState.valid = false
    repoSlugCacheState.result = ""
    repoSlugCacheState.err = nil
    repoSlugCacheState.mu.Unlock()
}

func GetCurrentRepoSlug() (string, error) {
    repoSlugCacheState.mu.RLock()
    if repoSlugCacheState.valid {
        r, e := repoSlugCacheState.result, repoSlugCacheState.err
        repoSlugCacheState.mu.RUnlock()
        return r, e
    }
    repoSlugCacheState.mu.RUnlock()

    repoSlugCacheState.mu.Lock()
    defer repoSlugCacheState.mu.Unlock()
    if !repoSlugCacheState.valid { // double-check after acquiring write lock
        repoSlugCacheState.result, repoSlugCacheState.err = getCurrentRepoSlugUncached()
        repoSlugCacheState.valid = true
    }
    return repoSlugCacheState.result, repoSlugCacheState.err
}

Validation:

• Run go test -race ./pkg/cli/... ./pkg/workflow/... — should report no data races
• Confirm TestClearCurrentRepoSlugCache and TestGetCurrentRepoSlug still pass
• Apply same pattern to getCurrentRepositoryOnce in repository_features_validation.go

Estimated Effort: Small–Medium

Task 2: Thread ctx context.Context through checkActorPermission

Issue Type: Context Propagation

Problem: checkActorPermission in pkg/cli/mcp_server_helpers.go creates a fresh context.WithTimeout(context.Background(), 5*time.Second) instead of deriving from the caller's MCP request context. Both call sites have a live ctx context.Context available from the MCP handler closure.

Locations:

• pkg/cli/mcp_server_helpers.go:169 — function signature (missing ctx param)
• pkg/cli/mcp_server_helpers.go:205 — context.Background() usage
• pkg/cli/mcp_tools_privileged.go:73-74 — caller ignoring ctx
• pkg/cli/mcp_tools_privileged.go:264-265 — second caller ignoring ctx

Impact:

• Severity: Medium
• Affected Files: 2
• Risk: Permission checks run to completion even after client disconnect or server shutdown; adds unnecessary latency to cancellation path

Recommendation:

// Before
func checkActorPermission(actor string, validateActor bool, toolName string) error {
    ...
    ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
    defer cancel()
    permission, err := queryActorRole(ctx, actor, repo)
    ...
}

// After — add ctx parameter, derive timeout from it
func checkActorPermission(ctx context.Context, actor string, validateActor bool, toolName string) error {
    ...
    timeoutCtx, cancel := context.WithTimeout(ctx, 5*time.Second)
    defer cancel()
    permission, err := queryActorRole(timeoutCtx, actor, repo)
    ...
}

// Call sites (mcp_tools_privileged.go)
if err := checkActorPermission(ctx, actor, validateActor, "logs"); err != nil {

Validation:

• Update both call sites in mcp_tools_privileged.go
• Verify compilation: go build ./pkg/cli/...
• Confirm MCP server integration tests still pass

Estimated Effort: Small

Task 3: Restore error chain in run_workflow_validation.go ExitError branch

Issue Type: Error Wrapping

Problem: When RunGH returns an *exec.ExitError, the error branch at pkg/cli/run_workflow_validation.go:288 discards the original error and formats only the stderr bytes using %s. The immediately following branch correctly uses %w. This inconsistency breaks errors.Is/errors.As for the most-common failure case (non-zero exit from gh workflow list).

Location:

• pkg/cli/run_workflow_validation.go:286-290

Impact:

• Severity: Medium
• Affected Files: 1
• Risk: Callers cannot programmatically distinguish gh exit errors from other errors; structured error handling (retry logic, user-facing error classification) is silently defeated for the ExitError path

Recommendation:

// Before — drops original error
var exitError *exec.ExitError
if errors.As(err, &exitError) {
    return fmt.Errorf("failed to list workflows in repository '%s': %s",
        repoOverride, string(exitError.Stderr))
}
return fmt.Errorf("failed to list workflows in repository '%s': %w", repoOverride, err)

// After — preserves full error chain, adds stderr context
var exitError *exec.ExitError
if errors.As(err, &exitError) {
    return fmt.Errorf("failed to list workflows in repository '%s' (stderr: %s): %w",
        repoOverride, strings.TrimSpace(string(exitError.Stderr)), err)
}
return fmt.Errorf("failed to list workflows in repository '%s': %w", repoOverride, err)

Validation:

• go vet ./pkg/cli/... — passes
• Verify callers of validateWorkflowExists can now use errors.As(err, &exec.ExitError{}) successfully
• Run existing test suite: go test ./pkg/cli/...

Estimated Effort: Small

Success Metrics

This Run

• Findings Generated: 3 (1 High, 2 Medium)
• Tasks Created: 3
• Files Analyzed: ~40 non-test Go files across pkg/cli and pkg/workflow
• Success Score: 8/10

Score Reasoning

• Findings Quality (3/4): High-severity sync.Once race is a genuine Go memory model violation; both medium findings are concrete and actionable. Minus one point because all three callers of ClearCurrentRepoSlugCache are test-only, reducing immediate production risk.
• Coverage (2/3): Good coverage of cache/concurrency and error handling patterns; did not explore pkg/parser or utility packages this run.
• Task Generation (3/3): All three tasks are well-scoped with before/after code examples and validation steps.

Historical Context

Cumulative Statistics

• Total Runs: 4
• Total Findings: 12
• Total Tasks Generated: 12
• Average Success Score: 8.25/10
• Most Successful Strategy: error-patterns-plus-field-registry-audit (9/10)

Recommendations

Immediate Actions

1. Fix sync.Once reset data race (High) — Replace both ClearCurrentRepoSlugCache and ClearRepositoryFeaturesCache with sync.RWMutex-guarded resettable cache structs. Run with -race to validate.
2. Thread context through checkActorPermission (Medium) — Small, safe change: add ctx context.Context parameter and derive the 5-second timeout from it. Two call sites to update.
3. Restore error chain in ExitError branch (Medium) — One-line change: replace %s with %w and include err as wrap target, preserving stderr bytes in the format string.

Long-term Improvements

• Adopt a ScriptRegistry-style pattern for resettable caches: The pkg/workflow/script_registry.go already documents a centralized registry pattern to eliminate scattered sync.Once globals. A similar CacheRegistry pattern would give a single place to reset all caches (e.g., in tests) without touching sync.Once internals.
• Add go test -race to CI for pkg/cli and pkg/workflow: The sync.Once assignment race would be caught automatically. This is the idiomatic Go safety net for concurrent code patterns.

Next Run Preview

Suggested Focus Areas

• pkg/parser/ — the largest file (import_processor.go, 1106 LOC) and mcp.go (796 LOC) have not been analyzed for symbol quality, type patterns, or error handling
• pkg/workflow/cache.go (871 LOC) — large cache file likely has mutex/sync patterns worth auditing
• Goroutine lifecycle management — search for go func() calls without associated context.Done() selects

Strategy Evolution

• Consider a "goroutine-lifecycle-plus-large-file-audit" strategy next: combine tracing of goroutine spawns without context propagation with deep analysis of the two largest yet-unanalyzed files
• The error-patterns-plus-field-registry-audit strategy scored highest (9/10) — another registry-focused audit of a different subsystem (e.g., parser hooks or workflow job registry) could be effective

References:

• §22372431560

AI generated by Sergo - Serena Go Expert

• expires on Feb 25, 2026, 10:25 PM UTC