Repository navigation
[typist] Typist: Go type consistency analysis — 2 duplicate type clusters, 13 untyped-usage opportunities #68393
Closed
Replies: 1 comment
|
This discussion has been marked as outdated by Typist - Go Type Analysis. A newer discussion is available at Discussion #68675. |
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,532 non-test
.gofiles underpkg/(1,534 top-level type definitions: struct/interface bodies, aliases, and named base types) looking for duplicated types and weakly-typed code. The codebase is in good shape overall — most apparent "duplicates" turned out to be intentional platform stub pairs ((go/redacted):build js || wasm) or a correct adapter pattern (per-scanner*Findingstructs normalized into a sharedscanfindings.Finding). After filtering those out, 2 real near-duplicate clusters survived, both caused by the same root issue: a producer package and a consumer package each independently re-declaring the shape of an on-disk artifact (a macro reference regex match, and a JSON manifest entry) instead of sharing one type.On the untyped-usage side,
interface{}itself is essentially unused (2 hits) since the codebase already follows the modernanyconvention — butanyappears roughly 2,800 times, and a good chunk of those are genuine opportunities: atools map[string]anysignature repeated across 80 occurrences in 36 files inpkg/workflow(every engine renderer re-implements its owntools["x"].(map[string]any)lookups), twoID anyJSON fields whose real domain (number-or-string) is already documented in a comment, and about a dozen untyped numeric/string constants representing ports, memory sizes, and closed-set modes that are currently indistinguishable from any other bareint/stringat compile time. None of this is urgent, but thetools map[string]anypattern in particular is worth a shared type given how many call sites re-derive the same assertions.Full Analysis Report
Duplicated Type Definitions
Summary Statistics
Three other name collisions were investigated and ruled out as intentional, not duplication:
ValidationError— interface (pkg/validationerror/validationerror.go:49) vs. struct (pkg/parser/validation_error.go:12) that deliberately embeds the sharedvalidationerror.Payloadand implements the interface — this is the project's existing de-duplication mechanism.SpinnerWrapper(pkg/console/spinner.go:92/spinner_wasm.go:12) andProgressBar(pkg/console/progress.go:31/progress_wasm.go:9) —(go/redacted):build js || wasmvs.!js && !wasmplatform stub pairs.Cluster 1:
runtimeImportReferencenear-duplicateType: Near duplicate
Occurrences: 2
Impact: Medium
Locations:
pkg/parser/frontmatter_hash.go:572—type runtimeImportReference struct { path string; startLine int; endLine int }pkg/workflow/runtime_import_validation.go:38—type runtimeImportReference struct { importPath string; startLine int; endLine int }Both files also independently embed a byte-for-byte identical regex literal to extract the reference:
(
pkg/parser/frontmatter_hash.go:579andpkg/workflow/runtime_import_validation.go:33), and each package has its ownextractRuntimeImportReferencesfunction building it. A third file,pkg/workflow/compiler_yaml_prompt.go:407, consumes thepkg/workflowvariant.Recommendation:
pkg/parser, since it already owns frontmatter/macro parsing) as the canonical owner ofruntimeImportReferenceand its extraction regex/function.pkg/workflowimport and reuse it instead of maintaining its own copy with a differently-named field (pathvsimportPath).Cluster 2:
graderManifestEntryproducer/consumer splitType: Near duplicate (field subset)
Occurrences: 2
Impact: Medium
Locations:
pkg/workflow/compiler_yaml_graders.go:84— write-side struct the compiler marshals into theGH_AW_GRADER_MANIFESTJSON consumed by the JS runtime:ID, Name, Description, Source string; Enabled bool; Unit, Direction string; Threshold, Max, Min *float64; Digest, Run, Inline string; Config map[string]any.pkg/cli/audit_report_graders.go:67— read-side struct used only to re-parse that same manifest for the audit report:ID string; Name string; Unit string; Direction string; Threshold *float64(json tagsid/name/unit/direction/thresholdmatch the producer exactly).Recommendation:
graderManifestEntryshape into a small shared package (e.g.pkg/gradersorpkg/types) that bothpkg/workflow(writer) andpkg/cli(reader) import.pkg/clireader can keep using only the fields it displays, but sharing the struct removes the risk that a json-tag rename in the writer silently stops populating the reader.pkg/workflow/graders_workflow_integration_test.go:78already round-tripsgraderManifestEntry— this would also confirm cross-package parity).Untyped Usages
Summary Statistics
interface{}usages: ~2 (codebase already standardized onany)anyusages: ~2,800Category 1:
anyin function parameters / struct fieldsImpact: High — repeated type assertions, no compile-time shape guarantee
tools map[string]anyacross the engine layerpkg/workflow/agentic_engine.go:253(RenderMCPConfig(yaml *strings.Builder, tools map[string]any, mcpTools []string, workflowData *WorkflowData) error), repeated with the same shape inclaude_mcp.go,codex_mcp.go,gemini_mcp.go,copilot_mcp.go,pi_mcp.go,agy_mcp.go,behavior_defined_engine.go,tools.go,tools_validation.go, and more.pkg/workflow, all keyed by a fixed, known set of tool names (github,bash,edit,web-fetch,playwright, custom MCP server names) and accessed viatools["x"].(map[string]any).tools.GitHub(),tools.Bash()) replace scatteredtools["x"].(map[string]any)assertions, and documents intent at every one of the 36 call sites.ID anyfields with a documented two-way domainpkg/cli/audit_report.go:169,pkg/cli/gateway_logs_types.go:221and:232,pkg/cli/mcp_tools_privileged.go:456.pkg/cli/audit_report.go's own comment onisUnconstrainedSchema/fixCreatedItemIDSchemadocuments the field as either a numeric database ID (int64) or a string identifier (Jira/Linear) — the JSON Schema generator is patched after the fact specifically to narrow this field.gateway_logs_types.go'sID anyis a JSON-RPC id, which per spec is number-or-string only.Category 2: Untyped constants
Impact: Medium — unit/enum confusion
Network ports
pkg/constants/constants.go:169—DefaultMCPGatewayPort,DefaultMCPServerPort,MinNetworkPort,MaxNetworkPort,CodexLLMGatewayPort, etc. (bareint, explicitly re-cast withint(...)at several call sites inmcp_setup_gateway.go,mcp_gateway_config.go,awf_command_builder.go).type Port uint16with the constants typed accordingly, plus aValid()method — removes the need for the existing re-casts and prevents a timeout/retry-countintfrom being passed where a port is expected.Resource sizes mixing units
pkg/constants/constants.go:251—DefaultCloudHypervisorMemoryMiB,DefaultNVXMemoryMiB,DefaultNVXMemoryMaxBytes(MiB and bytes, unmarked, sitting next to each other —DefaultNVXMemoryMiB(512) andDefaultNVXMemoryMaxBytes(536,870,912) encode the same quantity in two different units).type MiB int64,type Bytes int64to make the unit mismatch impossible to introduce silently.Closed-set string "enums"
pkg/cli/mcp_registry_types.go:108(StatusActive/StatusInactive),:114(ArgumentTypePositional/ArgumentTypeNamed),pkg/workflow/behavior_defined_engine.go:20(behaviorSecretStrategy*,behaviorProviderEnvMode*,behaviorConfigMergeJSON).type ServerStatus string,type ArgumentType string,type SecretStrategy string, etc. — each is a 2-4 value closed set currently compared with plain==/!=against a barestring, so a YAML/API typo compiles cleanly and only fails at runtime.Category 3: Recurring untyped recursive-walk pattern (informational)
Not a direct replacement candidate —
Options map[string]anyonEngineDefinition(pkg/workflow/mcp_registry.go:592) is genuinely open-ended per-engine config — but theswitch v := v.(type) { case map[string]any: ...; case []any: ... }walk that processes it is duplicated verbatim across at least 6 files (behavior_defined_engine.go,pkg/parser/yaml_import.go,schema_suggestions.go,tools_merger.go,schema_deprecation.go,pkg/workqueue/identity.go). A sharedYAMLValuesum-type alias plus one shared walk helper would cut the duplicated assertion logic without changing the underlying dynamism.Refactoring Recommendations
Priority 1: Medium — Consolidate the two near-duplicate types
Recommendation: Share
runtimeImportReferenceandgraderManifestEntrybetween their producer/consumer packages instead of maintaining parallel copies.Steps:
pkg/parserfor the import-reference regex/struct; a new small shared package for the grader manifest entry).make test-unitto confirm no behavior change.Estimated effort: 2-4 hours combined
Impact: Medium — removes two spots where the two packages could silently drift.
Priority 2: High-value, low-risk — Type the
tools map[string]anysignatureRecommendation: Introduce a
ToolsConfignamed type (even a plain alias) for the 80-occurrencetools map[string]anypattern inpkg/workflow.Steps:
type ToolsConfig map[string]any(or a struct) inpkg/workflow.map[string]anyis the underlying type.Estimated effort: 3-5 hours (mechanical signature change) + incremental follow-up for accessors
Impact: High — this is the single most duplicated untyped pattern in the codebase by call-site count.
Priority 3: Low — Type the closed-set constants
Recommendation: Add named types to the port, resource-size, and string-enum constant groups in
pkg/constants/constants.go,pkg/cli/mcp_registry_types.go, andpkg/workflow/behavior_defined_engine.go.Steps:
Port,MiB/Bytes,ServerStatus,ArgumentType,SecretStrategytypes.make fmtandmake test-unit.Estimated effort: 2-3 hours
Impact: Low-medium — mostly prevents future unit/typo mistakes rather than fixing an active bug.
Implementation Checklist
runtimeImportReferenceinto one owning packagegraderManifestEntrybetweenpkg/workflow(writer) andpkg/cli(reader)ToolsConfigtype for the 80-occurrencetools map[string]anypatternItemID/JSON-RPC id wrapper for the twoID anyfieldsmake test-unitafter each changeAnalysis Metadata
pkg/)anyhits, triaged for genuine impact)References: §38390786472
All reactions