[typist] Typist: Go type consistency findings in pkg/ (duplicate types + untyped usages) #59705
Closed
Replies: 1 comment
|
This discussion was automatically closed because it expired on 2026-09-10T11:36:20.561Z.
|
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,354 non-test Go files under
pkg/(1,195 exported/unexported struct definitions plus hundreds oftype X string|int|...declarations) using Serena's semantic search and pattern matching. The codebase is in good shape overall — most of the ~1,200 struct names are unique, and the two "duplicate wasm/native" pairs I found (ProgressBar,SpinnerWrapperinpkg/console) are legitimate(go/redacted):build js || wasmplatform variants, not real duplication.That said, I found two genuine duplicated-logic clusters worth consolidating, a repeated
any-typed field pattern across five Azure DevOps safe-output configs (and three memory-tool configs) that could be a single shared type, and a concrete case inpkg/constantswhere a semantic string type already exists but a related constant doesn't use it. None of these are urgent bugs, but each is a small, well-scoped refactor with a clear payoff: less schema drift risk, less copy-pasted parsing logic, and more consistent use of the typing conventions the codebase already established for itself.Full Analysis Report
Duplicated Type Definitions
Summary Statistics
pkg/, excluding_test.go)testdatafixtures)graderManifestEntry)runtimeImportReference)Cluster 1:
graderManifestEntry— schema drift between writer and readerType: Near duplicate (structurally divergent)
Occurrences: 2
Impact: Medium-High — one struct is the writer's full schema, the other is a reader's narrower view of the same JSON document; if the writer adds/renames a field the reader silently keeps parsing the old subset
Locations:
pkg/workflow/compiler_yaml_graders.go:86— the manifest writerpkg/cli/audit_report_graders.go:67— the manifest readerRecommendation:
pkg/workflowalready owns the manifest format, sopkg/clicould import the exported version instead of re-declaring a private copypkg/cliintentionally only needs 5 fields, rename its local type (e.g.graderManifestSummary) so the name collision doesn't imply "this is the same shape" when it isn'tSource,Enabled,Config) become visible to the CLI reader instead of being silently droppedCluster 2:
runtimeImportReference— parsing logic duplicated across packagesType: Near duplicate (same concept, independently re-implemented)
Occurrences: 2
Impact: Medium — two regex-based extractors for the same
{{#runtime-import ...}}/{{#import ...}}macro syntax, which can drift out of syncLocations:
pkg/workflow/runtime_import_validation.go:38-42pkg/parser/frontmatter_hash.go:572-576The regex literal is copy-pasted verbatim between the two files; the struct differs only in the field name (
importPathvspath).Recommendation:
pkg/parserlooks like the natural home sincepkg/workflowalready imports it elsewhere), and havepkg/workflowcall the shared helperFalse positives (ruled out, no action needed)
ProgressBar(pkg/console/progress.govsprogress_wasm.go) andSpinnerWrapper(pkg/console/spinner.govsspinner_wasm.go) are(go/redacted):build js || wasmvs native pairs — this is the standard Go cross-compilation pattern, not duplication.Worker,fakeOS,statecollisions all live underpkg/linters/*/testdata/src/...— these are golden-file fixtures for custom static-analysis linters, isolated by design, not production duplication.Untyped Usages
Summary Statistics
map[string]interface{}/map[string]anyoccurrences: 887 across 289 filesany: dozens (see patterns below)Most of the 887
map[string]anyoccurrences are legitimate:pkg/workflowandpkg/parsercompile arbitrary YAML frontmatter, so an untyped map is the correct representation until a field is validated and copied into a typed struct. I did not flag these as a bulk cleanup target — that would fight the codebase's actual parsing model. Instead, below are the specific patterns that stood out as avoidable.Category 1: Repeated
Target anyfield across 5 Azure DevOps configsImpact: Medium — same untyped field, copy-pasted, in 5 different config structs in one file
Location:
pkg/workflow/safe_outputs_azure_devops.goEach
Targetaccepts a work item ID (int) or a lookup expression (string) — the same "accepts int-or-string-or-expression" shape used elsewhere in the safe-outputs configs.Suggested fix:
UnmarshalYAMLthat validates "int or string" instead ofanyCategory 2: Repeated
Raw anysingle-field wrapper across 3 memory-tool configsImpact: Low-Medium — identical shape, three names
Location:
pkg/workflow/tools_types.go:480-496Suggested fix: since all three are structurally
struct { Raw any }and each is immediately handed off to its own dedicated parser file, consider a single generic-ish wrapper (type RawToolConfig struct { Raw any }) embedded by all three, or at minimum a shared comment/doc convention so the pattern reads as intentional rather than copy-pasted.Category 3: Polymorphic ID fields (
anyfor int-or-string)Impact: Low — narrow, well-contained, but recurring
Examples:
pkg/cli/mcp_tools_privileged.go:422-423—RunID any,RunIDOrURL any("Accepts run ID or run/job URL... String or number.")pkg/cli/gateway_logs_types.go:179,190—ID any(JSON-RPC style ID, string or number per spec)Suggested fix: a small custom type (e.g.
type FlexibleID anywith a documented contract, or a realUnmarshalJSONthat normalizes to a string) would let call sites stop re-deriving "is this a string or a number?" logic ad hoc. Lower priority than Categories 1-2 since these are single-field, well-isolated, and the JSON-RPC one arguably has to stay untyped to match the wire spec.Category 4: Untyped constant alongside an existing sibling semantic type
Impact: Low, but a clean and safe one-line fix
Location:
pkg/constants/constants.goSuggested fix:
pkg/constantsalready has the convention established (WorkflowID,Filename,FilePath,ArtifactName,URL,DocURL, etc. are all typed) — this is one of the few constants in the file that didn't get the same treatment, most likely just an oversight when it was added.go vet/go buildwill catch it immediately since Go string-typed constants are not implicitly assignable from untyped string vars without conversion, but usages assigning to astring-typed variable are unaffected since Go permits assigning a defined string type tostringonly via conversion — check callers first)Refactoring Recommendations
Priority 1: Consolidate
graderManifestEntry(Cluster 1)Steps:
pkg/workflow, since it writes the manifest)pkg/cliimport itgo build ./...and the grader/audit test suite to confirm no field-name assumptions brokeEstimated effort: 1-2 hours · Impact: Medium-High
Priority 2: Share the runtime-import regex/struct (Cluster 2)
Steps:
extractRuntimeImportReferences(workflow) and itsfrontmatter_hash.gocounterpart line-by-line for behavioral differencespkg/parser, export whatpkg/workflowneedspkg/workflow/runtime_import_validation.goEstimated effort: 2-3 hours · Impact: Medium
Priority 3: Name the repeated
anyfield patterns (Categories 1-2)Steps:
WorkItemTarget(or similar) and switch the 5 Azure DevOps configs to use itRaw anymemory-tool configs together (or a shared embed)Estimated effort: 1.5 hours · Impact: Low-Medium, mostly documentation value
Priority 4: Fix the
FirewallAuditArtifactNametyping gap (Category 4)Steps:
ArtifactNamego build ./...Estimated effort: 15 minutes · Impact: Low, but essentially free
Implementation Checklist
graderManifestEntrywriter/reader schemasruntimeImportReferenceextraction logic betweenpkg/workflowandpkg/parserTarget anyAzure DevOps config fieldsRaw anymemory-tool config wrappersFirewallAuditArtifactNameasArtifactNameinpkg/constants/constants.gogo build ./...and the affected package's test suite after each changeAnalysis Metadata
.gofiles underpkg/: 1,354map[string]interface{}/anyoccurrences surveyed: 887 (across 289 files; not individually cataloged — see note above)go,typescript,bashlanguage servers) +grep-based pattern matching overpkg/**/*.go, manual verification of each flagged cluster by reading the actual struct/function bodiesWarning
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