[typist] π€ Typist β Go Type Consistency Analysis #46823
Closed
Replies: 1 comment
|
This discussion has been marked as outdated by Typist - Go Type Analysis. A newer discussion is available at Discussion #47070. |
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
Good news first: this codebase is already in strong shape on type safety. I scanned 1,112 non-test
.gofiles underpkg/(~850 exported type definitions) and found that the team has already done most of the heavy lifting β rawinterface{}is effectively extinct in production code (only ~2 stray mentions, both in comments), shared base structs likeBaseSafeOutputConfig,AggregatedSummaryBase, andBaseMCPServerConfigare already factored out, andpkg/constantsestablishes a clean typed-constant pattern (EngineName,JobName,MCPServerID, ...). So this is less "fix a mess" and more "a handful of spots that drifted from patterns you already use."What's left falls into two buckets. On duplication, there are no exact cross-package clones to consolidate β the one true name collision is
PolicyRule(a firewall ACL rule vs. an intent-governance rule, unrelated concepts), plus a few near-duplicates where a struct copy-pastes fields it could embed instead (most notablyMCPFailureSummary, whose sibling types already embed the shared base β the base type's own doc comment even flags it). On weak typing, the real opportunity isn'tinterface{}at all β it's stringly-typed enums: struct fields and constants likeLLMProvider,PermissionMode, and the variousMode stringfields that hold a small closed set of values but are declared as barestring. Typing these (following theconstants.EngineNamepattern you already have) buys compile-time exhaustiveness on theswitchstatements that consume them. Nothing here is urgent; it's all low-risk polish that reduces copy-paste drift and magic strings.Full Analysis Report
Duplicated Type Definitions
Summary Statistics
Cluster 1:
MCPFailureSummaryβ inline fields instead of embedded baseKind: Near duplicate β’ Impact: Medium
Locations:
pkg/cli/logs_models.go:174βMCPFailureSummaryre-declaresCount,Workflows,WorkflowsDisplay,RunIDsinlinepkg/cli/logs_models.go:158βAggregatedSummaryBase(the base that siblings embed)Its siblings
MissingToolSummaryandMissingDataSummaryembedAggregatedSummaryBase;MCPFailureSummarycopy-pastes a subset instead. The base type's own doc comment already notes this.Recommendation: Embed
AggregatedSummaryBaseinMCPFailureSummary(the two extra base fields it doesn't need are harmless withomitempty). Removes copy-paste drift risk. Effort: ~1 hour.Cluster 2:
Create*Configsafe-output family β second-tier field duplicationKind: Near duplicate β’ Impact: Medium
Locations:
pkg/workflow/create_issue.go:12βCreateIssuesConfigpkg/workflow/create_discussion.go:14βCreateDiscussionsConfigpkg/workflow/create_pull_request.go:33βCreatePullRequestsConfigThese already share
BaseSafeOutputConfig, but a second tier of ~8 entity-creation fields (TitlePrefix,Labels,AllowedLabels,TargetRepoSlug,AllowedRepos,CloseOlderKey,Expires,Footer) is copy-pasted with identicalyamltags across all three.Recommendation: Extract a
CreateEntityBaseConfig(embeddingBaseSafeOutputConfig+ the shared entity fields) and embed it. Cuts the drift surface for future field additions. Effort: 2β3 hours (mechanical, plus test verification).Cluster 3:
PolicyRuleβ name collision across packagesKind: Semantic (name-only) β’ Impact: Low
Locations:
pkg/cli/firewall_policy.go:36β firewall ACL rule (ID,Order,Action,ACLName,Protocol,Domains, ...)pkg/intent/policy.go:46β intent execution-policy rule (ID,Scope,When,Set)Same name, unrelated concepts, sharing only
ID. Not a merge candidate.Recommendation: Rename one for clarity (e.g.
FirewallPolicyRule) to avoid cross-package confusion in reviews/search. Effort: <1 hour.Clusters 4β6: lower-priority audit-diff shape repetition
Cluster 4:
*DiffSummarytrioKind: Semantic β’ Impact: Low β’ all in
pkg/cli/audit_diff.goFirewallDiffSummary:50,MCPToolsDiffSummary:246,ToolCallsDiffSummary:305HasAnomalies/AnomalyCountshape with differing prefixes.DiffCountsSummary{New, Removed, Changed int; HasAnomalies bool; AnomalyCount int}embed. Low priority β JSON field names intentionally diverge.Cluster 5:
*DiffEntrytrioKind: Semantic β’ Impact: Low β’ all in
pkg/cli/audit_diff.goDomainDiffEntry:24,MCPToolDiffEntry:224,ToolCallDiffEntry:282Status+Run1X/Run2X+XChange+IsAnomaly/AnomalyNote).DiffEntryBase{Status string; IsAnomaly bool; AnomalyNote string}embed removes the repeated anomaly-flagging fields; count pairs stay type-specific.Cluster 6:
RedactedDomains*Kind: Near β’ Impact: Low β’
pkg/cli/redacted_domains.goRedactedDomainsLogSummary:29duplicatesTotalDomains+Domains(identical tags) fromRedactedDomainsAnalysis:21, which it already references viaByWorkflow.RedactedDomainsAnalysisto keep the field sets in sync.Acceptable (no action): build-tag variants
SpinnerWrapper(console/spinner.go+spinner_wasm.go),ProgressBar(console/progress.go+progress_wasm.go), andRepositoryFeatures(workflow/repository_features_validation.go+_wasm.go) are intentional wasm-vs-native build-tag pairs β not duplication.Untyped Usages
Summary Statistics
interface{}literals in production code: ~2 (both in comments) β effectively eliminated βanyoccurrences: ~1,800, of which ~1,720 are idiomatic YAML/JSON frontmatter decoding (map[string]anyfromyaml.Unmarshal) β acceptableinterface{}β it's barestringfields/constants that model closed enumsCategory 1: Stringly-typed enum fields & constants (the real opportunity)
The repo already defines named string types for enums (
constants.EngineName,workflow.GitHubMCPMode,workflow.AuthStrategy). These spots hold a small closed value set but were left as barestring, so the consumingswitchstatements get no compile-time exhaustiveness.Example 1:
LLMProviderβ highest valuepkg/workflow/llm_provider.go:13(constants) +pkg/workflow/engine.go:49(field)normalizeLLMProviderswitch and threaded through ~8 helpers (llmProviderProfileFor,...SecretNames,...GatewayBaseURL, ...), all taking a plainstring.constants.EngineNamepattern.Example 2:
PermissionMode(Claude CLI permission modes)pkg/workflow/engine.go:50βPermissionMode string"acceptEdits","auto", ... hardcoded as magic strings atclaude_engine.go:230-233; zero named constants exist today.type PermissionMode string+ named constants for each literal.Example 3:
SafeOutputsURLsPolicypkg/workflow/safe_outputs_validation.go:13(constants) +safe_outputs_config_types.go:92(fieldURLs string)allowed-only,allowed-or-code-region) switched on invalidateSafeOutputsURLs.type SafeOutputsURLsPolicy string; type theURLsfield.Additional enum-typing candidates (lower priority)
pkg/workflow/tools_types.go:441Mode string// stdio/http/remote/localtype MCPServerMode stringpkg/workflow/tools_types.go:384Mode string// mcp/cli (Playwright)type PlaywrightMode stringpkg/workflow/engine_definition.go:200SecretStrategy stringtype SecretStrategy stringpkg/workflow/engine_definition.go:184ProviderEnvMode stringtype ProviderEnvMode stringpkg/workflow/mcp_scripts_parser.go:63MCPScriptsModeHTTP = "http"(untyped)type MCPScriptsMode stringPattern to follow:
type AuthStrategy stringalready exists inengine_definition.go:44.Category 2:
anyfields with a knowable concrete typeImpact: Low β these sit at JSON boundaries but accept a narrower set than
anyimplies.Example 1: run identifiers
pkg/cli/logs_models.go:322βRunID any,RunNumber anyaw_info.jsonβ always numeric (or numeric-string); only ever coerced to string/int.json.Number(orstring) to constrain the accepted representations.Example 2: observability headers
pkg/parser/import_observability.go:18βHeaders anymap[string]string; switched on[]anyat line 56 only to flatten, no scalar case.map[string]string(verify no scalar form is accepted first).Category 3:
map[string]anyfor structured recordsImpact: Low (borderline β acceptable if upstream schema is unstable)
Example: token usage records
pkg/cli/token_usage.go:570βextractUsageRecord(value any) map[string]any+usageNumericValue(...)string-key lookupsUsageRecordstruct decoded at the boundary β removes stringly-typed key access and per-key coercion helpers.π― What Should We Do About This?
Prioritized by value-per-effort. Everything here is low-risk β no behavioral changes, just tighter types.
Priority 1 (Medium) β Kill copy-paste drift in the two near-duplicate structs
AggregatedSummaryBaseinMCPFailureSummary(Cluster 1).CreateEntityBaseConfigfor theCreate*Configfamily (Cluster 2).Priority 2 (Medium/High) β Type the enum strings
LLMProvider(highest leverage), thenPermissionModeandSafeOutputsURLsPolicy.constants.EngineName/AuthStrategypattern.Priority 3 (Low) β Cleanups
cli.PolicyRuleβFirewallPolicyRule(Cluster 3).DiffEntryBase/DiffCountsSummaryembeds (Clusters 4β5).RunID/RunNumber/Headersanyfields (Category 2).Implementation Checklist
AggregatedSummaryBaseinMCPFailureSummaryCreateEntityBaseConfigfor the create-entity safe-output familytype LLMProvider string+ typed constants and fieldtype PermissionMode string+ named constants (currently zero exist)type SafeOutputsURLsPolicy stringcli.PolicyRuleβFirewallPolicyRuleDiffEntryBase/DiffCountsSummaryinaudit_diff.gogo build ./...+ full test suite after each changeAnalysis Metadata
pkg/)interface{})All reactions