Repository navigation
[typist] π€ Typist β Go Type Consistency Analysis #68675
Closed
Replies: 1 comment
|
This discussion was automatically closed because it expired on 2026-10-15T11:41:19.037Z.
|
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 Β· scope: non-test
.gofiles underpkg/Executive Summary
I scanned all 1,542 non-test Go files under
pkg/for duplicated type definitions and untyped (interface{}/any) usage. The good news first:interface{}is basically extinct here β only 2 real occurrences remain, the rest of the codebase has already migrated toany. The bigger story is thatanyitself is everywhere (map[string]anyalone shows up 3,262 times across 516 files), almost entirely at the YAML/JSON frontmatter-parsing boundary. That's largely an intentional architectural pattern rather than scattered carelessness, but a few spots β especiallypkg/workflow/tools_parser.goand the MCP audit-tool input types β would benefit from real structs withUnmarshalYAML/UnmarshalJSONinstead of manual type assertions.On the duplication side, I found 7 clusters worth a look. Two are genuinely interesting: a three-way
ValidationError/WorkflowValidationError/Payloadsplit acrossparser,workflow, andvalidationerrorthat already shares a base type but still models "a validation error" three separate ways, and a pair ofgraderManifestEntrystructs incliandworkflowwith only ~36% field overlap that look like a reader/writer schema split worth documenting (or unifying) explicitly. A couple of others (bareConfig,LogParser) are same-name-coincidence rather than real duplication β flagged but low priority. I also found a handful of untyped constants (DefaultMaxRuns,DefaultRateLimitMax/Window, and one string-typed numeric constantDefaultMaxDailyAICredits = "5000") sitting right next to already-typed siblings in the same file, which is the cheapest fix in this whole report.Full Analysis Report
Duplicated Type Definitions
Summary Statistics
Cluster 1:
ValidationErrorfamily β semantic duplicateImpact: Medium β three packages model the same concept independently
Locations:
pkg/validationerror/validationerror.goβPayloadstruct:Field string,Value string,Reason string,Suggestion stringpkg/parser/validation_error.goβValidationErrorstruct embeddingvalidationerror.Payloadpkg/workflow/workflow_errors.goβWorkflowValidationErrorstruct embeddingvalidationerror.PayloadplusSeverity,Category,Timestamp,File,Line,ColumnRecommendation: The shared
Payloadbase is a good pattern already in place β the duplication is in having three different named wrapper types for essentially the same error concept. Consider whetherworkflow.WorkflowValidationError's extra fields (severity/category/location) could be folded into a single richervalidationerror.ValidationErrorconcrete type used everywhere, collapsing three types into one.Estimated effort: 3-4 hours (touches error-handling call sites in parser and workflow)
Cluster 2:
MCPServerConfigvariants β near duplicate (already partially addressed)Impact: Low β base type already factored out
Locations:
pkg/types/mcp.goβBaseMCPServerConfig(12 shared fields: Command, Args, Env, Type, Version, URL, Headers, Auth, Container, Entrypoint, EntrypointArgs, Mounts)pkg/parser/mcp.goβRegistryMCPServerConfigembedsBaseMCPServerConfig+ registry-specific fieldspkg/workflow/tools_types.goβMCPServerConfigembedsBaseMCPServerConfig+ workflow-specific fieldsRecommendation: No action needed β comments already explain this split exists "to eliminate duplication." Listed here for completeness since the near-duplicate name-matching rule caught it.
Cluster 3:
runtimeImportReferenceβ exact duplicateImpact: Medium β identical concept, independently reimplemented
Locations:
pkg/parser/frontmatter_hash.goβ{path string, startLine int, endLine int}pkg/workflow/runtime_import_validation.goβ{importPath string, startLine int, endLine int}Recommendation: Both represent a parsed
{{#runtime-import ...}}macro occurrence. Move to a sharedpkg/types(or similar) package as a singleRuntimeImportReferencetype.Estimated effort: 1-2 hours
Cluster 4:
graderManifestEntryβ exact name, partial field overlapImpact: Medium β reader/writer schema split, undocumented
Locations:
pkg/cli/audit_report_graders.goβ 5 fields (ID, Name, Unit, Direction, Threshold)pkg/workflow/compiler_yaml_graders.goβ 14 fields (adds Description, Source, Enabled, Max, Min, Digest, Run, Inline, Config, ...)Recommendation:
cli's version looks like a read-side subset ofworkflow's write-side struct (~36% field overlap). Either document explicitly that this is an intentional reader/writer split, or share one type with optional fields.Estimated effort: 2 hours (verify no JSON tag mismatches before unifying)
Cluster 5:
LogParserβ exact name, unrelated kindsImpact: Low β naming collision, not structural duplication
Locations:
pkg/workflow/agentic_engine.goβLogParserinterfacepkg/cli/log_aggregation.goβLogParser[T MutableLogAnalysis]generic function typeRecommendation: No structural fix needed, but the shared name across an interface and a generic func type in sibling packages is a readability trap for grep-based navigation. Consider renaming one (e.g.
cli.LogParseFunc[T]).Cluster 6: bare
Configβ coincidental exact-name matchImpact: Low β unrelated domains (log-drain tuning vs. linter config)
Locations:
pkg/agentdrain/types.goβConfig(Depth, SimThreshold, MaxChildren, ...)pkg/linters/internal/resourcetracker/resourcetracker.goβConfig[K comparable](static-analysis linter config)Recommendation: No action β flagged only because of the exact-name clustering rule; zero field overlap, unrelated purposes.
Untyped Usages
Summary Statistics
interface{}usages: 2 (effectively migrated away from; rest of 79 raw grep hits are test fixtures/analyzer testdata, excluded)anyusages (Go-syntax): several thousand;map[string]anyalone = 3,262 occurrences / 516 filesCategory 1:
interface{}/anyin function parametersImpact: High in a few hot spots, acceptable elsewhere (generic utility packages)
pkg/workflow/tools_parser.go:112βfunc NewTools(toolsMap map[string]any) *Tools, the YAMLβtyped boundary for all tool configs, followed by 50+ guarded.(string)/.(bool)/.(int)assertions through the file (e.g. lines 183, 213, 236-416). Suggested fix: give eachXToolConfigits ownUnmarshalYAML, soparseXTooltakes a typed value instead ofany+ manual assertions.pkg/workflow/compiler_pre_activation_job.go:690,721,749,819,857,881,897,921β same map-of-any YAML boundary pattern (validatePreActivationJobConfig,extractOnSteps, ...); aFrontmatterOnstruct (partially modeled already infrontmatter_types.go) could replace several of these.pkg/cli/mcp_tools_privileged.go:468βnormalizeAuditRunInput(input any, fieldName string)switches onnil/string/float64/int/int64. Suggested fix: atype RunIDValuewith customUnmarshalJSONaccepting string-or-number, used as the field type directly (see struct-fields section) β moves validation from runtime to deserialization time.pkg/workflow/service_ports.go:185βparsePortSpec(spec any)switches onint/string; genuine multi-shape GitHub Actions value, acceptable.pkg/typeutil/convert.goandpkg/typeutil/lookup.goβ documented generic JSON/YAML coercion helpers (ParseIntValue,ConvertToInt,LookupMap, ...). Acceptable as-is β this is the intended generic-utility package.Category 2:
interface{}/anyin return typesImpact: Mostly acceptable (genuinely heterogeneous JSON/schema values)
pkg/parser/schema_suggestions.goβgenerateExampleFromSchema(schema map[string]any) anyand siblings building JSON-schema example values; inherently heterogeneous, acceptable, though the externally-called top-level function could return a smallExampleValuesum type with a.JSON()method.pkg/importinpututil/import_input.go:17,42βResolvePathValue(...) (any, bool)feeding straight intoFormatResolvedValue(value any). Candidate: a small discriminatedResolvedValuestruct instead ofany+ a second formatter function.pkg/workqueue/codec.goβ hand-rolled strict JSON decoder returningany; genuine generic JSON AST, same shape asencoding/json. Acceptable.pkg/cli/jsonworkflow_to_markdown.go:373,394β produce GH Actionson:value (string|list|map by spec). Acceptable, mirrors the struct-field case below.Category 3:
interface{}/anyin struct fieldsImpact: High for one clear candidate, low/acceptable for the rest
pkg/cli/mcp_tools_privileged.go:454-455βauditArgs.RunID any,RunIDOrURL any, paired directly with thenormalizeAuditRunInputtype-switch above. AStringOrNumbertype withUnmarshalJSONwould also make the generated MCP tool JSON schema clearer (currently falls back to an untyped schema per the comment atpkg/cli/mcp_schema.go:121).pkg/cli/audit_report.go:169βCreatedItemReport.ID any(holds a numeric GitHub ID or occasionally a string SHA). Worth checking call sites β likely reducible tostringif producers already stringify, or a smallEntityIDwrapper otherwise.pkg/cli/gateway_logs_types.go:221,232βrpcRequestPayload.ID any/rpcResponsePayload.ID anyβ this is the correct shape per the JSON-RPC 2.0 spec (id is string|number|null); a namedtype JSONRPCID anywould self-document intent but isn't urgent.pkg/workflow/workflow_file.go:10,16(On any,RunsOn any),pkg/cli/list_workflows_command.go:29(On any),pkg/cli/runner_guard_activation_gate.go:19(Needs any) β all mirror GitHub Actions' own multi-shape YAML fields (string or list or map). Acceptable;Needs anylacks a doc comment unlike its siblingPorts anyfield β worth adding one for consistency.pkg/workflow/frontmatter_types.go:356,480,485βEngine any,Evals any,Graders any, all already comment-documented as multi-shape YAML. Acceptable, but a sum-type with customUnmarshalYAMLwould let call sites inengine_config_parser.go/evals_config.gostop re-deriving the shape via scattered assertions.pkg/workflow/tools_types.goβPrivateToPublicFlows anyand threeRaw anyfields (taggedyaml:"-"), well-commented deferred-parsing pattern. Acceptable.pkg/parser/import_observability.go:18βHeaders any; likely alwaysmap[string]stringin practice β worth confirming via call sites and narrowing.Category 4:
map[string]interface{}/anyImpact: Architectural pattern, not scattered bugs
map[string]interface{}is fully gone (0 occurrences).map[string]anyat 3,262 occurrences / 516 files is overwhelmingly the YAML/JSON frontmatter-parsing boundary:pkg/workflow/tools_parser.goβ heaviest concentration (50+ assertions in one file); best lever is per-toolUnmarshalYAML, not chasing individual sites.pkg/cli/codemod_*.go(20+ files) β one-shot migration scripts doing generic YAML tree surgery. Acceptable by nature.pkg/workqueue/*.goβ JSON-Schema-validated queue protocol (commitSchemavalidates shape before typed decode). Schema-enforced, not loosely typed. Acceptable.pkg/typeutil/lookup.goβ intentional generic accessor library. Acceptable.Category 5: Untyped Constants
Impact: Medium β cheap fix, and the codebase already proves the convention works
pkg/constants/constants.goandjob_constants.goalready define semantic wrapper types (CommandPrefix,FilePath,LineLength,JobName,StepID,MCPServerID) and apply them to most constants in those same files β these are the untyped stragglers that break that established local convention:pkg/constants/constants.go:329βconst DefaultMCPGatewayPayloadSizeThreshold = 524288β untyped int byte-size threshold; type asint64(or a semanticByteSize), matching theLineLength-style convention a few lines away.pkg/constants/constants.go:432βconst DefaultMaxRuns = 500β untyped int; compare toDefaultMaxAICredits int64 = 1000nearby, which is typed.pkg/constants/constants.go:429βconst DefaultMaxDailyAICredits = "5000"β highest-risk finding: a numeric value stored as an untyped string constant; any arithmetic use needs a parse step with no compile-time guarantee it's numeric. Recommendint64.pkg/constants/constants.go:435βconst DefaultMaxTurnCacheMisses = 5β untyped int count/threshold.pkg/constants/constants.go:738βconst MaxSymlinkDepth = 5β untyped int recursion/traversal limit.pkg/constants/job_constants.go:309-310βDefaultRateLimitMax = 5,DefaultRateLimitWindow = 60β untyped ints; the window is documented as minutes in a comment but nothing enforces the unit. Recommendtime.Durationfor the window (60 * time.Minute).pkg/workqueue/codec.go:26-27βmaxParseBytes = 80 << 20,maxLineBytes = 8 << 20β untyped int byte-size limits compared againstlen([]byte); add explicitint/int64type annotations.pkg/cli/token_usage_types.go:289-291βmodelMismatchReasonTokenUsageMissing/ModelNotObserved/SubagentFailedβ three untyped string constants forming an enum family with no named type tying them together. Recommendtype ModelMismatchReason string.pkg/constants/constants.go:315βconst AWFDefaultLogLevel = "info"β untyped string, enum-like log-level domain; candidate for aLogLeveltype if other level literals ("debug","warn") appear elsewhere (worth a follow-up grep before committing).Recommendations, Prioritized
Priority 1 β Untyped constants (cheapest, highest clarity win)
Apply the existing
pkg/constantssemantic-type convention to the stragglers:DefaultMaxRuns,DefaultMaxDailyAICredits(stringβint64 β the riskiest one),DefaultMaxTurnCacheMisses,MaxSymlinkDepth,DefaultRateLimitMax/Window, and the twoworkqueue/codec.gobyte-size constants. Also introducetype ModelMismatchReason stringfor the threetoken_usage_types.goenum constants.Estimated effort: 2-3 hours Β· Impact: Medium (clarity, near-zero risk)
Priority 2 β
RunID/RunIDOrURLtyped union (best interface{}/any fix)Replace
anywith aStringOrNumber-style type (customUnmarshalJSON) forauditArgs.RunID/RunIDOrURLinpkg/cli/mcp_tools_privileged.go, removing the runtime type-switch innormalizeAuditRunInputand producing a clearer generated MCP schema.Estimated effort: 2-3 hours Β· Impact: Medium-High (public MCP tool input surface)
Priority 3 β
tools_parser.goper-toolUnmarshalYAMLShrink the
map[string]any+ manual-assertion boundary inpkg/workflow/tools_parser.goby giving eachXToolConfigits ownUnmarshalYAML. Highest density of assertions in the repo; biggest single-file win, but also the biggest effort.Estimated effort: 1-2 days Β· Impact: High (maintainability of the densest parsing file in the codebase)
Priority 4 β Consolidate
ValidationErrorfamilyFold
workflow.WorkflowValidationError's extra fields into a single richervalidationerrortype, or clearly document why three wrapper types exist around the sharedPayload.Estimated effort: 3-4 hours Β· Impact: Medium (touches error-handling call sites across two packages)
Lower priority / cleanup
runtimeImportReferenceto a shared location (1-2 hours).graderManifestEntryreader/writer split (2 hours).cli.LogParser[T]to avoid the naming collision withworkflow.LogParser(30 min).Implementation Checklist
pkg/constants/constants.goandjob_constants.go(Priority 1)DefaultMaxDailyAICreditsstringβtyped-numeric constant specifically (highest risk item)ModelMismatchReasontype inpkg/cli/token_usage_types.goRunID/RunIDOrURLunion for the MCP audit tool input (Priority 2)UnmarshalYAMLrefactor forpkg/workflow/tools_parser.go(Priority 3)ValidationErrorfamily (Priority 4)runtimeImportReferenceto a shared packagegraderManifestEntrycli/workflow splitAnalysis Metadata
get_symbols_overview) + targeted grep + manual call-site reviewAll reactions