[typist] Typist: Go type consistency findings - duplicated types and untyped any/interface usage #63165
Closed
Replies: 1 comment
|
This discussion was automatically closed because it expired on 2026-09-25T11:45:54.309Z.
|
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 scanned 1,380 non-test
.gofiles underpkg/(1,335 type definitions) for duplicated type definitions and untyped (interface{}/any) usage. The good news: the codebase is disciplined about the oldinterface{}spelling — production code has essentially none of it (33 hits, and every one is either linter testdata or a doc comment). The interesting findings are elsewhere: a handful of small, unexported structs quietly reimplemented in two packages, and a much bigger pattern wheremap[string]anyis passed around as a stand-in for several genuinely-shaped config objects — including one case where a proper named type (MapToolConfig) already exists but is used in only 4 of the ~33 places that could use it.Nothing here is an emergency; most of the
anyusage is deliberate (JSON Schema / YAML frontmatter is inherently dynamic before validation, and several fields are commented as "too dynamic to type"). But a few clusters — the Azure DevOpsTarget anyfield repeated across 5 structs, and thetoolConfig/GuardPoliciesmaps threaded through 15+ files — are prime candidates for a shared named type that would remove a lot of repeated.(string)/.(map[string]any)assertions.Full Analysis Report
Duplicated Type Definitions
Summary Statistics
Cluster 1:
runtimeImportReference— Exact DuplicateOccurrences: 2
Impact: Low-medium — identical unexported struct, easy to consolidate
Locations:
pkg/parser/frontmatter_hash.go:572pkg/workflow/runtime_import_validation.go:38Recommendation: Both track "an import reference found at a line range in markdown/frontmatter". Move a single
RuntimeImportReference(exported) intopkg/types(which already centralizes shared cross-package types) and have bothparserandworkflowuse it.Estimated effort: 1 hour
Benefit: One definition to update if the shape needs to change; removes the risk of the two silently drifting.
Cluster 2:
graderManifestEntry— Near DuplicateOccurrences: 2
Impact: Medium — the
clicopy is a strict subset of theworkflowcopy, read from the same JSON manifest fileLocations:
pkg/cli/audit_report_graders.go:67— 5 fields (ID,Name,Unit,Direction,Threshold)pkg/workflow/compiler_yaml_graders.go:86— 13 fields (addsDescription,Source,Enabled,Max,Min,Digest,Run,Inline,Config map[string]any)Both carry
json:"..."tags for the same manifest document thatworkflowwrites andclilater reads back for reporting.Recommendation: Export the
workflowstruct (or move it topkg/types) and havecli/audit_report_graders.godecode into that same type instead of a hand-maintained subset. Today, adding a field inworkflowsilently does nothing incliuntil someone remembers to mirror it.Estimated effort: 1-2 hours
Benefit: Guarantees the CLI report never silently drops a field the compiler starts emitting.
Cluster 3: Log entry family — Semantic Duplicates
Occurrences: 4 (all in
pkg/cli)Impact: Low — different log sources, but heavily overlapping shape
pkg/cli/access_log.go—AccessLogEntry(Timestamp, Duration, ClientIP, Status, Size, Method, URL, User, Hierarchy, Type — allstring)pkg/cli/firewall_log.go—FirewallLogEntry(Timestamp, ClientIPPort, Domain, DestIPPort, Proto, Method, Status, Decision, URL, UserAgent)pkg/cli/gateway_logs_types.go—GatewayLogEntry(22 fields, superset-ish of the others plus MCP-specific fields)pkg/cli/firewall_policy.go—AuditLogEntry(Schema, Timestamp (float64, not string!), Client, Host, Dest, Method, Status (int), Decision, URL)Recommendation: Not a "merge them all" situation — these represent genuinely different log formats. But
Timestamp/Method/URL/Status/Decisionrecur in 3-4 of them with inconsistent types (stringvsfloat64timestamp,stringvsintstatus). Worth a sharedtype CommonLogFields struct{ Timestamp string; Method string; URL string }embed for the fields that really are the same concept, and picking one representation (probablytime.Time/RFC3339 string) for timestamps across all four.Estimated effort: 2-3 hours
Benefit: Removes a source of "which log has
Statusas a string vs an int" bugs when writing shared formatting/filtering helpers.Cluster 4:
SourcePosition-shaped types — Semantic DuplicatesOccurrences: 2
Impact: Low
pkg/console/console_types.go—ErrorPosition{ File string; Line int; Column int }pkg/parser/json_path_locator.go—JSONPathLocation{ Line int; Column int; Found bool }Recommendation: Both represent "a place in a source file". Low priority since the field sets aren't identical (
File/Founddiffer), but a sharedpkg/types.SourcePosition{Line, Column int}embedded in both would remove the duplicatedLine/Columnpair.Estimated effort: 30-60 minutes
Benefit: Minor clarity gain; do this only if touching either file for other reasons.
Untyped Usages
Summary Statistics
interface{}usages (production code): 0 — the only 33 hits repo-wide are in linter testdata fixtures (which intentionally test for old-styleinterface{}) and one doc comment (pkg/cli/mcp_schema.go:121). No action needed here; the codebase has already standardized onany.anyusages: hundreds of legitimate type-position uses after filtering out the English word "any" in comments/strings. The categories below are the ones worth typing more strongly.const X = "..."acrosspkg/— the ~80 hits are all path/URL/flag-name/env-var literals (e.g.GhAwRootDir,DefaultAlpineImage), which is the idiomatic Go pattern for this kind of value. No untyped-constant cluster worth flagging.Category 1:
toolConfig map[string]any— parameter type used instead of an existing named typeImpact: High — a named type (
MapToolConfig) already exists with helper methods, but is bypassed almost everywherepkg/workflow/mcp_config_types.go:70defines:But
MapToolConfigis referenced in only 4 files (mcp_config_custom.go,mcp_config_types.go, its test, and the README), while the rawmap[string]anyspelling for the same "MCP tool config" concept is used as a parameter type in 29 places across 14 files, including:pkg/parser/mcp.go(5 functions:buildExpandedLinearBuiltinConfig,buildLinearAllowedTools,appendBuiltinAllowedTools,applyGitHubBuiltinOverrides,parseMCPStdioTypeConfig, ...)pkg/workflow/codex_mcp.go,copilot_mcp.go,mcp_github_config.go,mcp_property_validation.go,mcp_manifest.go,args.gopkg/cli/mcp_add.go,mcp_secrets.goSuggested fix: Switch these signatures from
toolConfig map[string]anytotoolConfig MapToolConfig(it's a defined map type, so this is typically a zero-cost, mostly mechanical rename — call sites passing amap[string]anyliteral convert implicitly). Then replace ad hoc.(string)assertions in those functions with the existingGetStringhelper.Estimated effort: 3-4 hours (mechanical, but touches many files — good candidate for a single focused PR)
Benefit: One place to add new accessors (
GetBool,GetStringSlice, ...) instead of every call site re-deriving the assertion pattern; makes "this is MCP tool config" visible in every signature instead of "this is a map".Category 2:
Target anyrepeated across 5 Azure DevOps safe-output configsImpact: Medium — same field, same YAML tag, same (implicit) semantics, defined 5 times
pkg/workflow/safe_outputs_azure_devops.go:All five accept the same shape at parse time (a work-item ID as string or number, based on comments elsewhere in the file), but the type gives no hint of that to a reader or to whatever code eventually consumes
Target.Suggested fix: Introduce a small
AzureDevOpsTargettype (e.g. one that unmarshals string-or-int and exposes a normalized accessor), and either give it a single field name via an embeddedtype TargetableConfig struct { Target AzureDevOpsTarget }mixed into all 5 structs, or at minimum change all 5 fields to the same named type instead of bareany.Estimated effort: 2 hours
Benefit: Whatever code currently type-switches on
Target(string vs int) only has to do it once; new work-item config types get the right shape by construction.Category 3:
GuardPolicies map[string]any— same field name/shape, 4 different structsImpact: Medium
pkg/workflow/tools_types.go:513pkg/workflow/mcp_renderer_types.go:21(WriteSinkGuardPolicies),:92,:129pkg/workflow/mcp_config_types.go:54All in
pkg/workflow, all representing the same "per-tool guard policy" concept threaded through the MCP tool pipeline.Suggested fix: Define
type GuardPolicyMap map[string]any(or a more specific shape if the policy structure is known) once inmcp_config_types.goand reuse it for all 4 fields.Estimated effort: 1 hour
Benefit: Signals intent at each use site; a future refactor to a real
GuardPolicystruct only needs to change one type definition.Category 4:
RunsOn/RunsOnSlim any— polymorphic GitHub Actions runner valueImpact: Medium — appears in
pkg/workflow/workflow_file.go:16,frontmatter_types.go:409-410, and is unmarshaled twice (near-identically) inruns_on_unmarshal.go:16,38Comments already document the intended shape: "Supports string, array, or object GitHub Actions runner forms." That's a textbook case for a small custom type with its own
UnmarshalYAML/UnmarshalJSON, rather than leakinganyto every consumer and re-deriving the same type switch inruns_on_unmarshal.gotwice.Suggested fix:
type RunsOnValue struct { /* normalized form */ }with custom (un)marshaling, used consistently instead ofany+ ad hoc unmarshal shims.Estimated effort: 3-4 hours (touches the unmarshal logic, which needs care)
Benefit: Removes the duplicated unmarshal-shim logic and gives downstream code a real API instead of a type switch.
Category 5:
Frontmatter map[string]anyduplicated across packagesImpact: Low-medium
pkg/parser/frontmatter_content.go:20pkg/cli/mcp_workflow_scanner.go:21Both represent "parsed YAML frontmatter of a workflow file" as a bare map. Since
pkg/parseralready owns frontmatter parsing,pkg/clire-declaring its ownmap[string]any-typed field for the same concept is a missed reuse opportunity (whether or not it's the exact same struct).Suggested fix: Have the
clistruct reference/alias theparserpackage's frontmatter map type (or the eventual typed frontmatter struct, if one exists) instead of independently declaringmap[string]any.Estimated effort: 30-60 minutes
Benefit: One less place where "frontmatter" means something different by convention only.
Category 6:
Config map[string]anyfor grader/tool config, duplicated acrosscliandworkflowImpact: Low-medium
pkg/cli/mcp_registry.go:28,pkg/cli/graders_run.go:60pkg/workflow/compiler_yaml_graders.go:100,pkg/workflow/graders_config.go:79(commented "arbitrary config passed to grader at runtime")Same pattern as Category 3 but for grader/registry config rather than guard policies — reinforces that
map[string]anyis being used as the de facto "arbitrary config bag" type across the whole compiler/CLI boundary, with no shared name tying the occurrences together.Suggested fix: A single
type RawConfig = map[string]any(type alias, not a new named type, to avoid conversion churn) inpkg/types, imported everywhere this pattern appears — mostly a documentation/searchability win rather than a type-safety one, but makes grepping for "everywhere we still have an arbitrary config bag" trivial going forward.Estimated effort: 1 hour
Benefit: Cheap, low-risk first step; makes the remaining "should this be a real struct" work easier to scope later.
What Should We Do About This?
Prioritized by impact-to-effort ratio:
Priority 1 — Use the type you already wrote
Recommendation: Migrate the 29
toolConfig map[string]anyparameters to the existingMapToolConfigtype (Category 1).Steps:
map[string]anytoMapToolConfigin the 14 identified files..(string)assertions withMapToolConfig.GetString/new accessors as needed.go build ./...— conversions should be implicit; fix any explicitmap[string]any{...}literals that need a cast.Estimated effort: 3-4 hours | Impact: High
Priority 2 — Consolidate the two exact/near-duplicate structs
Recommendation: Merge
runtimeImportReference(Cluster 1) and aligngraderManifestEntry(Cluster 2) on a single shared type inpkg/types.Estimated effort: 2-3 hours combined | Impact: Medium — prevents silent drift between packages that read/write the same data.
Priority 3 — Name the repeated
anyfieldsRecommendation: Introduce
AzureDevOpsTarget,GuardPolicyMap, andRunsOnValuetypes (Categories 2-4).Estimated effort: 6-8 hours combined | Impact: Medium — mostly clarity and future-proofing, not urgent.
Priority 4 — Cheap documentation-level wins
Recommendation:
RawConfigalias (Category 6), sharedFrontmattertype (Category 5),SourcePosition(Cluster 4).Estimated effort: 2-3 hours combined | Impact: Low, good "while you're in the area" cleanups.
Implementation Checklist
toolConfig map[string]anycall sites toMapToolConfigruntimeImportReferenceinto a shared exported typecli.graderManifestEntrywithworkflow.graderManifestEntry(single source of truth)AzureDevOpsTargettype for the 5Target anyfieldsGuardPolicyMapfor the 4GuardPolicies/WriteSinkGuardPoliciesfieldsRunsOnValuetype with custom (un)marshalingFrontmattermap type betweenpkg/parserandpkg/cliRawConfigalias for the recurring "arbitrary config bag" patternAnalysis Metadata
pkg/)get_symbols_overview,find_referencing_symbols) + targeted pattern searchWarning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
api.anthropic.comTo allow these domains, add them to the
network.allowedlist in your workflow frontmatter:See Network Configuration for more information.
All reactions