[sergo] Sergo Report: Regex Duplication & Goroutine Context Analysis - 2026-04-01

[github-actions[bot]]

Date: 2026-04-01
Strategy: regex-duplication-and-goroutine-context
Success Score: 9/10
Run ID: §23869084025

Executive Summary

Today's analysis combined a concurrency/goroutine review (extending last run's mutex-safety findings) with a brand-new regex duplication audit. The concurrency review confirmed that signal handling, atomic operations, and spinner goroutines are all correct — a reassuring negative result. The regex audit uncovered a systematic pattern of centralization violations: the workflow package's dedicated expression_patterns.go file was created precisely to be a single source of truth for regex patterns, yet expression_safety_validation.go ignores it entirely with 9 duplicate package-level variables, and template.go recompiles TemplateIfPattern every time it's called. In the cli package, three functions compile static patterns inline, and includes.go duplicates a pattern already defined in the same package.

Three actionable tasks follow: (1) migrate expression_safety_validation.go and template_injection_validation.go to use the centralized exports, (2) hoist static regexp.MustCompile calls to package-level vars in the cli package, and (3) fix includes.go to use the existing includePattern.

---

🛠️ 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

• search_for_pattern — regex pattern search across Go files for regexp.MustCompile, go func(, make(chan, signal.Notify, atomic., sync.Map
• find_referencing_symbols — verified call frequency of parseIssueSpec and sanitizeBranchName
• find_symbol — located TemplateIfPattern, InlineExpressionPattern, ExpressionPatternDotAll in centralized file
• get_symbols_overview — scoped searches to relevant files
• read_file — verified full context of findings before categorizing

---

📊 Strategy Selection

Cached Reuse Component (50%)

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

• Original Success Score: 8/10
• Last Used: 2026-03-31
• Why Reused: That run successfully identified missing "centralization enforcement" (compile-time interface assertions). Today I applied the same principle to regex patterns: are shared patterns actually shared?
• Modifications: Instead of checking interface assertion guards, I audited whether expression_patterns.go's centralized exports are consistently used vs. re-declared. Also extended the goroutine/concurrency scope from mutex to goroutine lifecycle.

New Exploration Component (50%)

Novel Approach: Regex Duplication Audit

• Tools Employed: search_for_pattern with regexp.MustCompile\( pattern, cross-referenced against known centralized exports
• Hypothesis: Large codebase with a dedicated regex centralization file (expression_patterns.go) likely has drift — older files that predate centralization still declare their own copies
• Target Areas: pkg/workflow/ (focused on expression_patterns.go consumers) and pkg/cli/ (inline compilation in helper functions)

Combined Strategy Rationale

The cached component validated the concurrency picture (no new issues), freeing focus for the new exploration. The regex duplication angle was chosen because prior runs hadn't examined it, the codebase has an explicit centralization artifact (expression_patterns.go), and duplication here creates real risk: if a pattern in expression_patterns.go is refined for security/correctness, the duplicates silently diverge.

---

🔍 Analysis Execution

Codebase Context

• Total Go Files (non-test): 641
• Packages Analyzed: pkg/workflow, pkg/cli, pkg/parser
• LOC Analyzed: ~150,000 total; focused scan on ~20 files
• Focus Areas: expression_patterns.go consumers, function-level regexp.MustCompile calls

Findings Summary

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

---

📋 Detailed Findings

High Priority Issues

Finding 1: expression_safety_validation.go re-declares 9 centralized regex patterns

pkg/workflow/expression_safety_validation.go declares 9 package-level regex variables that are exact duplicates of exports in pkg/workflow/expression_patterns.go. The expression_patterns.go file was explicitly created as a "single source of truth" (per its file-level comment), yet this file predates or ignores that centralization.

Duplicated pairs (local var → centralized export):

Local variable	Centralized export	File:Line
expressionRegex ((?s)\$\{\{(.*?)\}\})	ExpressionPatternDotAll	expression_safety_validation.go:26
needsStepsRegex	NeedsStepsPattern	:27
inputsRegex	InputsPattern	:28
workflowCallInputsRegex	WorkflowCallInputsPattern	:29
awInputsRegex	AWInputsPattern	:30
awImportInputsRegex	AWImportInputsPattern	:31
envRegex	EnvPattern	:32
comparisonExtractionRegex	ComparisonExtractionPattern	:35
orExpressionPattern	OrPattern	:37

Risk: If any centralized pattern is updated (e.g., for a new expression syntax or security fix), expression_safety_validation.go silently keeps the old version. Two separate regex objects also consume extra memory.

Medium Priority Issues

Finding 2: template_injection_validation.go duplicates 2 centralized patterns

pkg/workflow/template_injection_validation.go declares:

• inlineExpressionRegex (\$\{\{[^}]+\}\}) at line 63 → duplicates InlineExpressionPattern
• unsafeContextRegex at line 67 → duplicates UnsafeContextPattern

Same risk as Finding 1 — these files are in the same workflow package and should reference the centralized exports directly.

Finding 3: template.go:24 recompiles TemplateIfPattern inside a function

pkg/workflow/template.go function wrapExpressionsInTemplateConditionals compiles this at runtime:

re := regexp.MustCompile(`\{\\{\#if\s+((?:\$\{\{[^\}]*\}\}|[^\}])*)\s*\}\}`)

This is byte-for-byte identical to TemplateIfPattern in expression_patterns.go:142, which is in the same package. The function uses re twice (once for ReplaceAllStringFunc, once inside the closure), so it's called on every invocation. This should simply be TemplateIfPattern.

Finding 4: includes.go:358 duplicates includePattern from packages.go

pkg/cli/includes.go:358 (inside fetchAndSaveRemoteIncludes) compiles:

includePattern := regexp.MustCompile(`^`@include`(\?)?\s+(.+)$`)

pkg/cli/packages.go:20 already declares a package-level variable with the same name and pattern:

var includePattern = regexp.MustCompile(`^`@include`(\?)?\s+(.+)$`)

Both files are in package cli. The includes.go function shadows the package-level variable with a local one, missing the existing optimization and creating a duplication risk. The local variable should simply be removed — the function already has access to the package-level includePattern.

Low Priority Issues

Finding 5: Static regex compiled inside functions in cli package

Three cli functions compile static, unchanging patterns on every call:

• pkg/cli/trial_helpers.go:234,240,246 (parseIssueSpec) — 3 patterns
• pkg/cli/add_workflow_pr.go:29,33 (sanitizeBranchName) — 2 patterns
• pkg/cli/generate_action_metadata_command.go:180,206,238,268 — 4 patterns across 3 functions

These functions are not called in tight loops (call-site analysis via find_referencing_symbols confirmed once-per-command invocation), so the performance impact is minimal. However, hoisting them to package-level follows Go best practice and makes intent clear.

Concurrency Audit (Negative Result)

The cached component scanned the following — all patterns were found correct:

• Signal handling (signal.Notify): 3 locations in compile_watch.go, signal_aware_poll.go, retry.go — all use defer signal.Stop() correctly
• Goroutine tracking: spinner.go uses sync.WaitGroup with proper wg.Add(1) before goroutine launch and wg.Wait() in Stop()/StopWithMessage()
• Goroutine cleanup with timeout: mcp_inspect_inspector.go:200-215 uses a done channel + time.After(5s) select — correct pattern
• Atomic operations: logs_orchestrator.go:637,841 use atomic.AddInt64 correctly
• sync.Map usage: yaml.go:105, repository_features_validation.go:75-76 — appropriate use for concurrent read-heavy workloads
• CheckForUpdatesAsync (update_check.go:242): Goroutine does not propagate context into checkForUpdates, but this is intentional fire-and-forget behavior with a panic recovery guard

---

✅ Improvement Tasks Generated

Task 1: Migrate expression_safety_validation.go to use centralized regex exports

Issue Type: Code Duplication / Centralization Violation

Problem:
expression_safety_validation.go declares 9 package-level regex variables that are exact duplicates of exports in expression_patterns.go. The centralized file's own header comment states it is the "single source of truth" for expression matching logic. Having 9 stale copies creates a maintenance hazard: any future fix or refinement to the centralized patterns silently leaves the safety validation using an older version.

Locations:

• pkg/workflow/expression_safety_validation.go:26-37 — 9 local declarations to remove
• pkg/workflow/template_injection_validation.go:63,67 — 2 local declarations to remove
• pkg/workflow/template.go:24 — local compilation to replace with TemplateIfPattern

Impact:

• Severity: High
• Affected Files: 3
• Risk: A security-motivated change to a centralized pattern (e.g., tightening UnsafeContextPattern) would not take effect in template_injection_validation.go

Recommendation:
Remove the 11 local declarations and replace all usages with the corresponding centralized exports:

// Before (expression_safety_validation.go)
var (
    expressionRegex         = regexp.MustCompile(`(?s)\$\{\{(.*?)\}\}`)
    needsStepsRegex         = regexp.MustCompile(`^(needs|steps)\.[a-zA-Z0-9_-]+(\.[a-zA-Z0-9_-]+)*$`)
    // ... 7 more
)

// After — delete all 9 declarations; use centralized exports directly:
// ExpressionPatternDotAll, NeedsStepsPattern, InputsPattern,
// WorkflowCallInputsPattern, AWInputsPattern, AWImportInputsPattern,
// EnvPattern, ComparisonExtractionPattern, OrPattern

// Before (template.go:24)
re := regexp.MustCompile(`\{\\{\#if\s+((?:\$\{\{[^\}]*\}\}|[^\}])*)\s*\}\}`)

// After
re := TemplateIfPattern

Validation:

• All existing tests pass (no behavior change, only reference substitution)
• Remove the regexp import from files where it's no longer needed
• Verify pattern byte-for-byte equivalence before replacing

Estimated Effort: Small

---

Task 2: Remove shadowed includePattern in includes.go

Issue Type: Package-Level Variable Shadowing

Problem:
pkg/cli/includes.go:358 declares a local variable includePattern that shadows the package-level includePattern from packages.go:20. Both are in package cli and compile the identical pattern ^@include(\?)?\s+(.+)$. The local variable compiles a new regex object on every call to fetchAndSaveRemoteIncludes instead of reusing the pre-compiled package-level one.

Location:

• pkg/cli/includes.go:358 — line to remove
• pkg/cli/packages.go:20 — the canonical declaration (no change needed)

Impact:

• Severity: Medium
• Affected Files: 1
• Risk: If the pattern in packages.go is updated (e.g., to support @import directive unification), includes.go silently keeps the old pattern

Recommendation:

// Before (includes.go:358 inside fetchAndSaveRemoteIncludes)
includePattern := regexp.MustCompile(`^`@include`(\?)?\s+(.+)$`)

// After — delete the local declaration entirely.
// The function automatically uses the package-level includePattern from packages.go.

Validation:

• Verify fetchAndSaveRemoteIncludes still compiles (the package-level includePattern is in scope)
• Run go vet ./pkg/cli/... to confirm no shadowing warnings
• Existing tests for include directives pass

Estimated Effort: Small (single line deletion)

---

Task 3: Hoist static regexp.MustCompile calls to package-level in cli package

Issue Type: Performance / Go Best Practice

Problem:
Three functions in pkg/cli/ compile static regex patterns on every call. While none are in tight loops (confirmed via reference analysis), Go best practice requires package-level pre-compilation for any reusable pattern. This ensures the pattern is compiled exactly once at startup, the variable name documents intent, and the pattern is testable in isolation.

Locations:

• pkg/cli/trial_helpers.go:234,240,246 — parseIssueSpec: 3 patterns
• pkg/cli/add_workflow_pr.go:29,33 — sanitizeBranchName: 2 patterns
• pkg/cli/generate_action_metadata_command.go:180,206,238,268 — 4 patterns across extractDescription, extractInputs, extractOutputs, extractDependencies

Recommendation:

// Before (trial_helpers.go inside parseIssueSpec)
urlRegex := regexp.MustCompile(`(github/redacted)\.com/[^/]+/[^/]+/issues/(\d+)`)
refRegex := regexp.MustCompile(`^#(\d+)$`)
numberRegex := regexp.MustCompile(`^\d+$`)

// After — at package level
var (
    issueURLRegex    = regexp.MustCompile(`(github/redacted)\.com/[^/]+/[^/]+/issues/(\d+)`)
    issueRefRegex    = regexp.MustCompile(`^#(\d+)$`)
    issueNumberRegex = regexp.MustCompile(`^\d+$`)
)

// Before (add_workflow_pr.go inside sanitizeBranchName)
invalidChars := regexp.MustCompile(`[^a-zA-Z0-9_-]+`)
consecutiveHyphens := regexp.MustCompile(`-{2,}`)

// After — at package level
var (
    branchInvalidCharsRE   = regexp.MustCompile(`[^a-zA-Z0-9_-]+`)
    branchConsecutiveHyphRE = regexp.MustCompile(`-{2,}`)
)

Validation:

• All existing tests pass
• go vet ./pkg/cli/... clean
• Benchmark confirms no regression (optional; impact is negligible for these call sites)

Estimated Effort: Small

---

📈 Success Metrics

This Run

• Findings Generated: 5 (grouped into 3 task-worthy issues)
• Tasks Created: 3
• Files Analyzed: ~25 non-test Go files
• Success Score: 9/10

Reasoning for Score

• Findings Quality (4/4): All findings are actionable, non-trivial, and tied to the explicit design intent of expression_patterns.go. Finding 1 (9 duplicated patterns) carries real maintenance risk.
• Coverage (3/3): Two distinct areas (workflow package regex centralization + cli package inline compilation) plus a thorough negative concurrency audit covering all 4 goroutine/channel patterns.
• Task Generation (2/3): 3 tasks generated; all are small-effort/high-value. Slight deduction because none reach "Critical" severity.

---

📊 Historical Context

Strategy Performance

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

Cumulative Statistics

• Total Runs: 5
• Total Findings: 22
• Total Tasks Generated: 15
• Average Success Score: 8.0/10
• Most Successful Strategy: regex-duplication-and-goroutine-context (this run)

---

🎯 Recommendations

Immediate Actions

1. [High] Migrate expression_safety_validation.go + template.go — Remove 11 regex duplicates and use centralized exports. Zero behavior change, eliminates silent drift risk.
2. [Medium] Delete shadowing includePattern in includes.go — One line deletion, uses already-present package-level variable.
3. [Low] Hoist inline regexp.MustCompile in cli — 9 patterns across 3 files; low urgency but consistent with Go idiom.

Long-term Improvements

• Consider adding a go vet or golangci-lint custom rule to flag regexp.MustCompile inside functions (e.g., via bodyclose or a custom analyzer)
• Add a comment to the local vars in expression_safety_validation.go pointing to expression_patterns.go as a migration note until cleaned up
• Consider a regular "pattern audit" as a recurring Sergo strategy now that centralization gaps have been found in two separate areas (interface assertions in run 3, regex patterns today)

---

🔄 Next Run Preview

Suggested Focus Areas

• Error wrapping consistency: Are all fmt.Errorf calls using %w for wrappable errors? Previous run 1 found %s anti-patterns; a systematic audit may find more.
• context.Context propagation completeness: Run 2 found 15 context.Background() misuses. Verify if those were addressed and check for new ones in recently-added files.
• Unused exported symbols: With the Serena find_referencing_symbols tool, scan for exported functions/types in pkg/ that have zero callers outside their own package.

Strategy Evolution

• The 50/50 cached/new split is working well — 8.0 average score across 5 runs. Next run: consider a 70/30 split toward new exploration since the cached pool now covers concurrency, error handling, type safety, and regex centralization.

---

References:

• §23869084025

AI generated by Sergo - Serena Go Expert · history

• expires on Apr 2, 2026, 8:32 PM UTC

[github-actions[bot]]

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

A newer discussion is available at Discussion #24168.