[typist] π€ Typist: Go Type Consistency Analysis β 8 duplicate clusters + enum-typing opportunities #45983
Closed
Replies: 1 comment
|
This discussion has been marked as outdated by Typist - Go Type Analysis. A newer discussion is available at Discussion #46240. |
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.
π€ Typist - Go Type Consistency Analysis
Analysis of repository: github/gh-aw
Executive Summary
I swept every non-test Go file under
pkg/(1,099 files, ~937 type definitions) looking for two things: types we've modeled more than once, and places where we lean on untyped values that could be strongly typed. The good news up front β this codebase is already in strong shape on typing hygiene. There are zero realinterface{}declarations in productionpkg/code, and the ~3,781any/ ~2,235map[string]anyoccurrences are overwhelmingly legitimate: parsed YAML frontmatter, polymorphic GitHub Actions config (on/runs-on/with/permissionscan each be string, array, or object), JSON decode targets, reflection, and the requiredgo/analysisrun(pass) (any, error)signature (~50Γ). So there's nointerface{}-cleanup crusade to run here.Where the real opportunities live is narrower and higher-signal: 8 duplicate/near-duplicate type clusters (the standout being an Experiment state pair that's byte-identical within
pkg/cli), and a handful of enum-likestringconst groups and struct fields that lack a named type β inconsistent with the 40+type X stringenums this repo already defines. Fixing these is mostly low-risk consolidation that removes copy-paste drift and turns a few stringly-typed comparisons into compiler-checked ones. Nice, bounded wins.Full Analysis Report
Duplicated Type Definitions
Summary Statistics
_test.goandpkg/linters/*/testdata/)Cluster 1: Experiment state β
ExperimentState/experimentStateJSONKind: Exact duplicate Β· Occurrences: 2 Β· Impact: High (single source of truth, both in
pkg/cli)Locations:
pkg/cli/experiments_command.go:35βtype ExperimentState struct { ... }pkg/cli/audit_report_experiments.go:35βtype experimentStateJSON struct { ... }The exported
ExperimentStateand the privateexperimentStateJSONare field- and tag-identical (they both mirror thestate.jsonwritten bypick_experiment.cjs). The audit file keeps its own lowercase copy instead of reusing the exported type in the same package.Recommendation: Delete
experimentStateJSONand useExperimentState(same package, no import needed). Effort: ~30 min Β· Benefit: one schema, no drift between the writer and the auditor.Cluster 2: Experiment run record β
ExperimentRunRecord/experimentRunRecordKind: Exact duplicate Β· Occurrences: 2 Β· Impact: High
Locations:
pkg/cli/experiments_command.go:41pkg/cli/audit_report_experiments.go:42Recommendation: Same fix as Cluster 1 β drop the private
experimentRunRecord, reuseExperimentRunRecord. These two clusters are one PR. Effort: rolls into Cluster 1.Cluster 3:
AuditComparisonIntDelta/AuditComparisonStringDeltaKind: Near duplicate Β· Occurrences: 2 Β· Impact: Medium
Locations:
pkg/cli/audit_comparison.go:45pkg/cli/audit_comparison.go:51Same
{Before, After, Changed}shape; the only difference is the element type (intvsstring). Textbook case for a generic type.Effort: ~1 hr Β· Benefit: one implementation, trivially extends to other element types.
Cluster 4:
AccessLogSummary/FirewallLogSummaryKind: Near duplicate (~80% overlap) Β· Occurrences: 2 Β· Impact: Medium
Locations:
pkg/cli/logs_report_firewall.go:11pkg/cli/logs_report_firewall.go:21Both carry
TotalRequests, allowed/blocked counters,AllowedDomains,BlockedDomains,ByWorkflow. They differ only in field naming (AllowedCount/BlockedCountvsAllowedRequests/BlockedRequests) and one extraRequestsByDomainmap on the firewall variant.Recommendation: Extract a shared base struct and embed it, aligning the counter field names. Effort: 1β2 hr.
Cluster 5:
DomainAnalysis/FirewallAnalysisKind: Near duplicate Β· Occurrences: 2 Β· Impact: Medium
Locations:
pkg/cli/access_log.go:34pkg/cli/firewall_log.go:133Both embed
DomainBuckets+TotalRequests+ allowed/blocked counts and implementAddMetrics(LogAnalysis);FirewallAnalysisonly addsRequestsByDomain. A shared analysis base would remove the parallel maintenance. Effort: 1β2 hr.Cluster 6:
MCPFailureSummaryshould embedAggregatedSummaryBaseKind: Near duplicate Β· Occurrences: 2 Β· Impact: Medium (self-documented drift)
Locations:
pkg/cli/logs_models.go:173βMCPFailureSummarypkg/cli/logs_models.go:157βAggregatedSummaryBaseMCPFailureSummaryre-declares 4 ofAggregatedSummaryBase's fields (Count,Workflows,WorkflowsDisplay,RunIDs) instead of embedding it β and the base's own doc comment already flags that sibling types should embed it.MissingToolSummary/MissingDataSummaryalready do.Recommendation: Embed
AggregatedSummaryBaseinMCPFailureSummary. Effort: ~30 min Β· Benefit: kills copy-paste drift, matches the established pattern.Cluster 7: Per-server MCP metrics (semantic, 4Γ)
Kind: Semantic duplicate Β· Occurrences: 4 Β· Impact: Medium (larger refactor)
Locations:
pkg/cli/audit_report.go:211βMCPServerStatspkg/cli/gateway_logs_types.go:85βGatewayServerMetricspkg/cli/audit_cross_run.go:81βMCPServerCrossRunHealthpkg/cli/audit_expanded.go:90βMCPServerHealthDetailFour separate per-MCP-server stat structs, all keyed on
ServerName, all with overlappingRequestCount/ToolCallCount(TotalCalls) /ErrorCountplus error-rate/latency/size variants. Same concept modeled four times across the audit/gateway reports.Recommendation: Extract a shared
mcpServerMetricsCoreand embed it in each report-specific struct. Bigger surface β do it only if these reports keep evolving together. Effort: 3β4 hr.Cluster 8: Token-usage metrics family (semantic, 4Γ)
Kind: Semantic duplicate Β· Occurrences: 4 Β· Impact: Medium
Locations:
pkg/cli/copilot_events_jsonl.go:100βcopilotUsageMetricspkg/cli/token_usage.go:75βModelTokenUsagepkg/cli/token_usage.go:90βModelTokenUsageRowpkg/cli/token_usage.go:27βTokenUsageEntryThe
{InputTokens, OutputTokens, CacheReadTokens, CacheWriteTokens}quartet (plusEffective/Reasoning) repeats across all four. Note the json-tag inconsistency:copilotUsageMetricsuses camelCase while thetoken_usage.gotrio uses snake_case;ModelTokenUsagevsModelTokenUsageRoware themselves a flattened near-dup.Recommendation: Extract a shared
tokenCountsembed. Reconcile the json tags carefully (they're on the wire). Effort: 3β4 hr.Untyped Usages
Summary Statistics
interface{}declarations (non-test pkg): 0 (the one hit is a doc-comment reference)anyoccurrences: ~3,781 β overwhelmingly idiomatic (YAML/Actions config, JSON decode, reflection,go/analysis)map[string]anyoccurrences: ~2,235 β overwhelmingly idiomatic polymorphic config; not recommended for typingCategory 1: Enum-like string const groups missing a named type
Impact: Medium β turns comment-documented "closed sets" into compiler-enforced ones and makes
switchstatements exhaustively checkable.pkg/workflow/llm_provider.go:13const ( LLMProviderGitHub = "github"; ...Anthropic; ...OpenAI )type LLMProvider string+ typed consts; use fornormalizeLLMProviderparam/return andEngineConfig.LLMProviderpkg/workflow/safe_outputs_validation.go:13SafeOutputsURLsPolicyAllowedOnly = "allowed-only"; ...AllowedOrCodeRegiontype SafeOutputsURLsPolicy string; type the config fieldpkg/github/label_objective_mapping_constants.go:136MultiLabelLogicMax/Sum/First = "max"/"sum"/"first"type MultiLabelLogic string; type field atlabel_objective_mapping.go:27"max") documents its closed setpkg/cli/mcp_registry_types.go:107StatusActive/Inactive = "active"/"inactive"type ServerStatus stringpkg/cli/mcp_registry_types.go:113ArgumentTypePositional/Namedtype ArgumentType stringpkg/workflow/mcp_scripts_parser.go:63MCPScriptsModeHTTP = "http"type MCPScriptsMode string(comment already says "transport modes")The strongest three are #1
LLMProvider, #2SafeOutputsURLsPolicy, and #3MultiLabelLogic, since they're validated/switched-on values flowing through several functions.Example: LLMProvider β before / after
Category 2:
stringstruct fields whose comment already declares an enumImpact: Medium β the closed set exists only in a
//comment today; a named type + consts makes it real.pkg/cli/run_workflow_execution.go:52Status string // "triggered", "dry_run", "error"type RunStatus string+ named constspkg/cli/audit_cross_run.go:106(&:113)OverallStatus string // "allowed","denied","mixed"type DomainStatus string(allowed/denied/mixed/absent)pkg/cli/gateway_logs_timeline.go:91Status string // "success" or "error"type TimelineStatus string(file already definesTimelineEventSource/Kindenums)Category 3: Small
anystruct-field cleanupsImpact: LowβMedium.
pkg/cli/logs_models.go:321-322βRunID any/RunNumber any. The same fields are typedint64elsewhere (run_workflow_execution.go:53,audit_cross_run.go:112). Candidates forint64β but verify theaw_info.jsonsource shape first, sinceanymay have been chosen to tolerate string-vs-number JSON; if so, keepanyor add an explicitjson.Numberdecode.pkg/workflow/mcp_scripts_generator.go:21βInputSchema map[string]any. The pairedMCPScriptParam(mcp_scripts_parser.go:56) already models each field, so a typed JSON-schema struct is possible β but borderline, since JSON-schema shapes are open-ended. Low priority.Refactoring Recommendations (prioritized)
Priority 1 β Quick, unambiguous consolidation (Clusters 1, 2, 6)
Delete
experimentStateJSON+experimentRunRecord(reuse the exported types), and embedAggregatedSummaryBaseinMCPFailureSummary. All same-package, no API changes. ~1.5 hr total, high value.Priority 2 β Named enum types (Category 1, top 3)
Introduce
type LLMProvider string,type SafeOutputsURLsPolicy string,type MultiLabelLogic stringand thread them through their consts/fields/switches. Low risk, aligns with 40+ existing enums. ~3β4 hr.Priority 3 β Structural near-dup consolidation (Clusters 3, 4, 5)
Generic
Delta[T], shared log-summary base, shared analysis base. Mechanical but touches more sites. ~4β5 hr.Priority 4 β Larger semantic families (Clusters 7, 8) + Category 2/3
Shared MCP-server-metrics and token-counts embeds; enum types for comment-documented status fields; the
RunID/RunNumbercleanup after verifying the JSON source. Optional / as those areas evolve.Implementation Checklist
ExperimentState/ExperimentRunRecordduplicates (drop private copies)AggregatedSummaryBaseinMCPFailureSummaryLLMProvider,SafeOutputsURLsPolicy,MultiLabelLogicnamed typesDelta[T]for the audit-comparison deltasaw_info.jsonshape before typingRunID/RunNumbermake test) after each consolidationAnalysis Metadata
pkg/)interface{}declarations: 0All reactions