Typist - Go Type Consistency Analysis #22413
Closed
Replies: 1 comment
-
|
This discussion has been marked as outdated by Typist - Go Type Analysis. A newer discussion is available at Discussion #22655. |
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 — §23434646322
Executive Summary
601 non-test Go source files in
pkg/were analyzed. The codebase demonstrates strong awareness of type safety concerns —pkg/workflow/tools_types.gocontains explicit in-code comments documenting an in-progress migration away from the pervasivemap[string]anypattern toward typed structs. No use of legacyinterface{}was found (0 occurrences). The four identified "duplicate" struct names are all intentional build-tag patterns (WASM vs. native), not accidental duplication. The primary refactoring opportunities are the 1,413map[string]anyusages for YAML frontmatter/tools parsing, three untyped numeric constants inpkg/constants/constants.go, and one struct field that could use a richer type.Full Analysis Report
Duplicated Type Definitions
Summary Statistics
pkg/types)Cluster 1:
ProgressBarandSpinnerWrapper— Build-tag pattern (intentional)Type: Platform-specific implementation split
Occurrences: 2 each
Verdict: ✅ Intentional — standard Go idiom for WASM compatibility
pkg/console/progress.go:31bubbles/progressmodelpkg/console/progress_wasm.go:7//go:build js || wasmpkg/console/spinner.go:96tea.Program, mutex, waitgrouppkg/console/spinner_wasm.go:10//go:build js || wasmenabled boolonly)This is a correct idiomatic Go pattern. No action needed.
Cluster 2:
RepositoryFeatures— Build-tag pattern (intentional)Type: WASM stub for live feature detection
Occurrences: 2
Verdict: ✅ Intentional
pkg/workflow/repository_features_validation.go:57HasDiscussions bool,HasIssues boolpkg/workflow/repository_features_validation_wasm.go:5//go:build js || wasmvalidateRepositoryFeaturesis a no-opFields are identical and the WASM stub is a compile-time placeholder. No action needed.
Cluster 3:
MCPServerConfig— Different types with shared base (by design)Type: Purposeful domain separation, shared base already extracted
Occurrences: 2
Verdict: ✅ Well-structured —
pkg/types.BaseMCPServerConfigalready consolidates shared fieldspkg/workflow/tools_types.go:380workflowMode,Toolsets,GuardPolicies map[string]any,CustomFields map[string]anypkg/parser/mcp.go:35parserName,Registry,ProxyArgs,AllowedBoth embed
types.BaseMCPServerConfigwhich lives inpkg/types/mcp.go. The shared base pattern is already implemented correctly. The remainingmap[string]anyfields (GuardPolicies,CustomFields) are covered below.Untyped Usages
Summary Statistics
interface{}usagesmap[string]anyusages (total)map[string]anymap[string]anyCategory 1:
map[string]anyfor YAML frontmatter parsing — in-progress migrationImpact: Medium — codebase is already aware and partially migrated
Location: Predominantly
pkg/workflow/The files with the highest concentration:
pkg/workflow/compiler_safe_outputs_config.gopkg/workflow/frontmatter_types.gopkg/workflow/safe_outputs_tools_filtering.gopkg/workflow/compiler_orchestrator_workflow.gopkg/workflow/trigger_parser.goThe codebase already documents this as an active migration target.
pkg/workflow/tools_types.gocontains these explicit comments at lines 13 and 20:ToolsConfig(introduced intools_types.go) provides a typed wrapper withParseToolsConfig(map[string]any)andToMap() map[string]anyconverters for backward compatibility. The migration pattern is already established — continuing to push call sites toward*ToolsConfigwill progressively reducemap[string]anyusage.Recommendation: Continue the existing migration, focusing on these high-value sites:
pkg/workflow/compiler_orchestrator_workflow.go— Functions likeextractYAMLSections,processAndMergeSteps, andprocessAndMergeServicesall acceptfrontmatter map[string]any. A typedFrontmatterDatastruct (partially represented already byWorkflowData) could replace the raw map.pkg/workflow/compiler_safe_outputs_config.go—handlerBuilder func(*SafeOutputsConfig) map[string]any(line 132) returns a raw map. Returning a dedicatedSafeOutputsHandlerResultstruct would eliminate downstream type assertions.pkg/workflow/frontmatter_types.go— Already has strongly typed structs likeRuntimesConfigandGitHubActionsPermissionsConfig. Remainingmap[string]anyusages are likely in parsing glue code that could adopt these types.Category 2: Struct fields typed as
map[string]anyImpact: Low-Medium — these are in boundary/integration types
Example 1:
MCPServerConfig.GuardPoliciesandCustomFieldspkg/workflow/tools_types.go:391-394GuardPoliciesis explicitly noted as server-specific and extensible.CustomFieldsis an inline catch-all for unknown YAML keys.CustomFields(inline catch-all) is acceptable.GuardPoliciescould benefit from a typed base interface:Example 2:
MCPToolDefinition.InputSchemapkg/workflow/mcp_scripts_generator.go:20InputSchemarepresents a JSON Schema object, which is inherently dynamic and polymorphic. This is a widely-accepted pattern for JSON Schema embedding in Go.json.RawMessageor a dedicatedJSONSchematype if validation is needed.Example 3:
DevcontainerFeaturestype aliaspkg/cli/devcontainer.go:39map[string]anyis hidden behind a named type, which is the right approach.Category 3: Untyped numeric constants
Impact: Low — clarity and misuse prevention
Location:
pkg/constants/constants.goDefaultMCPGatewayPayloadSizeThreshold524288type ByteSize int64; const DefaultMCPGatewayPayloadSizeThreshold ByteSize = 524288DefaultRateLimitMax5type RunCount int; const DefaultRateLimitMax RunCount = 5DefaultRateLimitWindow60inttime.Duration:const DefaultRateLimitWindow = 60 * time.MinuteMaxSymlinkDepth5intgiven context; a comment sufficesThe highest-value fix is
DefaultRateLimitWindow— the comment says "Default time window in minutes (1 hour)" but the raw integer could be accidentally used as seconds elsewhere:Estimated effort: 30-60 minutes
Priority: Medium (prevents unit confusion bugs)
Refactoring Recommendations
Priority 1: Medium — Convert
DefaultRateLimitWindowtotime.DurationFile:
pkg/constants/constants.go:740Risk: Low — isolated constant
Steps:
const DefaultRateLimitWindow = 60 * time.Minutetime.DurationPriority 2: Medium — Continue
ToolsConfigmigration incompiler_orchestrator_workflow.goFile:
pkg/workflow/compiler_orchestrator_workflow.goRisk: Medium — central orchestration logic
Steps:
frontmatter map[string]anyto use structured types where possibleToolsConfigwrapper where tools maps are extractedPriority 3: Low — Type
handlerBuilderreturn valueFile:
pkg/workflow/compiler_safe_outputs_config.go:132Current:
type handlerBuilder func(*SafeOutputsConfig) map[string]anyRisk: Low — internal type
Steps:
SafeOutputsHandlerResultstruct capturing the fields currently returned as a raw mapImplementation Checklist
DefaultRateLimitWindowto usetime.Duration(quick win)DefaultRateLimitWindowacrosspkg/for unit mismatchesmap[string]anyfrontmatter parameter to migrate usingToolsConfigpatternGuardPolicyinterface forMCPServerConfig.GuardPoliciesAnalysis Metadata
interface{}usagesmap[string]anyusagesReferences: §23434646322
Beta Was this translation helpful? Give feedback.
All reactions