Typist - Go Type Consistency Analysis #17885
Replies: 1 comment
-
|
/plan |
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.
-
Analysis of repository: github/gh-aw | Run: §22303802656 | Date: 2026-02-23
Executive Summary
Analysis of 558 non-test Go files across 18 packages in
pkg/reveals a codebase that has already invested significantly in type safety —pkg/constantsuses 12 custom semantic types covering versions, URLs, job names, engine names, and more. The main refactoring opportunities are 35 untyped string constants and 6 untyped numeric constants inconstants.gothat could benefit from the same treatment applied to their neighbors, plus targetedany/map[string]anyusages that could be progressively strengthened. No true type duplication was found; the three apparent duplicate type names are all intentional build-tag-separated (wasmvs non-wasm) implementations.Total findings: 0 type duplicates, 1
interface{}usage (wasm compatibility stub), 297 files usingany(mostly justified YAML parsing), 41 untyped constants with clear typing opportunities.Full Analysis Report
Duplicated Type Definitions
Summary Statistics
Cluster 1:
SpinnerWrapper— Build-Tag VariantClassification: Intentional platform split — NOT a duplicate
Files:
pkg/console/spinner.go—//go:build !js && !wasm— Full implementation using charmbraceletpkg/console/spinner_wasm.go—//go:build js || wasm— Stub for WASM environmentsAssessment: Correct Go pattern for platform-specific implementations. No action required.
Cluster 2:
ProgressBar— Build-Tag VariantClassification: Intentional platform split — NOT a duplicate
Files:
pkg/console/progress.go—//go:build !js && !wasmpkg/console/progress_wasm.go—//go:build js || wasmAssessment: Same pattern as SpinnerWrapper. No action required.
Cluster 3:
RepositoryFeatures— Build-Tag VariantClassification: Intentional platform split — NOT a duplicate
Files:
pkg/workflow/repository_features_validation.go—//go:build !js && !wasm— Full implementationpkg/workflow/repository_features_validation_wasm.go—//go:build js || wasm— Stub withClearRepositoryFeaturesCache() {}Assessment: Expected pattern. No action required.
Notable Non-Duplicate:
MCPServerConfigBoth
pkg/parserandpkg/workflowdefineMCPServerConfig, but this is architecturally sound:types.BaseMCPServerConfigfrom the sharedpkg/typespackageparser.MCPServerConfigadds:Name,Registry,ProxyArgs,Allowed(JSON tags, parsing context)workflow.MCPServerConfigadds:Mode,Toolsets,CustomFields(YAML tags, compilation context)The
pkg/types.BaseMCPServerConfigshared base is itself evidence of proactive type consolidation already done.Untyped Usages
Summary Statistics
interface{}usages: 1anyusages (files): 297 (predominantly justified YAML parsing)Category 1: The Single
interface{}UsageLocation:
pkg/console/layout_wasm.go:16Impact: Low — WASM stub only; signature inconsistency with non-wasm version
Issue: The WASM stub uses
interface{}(old style) instead ofany(Go 1.18+). The parameter is intentionally ignored sincelipglossisn't available in WASM.Suggested fix:
Benefits: Consistent with modern Go style (
anyvsinterface{}); no functional change needed.Effort: 1 line change.
Category 2: Justified
map[string]anyUsages (YAML/JSON Parsing)The widespread use of
map[string]anyin the workflow and parser packages is largely justified by the nature of YAML frontmatter — it is inherently dynamic. The codebase already demonstrates awareness of this tradeoff:Already well-typed (these are NOT issues):
pkg/workflow/tools_types.go—ToolsConfigwrapsmap[string]anywith a structured typed API and explicit// ToMap()for backward compatibility. Comments explain the reasoning at lines 13–20.pkg/workflow/frontmatter_types.go:92— Comment explicitly notes:"This provides compile-time type safety and clearer error messages compared to map[string]any", with inline explanations on each remainingmap[string]anyfield (e.g.,// Legacy field,// too dynamic to type,// Deprecated).pkg/workflow/tools_types.go:343—CustomFields map[string]anyhas an explicit comment:// For truly dynamic configuration (server-specific fields not covered above).Partially addressable:
Example:
SafeInputParam.Default any—pkg/workflow/safe_inputs_parser.go:46The
Defaultfield represents a JSON schema default value, which by the JSON Schema spec can be any type. Usinganyis correct here, though ajson.RawMessagealternative could provide better serialization control:Effort: Medium — requires updating all callers that set/read
Default.Example:
DevcontainerVSCode.Settings map[string]any—pkg/cli/devcontainer.go:27VSCode settings are extensible and arbitrary — this is a justified use case. No action needed.
Example:
DevcontainerFeatures map[string]any—pkg/cli/devcontainer.go:39The DevContainer Features specification defines feature options as arbitrary JSON — the
anyvalue is spec-mandated. No action needed.Category 3: Untyped Constants — High Value Opportunity
The
pkg/constants/constants.gofile shows a clear evolution toward typed constants: 39 constants already use typed declarations (e.g.,Version,URL,JobName,ModelName). However, 35 untyped string constants and 6 untyped numeric constants remain that follow similar patterns to already-typed neighbors.Group A: Output Name Constants (10 untyped strings, lines 682–691)
Benefits: Prevents accidentally mixing output names with arbitrary strings at call sites. Consistent with the existing
StepIDandJobNametyped constants pattern.Used in: 19 locations across
pkg/workflowandpkg/cli.Group B: MCP Server ID Constants (3 untyped strings, lines 648–657)
Benefits: Prevents passing arbitrary strings where an MCP server ID is expected. These are used in 15+ locations in string concatenation for YAML generation — typing them would make the intent clear.
Group C: Artifact Name Constants (3 untyped strings, lines 641–645)
Group D: Version Constant Missing Type (1 untyped string, line 654)
This is inconsistent with
DefaultClaudeCodeVersion Version,DefaultGitHubMCPServerVersion Version, and 20+ other version constants that are correctly typed. Likely an oversight.Group E: Rate Limit Constants (2 untyped ints, lines 694–695)
Benefits: Eliminates ambiguity about units (minutes vs seconds vs hours). The comment currently carries semantic weight that a type could encode.
Group F:
MaxTimeDeltaXConstants —pkg/workflow/time_delta.go:484–498Benefits: Prevents accidentally using time limits as counts or other integer values.
Group G: Container Image and Path Constants (14 untyped strings)
Assessment: These are lower priority. Container image references are strings by convention and adding a
ContainerImage stringtype would require updating all Docker-related code. TheDocURLtype already exists for documentation URLs. Consider aContainerImage stringtype for the container constants to match the existingURLtype pattern.Refactoring Recommendations
Priority 1: Quick Win — Fix
interface{}toanyFile:
pkg/console/layout_wasm.go:16Change:
color interface{}→color anyEffort: 1 line, 0 functional impact, modernizes to Go 1.18+ idiom
Priority 2: High Value — Type the
SafeInputsMCPVersionConstantFile:
pkg/constants/constants.go:654Change:
const SafeInputsMCPVersion = "1.0.0"→const SafeInputsMCPVersion Version = "1.0.0"Effort: 1 line — fixes obvious inconsistency with 20+ neighboring typed Version constants
Priority 3: Medium — Type Output Name Constants
File:
pkg/constants/constants.go:682–691Change: Add
type OutputName string, apply to 10 constantsEffort: ~15 lines in constants.go + verify ~19 usage sites accept string conversion
Benefits: Compile-time protection against string mix-up at output-name-sensitive call sites
Priority 4: Medium — Type MCP Server ID Constants
File:
pkg/constants/constants.go:648–657Change: Add
type MCPServerID string, apply to 3 constantsEffort: ~10 lines in constants.go + review 15 usage sites (many use string concatenation in YAML builders — may require
.String()calls)Note: Usage in string concatenation (
"[mcp_servers." + constants.SafeOutputsMCPServerID + "]") would requirestring(constants.SafeOutputsMCPServerID)or a.String()methodPriority 5: Low — Type Artifact Names and Rate Limit Constants
Files:
pkg/constants/constants.go:641–645, 694–695Effort: Small — add 2 types, update ~5 constants each
Benefits: Semantic clarity; rate limit typing prevents unit confusion
Implementation Checklist
interface{}→anyinpkg/console/layout_wasm.go:16Versiontype toSafeInputsMCPVersioninpkg/constants/constants.go:654OutputName stringtype and apply to 10 output constants (lines 682–691)MCPServerID stringtype and apply to 3 MCP server ID constants (lines 648–657)ArtifactName stringtype for artifact name constants (lines 641–642)DefaultRateLimitMaxandDefaultRateLimitWindow(lines 694–695)TimeDeltaLimittype forMaxTimeDeltaXconstants inpkg/workflow/time_delta.goAnalysis Metadata
pkg/)interface{}usagesanyanyusagesReferences:
Beta Was this translation helpful? Give feedback.
All reactions