[typist] 🔤 Typist - Go Type Consistency Analysis #63418
Closed
Replies: 1 comment
|
This discussion was automatically closed because it expired on 2026-09-26T11:38:29.380Z.
|
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
Executive Summary
I scanned roughly 1,245 non-test top-level type declarations and ~4,700
interface{}/anyusages across the ~1,380 non-test.gofiles underpkg/. The good news: the codebase is disciplined about idiomatic Go — almost allanyusage is either generic type parameters ([T any]) or genuinely dynamic YAML/JSON frontmatter parsing backed by a dedicatedpkg/typeutilhelper package, and the oldinterface{}syntax barely exists outside linter test fixtures. The codebase also already knows how to deduplicate well:pkg/types.BaseMCPServerConfigis a clean example of extracting a shared base type.That said, a handful of real opportunities stand out. Two structs —
graderManifestEntryandruntimeImportReference— are copy-pasted verbatim (including one shared regex) acrosspkg/parser/pkg/cli/pkg/workflow, and the cli copy ofgraderManifestEntryhas silently drifted out of sync with the newer workflow-side fields. Separately, a "string or number from JSON/YAML" type-switch (case float64: ... case string: ...) is hand-rolled in at least 39 files instead of using one shared type. Fixing these two things alone would remove a meaningful amount of near-duplicate boilerplate without touching the parts of the codebase that are intentionally dynamic.Full Analysis Report
Duplicated Type Definitions
Summary Statistics
Cluster 1:
graderManifestEntry— diverging near-duplicateType: Near duplicate
Occurrences: 2 (+1 test file)
Impact: High — same JSON manifest, silently diverging schemas
Locations:
pkg/cli/audit_report_graders.go:67— reader sidepkg/workflow/compiler_yaml_graders.go:86— writer sideRecommendation: Move the canonical struct into a shared package (e.g.
pkg/types) so the reader can't silently drop fields (Enabled,Source,Max,Min,Digest,Run,Inline,Config) the writer emits. Estimated effort: 1-2 hours.Cluster 2:
runtimeImportReference— struct + regex copy-pastedType: Near duplicate
Occurrences: 2 real usage sites (+1 additional consumer file)
Impact: High — parsing logic for the same macro syntax duplicated in two packages
Locations:
pkg/parser/frontmatter_hash.go:572pkg/workflow/runtime_import_validation.go:38pkg/workflow/compiler_yaml_prompt.go(consumer)Both files also define byte-for-byte identical regexes for the
{{#runtime-import ...}}macro. If the macro syntax ever changes, it must be changed in two places by hand.Recommendation: Extract the regex + struct into a shared helper package. Estimated effort: 1-2 hours.
Cluster 3:
HostedWebPolicyvsAWFHostedWebPolicyType: Near duplicate
Occurrences: 78 references across 12 files
Impact: Medium — recently added feature (commit #63212, "Add hosted web domain policies to workflow frontmatter")
Same 4 conceptual fields, different tag/naming conventions. This mirrors the codebase's existing "Definition → Runtime" converter pattern (see
EngineCapabilitiesDefinition.ToRuntimeCapabilities()).Recommendation: Confirm an explicit converter function exists between the two (as with
EngineCapabilities); if not, add one so field additions can't drift silently. Estimated effort: 1 hour.Cluster 4: "Wire"/"WireSchema" triple-duplication for custom
MarshalJSONtypesType: Near duplicate (systemic pattern)
Occurrences: At least 5 triplets in
pkg/cliImpact: Medium — same field set hand-maintained 3x per type
Examples:
domainAnalysisWire/domainAnalysisWireSchema,mcpServerHealthDetailWire,mcpFailureSummaryWire. A comment inpkg/cli/mcp_schema.goalready acknowledges this: "These wire types mirror custom MarshalJSON output, which reflection cannot infer."Recommendation: Generate the schema struct directly from the
MarshalJSONanonymous struct via a shared reflection-friendly builder instead of hand-maintaining a third copy per type. Estimated effort: 3-4 hours (affects 5 types).Cluster 5: Definition/Runtime twin structs (systemic, mostly intentional)
EngineCapabilities/EngineCapabilitiesDefinition,EngineNetworkConfig/EngineNetworkDefinition,EngineAuthConfig/AuthDefinition— an intentional declarative-YAML-to-runtime pattern with explicitToRuntimeX()converters. Not a bug, but ~6 pairs must be kept in sync by hand.Recommendation: Document the pattern once and add a test that fails if a
*Definitionstruct's field set diverges from its runtime counterpart without updating the converter.Cluster 6:
DomainAnalysis/RedactedDomainsAnalysis/WorkflowDomainsSummary/DomainBuckets/DomainBreakdownFive separate "domain summary" structs across
pkg/clibuilt for different reports (access logs, redaction audit, domain inventory, generic bucketing, outcome breakdown). Not confirmed field-identical, but worth a follow-up pass to see if 2-3 can share a common base struct.Noted false positives (search hygiene, no action needed)
Cluster(pkg/agentdrain) vsRunCluster(pkg/cli/audit_cross_run_clusters.go) — unrelated domains (log-template mining vs. run-grouping), fields don't overlap.PolicyRule/PolicyCondition(pkg/intent) vsFirewallPolicyRule(pkg/cli/firewall_policy.go) — agent-governance policy vs. network-firewall ACL rules, only vocabulary overlaps.safeOutputTargetConfigvsSafeOutputTargetConfig(pkg/workflow) — case-only name collision between structurally unrelated types; low-risk but a grep/readability hazard worth a rename.Untyped Usages
Summary Statistics
interface{}/anyhits scanned: ~4,703interface{}(legacy syntax) in production code: effectively 0 (confined to linter testdata)Category 1:
map[string]anywith a fully-known schemaImpact: High
Example:
pkg/workflow/trigger_parser.go:29Built by hand in ~15
parse*Triggerfunctions (parsePushTrigger,parsePullRequestTrigger,parseIssueTrigger,parseManualTrigger, ...), always with the same small set of well-known keys (branches,tags,paths,workflow_dispatch,inputs...). The schema is fully known per event type at compile time, somap[string]anyhides key typos the compiler could otherwise catch.Suggested fix: Per-event concrete filter structs (
PushFilters{Branches, BranchesIgnore, Tags, TagsIgnore, Paths, PathsIgnore []string},PullRequestFilters{...}), unified via a small interface with aToYAMLMap()method.Category 2: Duplicated
Target anyfield across 5 config structsImpact: High
Location:
pkg/workflow/safe_outputs_azure_devops.go(lines 40, 52, 57, 64, 70)All 5 are consumed by the same helper,
addAzureDevOpsTarget(confirmed: 6 references in this file), which just nil-checks and forwards. The value is always a work-item ID (string/int) or a GH Actions expression string — a bounded, knowable domain.Suggested fix:
Category 3: Repeated "string or number" type-switch — 39 files
Impact: High (confirmed via grep:
case float64:appears in 39 non-test files)Location (representative):
pkg/cli/mcp_tools_privileged.go:458with a
normalizeAuditRunInput(input any, fieldName string)type switch onnil/string/float64/int/int64. The same 4-case pattern recurs nearly verbatim acrosscheckout_config_parser.go,service_ports.go,runtime_overrides.go,step_types.go,role_checks.go,engine.go,mcp_github_config.go, and ~32 other files.Suggested fix: A single shared
StringOrNumbertype inpkg/typeutilimplementingjson.Unmarshalerwith a.String()accessor, replacing ~39 ad-hoc copies of the same type switch.Estimated effort: 4-6 hours (mechanical, high test coverage already exists per call site).
Category 4: Discriminated-union fields left as
anydespite documented shapesImpact: Medium
Location:
pkg/workflow/frontmatter_types.go:252,280,289Both fields have doc comments enumerating exactly 2-3 accepted concrete shapes — a closed discriminated union, not truly dynamic data. The sibling field
WorkflowFilePermissionsin the very same file already demonstrates the intended pattern (customUnmarshalYAMLaccepting scalar-or-map).Suggested fix:
OTLPHeadersandOTLPEndpointValuetypes with customUnmarshalJSON, following the existingWorkflowFilePermissionspattern forOn/RunsOninpkg/workflow/workflow_file.gotoo.Category 5: Untyped constants
Impact: Medium
Example 1:
pkg/constants/job_constants.go:310Every other timeout-like constant in
pkg/constants/constants.gois a typedtime.Duration(e.g.DefaultToolTimeout = 60 * time.Second), butDefaultRateLimitWindow's unit ("minutes") is only documented in a comment — a future caller could easily misread it as seconds. Consumed atpkg/workflow/role_checks.go:84.Suggested fix:
const DefaultRateLimitWindow = 60 * time.Minute.Example 2:
pkg/workflow/model_alias_validation.go:53The same closed 3-value enum recurs as bare strings in
pkg/cli/audit_report.go:86(Priority) andpkg/cli/audit_cross_run_clusters.go:45(Severity), documented only by comments.Suggested fix:
Verified non-issues (excluded after sampling)
map[string]anyinpkg/parser(2,780+ occurrences, concentrated in frontmatter/schema files) backs genuinely dynamic YAML/JSON parsing, already served by a purpose-builtpkg/typeutilpackage (ParseIntValue,ConvertToInt,LookupMap,LookupString). This is idiomatic for the domain and was intentionally excluded.🎯 What Should We Do About This?
Priority 1: High-impact, low-effort — shared "string or number" type
Recommendation: Add
pkg/typeutil.StringOrNumber(or similar) and migrate the ~39 files with hand-rolledcase float64:type switches.Estimated effort: 4-6 hours
Impact: High — removes the single most-duplicated pattern found in this scan
Priority 2: High — fix drifting
graderManifestEntryand sharedruntimeImportReferenceRecommendation: Extract both into shared types (
pkg/typesor a new small package) so the cli reader can't silently drop fields the workflow writer emits, and the runtime-import regex/struct isn't hand-copied.Estimated effort: 2-4 hours combined
Impact: High — closes an active schema-drift bug risk
Priority 3: Medium — Azure DevOps
Target anyand OTLPHeaders/EndpointfieldsRecommendation: Replace with small typed unions (
AzureDevOpsWorkItemTarget,OTLPHeaders,OTLPEndpointValue) using customUnmarshalYAML/UnmarshalJSON, following theWorkflowFilePermissionspattern already established in the same file.Estimated effort: 3-4 hours
Impact: Medium — removes type assertions at well-defined boundaries
Priority 4: Low — constant typing and naming hygiene
Recommendation: Type
DefaultRateLimitWindowastime.Duration; introduce a sharedEffortLevel(or similar) enum type for the low/medium/high pattern; renamesafeOutputTargetConfigto avoid the case-only collision withSafeOutputTargetConfig.Estimated effort: 1-2 hours
Impact: Low-medium — clarity and search-hygiene
Implementation Checklist
pkg/typeutil.StringOrNumbertype, migrate the highest-traffic call sites first (mcp_tools_privileged.go,checkout_config_parser.go,service_ports.go)graderManifestEntryto a shared package; add a field-parity test between writer and readerruntimeImportReferencestruct + regex used bypkg/parserandpkg/workflowHostedWebPolicy→AWFHostedWebPolicyconverterTargetfield and OTLPHeaders/Endpointfields as documented unionsDefaultRateLimitWindowastime.Duration; add sharedEffortLevelenumsafeOutputTargetConfigto remove the case-only collisionAnalysis Metadata
pkg/only)PATH— so semantic reference tracing fell back togrep-based cross-referencing)References:
All reactions