[typist] Typist: Go Type Consistency Analysis #53651
Closed
Replies: 2 comments
|
Smoke test comment: confirming discussion interaction works correctly. Automated validation from run 32134218478. Warning Firewall blocked 6 domainsThe following domains were blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "accounts.google.com"
- "android.clients.google.com"
- "clients2.google.com"
- "contentautofill.googleapis.com"
- "www.google.com"
- "www.gstatic.com"See Network Configuration for more information.
|
0 replies
|
This discussion has been marked as outdated by Typist - Go Type Analysis. A newer discussion is available at Discussion #53980. |
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 · scope:
pkg/**/*.go, excluding_test.goExecutive Summary
I scanned all 1,240 non-test
.gofiles underpkg/(roughly 1,023 type definitions sampled) for two things: types that are defined more than once, and code that leans oninterface{}/anyor bare untyped constants where a real type would catch bugs at compile time instead of at runtime.The good news: this codebase is already fairly disciplined — genuine
interface{}usage is essentially zero in real (non-fixture) code, and there's an existing convention (BaseSafeOutputConfig,AggregatedSummaryBase) for sharing fields across similar config/report types that just hasn't been applied everywhere yet. The most actionable finds are a handful of field-for-field duplicated structs inpkg/workflowandpkg/clithat drifted apart in small, risky ways (e.g. one copy uses*int, the sibling usesint), aNewConfig() anyfactory pattern repeated ~44 times that could be a marker interface instead, and severalany-typed struct fields whose "actual" types are fully pinned down by an adjacent type-switch a few lines away — meaning the type safety is already known, it's just not expressed in the type system.Full Analysis Report
Duplicated Type Definitions
Summary Statistics
Cluster 1:
ProgressBar/SpinnerWrapper/RepositoryFeatures(native vs. wasm)Type: Exact duplicate
Impact: Low — intentional build-tag split, but worth flagging
Locations:
pkg/console/progress.go:31vspkg/console/progress_wasm.go:9—type ProgressBar struct { ... }pkg/console/spinner.go:92vspkg/console/spinner_wasm.go:12—type SpinnerWrapper struct { ... }pkg/workflow/repository_features_validation.go:83vspkg/workflow/repository_features_validation_wasm.go:31—type RepositoryFeatures struct { ... }Recommendation: These are legitimate platform variants (native vs. WASM), not accidental duplication — no action needed beyond awareness. If it ever becomes a maintenance burden, a shared interface (e.g.
ProgressReporter) implemented by both variants would let the compiler verify the two stay API-compatible instead of relying on convention.Cluster 2:
BoundedQueriesConfigvsAWFBoundedQueriesConfigType: Near duplicate
Impact: High — same package, field-for-field parallel structs that have already drifted
Locations:
pkg/workflow/tools_types.go:410—BoundedQueriesConfig(yaml-tagged, frontmatter shape)pkg/workflow/tools_types.go:450—BoundedQueryPrivateRepopkg/workflow/awf_config.go:191—AWFBoundedQueriesConfig(json-tagged, AWF-config-file shape)pkg/workflow/awf_config.go:223—AWFBoundedQueryPrivateRepoPrivateRepos,Runtime,Timeout,MemoryLimit,Interpreter, andMaxInvocationsappear in both, and the two*PrivateRepostructs are identical except for tag style. They've already drifted:Timeoutis*intin one andintin the other.Recommendation: Extract a shared base struct (following the existing
BaseMCPServerConfigpattern already used elsewhere) for the common fields, with each variant embedding it and adding only its serialization-specific concerns.Estimated effort: 2-3 hours
Cluster 3:
CloseEntity*vsUpdateEntity*helper frameworksType: Semantic duplicate
Impact: Medium — two independently-built generic dispatch frameworks solving the same problem
Locations:
pkg/workflow/close_entity_helpers.go:80—CloseEntityConfigpkg/workflow/close_entity_helpers.go:91—CloseEntityJobParamspkg/workflow/update_entity_helpers.go:93—UpdateEntityConfigpkg/workflow/update_entity_helpers.go:120—UpdateEntityJobParamsConfigKey,JobName,StepName, andPermissionsFuncare duplicated field-for-field between the two*JobParamsstructs, and each verb (close,update) reinvents its own entity-type enum and job-builder pair rather than sharing one generic helper.Recommendation: Factor out a shared
EntityJobParamsbase (generic over the entity-type enum) that bothCloseEntity*andUpdateEntity*embed.Estimated effort: 3-4 hours
Cluster 4:
MCPToolUsageDatavsMCPToolUsageSummaryType: Near duplicate
Impact: Medium
Locations:
pkg/cli/audit_report.go:165—MCPToolUsageDatapkg/cli/logs_models.go:216—MCPToolUsageSummaryBoth reuse the exact same element types (
MCPToolSummary,MCPServerStats,MCPToolCall,DifcFilteredEvent) but the wrapper struct itself is declared twice, differing only by field order,console:tags, and one extraGuardPolicySummaryfield.Recommendation: Have
MCPToolUsageDataembedMCPToolUsageSummary(or vice versa) instead of re-declaring the shared fields.Estimated effort: 1-2 hours
Cluster 5:
RepoMemoryEntryvsCacheMemoryEntryType: Near duplicate
Impact: Medium
Locations:
pkg/workflow/repo_memory.go:47—RepoMemoryEntrypkg/workflow/cache_config.go:106—CacheMemoryEntrypkg/workflow/comment_memory.go:8—CommentMemoryConfig(looser third member of the same family)ID,Description,AllowedExtensions, andValidation *MemoryValidationConfigare byte-for-byte identical (name, type, yaml tag) in both structs — only the storage-specific fields differ.Recommendation: Extract a shared
MemoryEntryBasestruct for the four common fields.Estimated effort: 1-2 hours
Cluster 6:
GitHubRateLimitUsagevsGitHubRateLimitDiffType: Near duplicate
Impact: Medium
Locations:
pkg/cli/logs_github_rate_limit_usage.go:46—GitHubRateLimitResourceUsagepkg/cli/logs_github_rate_limit_usage.go:57—GitHubRateLimitUsagepkg/cli/audit_diff.go:366—GitHubRateLimitDiffGitHubRateLimitDiff's own doc comment says it's "populated from...GitHubRateLimitUsage" — yet it re-declaresTotalRequestsMade/CoreConsumed/CoreRemaining/CoreLimitasRun1X/Run2Xpairs instead of embedding twoGitHubRateLimitUsagevalues.Recommendation:
type GitHubRateLimitDiff struct { Run1, Run2 GitHubRateLimitUsage }removes the duplicated field names entirely.Estimated effort: 1-2 hours
Cluster 7: MCP server health/stats structs (4-way)
Type: Semantic duplicate
Impact: Medium
Locations:
pkg/cli/audit_cross_run.go:81—MCPServerCrossRunHealthpkg/cli/audit_expanded.go:76—MCPServerHealthpkg/cli/audit_expanded.go:90—MCPServerHealthDetailpkg/cli/audit_report.go:202—MCPServerStatsFour separate structs all model "server name + call/request count + error count + error rate," each with its own field names (
TotalCallsvsToolCallsvsRequestCountvsToolCallCount;TotalErrorsvsErrorCount) and no shared base — unlikeAggregatedSummaryBase(pkg/cli/logs_models.go:172), which the codebase already uses to deduplicate the analogousMissingToolSummary/MissingDataSummary/MCPFailureSummarysituation.Recommendation: Apply the same
AggregatedSummaryBasepattern here; standardize field names across the four structs.Estimated effort: 3-4 hours
Cluster 8:
Create*Configsafe-output familyType: Near duplicate
Impact: Low-Medium — likely recurs across ~15 sibling files
Locations:
pkg/workflow/create_issue.go:12—CreateIssuesConfigpkg/workflow/create_discussion.go:14—CreateDiscussionsConfigEven though both already embed
BaseSafeOutputConfigandSafeOutputAllowedLabelsConfig, six additional fields (TitlePrefix,TargetRepoSlug,AllowedRepos,CloseOlderKey,Expires,Footer) are declared identically in both instead of being pulled into another shared embedded struct.Recommendation: Extend the existing base-config composition pattern with a
SafeOutputRepoTargetConfig-style struct for these six fields, and audit the otherCreate*/Update*/Assign*Configfiles for the same gap.Estimated effort: 3-5 hours (audit + refactor across the family)
Untyped Usages
Summary Statistics
interface{}usages in non-test code: ~1 (of 18 raw hits, 17 are inpkg/linters/*/testdatafixtures and out of scope; the remaining one is inside a comment, not code)anyusages (genuine type positions): ~3,277 raw matches, narrowed to the examples below after excluding prose/commentsThis codebase already avoids bare
interface{}almost entirely in real code — the opportunities are inany-typed fields whose real shape is already pinned down by adjacent code, and in bare numeric constants that share a "kind" (byte sizes, concurrency limits, durations) with siblings elsewhere.Category 1:
anyfields with a fully-known shapeImpact: Medium — the type safety already exists as a runtime type-switch; it's just not expressed in the type
pkg/workflow/service_ports.go:86—Ports anyinserviceContainerConfig.parsePortSpec(lines 185-222) type-switches on exactlyint | uint64 | int64 | float64 | string. Suggested: aPortSpecstring type with customUnmarshalYAMLnormalizing all five forms.pkg/cli/mcp_tools_privileged.go:413—RunID any,RunIDOrURL anyinauditArgs.normalizeAuditRunInput(lines 427-445) exhaustively switches onnil | string | float64 | int | int64. Suggested: aStringOrNumbertype with customUnmarshalJSON, which would let the switch innormalizeAuditRunInputbe deleted entirely.pkg/workflow/frontmatter_types.go:386—Imports any/Include any, doc-commented as "can be string or array." Suggested:type StringOrSlice []stringwith custom unmarshaling.pkg/types/input_definition.go:18—Default any, resolved via an exhaustive switch inGetDefaultAsString(lines 29-50) in the same file. Lower priority sinceTypealready records the declared kind.Category 2:
any-based factory patternImpact: Medium — repeated ~44 times
pkg/workflow/safe_output_handlers.go:18—NewConfig func() any, always returning*XxxConfigfor one of ~44 concrete config types (confirmed via reflection-based assignability checks insafe_outputs_permissions.go:220and a dedicatedsafe_outputs_fix_test.goassertion). Suggested: a marker interfaceSafeOutputConfigimplemented by every*XxxConfig, changing the field toNewConfig func() SafeOutputConfig— this catches factory/field mismatches at compile time instead of via a dedicated test.Category 3: Function parameters that re-derive the same
anyshapeImpact: Medium
pkg/parser/schema_triggers.go:97,141—IsLabelOnlyEvent(eventValue any)andIsNonConflictingCommandEvent(eventValue any)both independently asserteventValue.(map[string]any)and extract a"types"key. Suggested: a sharedextractEventTypes(eventValue any) ([]string, bool)helper so both callers work with[]stringinstead of re-deriving it fromany.Category 4: Dead/unused
anyparameterpkg/console/layout_wasm.go:16—LayoutEmphasisBox(content string, color any):coloris never referenced in the body, and no caller exists anywhere inpkg/. Either remove it, or give it a realconsole.Colortype if it's meant to mirror a non-wasm counterpart's signature.Category 5: Untyped constants sharing a "kind"
Impact: Medium — same semantic quantity (bytes, concurrency, duration) declared as bare, unit-less integers in multiple unrelated files
pkg/constants/constants.go:257(DefaultMCPGatewayPayloadSizeThreshold = 524288),pkg/cli/gateway_logs_types.go:15(maxScannerBufferSize),pkg/cli/workflows.go:30(workflowTitleScannerBufferSize),pkg/cli/import_url_fetcher.go:20(importURLMaxBytes). Suggested: atype Bytes int64semantic alias so a byte-size constant can't accidentally be mixed with a plain count.pkg/cli/shellcheck.go:81(shellcheckMaxConcurrency = 8),pkg/cli/mcp_subprocess_guardrail.go:13(maxActiveMCPChildProcesses = 4),pkg/cli/forecast_compute.go:28(defaultForecastDownloadConcurrency = 8).time.Durationconvention:pkg/cli/run_workflow_execution.go:25(workflowCompletionWaitTimeoutMinutes = 6 * 60) requires a manualtime.Duration(timeoutMinutes) * time.Minuteconversion at its call site (pkg/cli/pr_automerge.go:123), andpkg/workflow/compiler_safe_outputs_job.go:516(defaultSafeOutputsTimeoutMinutes = 45) has the same issue — whilepkg/constants/constants.go:325-334already does this correctly (e.g.DefaultAgenticWorkflowTimeout = 20 * time.Minute). Suggested: convert both totime.Durationconstants matching the existing convention.maxTagPeelDepth = 10is declared identically in bothpkg/workflow/action_resolver.go:238andpkg/cli/update_actions_release.go:389;maxDepth = 10is declared twice within the same file,pkg/parser/include_expander.go:64and:238.Refactoring Recommendations, Prioritized
Priority 1 — High-risk drift (do first)
BoundedQueriesConfig/AWFBoundedQueriesConfig): the*intvsintTimeoutmismatch is a real latent bug risk. ~2-3 hours.GitHubRateLimitDiff): embedGitHubRateLimitUsagetwice instead of re-declaring 4 field pairs. ~1-2 hours.Priority 2 — Established pattern, just needs extending
AggregatedSummaryBasepattern. ~3-4 hours.Create*Configfamily): extendBaseSafeOutputConfig-style composition; audit ~15 sibling files. ~3-5 hours.Bytes/ConcurrencyLimitsemantic types and convert minute-based timeouts totime.Duration, matchingpkg/constants/constants.go's existing convention. ~2-3 hours.Priority 3 — Compile-time safety wins
NewConfig() anyfactory, ~44 call sites): introduceSafeOutputConfigmarker interface. ~2-3 hours, removes a dedicated fix-test's reason for existing.anyfields with known shapes): introducePortSpec/StringOrNumber/StringOrSlicetypes at the 3-4 identified call sites. ~2-3 hours total.Priority 4 — Lower-impact cleanup
CloseEntity*/UpdateEntity*,MCPToolUsageData/Summary,RepoMemoryEntry/CacheMemoryEntry): straightforward base-struct extractions, ~1-4 hours each.Implementation Checklist
Timeout *intvsintdrift betweenBoundedQueriesConfigandAWFBoundedQueriesConfigGitHubRateLimitUsageinGitHubRateLimitDiffinstead of duplicating fieldsAggregatedSummaryBaseto the 4-way MCP server health/stats structsCreate*/Update*/Assign*ConfigfamilyBytes/ConcurrencyLimittypes; convert minute-based timeouts totime.DurationSafeOutputConfigmarker interface for theNewConfig() anyfactory patternanyfields whose shape is already known (PortSpec,StringOrNumber,StringOrSlice)CloseEntity*/UpdateEntity*,MCPToolUsageData/Summary,RepoMemoryEntry/CacheMemoryEntrycolor anyparameter inpkg/console/layout_wasm.goAnalysis Metadata
any-typed fields/params/returns and untyped constantsAll reactions