[typist] Typist: Go Type Consistency Analysis - duplicated types and untyped usages in pkg/ #49984
Closed
Replies: 1 comment
|
This discussion was automatically closed because it expired on 2026-08-04T12:44:41.876Z.
|
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 all 1,179 non-test
.gofiles underpkg/(roughly 1,089 top-level type declarations) for two things: types that are effectively defined more than once, and places whereinterface{}/anyor untyped constants are hiding a concrete type that already exists in practice. The good news first: this codebase already has the right instincts — it defines named sum types likeRunsOnValue,TemplatableBool,TemplatableBoolOrInt,ActionMode, andLockSchemaVersionfor exactly this class of problem. The findings below are mostly places where that same pattern just hasn't been applied yet, plus a handful of genuine duplicate structs worth consolidating.The most interesting finding: there's a recurring "legacy vs AWF" config duplication pattern —
BoundedQueriesConfig/AWFBoundedQueriesConfigandSRTNetworkConfig/AWFNetworkConfigmodel the same sandbox concepts twice, with hand-written field-by-field copy functions bridging them. That's the highest-value consolidation target. On the untyped side,FrontmatterConfig.RunsOn,SafeJobConfig.RunsOn,Imports/Include, andReportFailureAsIssueare all documented (in comments!) as accepting 2-3 known shapes but are typedany— each is a straightforward port of the existingRunsOnValue/TemplatableBoolpattern. Fixing these removes several manual type assertions and closes off a class of "silent typo" bugs (e.g.CompilerError.Typeaccepting arbitrary strings like"error"/"warning"/"info"with no shared constants anywhere).Full Analysis Report
Duplicated Type Definitions
Summary Statistics
Cluster 1:
RepositoryFeatures(exact duplicate)Impact: Medium — same struct declared twice purely to satisfy build tags
Locations:
pkg/workflow/repository_features_validation.go:73(//go:build !js && !wasm)pkg/workflow/repository_features_validation_wasm.go:31(//go:build js || wasm)Identical field-for-field struct (
HasDiscussions bool,HasIssues bool) declared twice so the WASM stub can compile against the same field names.Recommendation: Move the struct to a shared, non-build-tagged file (e.g.
repository_features_types.go) and keep only thevalidateRepositoryFeaturesbehavior split behind build tags. Effort: <1 hour.Cluster 2:
BoundedQueriesConfigvsAWFBoundedQueriesConfig(semantic duplicate)Impact: High — part of a systemic "legacy vs AWF" split
Locations:
pkg/workflow/tools_types.go:410(BoundedQueryPrivateRepo{Repo, Sensitivity})pkg/workflow/tools_types.go:450(BoundedQueriesConfig)pkg/workflow/awf_config.go:192(AWFBoundedQueryPrivateRepo{Repo, Sensitivity})pkg/workflow/awf_config.go:224(AWFBoundedQueriesConfig)AWFBoundedQueryPrivateRepois a verbatim copy ofBoundedQueryPrivateRepo;AWFBoundedQueriesConfigmirrorsBoundedQueriesConfig'sRuntime/MemoryLimit/Interpreter/PrivateReposfields exactly.extractBoundedQueriesConfigmanually copies every field one-by-one with no transformation.Recommendation: Since the AWF struct carries no extra fields, marshal
AWFBoundedQueriesConfigdirectly fromBoundedQueriesConfig(or embed it) instead of maintaining a hand-written 1:1 copy function. Effort: 2-3 hours.Cluster 3:
SRTNetworkConfigvsAWFNetworkConfig(semantic duplicate)Impact: High — same "legacy vs AWF" pattern as Cluster 2, suggesting it's systemic
Locations:
pkg/workflow/sandbox.go:147(SRTNetworkConfig)pkg/workflow/awf_config.go:247(AWFNetworkConfig)Both model firewall/network egress policy for a workflow sandbox:
AllowedDomains/BlockedDomains(SRT) vsAllowDomains/BlockDomains(AWF) are the same concept with only naming differences. SRT adds unix-socket options; AWF adds isolation/topology-attach options.Recommendation: Consolidate SRT and AWF network/filesystem config schemas into one canonical internal representation with format-specific marshalers, rather than two independently-evolving schemas for the same sandboxing concern. Effort: 4-6 hours (larger, cross-cutting).
Cluster 4:
AuditComparisonIntDeltavsAuditComparisonStringDelta(near duplicate)Impact: Medium — textbook case for Go generics
Locations:
pkg/cli/audit_comparison.go:45,pkg/cli/audit_comparison.go:51Identical shape except value type:
{Before T, After T, Changed bool}forT=intandT=string, defined back-to-back in the same file.Recommendation: Replace both with
type Delta[T any] struct { Before, After T; Changed bool }— generics are already used elsewhere in this module. Effort: <1 hour.Cluster 5: Delta-value representation inconsistency (near duplicate, same package)
Impact: Medium — same concept modeled two different ways in one package
Locations:
pkg/cli/audit_comparison.go:45,51,pkg/cli/audit_diff.go:276,346,366audit_comparison.gomodels "value that changed between two points" as{Before, After, Changed}, butaudit_diff.go'sTokenUsageDiff,RunMetricsDiff, andGitHubRateLimitDiffmodel the exact same concept as inline field triples repeated 4-10+ times per struct (e.g.Run1InputTokens/Run2InputTokens/InputTokensChange), never reusing a shared delta type.Recommendation: Introduce one shared
Delta[T]type inpkg/cli(see Cluster 4) and use it consistently in both files. Effort: 2-3 hours.Cluster 6:
FirewallLogEntry/AccessLogEntry/AuditLogEntry(semantic duplicate)Impact: Medium
Locations:
pkg/cli/firewall_log.go:118,pkg/cli/access_log.go:21,pkg/cli/firewall_policy.go:50All three represent a proxied network request log line with overlapping fields (
Timestamp,Method,Status,URLcommon to all three;Decisionshared betweenFirewallLogEntryandAuditLogEntry). They differ mainly in incidental details — plain-text parser structs (Squid access log vs firewall text log) vs a JSON-tagged struct for the Squid audit log format.Recommendation: Extract a common
LogRequestCore{Timestamp, Method, Status, URL}embeddable type, keeping format-specific fields (Decision,UserAgent,Hierarchy,Schema, etc.) in each source-specific struct. Effort: 2-3 hours.Untyped Usages
Summary Statistics (approximate — see notes)
interface{}/anylocations (all forms): ~3,400, dominated bymap[string]any(2,399 hits) and[]any(449 hits)const X = ...plus ~122const()blocks)interface{}: nearly extinct — only 2 real (non-testdata) occurrencesCategory 1:
interface{}/anyin function parameters — Medium impactrenderRunsOnSnippet(value any) string—pkg/workflow/runs_on_snippet.go:19Called from 5 sites; body only ever branches on
map[string]anyor "everything else" (string/[]any) — same shape the codebase already solved withRunsOnValue(repo_config.go:69).→ Define a
RunsOnSpecsum type (string | []string | map[string]any) withUnmarshalJSON.ParseCheckoutConfigs(raw any) ([]*CheckoutConfig, error)—pkg/workflow/checkout_config_parser.go:17Only caller (
frontmatter_parsing.go:68-73) already asserts.(bool)first; body only ever handlesmap[string]anyor[]any— all 3 shapes are documented inFrontmatterConfig.Checkout's own comment.→ Introduce a
CheckoutSpectype resolving directly to[]*CheckoutConfig.getFeatureValueCaseInsensitive(features map[string]any, flagName string) (any, bool)—pkg/workflow/sandbox_validation.go:497Single caller immediately does
.(string)and errors if it isn't. Theanyreturn is never used for anything but a string.→ Return
(string, bool, error)directly; move the type check inside.Category 2:
interface{}/anyin struct fields — High impactFrontmatterConfig.RunsOn any/RunsOnSlim any—pkg/workflow/frontmatter_types.go:356-357Comment: "Supports string, array, or object". Sibling
SafeOutputsConfig.RunsOnalready solves this viaUnmarshalJSON+renderRunsOnSnippet, normalizing to a plainstring.→ Give
FrontmatterConfigthe sameUnmarshalJSONtreatment.SafeJobConfig.RunsOn any,RawPermissions any—pkg/workflow/safe_jobs.go:21,27Immediately type-switched into exactly two shapes (
string/[]any) right after parsing (safe_jobs.go:236-238).→ Reuse the
RunsOnSpec/RunsOnValuesum type instead ofany+ inline type switch.FrontmatterConfig.Imports any,Include any—pkg/workflow/frontmatter_types.go:369,371Comments already document "Can be string or array" — identical problem already solved elsewhere via
RunsOnValue.→ Add a
StringOrStringSlice []stringtype with 2-branchUnmarshalJSON.SafeOutputsConfig.ReportFailureAsIssue any—pkg/workflow/safe_outputs_config_types.go:109Comment enumerates exactly 3 shapes (bool / templatable expression string / string list). The package already has
TemplatableBool/TemplatableBoolOrIntfor this exact ambiguity pattern.→ Add a
TemplatableBoolOrStringListtype following the existing pattern.AwInfo.RunID any,RunNumber any—pkg/cli/logs_models.go:337-338No call site anywhere in
pkg/cliactually reads these fields; siblingAwContext.RunIDis already a plainstring.→ Declare as
stringto match, or flag as currently-dead data.Category 3:
interface{}/anyin map/slice values — Medium impactFrontmatterConfig.MCPServers map[string]any—pkg/workflow/frontmatter_types.go:330Comment marks it "Legacy field, use Tools instead" — superseded by the already strongly-typed
*ToolsConfigfield two lines above.→ Move to a dedicated legacy/compat shim struct rather than keeping it on the primary config type.
parseGitHubAllowedToolsAndLimits(allowedSetting any)—pkg/workflow/mcp_github_config.go:266Per-item
map[string]any{name, max-calls}lookup could be a small struct.→ Keep the outer
any(YAML boundary unavoidable) but add anAllowedToolEntry{Name string; MaxCalls int}for the inner shape.Category 4: Untyped constants (numeric) — Medium impact
workflowCompletionWaitTimeoutMinutes = 6 * 60—pkg/cli/run_workflow_execution.go:26Passed as a bare
intinto atimeoutMinutes intparameter; the very next constant in the same file correctly usestime.Duration, highlighting the inconsistency.→
const workflowCompletionWaitTimeout = 6 * time.Hourand change the parameter totime.Duration.Five
*TimeoutMinutesconstants —pkg/cli/mcp_tools_privileged.go:21,28,34,40,44Unit ("minutes") lives only in the name/comment, not the type.
→ Type directly as
time.Duration(e.g.1 * time.Minute).defaultSafeOutputsTimeoutMinutes = 45—pkg/workflow/compiler_safe_outputs_job.go:539Same "Minutes-suffix-as-unit" pattern repeated across the codebase.
→ Same
time.Durationfix.Category 5: Untyped constants (string, enum-like) — High impact
CompilerError.Type string—pkg/console/console_types.go:13(values:"error","warning","info")Raw literals duplicated ad hoc across
pkg/parser/schema_compiler.go:466,pkg/parser/import_error.go:135,pkg/cli/grant.go:276, plus a switch inconsole.go:97. No shared constants exist anywhere — a typo like"eror"would silently fall through to a switch's default case.→
type ErrorSeverity stringwith named constants, used at every construction site.FormField.Type string—pkg/console/console_types.go:46(values:"input","password","confirm","select")A test already asserts against this hardcoded value list, implying it's fixed but not encoded as a type.
→
type FormFieldType stringwith named constants.ExperimentsStorage string—pkg/workflow/workflow_data.go:187, constants atpkg/workflow/compiler_experiments.go:25,29(values:"cache","repo")Exactly 2 valid values, already gated by a
switch/case— precisely the enum shape the codebase solves elsewhere with named types (ActionMode,LockSchemaVersion), but left untyped here.→
type ExperimentsStorageMode stringwithExperimentsStorageCache/ExperimentsStorageRepoconstants.🎯 What Should We Do About This?
Priority 1: Consolidate the "legacy vs AWF" sandbox config duplication
Recommendation: Merge
BoundedQueriesConfig/AWFBoundedQueriesConfigandSRTNetworkConfig/AWFNetworkConfiginto single canonical types with format-specific marshaling instead of hand-written field-copy functions.Steps: 1) Confirm the AWF variants carry no fields beyond the legacy type. 2) Replace copy functions with direct marshal/embed. 3) Run compiler + sandbox tests.
Estimated effort: 6-9 hours combined. Impact: High — removes a whole class of copy-paste-drift risk in sandbox security config.
Priority 2: Apply the existing
RunsOnValue/TemplatableBoolpattern to remaininganyfieldsRecommendation:
FrontmatterConfig.RunsOn/RunsOnSlim/Imports/Include,SafeJobConfig.RunsOn, andSafeOutputsConfig.ReportFailureAsIssueall have comments documenting 2-3 known shapes — port each to a small sum-type withUnmarshalJSON/UnmarshalYAML, exactly likeRunsOnValueandTemplatableBoolOrIntalready do elsewhere in this package.Steps: 1) Add
RunsOnSpec/StringOrStringSlice/TemplatableBoolOrStringListtypes. 2) Swap field types and remove now-redundant inline type assertions/switches. 3) Run frontmatter parsing tests.Estimated effort: 4-6 hours. Impact: High — removes manual type assertions at every call site.
Priority 3: Turn string-literal enums into named types
Recommendation:
CompilerError.Type,FormField.Type, andExperimentsStorageare all fixed small value sets currently passed around as barestring. Add named types + constants (matchingActionMode/LockSchemaVersionprecedent) and update construction/comparison sites.Steps: 1) Define the type + constants next to the struct. 2) Update literal construction sites to use the constants. 3) Update switch/comparison sites (no behavior change). 4) Run affected package tests.
Estimated effort: 3-4 hours. Impact: Medium — prevents silent typo bugs in string comparisons.
Priority 4 (smaller/opportunistic):
RepositoryFeaturesstruct duplication across build-tag files (<1 hour).AuditComparisonIntDelta/AuditComparisonStringDeltawith a genericDelta[T], and reuse it inaudit_diff.go's repeatedRun1X/Run2X/XChangefield triples (2-3 hours).*TimeoutMinutesint constants astime.Duration(1-2 hours).Implementation Checklist
BoundedQueriesConfig/AWFBoundedQueriesConfigandSRTNetworkConfig/AWFNetworkConfigRunsOn/Imports/Include/ReportFailureAsIssueand drop theanyfieldsErrorSeverity,FormFieldType,ExperimentsStorageModenamed typesRepositoryFeaturesbuild-tag structDelta[T]inpkg/cliand apply toaudit_comparison.go/audit_diff.go*TimeoutMinutesconstants astime.DurationAnalysis Metadata
pkg/interface{}/any, ~500 untyped constants (majority legitimate; ~20 flagged as real opportunities)All reactions