[typist] Typist type analysis #52484
Closed
Replies: 1 comment
|
This discussion has been marked as outdated by Typist - Go Type Analysis. A newer discussion is available at Discussion #52704. |
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,213 non-test
.gofiles underpkg/(roughly 1,000+ struct definitions acrosscli,workflow,parser,linters, and two dozen small utility packages), looking for duplicated type definitions and places whereinterface{}/anyor untyped constants stand in for a concrete type. Good news first: the codebase is already disciplined in the places that matter most —interface{}has been fully retired in favor ofany, and the safe-outputs config family inpkg/workflow(BaseSafeOutputConfig+ composable mixins likeSafeOutputTargetConfig) and the MCP server config embedding inpkg/parser/mcp.goare genuinely good "embed, don't copy" patterns worth pointing to as the house style.That said, I found 13 duplicate/near-duplicate type clusters and 16 untyped-usage findings worth fixing. The standouts:
pkg/cli/logs_models.gohas a struct (MCPFailureSummary) that hand-copies fields fromAggregatedSummaryBaseinstead of embedding it — and the source comment right next to it already calls this out as "copy-paste drift risk."pkg/agentdrainmaintains two structs (Cluster/SnapshotCluster) that are field-for-field identical except JSON tags, hand-copied on every save/load. And inpkg/intent— the package that encodes autonomy/write-scope security policy —AutonomyandWriteScopeare plainstringfields ranked against a lookup table of literals, so a typo silently resolves to the least-restrictive rank instead of failing to compile. That one's worth prioritizing given the security-policy context.Full Analysis Report
Duplicated Type Definitions
Summary Statistics
.gofiles underpkg/Cluster 1:
AggregatedSummaryBasedrift —pkg/cli(Near duplicate, High impact)pkg/cli/logs_models.go:171definesAggregatedSummaryBase{Count, Workflows, WorkflowsDisplay, FirstReason, FirstReasonDisplay, RunIDs}.MissingToolSummary(:181) andMissingDataSummary(:196) correctly embed it.MCPFailureSummary(:187) manually re-declares 4 of the 6 fields instead of embedding — and the comment at:171already flags this as a "copy-paste drift risk."Recommendation: embed
AggregatedSummaryBaseinMCPFailureSummarylike its siblings do. Mechanical, low-risk, ~15 minutes.Cluster 2:
AnalysisBasevsFirewallSummaryBase—pkg/cli(Exact duplicate, High impact)pkg/cli/domain_buckets.go:41AnalysisBaseembedsDomainBuckets{AllowedDomains, BlockedDomains}plusTotalRequests, AllowedRequests, BlockedRequests.pkg/cli/logs_report_firewall.go:17FirewallSummaryBasedeclares the exact same five fields independently.Recommendation: have
FirewallSummaryBasecomposeAnalysisBaseinstead of re-declaring its fields.Cluster 3:
ClustervsSnapshotCluster—pkg/agentdrain(Exact duplicate, High impact)pkg/agentdrain/types.go:32Cluster{ID, Template, Size, Stage}.pkg/agentdrain/persist.go:21SnapshotCluster— identical fields, only difference is JSON tags.persist.go:40-45and:74-79hand-copy every field between the two on every save/load.Recommendation: add
jsontags directly toCluster(or aMarshalJSON/UnmarshalJSONpair) and deleteSnapshotClusterplus the copy loops.Cluster 4:
AuditComparisonIntDeltavsAuditComparisonStringDelta—pkg/cli(Near duplicate, generics candidate)pkg/cli/audit_comparison.go:45{Before, After int; Changed bool}and:51{Before, After string; Changed bool}— identical shape, different value type.Recommendation:
type AuditComparisonDelta[T comparable] struct { Before, After T; Changed bool }.Cluster 5:
RunOptionsvsRunWorkflowOptions—pkg/cli(Near duplicate)pkg/cli/run_workflow_execution.go:31RunOptionsandpkg/cli/run_interactive.go:366RunWorkflowOptionsshare 8 of 9 fields (Verbose, EngineOverride, RepoOverride, RefOverride, AutoMergePRs, Push, DryRun, Approve) with identical names/types.Recommendation: extract a shared
WorkflowRunFlagsstruct and embed it in both.Cluster 6: Scanner
Finding/Outputboilerplate —pkg/cli(Semantic duplicate)Finding+Output{ Findings []Finding }shape:poutine.go:24,35,grype.go:41,60,runner_guard.go:22,34, plusgrant.go:47,zizmor.go:20,actionlint.go:98.Recommendation: the per-tool
Findingshapes genuinely differ (different JSON schemas), but the repeatedOutput{ Findings []X }wrapper is a good fit fortype ScanOutput[F any] struct { Findings []F }.Cluster 7:
RepeatOptionsvsPollOptions—pkg/cli(Semantic duplicate)pkg/cli/retry.go:20andpkg/cli/signal_aware_poll.go:35both model "run something repeatedly with signal-aware cancellation and start/progress messaging," implemented twice with parallel signal-handling logic.Recommendation: evaluate whether
ExecuteWithRepeatcan be reimplemented on top ofPollWithSignalHandling.Cluster 8:
GitHubMCPDockerOptionsvsGitHubMCPRemoteOptions—pkg/workflow(Near duplicate)pkg/workflow/mcp_renderer_types.go:67-98and:101-128share 7 fields verbatim (ReadOnly, Lockdown, LockdownFromStep, GuardPoliciesFromStep, Toolsets, Features, AllowedTools, GuardPolicies), diverging only in Docker-specific vs. remote-specific fields.Recommendation: extract a
GitHubMCPCommonOptionsstruct for the shared fields, mirroring theBaseSafeOutputConfigpattern already used elsewhere in this package.Cluster 9:
MCPConfigRenderervsMCPRendererOptions/MCPConfigRendererUnified—pkg/workflow(Semantic duplicate)pkg/workflow/mcp_config_types.go:40andpkg/workflow/mcp_renderer_types.go:6are two parallel "renderer options" abstractions carrying the same concepts (copilot-field toggle, format, guard policies, container pin mappings) under different field names, both still actively referenced (17 files useMCPConfigRenderer).Recommendation: this likely needs an owner's call on whether the legacy path can migrate onto the unified renderer — flagged for a design conversation, not a blind merge.
Cluster 10:
IntentRecordvsRootReference—pkg/intent(Semantic duplicate)pkg/intent/resolver.go:36IntentRecord'sRoot*-prefixed fields (RootNodeID, RootType, RootURL, Labels) are a hand-flattened mirror ofRootReference(:50). Every constructor (fromRoot,fromLabels,unlinked,ambiguous) manually maps one onto the other.Recommendation: embed
RootReference(or a pointer to it) insideIntentRecordinstead of duplicating four fields by hand.Cluster 11:
*VarStatedeferred-cleanup tracking — four linters underpkg/linters/*(Near duplicate)manualmutexunlock/manualmutexunlock.go:158(mutexVarState),httprespbodyclose/httprespbodyclose.go:166(respVarState),contextcancelnotdeferred/contextcancelnotdeferred.go:129(cancelVarState),fileclosenotdeferred/fileclosenotdeferred.go:162(fileVarState) each implement the identical shape/algorithm for "was this resource closed via defer, manually, or not at all," keyed bymap[types.Object]*XVarState.Recommendation: hoist a generic tracker into
pkg/linters/internal/(which already housesastutil,analyzerutil,coverage,filecheck,nolint) parameterized by the "is this a manual cleanup call" predicate.Cluster 12: Untyped-value conversion split —
pkg/typeutilvspkg/stringutil(Semantic duplicate)pkg/typeutil/convert.goalready hostsParseIntValue,ConvertToInt,ConvertToFloat(heterogeneous decoded-YAML/JSON value → typed).pkg/stringutil/stringutil.go:54independently implementsParseVersionValue(any) string, doing the same kind of coercion in a different package.Recommendation: move (or alias) the string-coercion helper next to
typeutil's other converters.Cluster 13:
setutil.Containsunderuse —pkg/linters/*(style/consistency, not a bug)pkg/setutil/setutil.go:6definesContains[K comparable](set map[K]struct{}, key K) bool, but most call sites (pkg/linters/internal/nolint/nolint.go:109-117,pkg/linters/globwalkignorederror/globwalkignorederror.go:22,pkg/linters/errormessage/errormessage.go:101-122) re-implement the_, ok := m[key]check inline. Onlypkg/linters/ssljson/ssljson.goactually uses it.Recommendation: low priority; switch inline checks to
setutil.Containsfor consistency when touching those files anyway.Patterns verified as fine (not findings)
RepositoryFeatures(repository_features_validation.govs._wasm.go),ProgressBar(console/progress.govs.progress_wasm.go), andSpinnerWrapper(console/spinner.govs.spinner_wasm.go) are legitimate(go/redacted):buildnative/WASM variants, not accidental drift.*Configfamily inpkg/workflow(CreateIssuesConfig,CreatePullRequestsConfig,AddCommentsConfig, etc.) already composesBaseSafeOutputConfig+ mixins (SafeOutputTargetConfig,SafeOutputAllowedLabelsConfig,SafeOutputFilterConfig, ...) viayaml:",inline"— a good pattern to hold up as the house style rather than a target for refactoring.pkg/parser/mcp.go:39'sRegistryMCPServerConfigexplicitly embedstypes.BaseMCPServerConfigwith a comment noting it's intentionally distinct from the workflow-side config — another positive example of doing this right.Untyped Usages
Summary Statistics
interface{}usages in production code: 0 (fully migrated toany; the only 18 repo-wide hits are in lintertestdata/fixtures used to test the linters that detect this exact anti-pattern)anyusages flagged as fixable: 10anyusages confirmed as legitimate (dynamic YAML/JSON, generic containers, required third-party signatures) and excluded: many — see notes belowFindings
pkg/intent/policy.go:39,48ExecutionPolicy.Autonomy,.WriteScopeare plainstringautonomyRank,writeScopeRank); a typo silently resolves to the least-restrictive rank in a security-policy engine — fail-open risktype Autonomy string/type WriteScope stringwith matching consts, mirroring this package's ownAttributionStatus/AttributionSourceenumspkg/github/label_objective_mapping.go:27ObjectiveMapping.MultiLabelLogic stringMultiLabelLogicMax/Sum/Firstalready exist inlabel_objective_mapping_constants.go:137-139but are never referenced — the field still compares raw string literalspkg/intent/resolver.go:41,46IntentRecord.Rule,.RootTypeare plainstring"issue","artifact","single_closing_issue", ...) scattered across the file, inconsistent with this package's own enum conventionpkg/workflow/awf_config.go:174AWFConfigFile.Enclaves []map[string]anyEnclaveConfig) viaaddEnclaveString/addEnclaveInthelpers just to skip zero valuesAWFEnclaveConfigwithomitemptytags and drop the manual map-building helperspkg/workflow/safe_outputs_config_types.go:115,threat_detection_config.go:9-10Steps/PostSteps []anyWorkflowSteptype andSliceToSteps([]any)converter already exist (step_types.go:19,195), but each consumer re-does the type assertion individually (e.g.compiler_safe_outputs_job.go:242-247)[]*WorkflowStepat parse time; delete the duplicated per-consumer assertion logicpkg/cli/run_workflow_execution.go:25const workflowCompletionWaitTimeoutMinutes = 6 * 60(untyped int "minutes")time.Duration(timeoutMinutes) * time.Minuteconversion atpr_automerge.go:120-123const workflowCompletionWaitTimeout = 6 * time.Hourastime.Duration; change the function signature to accepttime.Durationdirectlypkg/cli/update_org.go:27,31,37orgUpdateCoreBuffer,orgUpdateSearchBuffer,orgUpdateCriticalConsumed— untyped rate-limit budget constantstype rateLimitBudget intpkg/errorutil/errors.go:19,32,45"404","403","410"net/httpstrconv.Itoa(http.StatusNotFound)or a named constant blockpkg/console/console_types.go:50FormField.Value anypkg/console/layout_wasm.go:16LayoutEmphasisBox(content string, color any)colorparam is unused in the body and has zero callers repo-wideLower-confidence / awaiting a design call
pkg/workflow/safe_outputs_config_types.go:93—SafeOutputsConfig.Data anyis a genuine 4-way sum type (false/omitted,true, inline schema, or an expression string). A discriminated wrapper would remove theanybut is a larger refactor — flagged for awareness, not a quick fix.pkg/workflow/mcp_config_types.go:54and three sibling fields —GuardPolicies map[string]any, populated byderiveWriteSinkGuardPolicyFromWorkflowwhich does nested map navigation including key deletion. Closer to genuine dynamic tree manipulation; worth a conversation about aGuardPolicy/WriteSinkPolicystruct rather than a mechanical fix.pkg/cli/experiments_analyze_statistics.go:18,21—defaultMinSamples,balanceSignificanceThresholdare untyped but low-risk; aSampleCount/PValuenamed type would make intent explicit rather than fix a real bug.pkg/cli/logs_models.go:338-339—AwInfo.RunID any,.RunNumber any— comment suggests genuinely mixed-type input from varying agent-log JSON producers; no type-switch call sites found, so confidence is low on whether this is currently safe.Confirmed legitimate (excluded from findings)
pkg/typeutil— the package's entire purpose is being the typed boundary for decoding heterogeneous YAML/JSON config; itsanyusage is correct by design.pkg/types/input_definition.go:18—InputDefinition.Default anyis genuinely polymorphic (string/number/boolean from YAML), already resolved via a 5-way type switch, and explicitly documented as the sanctioned pattern otherpkg/parsercode points to.func run(pass *analysis.Pass) (any, error)across ~70 files inpkg/linters/*— required by the third-partygolang.org/x/tools/go/analysis.Analyzer.Runsignature; cannot be changed without forking that API.map[string]anyinpkg/parser(frontmatter/YAML traversal) andpkg/workflow(MCP tool config, model-pricing overlays) is genuine dynamic-document handling, already self-documented in-source as intentional.Refactoring Recommendations
Priority 1 — Mechanical, low-risk consolidations
AggregatedSummaryBaseinMCPFailureSummary(Cluster 1) — the code already flags this itself.FirewallSummaryBaseintoAnalysisBase(Cluster 2).SnapshotCluster, add JSON tags toClusterdirectly (Cluster 3).AuditComparisonIntDelta/StringDelta(Cluster 4).MultiLabelLogicMax/Sum/Firstconstants (Untyped Add workflow: githubnext/agentics/weekly-research #2).Estimated effort: half a day total across all five; each is independently shippable.
Priority 2 — Security-relevant and structural fixes
ExecutionPolicy.Autonomy/.WriteScopeas enums (Untyped rejig docs #1) — this is the one I'd bump to the top given it's a fail-open risk in policy evaluation, not just a style nit.Steps/PostStepsto[]*WorkflowStepat parse time (Untyped Add workflow: githubnext/agentics/weekly-research #5) — removes duplicated type-assertion logic at every call site.GitHubMCPCommonOptions(Cluster 8) and consider embeddingRootReferenceinIntentRecord(Cluster 10).*VarStatepattern intopkg/linters/internal(Cluster 11).Estimated effort: 1-2 days; touches more call sites, worth a quick design nod from a reviewer before starting.
Priority 3 — Worth a conversation, not a quick PR
MCPConfigRenderervs. the unified renderer path (Cluster 9),SafeOutputsConfig.Data anysum type, and theGuardPolicies map[string]anytree manipulation are all real but larger design questions — flagging them here so they're on the radar, not proposing a specific mechanical fix.Analysis Metadata
pkg/pkg/cli,pkg/workflow,pkg/parser+ small domain packages,pkg/linters+ utilities), cross-checked against call sites before flaggingAll reactions