[refactor] pkg/cli semantic clustering: 3 duplicate clusters in outcome_eval_*, scattered stats helpers, misplaced trial_types l [Content truncated due to length]

[github-actions]

🔧 Semantic Function Clustering Analysis — pkg/cli, pkg/colorwriter

Analyzed the precomputed package slice for this run: 392 non-test Go files (390 in pkg/cli, 2 in pkg/colorwriter), ~5,900 function declarations. Clustering was done by naming pattern + Serena symbol lookup, then each candidate was verified by reading the actual implementations.

Headline: the package is generally well-organized — the codemod_*.go family (66 files, one get*Codemod() per file) and the pkg/colorwriter build-tag pair are textbook one-file-per-feature. The real findings are concentrated in the outcome_eval_* family, plus a small set of scattered statistics/formatting helpers.

Key findings

#	Finding	Location	Severity
1	Human-comment counting loop duplicated 3×	outcome_eval_{issue,pr,comment}.go	High
2	Close-provenance walk duplicated 2× with divergent error semantics	outcome_eval_{issue,generic}.go	High
3	labelsToStringsFromNodes / labelsToStringsFromMaps near-identical	outcome_eval.go:608,623	Medium
4	Statistics helpers scattered across 4 files; one is O(n2)	outcome_eval.go, forecast_montecarlo.go, audit_math_helpers.go	Medium
5	Four non-type functions living in trial_types.go	trial_types.go:43–119	Low
6	10 hand-rolled ghAPIGet test-seam aliases	6 outcome_eval_*.go files	Low

1. Duplicate: human-comment counting (3 occurrences)

The same "fetch issue comments, count non-bot authors" loop appears three times:

// outcome_eval_issue.go:45
commentList, cerr := ghAPIGetArray(ctx, fmt.Sprintf("issues/%d/comments", num), repo)
if cerr == nil {
    for _, c := range commentList {
        user, _ := c["user"].(map[string]any)
        login, _ := user["login"].(string)
        if !isBotUser(login) {
            report.HumanComments++
        }
    }
}

// outcome_eval_pr.go:110 — identical apart from the aliased getter
comments, err := outcomeEvalPRGHAPIGetArray(ctx, fmt.Sprintf("issues/%d/comments", num), repo)
if err == nil {
    for _, c := range comments {
        user, _ := c["user"].(map[string]any)
        login, _ := user["login"].(string)
        if !isBotUser(login) { report.HumanComments++ }
    }
}

// outcome_eval_comment.go:68 — same shape, plus a created_at cutoff

Recommendation: extract into outcome_eval.go (which already hosts isBotUser, ghAPIGetArray, resolveItem*):

func countHumanComments(ctx context.Context, num int, repo string, after string) int

with after == "" meaning "no cutoff", which covers all three call sites.

2. Duplicate: close-provenance walk, with a behavioral divergence

isClosedByBot (outcome_eval_issue.go:103) and isClosedByLifecycleBot (outcome_eval_generic.go:77) walk the same issues/{n}/events timeline backward for the most recent closed event and return isBotUser(actor.login).

They differ only in error handling — and the divergence is meaningful:

• isClosedByBot returns bare bool; a fetch failure or a missing close event silently yields false → the issue is classified OutcomeRejected ("closed as not planned") when provenance was simply unavailable.
• isClosedByLifecycleBot returns (bool, error); the caller maps a failure to OutcomeError with "close provenance unavailable".

The second is the correct behavior. Consolidating on it removes ~15 duplicated lines and fixes a misclassification path in evalCreateIssue.

Recommendation: delete isClosedByBot, move isClosedByLifecycleBot into outcome_eval.go, and have evalCreateIssue handle the error branch the way evalCloseSticky already does.

3. Near-duplicate: label extraction (~90% identical)

outcome_eval.go:608 and :623 differ only in input type — []any requiring a type assertion per element vs. []map[string]any. Bodies are otherwise line-for-line identical.

Recommendation: this is a textbook generics case, or simply have labelsToStringsFromNodes convert and delegate:

func labelsToStringsFromNodes(nodes []any) []string {
    maps := make([]map[string]any, 0, len(nodes))
    for _, n := range nodes {
        if m, ok := n.(map[string]any); ok { maps = append(maps, m) }
    }
    return labelsToStringsFromMaps(maps)
}

4. Scattered statistics helpers — and an O(n2) median

Numeric/statistical helpers are spread across four unrelated files with no shared home:

Function	File	Note
safePercent, formatPercent, formatPercentagePointChange, formatCountChange, formatFloatDelta	audit_math_helpers.go	the de-facto home
meanStdDevInt, percentileInt	forecast_montecarlo.go	general-purpose, not Monte-Carlo-specific
medianFloat	outcome_eval.go	outlier — pure stats in a GitHub-API evaluation file
chiSquarePValue, expectedProportions	experiments_analyze_statistics.go	domain-specific, fine where it is

safePercent is already the proven shared-helper pattern — it is imported by 5 unrelated files (deps_report.go, health_metrics.go, audit_expanded.go, audit_cross_run_render.go, experiments_analyze_statistics.go). The others have not been given the same treatment.

medianFloat (outcome_eval.go:370) additionally uses a hand-written O(n2) selection sort:

for i := range sorted {
    for j := i + 1; j < n; j++ {
        if sorted[j] < sorted[i] { sorted[i], sorted[j] = sorted[j], sorted[i] }
    }
}

slices.Sort is O(n log n), already used elsewhere in the package, and is a one-line replacement.

Recommendation: rename audit_math_helpers.go → math_helpers.go (it is no longer audit-specific given its 5 external consumers) and move medianFloat, meanStdDevInt, and percentileInt into it. Replace the selection sort with slices.Sort.

5. Outlier: logic functions in a types file

trial_types.go is named for type declarations but holds four behavioral functions:

• extractSafeOutputErrors (:43) — artifact parsing
• aggregateTrialResults (:70) — result aggregation
• sanitizeControlChars (:90) — security-relevant string sanitization
• isControlRune (:117) — its predicate

All four are consumed exclusively by trial_helpers.go (lines 125, 176, 195, 230) — verified, no other call sites in the package. Compare with the correctly-scoped forecast_types.go and token_usage_types.go, which contain zero functions.

Recommendation: move all four to trial_helpers.go, leaving trial_types.go as declarations only. Note sanitizeControlChars is a security control (prevents terminal escape-sequence injection from agent-controlled text) — keep its doc comment intact through the move.

6. Repetitive test-seam aliases and evaluator preamble

Ten package-level function aliases exist purely as test seams:

outcome_eval_update.go:16    outcomeUpdateGHAPIGet     = ghAPIGet
outcome_eval_generic.go:12   genericOutcomeGHAPIGet    = ghAPIGet
outcome_eval_generic.go:13   closeStickyGHAPIGet       = ghAPIGet
outcome_eval_generic.go:14   closeStickyGHAPIGetArray  = ghAPIGetArray
outcome_eval_pr.go:14,15     outcomeEvalPRGHAPIGet(Array)
outcome_eval_workflow.go:17  workflowOutcomeGHAPIGet   = ghAPIGet
outcome_eval_review.go:15,16 outcomeReviewGHAPIGet(Array)
outcome_eval.go:23           objectiveMappingGHAPIGetArray = ghAPIGetArray

Some evaluators use an alias while others call ghAPIGet directly (outcome_eval_issue.go, outcome_eval_agent.go, and parts of outcome_eval_generic.go) — so the seam is inconsistently applied and a test stubbing one alias silently misses sibling code paths.

Separately, 13 evaluators across 7 files open with the same ~12-line preamble (resolveItemRepo → resolveItemNumber → build OutcomeReport → guard num == 0 || repo == ""), with the guard message drifting between "missing number or repo", "missing issue number or repo", and "missing PR number or repo".

Recommendation (lower priority, larger blast radius): replace the alias sprawl with a single injectable client value, and extract the preamble into newOutcomeReport(item, repoOverride) (OutcomeReport, bool). This is worth doing only if the outcome_eval_* family is being touched anyway.

What was checked and found clean

• codemod_*.go (66 files) — exactly one get*Codemod() per file, feature-named. No consolidation warranted.
• pkg/colorwriter — New/Stderr/Degrade appear twice, but as (go/redacted):build wasm variants. Correct Go idiom, not duplication.
• Common utility names (pluralize, boolPtr, parseNumberFromURL, containsControlCharacters) — each defined exactly once. Already centralized.
• sanitize* family — 8 functions, but each targets a genuinely different domain (shell, GHA expressions, branch names, HTTP errors, log injection). Not duplicates.

Suggested order of work

1. Findings 1–3 — self-contained, inside one file family, and Add workflow: githubnext/agentics/weekly-research #2 fixes a real misclassification bug. Best starting point.
2. Findings 4–5 — mechanical moves; Add workflow: githubnext/agentics/weekly-research #4 also drops an O(n2) sort.
3. Finding 6 — only worth bundling into other outcome_eval_* work.

Analysis metadata

• Files analyzed: 392 (non-test, from the precomputed run slice)
• Function declarations scanned: ~5,900
• Verified duplicate clusters: 3
• Verified outlier/scatter findings: 3
• Method: Serena semantic analysis (activate_project, symbol lookup) + naming-pattern clustering, with every reported finding confirmed by reading the implementation
• Analysis date: 2026-08-11

Generated by 🔧 Semantic Function Refactoring · sonnet46 · 297.7 AIC · ⌖ 21 AIC · ⊞ 9.6K · ◷

• expires on Aug 12, 2026, 7:06 PM UTC-08:00

[github-actions]

🍪 Issue Monster selected this for Copilot

I have identified this issue as a good candidate for automated resolution and requested assignment to the Copilot coding agent.

If assignment succeeds, the Copilot coding agent will analyze the issue and create a pull request with the fix.

Om nom nom! 🍪

🍪 Om nom nom by Issue Monster · gpt54 · 2.9 AIC · ⌖ 5.51 AIC · ⊞ 10.4K · ◷