[typist] π€ Typist β Go Type Consistency Analysis (pkg/) #47292
Closed
Replies: 1 comment
|
This discussion has been marked as outdated by Typist - Go Type Analysis. A newer discussion is available at Discussion #47564. |
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 swept every non-test
.gofile underpkg/β roughly 1,128 source files and ~740 exported type declarations β hunting for two things: types we've defined more than once, and places where we've reached forany/interface{}when a specific type would be safer. The good news up front: this codebase is already in remarkably good shape. There are essentially zero accidental name collisions β every same-name pair turned out to be a deliberatetype X = Yalias β and the team has clearly been doing the hard consolidation work already (shared base structs likeAnalysisBase,AggregatedSummaryBase,DiffEntryBase, and typed sidecar fields onFrontmatterConfig). So this isn't a rescue mission; it's a polish pass.That said, I did find 8 structural duplication clusters (distinct type names carrying identical or near-identical field sets) and a handful of genuinely improvable weak-typing spots. The single highest-value win is tiny: a few GitHub run-ID fields typed as
any(because the API returns string-or-number) could become one smallStringOrNumbertype and delete a scattering of runtime type assertions. Most of the ~2,290map[string]anyusages sit at the YAML/JSON boundary and are correctly left generic β I am not recommending a bulk rewrite there. Fixing the handful of real items below buys cleaner code and removes a little copy-paste drift risk, with almost no correctness exposure.Full Analysis Report
Duplicated Type Definitions
Summary Statistics
_test.go,testdata/, and_wasm.gobuild-tag variants)type X = Yaliases)Cluster 1: Run-analysis carrier structs
Type: Near duplicate β’ Occurrences: 3 β’ Impact: High (largest copy-paste block in
pkg/cli)Locations:
pkg/cli/logs_models.go:97βProcessedRunpkg/cli/logs_models.go:213βRunSummarypkg/cli/logs_models.go:239βDownloadResultAll three share the same ~16-field analysis core (
Run, AwContext, TaskDomain, BehaviorFingerprint, AgenticAssessments, AccessAnalysis, FirewallAnalysis, RedactedDomainsAnalysis, MissingTools, MissingData, Noops, MCPFailures, MCPToolUsage, TokenUsage, GitHubRateLimitUsage, JobDetails). They differ only in tails:ProcessedRunaddsPolicyAnalysis;RunSummaryaddsPolicyAnalysis+Metrics+ persistence fields;DownloadResultaddsMetrics+ result fields and omitsPolicyAnalysis.Recommendation: Extract an embedded
RunAnalysisbase holding the shared core and embed it in all three.RunSummary)Cluster 2: Identical safe-output config structs
Type: Exact duplicate β’ Occurrences: 6 (two sub-groups) β’ Impact: High, easy win
Sub-group 2a β embed set
BaseSafeOutputConfig + SafeOutputTargetConfig + SafeOutputFilterConfig + SafeOutputAllowBlockConfig, byte-identical:pkg/workflow/add_labels.go:10βAddLabelsConfigpkg/workflow/remove_labels.go:10βRemoveLabelsConfigpkg/workflow/unassign_from_user.go:10βUnassignFromUserConfigpkg/workflow/assign_to_user.go:10βAssignToUserConfig(same 4 embeds + one extraUnassignFirst *stringβ near)Sub-group 2b β embed set
BaseSafeOutputConfig + SafeOutputTargetConfig + SafeOutputFilterConfig, byte-identical:pkg/workflow/dismiss_pull_request_review.go:8βDismissPullRequestReviewConfigpkg/workflow/mark_pull_request_as_ready_for_review.go:10βMarkPullRequestAsReadyForReviewConfigpkg/workflow/resolve_pr_review_thread.go:12βResolvePullRequestReviewThreadConfigRecommendation: Define a single named base per embed set (e.g.
LabelsMutationConfig,SimpleTargetedConfig) and either embed it or alias to it β the existingtype CloseIssuesConfig = CloseEntityConfigpattern (pkg/workflow/close_entity_helpers.go:264-266) is the precedent.Cluster 3: Two base structs for the same "request counts + domains" concept
Type: Near/Exact duplicate β’ Occurrences: 2 β’ Impact: Medium
Locations:
pkg/cli/logs_report_firewall.go:17βFirewallSummaryBase(TotalRequests, AllowedRequests, BlockedRequests, AllowedDomains, BlockedDomains)pkg/cli/domain_buckets.go:41βAnalysisBase(embedsDomainBuckets{AllowedDomains, BlockedDomains}+TotalRequests, AllowedRequests, BlockedRequests)Same five logical fields with identical JSON tags (
total_requests/allowed_requests/blocked_requests/allowed_domains/blocked_domains); only theconsole:tags differ.Recommendation: Have
FirewallSummaryBaseembedAnalysisBase(or at leastDomainBuckets), or fold both into one shared base.console:tag placement)Cluster 4: Per-server MCP stats across pipeline stages
Type: Semantic duplicate β’ Occurrences: 3 β’ Impact: Medium
Locations:
pkg/cli/gateway_logs_types.go:85βGatewayServerMetricspkg/cli/audit_report.go:211βMCPServerStatspkg/cli/audit_expanded.go:90βMCPServerHealthDetailShared identity core:
ServerName, RequestCount, ToolCall(Count), ErrorCount. They diverge intentionally as raw-aggregation vs report-schema vs display view (float ms vs formatted strings).Recommendation: Optional shared
MCPServerCoreembed for the 4 common fields; keep stage-specific fields separate. Don't force full unification β the transform boundaries justify some separation.Cluster 5: Per-tool MCP metrics (raw vs report)
Type: Semantic duplicate β’ Occurrences: 2 β’ Impact: Low
Locations:
pkg/cli/gateway_logs_types.go:97βGatewayToolMetricspkg/cli/audit_report.go:183βMCPToolSummaryOverlap:
ToolName, CallCount, ErrorCount, TotalInputSize, TotalOutputSize, AvgDuration, MaxDuration. Duration type differs (float64 ms vs formatted string) β a deliberate rawβdisplay transform.Recommendation: Low value; leave as-is unless the raw/display split is being reworked anyway.
Cluster 6: Diff "summary" overview structs
Type: Semantic duplicate β’ Occurrences: 3 (all
pkg/cli/audit_diff.go) β’ Impact: MediumLocations:
pkg/cli/audit_diff.go:55βFirewallDiffSummary(NewDomainCount, RemovedDomainCount, StatusChangeCount, VolumeChangeCount, HasAnomalies, AnomalyCount)pkg/cli/audit_diff.go:249βMCPToolsDiffSummary(NewToolCount, RemovedToolCount, ChangedToolCount, HasAnomalies, AnomalyCount)pkg/cli/audit_diff.go:308βToolCallsDiffSummary(NewToolCount, RemovedToolCount, ChangedToolCount, Run1TotalCalls, Run2TotalCalls)Same "counts of new/removed/changed + anomaly flags" shape with domain-specific field names.
Recommendation: A shared
DiffCountsSummary{ New, Removed, Changed int; HasAnomalies bool; AnomalyCount int }embed.Cluster 7: Typed before/after delta pair (generics candidate)
Type: Near duplicate β’ Occurrences: 2 β’ Impact: Low, clean win
Locations:
pkg/cli/audit_comparison.go:45βAuditComparisonIntDelta(Before int, After int, Changed bool)pkg/cli/audit_comparison.go:51βAuditComparisonStringDelta(Before string, After string, Changed bool)Identical shape; only the value type differs β a textbook generics case.
Cluster 8: Un-consolidated aggregated summary (already flagged in-code)
Type: Exact/Near duplicate β’ Occurrences: 3 β’ Impact: Low, easy win
The code's own comment at
pkg/cli/logs_models.go:156already flags this.AggregatedSummaryBase(:159) is embedded byMissingToolSummary(:169) andMissingDataSummary(:184), butMCPFailureSummary(:175) re-declaresCount, Workflows, WorkflowsDisplay, RunIDsinline instead of embedding the base β it just omits the twoFirstReason*fields and uses different console headers.Recommendation: Make
MCPFailureSummaryembedAggregatedSummaryBase(with theFirstReason*fieldsomitempty), or split a smaller shared base for the truly common tail.Untyped Usages
Summary Statistics
interface{}usages (production code): rare β ~5 non-test occurrences; the codebase overwhelmingly uses theanyalias.anyin function parameters: ~648 across 213 files β dominated bymap[string]any frontmatterwalkers; mostly idiomatic.anyreturn types: 62 total β ~19 genuine (43 arefunc() any { return &XxxConfig{} }factory closures).anystruct fields: ~33 across 17 files β mostly union-shaped YAML fields.map[string]interface{}/map[string]anyvalues: ~2,290 across ~380 files β almost entirely at the YAML/JSON boundary; largely acceptable.Category 1:
anyin function parameters β mostly acceptableImpact: Low (the type genuinely isn't statically known)
Nearly all are
map[string]anyfrontmatter walkers orraw anytype-switch extractors operating on freshly-unmarshalled YAML (e.g.pkg/cli/outcome_eval_review.go:272-433,pkg/parser/mcp.go,pkg/workflow/engine.go).pkg/typeutil/convert.gois a deliberately-generic conversion utility. No bulk action recommended β the type isn't knowable at these call sites.Category 2:
anystruct fields β one strong winImpact: High for the run-ID case; Low for the rest
Most are union-shaped frontmatter fields where a comment already documents the allowed shapes and a
TemplatableXxxor typed sidecar mitigates it (e.g.frontmatter_types.go:311 Engine any,:356 RunsOn any,:369 Imports any,:413 Checkout any;step_types.go:28 ContinueOnError any;types/input_definition.go:18 Default any). These are fine as-is.The strongest single recommendation in this whole report β GitHub-API id fields typed as
anypurely because the API returns string-or-number:pkg/cli/logs_models.go:323-324βRunID any/RunNumber anypkg/cli/gateway_logs_types.go:148,159βID anypkg/cli/mcp_tools_privileged.go:299-300βRunID any/RunIDOrURL anyA dedicated type removes the scattered ad-hoc assertions:
.(string)/.(float64)assertions that can panic.Secondary:
ContinueOnError any(step_types.go:28) could become aTemplatableBool, mirroring the existingTemplatableInt32/TemplatableBoolOrIntinpkg/workflow/templatables.go.Category 3:
map[string]anyvalues β leave the boundary aloneImpact: Low (bulk); Medium (a few fixed-key-set maps)
The ~2,290 occurrences are almost entirely YAML frontmatter, JSON parsing, codemod tree-rewriting, and GitHub API responses. Heaviest files:
pkg/workflow/safe_outputs_handler_registry.go,pkg/workflow/workflow_builder.go,pkg/workflow/observability_otlp.go,pkg/workflow/tools.go. The team already demonstrates the preferred migration (FrontmatterConfig.Tools *ToolsConfig,SafeOutputs *SafeOutputsConfig,Network *NetworkPermissionsreplaced former maps).Recommendation: Continue the case-by-case typed-struct migration only for maps with a fixed, known key set (e.g.
DevcontainerFeaturesmerges inpkg/cli/devcontainer.go:314). Do not attempt a bulk change β most of these are legitimately dynamic.Category 4: Untyped constants β nice-to-have semantic types
Impact: Low (documentation / misuse-prevention, not correctness)
Most
pkg/constants/consts are env-var names and filenames as barestringβ correct. The numeric ones carrying real units are the candidates:pkg/constants/constants.go:105-135):DefaultMCPGatewayPort = 8080,MinNetworkPort = 1,MaxNetworkPort = 65535,ClaudeLLMGatewayPort = 10000.engine_constants.go:410 DefaultMaxToolDenials = 5;logs_models.go:28 BatchSize = 100,:44 RateLimitThreshold = 10.pkg/workflow/compiler.go:25 MaxLockFileSize = 512000,:30 MaxExpressionSize = 21000,:34 MaxPromptChunkSize = 20000.pkg/workflow/time_delta.go:501 MaxTimeDeltaMinutes = 525600.π― What Should We Do About This?
Prioritized by impact-vs-effort. The top two are quick, low-risk wins:
Priority 1: Quick wins (Low effort, high clarity)
MCPFailureSummaryembedAggregatedSummaryBase(the code already has a TODO comment for this).AuditComparison*Deltastructs with a genericDelta[T comparable].Priority 2: Highest type-safety win (Low effort)
StringOrNumbertype for GitHub run-ID fields (logs_models.go:323-324,gateway_logs_types.go:148,159,mcp_tools_privileged.go:299-300) and delete the ad-hoc assertions.Priority 3: Structural consolidation (Medium effort)
RunAnalysisbase forProcessedRun/RunSummary/DownloadResult.FirewallSummaryBaseandAnalysisBase.DiffCountsSummaryembed.Priority 4: Optional / leave-as-is
Port,Bytes,Minutes) β nice-to-have.map[string]anyvalues or the frontmatter unionanyfields β they live at the YAML/JSON boundary and are already documented and mitigated.Implementation Checklist
MCPFailureSummaryembedsAggregatedSummaryBase)Delta[T comparable]StringOrNumberfor GitHub run-ID fields; remove ad-hoc assertionsRunAnalysisbase (Cluster 1)Analysis Metadata
interface{}in production: ~5map[string]anyvalues: ~2,290 (mostly acceptable YAML/JSON boundary)All reactions