[typist] Typist - Go Type Consistency Analysis #39549
Closed
Replies: 1 comment
-
|
This discussion has been marked as outdated by Typist - Go Type Analysis. A newer discussion is available at Discussion #39788. |
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.
-
🔤 Typist - Go Type Consistency Analysis
Analysis of repository: github/gh-aw — 917 non-test
.gofiles underpkg/Executive Summary
Good news first: this codebase has strong type discipline. I scanned all 917 non-test Go files under
pkg/(≈941 type definitions) looking for the same type defined in two places and forinterface{}/anythat could be tightened up. I found no problematic cross-package duplicate type definitions — the only repeated type names are intentional//go:build js || wasmvariants (same package, mutually-exclusive build tags) and lintertestdatafixtures, neither of which should be consolidated. The team also already maintains sharedpkg/types,pkg/typeutil, andpkg/jsonutilhomes, which is exactly where consolidation belongs.On the untyped side, there's a lot of
any(≈2,095 non-test lines touchmap[string]any, plus ~1,600 type assertions), but the overwhelming majority is legitimate: YAML/JSON frontmatter parsing, codemod transformations over dynamic trees, and genuine union types likeon:/runs-on:that GitHub Actions allows to be a string or array or object — shapes Go can't express withoutany. So rather than a sweeping "type everything" recommendation, I've isolated a handful of small, high-signal opportunities that are worth the effort: a recurring "string-or-number ID" pattern, repeatedHeaders any, and one near-duplicate struct pair. Fixing these centralizes a few ad-hoc type assertions without fighting the YAML-driven design.Full Analysis Report
Duplicated Type Definitions
Summary Statistics
Cluster 1: Build-tag variants — NOT a problem (informational)
Type: Exact name, intentional
Impact: None — do not consolidate
These three type names each appear twice, but every pair is a
wasm/non-wasmimplementation of the same type in the same package, guarded by mutually-exclusive build constraints. This is the correct Go pattern for platform-specific code.Locations:
SpinnerWrapper:pkg/console/spinner.go:92(//go:build !js && !wasm) vspkg/console/spinner_wasm.go:10(//go:build js || wasm)ProgressBar:pkg/console/progress.go:31vspkg/console/progress_wasm.go:7RepositoryFeatures:pkg/workflow/repository_features_validation.go:58vspkg/workflow/repository_features_validation_wasm.go:5Recommendation: None. Leave as-is. (Listed only so a future run doesn't re-flag them.)
Cluster 2:
WorkflowListItem⊂WorkflowStatus— near-duplicate (low/medium)Type: Near duplicate (structural subset)
Occurrences: 2
Impact: Low–Medium —
WorkflowListItemis essentially a prefix ofWorkflowStatusRecommendation: Optional. Extract the shared identity fields (
Workflow,EngineID,Compiled,Labels,On) into an embeddedWorkflowIdentitystruct that both embed. Same-package, so this is a quick, safe refactor.Untyped Usages
Summary Statistics
map[string]any/map[string]interface{}(non-test lines): ≈2,095 — mostly justified (YAML/JSON frontmatter + codemod trees)x.(T)(non-test lines): ≈1,599 — concentrated in the same dynamic-tree codeany/interface{}: ~350+, concentrated inpkg/parser(import_field_extractor.go37,mcp.go25) andpkg/clicodemods — mostly justifiedOn,RunsOn,RunsOnSlim,ContinueOnError,Checkout,Imports,Include,DeduplicateByTitle, ...): pervasive — justified; Go has no sum typesOpportunity 1: Recurring "string-or-number ID" typed as
any(medium value, low effort)Impact: Medium — the same polymorphic-ID idea is re-implemented ad hoc in several places, each forcing a local type assertion / sprintf.
The GitHub API returns run/job IDs that may arrive as a JSON number or a string, so several structs declare them
any:Locations (8 fields across 5 files; the ID-shaped ones):
pkg/cli/mcp_tools_privileged.go:253—RunID any(“Accepts run ID or run/job URL... String or number.”)pkg/cli/gateway_logs_types.go:148—ID anypkg/cli/gateway_logs_types.go:159—ID anypkg/cli/logs_models.go:304—RunID anypkg/cli/logs_models.go:305—RunNumber anySuggested fix: Add one shared type in
pkg/types(which already exists) with custom JSON unmarshalling, and reuse it:Then
RunID FlexID,ID FlexID, etc. Each call site drops its.(string)/fmt.Sprintf("%v", id)dance.Opportunity 2: Repeated
Headers any(low/medium)Impact: Low–Medium — HTTP/MCP headers are modeled as
anyin 3 spots, then asserted on use.Locations:
pkg/parser/import_field_extractor.go:764—Headers anypkg/workflow/frontmatter_types.go:208—Headers anypkg/workflow/frontmatter_types.go:238—Headers anySuggested fix: If the source is consistently a string→string map, declare a named type and use it everywhere:
If headers can legitimately hold non-string values from raw YAML, leave as
any— verify before changing.Note: intentionally left alone
The following are
anyon purpose and should not be "fixed" without a sum-type need:On,RunsOn,RunsOnSlim,Checkout,Imports,Include,Engine,Endpoint,ContinueOnError,DeduplicateByTitle,Default,Ports,Raw, and themap[string]anycodemod/frontmatter trees. Each represents a YAML value with multiple legal shapes; the existing comments documenting those shapes are the right call.Refactoring Recommendations
Priority 1: Low-effort, real win — shared
FlexIDtypepkg/types/flexid.gowithUnmarshalJSON.anyfields (Opportunity 1) withFlexID.go test ./pkg/cli/....Effort: 2–3 h · Impact: Medium (centralizes a repeated pattern)
Priority 2: Optional cleanup —
WorkflowIdentityembedWorkflowListItemandWorkflowStatusto embed it.go test ./pkg/cli/....Effort: 1–2 h · Impact: Low–Medium (prevents drift)
Priority 3: Verify-then-type
Headerstype Headers map[string]stringand apply.Effort: 1–2 h · Impact: Low–Medium
Explicitly NOT recommended
map[string]anyfrontmatter/codemod trees to structs.on,runs-on, ...) — keepany+ doc comments.Implementation Checklist
pkg/types/FlexIDand migrate the 5 IDanyfieldsWorkflowIdentityshared embedtype Headers map[string]stringgo test ./...anyfields and dynamic frontmatter maps as-isAnalysis Metadata
map[string]anylines, ~350anysignatures, ≈1,599 assertionspkg/Beta Was this translation helpful? Give feedback.
All reactions