[typist] Typist - Go Type Consistency Analysis #53980
Closed
Replies: 1 comment
|
This discussion has been marked as outdated by Typist - Go Type Analysis. A newer discussion is available at Discussion #54213. |
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
I scanned the non-test Go files under
pkg/(~1,250 files acrosspkg/workflow,pkg/cli,pkg/parser,pkg/types, and ~25 smaller utility packages) for two things: types that are defined more than once, and code that's usinginterface{}/any/untyped constants where a concrete type would catch bugs at compile time instead of runtime.The good news first: this codebase already shows real anti-duplication discipline. Several structs that would have been flagged (
MCPServerConfigvariants,MCPServerStatsvariants, missing-tool/data summaries) have already been refactored to share embedded base types, with comments explicitly calling out "eliminate duplication." Likewise, productioninterface{}/anyusage is dominated by legitimate dynamic YAML/JSON frontmatter parsing (map[string]any) rather than laziness -- most of it is exactly as narrow as the underlying data model allows.That said, I did find 5 duplicate type clusters (1 exact, 2 near, 2 semantic) and a handful of genuinely fixable untyped spots -- most interestingly a
time.Duration-shaped constant that's stored as a bareintof "minutes" while every sibling timeout inpkg/constantsis a typedtime.Duration, and twoanyfields (RunID,RunNumber) inpkg/cli/logs_models.gothat areint64everywhere else in the same package. Both are small, low-risk, high-clarity fixes.Full Analysis Report
Duplicated Type Definitions
Summary Statistics
Method: grepped for
^type \w+ struct,^type \w+ interface, and^type \w+ (string|int|bool|...)acrosspkg/**/*.go(excluding_test.go), grouped by name across packages, then read field lists for every 2+ occurrence cluster. Sampled most heavily inpkg/cli(~450 struct decls) andpkg/workflow(~400 struct decls).Cluster 1:
RepositoryFeatures-- Exact duplicate (build-tag mirror)Occurrences: 2 | Impact: Low-medium -- manual-sync risk, not currently divergent
Locations:
pkg/workflow/repository_features_validation.go:83(build tag!js && !wasm)pkg/workflow/repository_features_validation_wasm.go:31(build tagjs || wasm)Byte-identical, gated by mutually exclusive build tags so they never compile together -- a legitimate WASM-stub pattern, but nothing forces the two copies to stay in sync if a field is ever added.
Recommendation: Move the struct definition itself to a small shared, build-tag-free file (e.g.
repository_features_types.go) that both build variants import, leaving only the behavior (the validation logic) behind the build tags.Estimated effort: 15 minutes | Benefits: Removes one manual-sync footgun.
Cluster 2:
ProgressBar-- Near duplicate (build-tag mirror)Occurrences: 2 | Impact: Low -- same risk pattern as Cluster 1
Locations:
pkg/console/progress.go:31(native)pkg/console/progress_wasm.go:9(WASM stub)Native carries
progress progress.Model,total/current int64,indeterminate bool,updateCount int64,ttyCheck func() bool; the WASM stub drops the bubbletea-specificprogress/ttyCheckfields but keeps the same 4 core fields and public API (NewProgressBar,Update).Recommendation: Extract the 4 shared fields into an embedded
progressStatestruct used by both variants, so the "core" state can't drift.Estimated effort: 20 minutes | Benefits: Single source of truth for shared fields.
Cluster 3:
SpinnerWrapper-- Near duplicate (build-tag mirror)Occurrences: 2 | Impact: Low
Locations:
pkg/console/spinner.go:92(wrapstea.Program+spinnerModel)pkg/console/spinner_wasm.go:12(one-fieldenabled boolstub)Same exported method set (
Start,Stop,StopWithMessage,UpdateMessage,IsEnabled) on both -- deliberate mirror, same pattern/risk as Clusters 1-2.Recommendation: No urgent action needed beyond awareness; if touched again, consider the same shared-state extraction as Cluster 2.
Cluster 4:
PRInfo/PullRequest-- Semantic duplicateType: Semantic duplicate | Occurrences: 2 | Impact: Medium -- same package, overlapping GitHub PR model
Locations:
pkg/cli/pr_command.go:28--PRInfo{Number, Title, Body, State, HeadSHA, BaseBranch, HeadBranch, SourceRepo, TargetRepo, AuthorLogin}pkg/cli/pr_automerge.go:21--PullRequest{Number, Title, IsDraft, Mergeable, CreatedAt, UpdatedAt}Both are ad-hoc partial models of a
gh pr list --json ...result, parsed independently for two different commands in the same package, with overlapping fields (Number,Title).Recommendation:
PullRequesttype inpkg/cli(orpkg/types) as the superset of both field setspr_command.goandpr_automerge.goto populate/consume the shared typeCluster 5:
ToolUsageSummary/MCPToolSummary-- Near/semantic duplicateType: Near duplicate | Occurrences: 2 | Impact: Medium
Locations:
pkg/cli/logs_report_tools.go:13--ToolUsageSummary{Name, TotalCalls, Runs, MaxOutputSize, MaxDuration}pkg/cli/audit_report.go:174--MCPToolSummary{ServerName, ToolName, CallCount, TotalInputSize, TotalOutputSize, MaxInputSize, MaxOutputSize, AvgDuration, MaxDuration, ErrorCount}Same purpose (per-tool aggregated call stats for console/report rendering), overlapping fields (
MaxOutputSize,MaxDuration, a call-count field).MCPToolSummaryreads like an evolved superset ofToolUsageSummaryfor the MCP-aware path that never got merged back.Recommendation: Consolidate onto
MCPToolSummary's shape (or a shared embedded base, following the pattern already used forMCPServerStatsBase/AggregatedSummaryBaseelsewhere in this package) and have both report paths use it.Estimated effort: 2-3 hours | Benefits: One tool-stats model instead of two independently-evolving ones.
Already resolved -- not flagged:
MCPServerConfig/RegistryMCPServerConfig/BaseMCPServerConfigandMCPServerStats/MCPServerHealthDetail/MCPServerCrossRunHealthalready share embedded base types (types.BaseMCPServerConfig,MCPServerStatsBase) with comments confirming this was a deliberate de-duplication. Good precedent to follow for Clusters 4-5.Untyped Usages
Summary Statistics
interface{}usages (old syntax): effectively 0 -- 15 of 16 raw hits are in linter testdata fixtures, not real logic (codebase has already migrated toany)anyinmap[string]anyform: 250+ files, dominant pattern for YAML/JSON frontmatter parsinganystruct fields (not map-shaped): ~30, mostly legitimately polymorphic per YAML union typesanyin function params/returns: ~150+, overwhelminglymap[string]anyconfig-parsing helpersMethod: grepped for
interface{}/anyin signatures, struct fields, and map values, then verified actual call-site types before recommending a fix (not guessing). Focused onpkg/workflowandpkg/cli(the two largest packages), pluspkg/constants,pkg/types,pkg/parser.Category 1:
anyfields that are always a concrete type in practiceImpact: High -- these are the clearest "should just be a real type" cases, since call-site evidence shows they're never actually polymorphic.
Example 1:
AwInfo.RunID/RunNumberpkg/cli/logs_models.go:358-359RunID any(json:"run_id,omitempty") /RunNumber any(json:"run_number,omitempty")RunIDfield in the same package (pkg/cli/logs_models.go:123,280,pkg/cli/audit_cross_run.go,pkg/cli/logs_episode.go) isint64-- GitHub Actions run IDs/numbers are always numeric.Example 2:
ServerStatus/ArgumentTypeenums hiding as bare stringspkg/cli/mcp_registry_types.go:63,71,100(structType stringfields), constants at:108-109(StatusActive/StatusInactive) and:114-115(ArgumentTypePositional/ArgumentTypeNamed)stringfields compared against untyped string constantstype ServerStatus stringandtype ArgumentType string, mirroring the existingpkg/constants.EngineNamepattern already used for engine identifiers, and retype both the constants and the struct fields.arg.Type == ArgumentTypePositional(used atpkg/cli/mcp_registry.go:171,180) becomes compile-time checked against a closed enum instead of arbitrary strings.Example 3:
service_ports.gofield narrower than its only usepkg/workflow/service_ports.go:86Ports any, immediately type-asserted to[]anyat every call site (pkg/workflow/service_ports.go:144)[]anydirectly, removing one runtime assertion +okcheck.Category 2: Untyped constants that should share a type with their consumer
Impact: Medium-high -- these create a type mismatch between a constant and the field/parameter it feeds, which is exactly where "wrong unit" or "wrong enum value" bugs hide.
Example 1: Timeout stored as bare minutes instead of
time.DurationCurrent (
pkg/cli/run_workflow_execution.go:25):Suggested -- matches every sibling timeout in
pkg/constants/constants.go(DefaultAgenticWorkflowTimeout,DefaultToolTimeout,DefaultHTTPClientTimeoutare alltime.Duration):Locations:
pkg/cli/run_workflow_execution.go:25, called frompkg/cli/pr_automerge.go:120Benefits: Matches the established
time.Durationconvention used everywhere else inpkg/constants; eliminates "is this seconds or minutes?" ambiguity for future readers.Example 2: Cache integrity default untyped vs. its typed consumer
Current (
pkg/workflow/cache_integrity.go:17):Suggested --
GitHubIntegrityLevelalready exists atpkg/workflow/tools_types.go:287:Locations:
pkg/workflow/cache_integrity.go:17,166, sibling field atpkg/workflow/tools_types.go:287,325Benefits: Closes a type gap between a default constant and the exact enum-typed field (
GitHubToolConfig.MinIntegrity) it exists to default.Not recommended for change (ruled out with reasoning):
defaultSafeOutputsTimeoutMinutes = 45inpkg/workflow/compiler_safe_outputs_job.go:513feeds a GitHub Actions YAMLtimeout-minutes:field, which is inherently minutes-denominated in the Actions schema itself -- an untyped int is the right call there. Mostpkg/constantssizing constants (DefaultMaxRuns,DefaultMCPGatewayPayloadSizeThreshold, per-functionmaxRetries/maxDepth) and the 100+ single-value string constants (image refs, file paths, command names) have no closed value set or enum-like semantics, so typing them would add ceremony without safety.Category 3:
map[string]anyfrontmatter/config parsing -- largely fine as-isThe overwhelming majority of
anyusage (250+ files) isfunc parseXConfig(m map[string]any) *XConfig-style helpers pulling one key out of already-yaml.Unmarshal'd frontmatter, plus MCP/JSON-RPC fields (likerpcRequestPayload.ID any) that are legitimately polymorphic per protocol spec, plus documented multi-form YAML fields (pkg/workflow/frontmatter_types.go'sHeaders,Endpoint,RunsOn,Imports, etc., each already parsed into a narrower type downstream). Retyping these would require a full frontmatter schema type system Go doesn't make easy (no discriminated unions) for marginal benefit -- not recommended.Recommendations, Prioritized
Priority 1:
RunID/RunNumberandtime.Durationtimeout fixesSmall, mechanical, zero ambiguity about the right type -- the surrounding code already establishes the pattern (see
pkg/cli/logs_models.goandpkg/constants/constants.go).Estimated effort: ~1 hour total | Impact: High confidence, low risk
Priority 2:
ServerStatus/ArgumentType/GitHubIntegrityLevelconstant typingMirrors the existing
EngineNameenum pattern already in the codebase -- a template to copy, not a new idiom to invent.Estimated effort: 2-3 hours | Impact: Medium -- prevents invalid string comparisons for status/arg-kind/integrity checks
Priority 3: Consolidate
PRInfo/PullRequestandToolUsageSummary/MCPToolSummaryFollow the precedent already set by
BaseMCPServerConfig/MCPServerStatsBasein the same package.Estimated effort: 3-5 hours combined | Impact: Medium -- one model per concept instead of two
Priority 4: Build-tag mirror structs (
RepositoryFeatures,ProgressBar,SpinnerWrapper)Low urgency since they're not currently divergent, but worth fixing opportunistically next time one of these files is touched.
Estimated effort: ~1 hour combined | Impact: Low -- removes manual-sync risk
Implementation Checklist
AwInfo.RunID/RunNumbertoint64(pkg/cli/logs_models.go)workflowCompletionWaitTimeoutMinutesto atime.Durationconstant and updateWaitForWorkflowCompletion's signaturedefaultCacheIntegrityLevelandcacheIntegrityLevel()toGitHubIntegrityLevelServerStatus/ArgumentTypenamed types inpkg/cli/mcp_registry_types.goPRInfo/PullRequestinpkg/cliToolUsageSummary/MCPToolSummaryinpkg/cliAnalysis Metadata
pkg/workflow,pkg/cli,pkg/console,pkg/constants,pkg/types,pkg/parserAll reactions