[sergo] Sergo Report: Exported-Symbol Dead-Code + Magic-Literal Duplication - 2026-05-13 #31867
Closed
Replies: 1 comment
-
|
This discussion has been marked as outdated by Sergo - Serena Go Expert. A newer discussion is available at Discussion #32058. |
Beta Was this translation helpful? Give feedback.
0 replies
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Uh oh!
There was an error while loading. Please reload this page.
-
Executive Summary
Run 8 of Sergo combined two fresh axes that had not been touched in the prior seven runs: (a) a dead-code audit across the nine utility sub-packages in
pkg/usingfind_referencing_symbols-style usage counts, and (b) a magic-literal duplication scan focused on file-permission octals and HTTP-client timeouts.The dead-code half came back clean — all nine utility packages (
sliceutil,typeutil,jsonutil,stringutil,semverutil,envutil,timeutil,repoutil,actionpins) showed healthy multi-site usage of their exported APIs. The magic-literal half was much more productive: 57 file-mode octal literals and 4 repeated HTTP-client timeouts are scattered acrosspkg/with no centralized constants, and the codebase itself already contains evidence (pkg/workflow/awf_helpers.go:182) that the team knows0644leaks secrets — yet 16 other prod sites still write at0644with no policy justification.Two high-value, well-scoped issues filed:
#aw_sg8a1(file-permission constants) and#aw_sg8a2(HTTP-client timeout). Success score: 8/10 — strong findings, but the dead-code half produced no actionable output, so coverage is asymmetric.Serena Tools Update
Tools Snapshot
Tool Capabilities Used Today
activate_project— bootstrap Go project stateget_symbols_overview— enumerate exported functions/types in 9 utility packagesfind_symbol(withname_path_pattern+include_body) — fetch SafeUint64ToInt and SafeUintToInt bodiesfind_symbolbroad queries take 70s+, Grep is faster for plain name lookups)Gotcha reconfirmed
find_symbolrejects--name_pathoutright; must use--name_path_pattern(Pydantic validation error otherwise). Cached.Strategy Selection
Cached Reuse Component (50%)
Adapted from: Run 7 (
switch-statement-complexity-plus-exported-api-usage-audit, 9/10).find_referencing_symbolswas the highest-leverage half of that run — it surfaced the 8-way dispatch-table issue (#aw_sg7a1) that Run 7's own switch-axis missed.envutil.GetIntFromEnvused at 1 prod site (pkg/cli/logs_orchestrator.go) — not worth filing.New Exploration Component (50%)
Novel approach: Magic-literal duplication scan — a fresh axis untouched in Runs 1-7.
os\.(MkdirAll|WriteFile|OpenFile|Mkdir|Chmod)\([^)]*0[0-7]+) and HTTP-client timeout literals (http\.Client\{Timeout:).pkg/constants/already exists with a healthyDefault*naming convention, so consolidation is mechanical.Combined Rationale
The dead-code half acts as a defensive sweep (confirming the utility surface area is well-utilized — useful signal even when finding nothing), while the magic-literal half is offensive search against a high-leverage refactor opportunity. They share a unifying theme: inventory the surface of pkg/, then check whether each entry has a clear home. Dead-code finds entries without consumers; magic literals find consumers without an entry.
Analysis Execution
Codebase Context
pkg/Go files (non-test): 784pkg/actionpins,pkg/agentdrain,pkg/cli,pkg/console,pkg/constants,pkg/envutil,pkg/fileutil,pkg/gitutil,pkg/jsonutil,pkg/logger,pkg/parser,pkg/repoutil,pkg/semverutil,pkg/sliceutil,pkg/stats,pkg/stringutil,pkg/styles,pkg/testutil,pkg/timeutil,pkg/tty,pkg/types,pkg/typeutil,pkg/workflow)pkg/for magic literalsFindings Summary
Detailed Findings
High Priority
Finding 1 — File-permission octal magic literals (57 prod sites) [Issue #aw_sg8a1]
57 raw octal literals across
pkg/non-test code inos.MkdirAll/os.WriteFile/os.OpenFile/os.Chmodcalls. Breakdown:0755064406000750No
FilePerm*orDirPerm*constants exist inpkg/constants/. The codebase already shows policy awareness —pkg/workflow/awf_helpers.go:182explicitly comments that the default0644would leave "secrets (e.g. MCP gateway tokens) world-readable on the runner host" — yet 16 other prod sites still use0644with no policy justification. Inconsistency examples:pkg/cli/includes.go:351,:455,pkg/cli/resources.go:207write downloaded remote content at0600(good) — butpkg/parser/import_cache.go:130writes equivalent remote-fetched cache content at0644.pkg/cli/audit.go:749uses0750for audit output, whilepkg/cli/copilot_setup.go:186uses0755for a workflows dir.Finding 2 — HTTP-client default timeout literal
30 * time.Second(4 prod sites) [Issue #aw_sg8a2]All use the same literal for the same conceptual purpose (network call with a generous best-effort timeout) but with no centralized constant — even though
pkg/constants/constants.goalready groups other defaults likeDefaultAgenticWorkflowTimeout,DefaultToolTimeout,DefaultMCPStartupTimeout.Bonus:
pkg/cli/mcp_inspect_mcp.go:253constructs an&http.Client{...}with custom-headerTransportbut omitsTimeoutentirely. Whether the wrapping mark3labs MCP transport applies its own deadline is unverified in the issue's validation steps.Medium Priority (folded into Finding 2)
Missing HTTP-client timeout at
pkg/cli/mcp_inspect_mcp.go:253— included as a validation step inside#aw_sg8a2rather than a separate issue, because the fix depends on auditing transport behavior.Low Priority Observations
pkg/typeutil/SafeUintToIntis a 1-line wrapper aroundSafeUint64ToInt. Both are used. Trivial.pkg/envutil/GetIntFromEnvused at 1 prod site. Acceptable — utility function still appropriate.pkg/workflow/markdown_security_scanner.gohas 28regexp.MustCompilecalls — all package-level vars, well-organized. No action.pkg/workflow/expression_patterns.gohas 21regexp.MustCompilecalls — package-level, named, documented. Confirms the team uses package-level regex var patterns idiomatically.N * time.{Second,Minute,Hour,Millisecond}literals total. Outside the four HTTP-client cluster, most are context-specific (polling delays, cache TTLs) and don't warrant centralization.http.Client{...}constructor sites total in prod (4 with explicit30sTimeout, 2 with named constants/short literals, 2 inspector-class with custom transports).Improvement Tasks Generated
Task 1: Centralize file-permission constants in
pkg/constants/constants.go(filed as #aw_sg8a1)Severity: High (security + maintainability)
Affected Files: 57 prod sites in
pkg/Risk: Inconsistent permission choices for sensitive content; a future contributor copy-pastes
0644for a file that should be0600.Recommendation: Add four typed
fs.FileModeconstants (FilePermSensitive=0o600,FilePermPublic=0o644,DirPermSensitive=0o750,DirPermPublic=0o755) with docstrings stating the policy. Replace all 57 sites, making the sensitive/public choice explicit at the call site.Task 2: Centralize default HTTP-client timeout (filed as #aw_sg8a2)
Severity: Medium
Affected Files: 4 explicit prod sites + 1 audit (mcp_inspect_mcp.go:253)
Risk: Drift if a future contributor changes the timeout in one place but not the others; hidden indefinite-hang risk in the missing-Timeout site.
Recommendation: Add
constants.DefaultHTTPClientTimeout = 30 * time.Secondand replace four sites. Audit the inspector-class site to confirm whether it should also adopt the default.Success Metrics
This Run
Score Reasoning
Historical Context
Strategy Performance Trend
Cumulative Statistics
Recommendations
Immediate Actions
Long-term Improvements
Magic-literal scanning is high-yield because the codebase already has the infrastructure for centralization (
pkg/constants/package,Default*naming, fs.FileMode-friendly Go API). Future Sergo runs could productize this as a recurring axis: regex for shared literals like file modes, network timeouts, retry counts, default ports, magic UUIDs, and known prefixes (e.g."/tmp/gh-aw/"appears as a literal multiple times in pkg/constants itself).Next Run Preview
Suggested Focus Areas for Run 9
gh-aw,/tmp/gh-aw/..., action names). Use the same Grep-driven approach.find_symbolon every interface inpkg/and verify each implementing type actually exists and is reachable. Untouched in Runs 1-8. Combine with error sentinel inventory (errors.Is/errors.Assite count vs. exported error variables).Strategy Evolution
The Run-8 result reinforces a heuristic: fresh axes pay off more than incremental extensions of cached strategies. The 50% cached half produced 0 issues today; the 50% new half produced 2. The next run should consider weighting slightly toward novel exploration (60/40) when prior cached strategies have been thoroughly mined.
Generated by Sergo - The Serena Go Expert
Run ID: §25779206526
Strategy: exported-symbol-dead-code-plus-magic-literal-duplication
Beta Was this translation helpful? Give feedback.
All reactions