[typist] Typist - Go Type Consistency Analysis #54506
Closed
Replies: 1 comment
|
This discussion was automatically closed because it expired on 2026-08-22T11:40:33.251Z.
|
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 · run §32477322358
Executive Summary
I scanned all 1,275 non-test
.gofiles underpkg/(~1,225 top-level type declarations) for duplicated types and untyped usage. The good news: this codebase has already done real type-safety work — there's a documented migration history (scratchpad/go-type-patterns.md) covering engine names, workflow IDs, tool names, and PAT types, and the bulk ofany/map[string]anyusage (~4,283 occurrences) is legitimately justified dynamic YAML/JSON frontmatter parsing, not laziness.That said, I found a handful of concrete, worthwhile opportunities: the sharpest one is in
pkg/cliitself, where two nearly-identical outcome enums (OutcomeResultandOutcomeStatus) live in the same package and are both embedded in the sameOutcomeReportstruct — a duplicate that's easy to consolidate and a good first PR. There's also a second, hand-rolled GitHub Actions workflow/step model inpkg/cli/copilot_setup.gothat duplicatespkg/workflow's existingWorkflowStep/Jobtypes, three overlapping "network log entry" structs with inconsistent field types (Statusasstringin two,intin a third), and a few enum-shaped string constants (reaction types, safe-outputs URL policy, MCP param types) still declared as barestringinstead of named types. None of this is urgent, but each item below is a small, low-risk, high-clarity fix.Full Analysis Report
Duplicated Type Definitions
Summary Statistics
Cluster 1:
OutcomeResultvsOutcomeStatus— same package, parallel enumsType: Near-duplicate | Occurrences: 2 | Impact: High — same concept, two competing enums in one package
Locations:
pkg/cli/outcome_eval.go:27—type OutcomeResult string(accepted, rejected, ignored, pending, unknown, lifecycle, lifecycle_close, error)pkg/cli/outcome_evaluation.go:12—type OutcomeStatus string(accepted, rejected, pending, ignored, skipped, unknown, lifecycle, lifecycle_close)7 of 8 values are shared verbatim; only
errorvsskippeddiffer. Worse,OutcomeReport(outcome_eval.go:41) embedsOutcomeEvaluation(which carriesOutcomeStatus) and has its ownResult OutcomeResultfield — the same struct carries both parallel enums simultaneously.Recommendation: Merge into one classification enum (add a
skipped/errorvalue to whichever wins) and drop the other. Estimated effort: 1-2 hours. Benefit: removes ambiguity about which enum a givenOutcomeReportfield actually reflects.Cluster 2: Network/audit "log entry" structs
Type: Near-duplicate | Occurrences: 3 | Impact: Medium — overlapping fields, inconsistent types
Locations:
pkg/cli/access_log.go:21—AccessLogEntry{Timestamp, Duration, ClientIP, Status, Size, Method, URL, User, Hierarchy, Type string}pkg/cli/firewall_log.go:118—FirewallLogEntry{Timestamp, ClientIPPort, Domain, DestIPPort, Proto, Method, Status, Decision, URL, UserAgent string}pkg/cli/firewall_policy.go:50—AuditLogEntry{Schema, Client, Host, Dest, Method string, Status int, Decision, URL string}Three independently-parsed "one network request line" records with overlapping fields (
Timestamp/Client*/Method/Status/URL/Decision), butStatusisstringin two andintin the third, andClientIPvsClientIPPortvsClientname the same thing differently.Recommendation: Extract a common
NetworkLogEntrybase (mirroring theAnalysisBase/FirewallSummaryBasepattern the same files already use one level up) and embed it. Estimated effort: 2-3 hours.Cluster 3:
ToolUsageSummaryvsToolUsageInfoType: Near-duplicate | Occurrences: 2 | Impact: Medium
pkg/cli/logs_report_tools.go:13—ToolUsageSummary{Name, TotalCalls, Runs, MaxOutputSize, MaxDuration}pkg/cli/audit_report.go:155—ToolUsageInfo{Name, CallCount, MaxInputSize, MaxOutputSize, MaxDuration, OutputSample}Same per-tool aggregation concept, computed twice for two different reports (
logsvsaudit) with different field names (TotalCallsvsCallCount).Recommendation: Unify into one
ToolUsageStatstype with optional fields, shared by both report builders. Estimated effort: 1-2 hours.Cluster 4: Duplicate GitHub Actions workflow model (
clivsworkflow)Type: Semantic duplicate | Occurrences: 2 | Impact: High — a second bespoke Actions-YAML model
pkg/workflow/step_types.go:17—WorkflowStep{Name, ID, If, Uses, Run, WorkingDirectory, Shell, With, Env, ContinueOnError, TimeoutMinutes}pkg/cli/copilot_setup.go:356,365,372—CopilotWorkflowStep{Name, Uses, Run, With, Env}+WorkflowJob{RunsOn, Permissions, Steps}+Workflow{Name, On, Jobs}pkg/clire-implements a second, smaller "GitHub Actions workflow file" object model from scratch just to scaffoldcopilot-setup-steps.yml, instead of reusingpkg/workflow's existing step/job types.Recommendation: Reuse
workflow.WorkflowStep/workflow.Job(or extract a minimal sharedactionsyamlpackage). Estimated effort: 2-4 hours. Benefit: one source of truth for "what a GitHub Actions step/job looks like."Cluster 5: MCP server config family — mostly deduped, one straggler
Type: Semantic duplicate | Occurrences: 4 | Impact: Low
pkg/types/mcp.go:6—BaseMCPServerConfig(shared base, already embedded by the next two — good pattern)pkg/parser/mcp.go:39—RegistryMCPServerConfig{types.BaseMCPServerConfig; ...}pkg/workflow/tools_types.go:519—MCPServerConfig{types.BaseMCPServerConfig; ...}pkg/cli/mcp_config_file.go:22—VSCodeMCPServer{Type, Command, Args, Tools, CWD}— does not embedBaseMCPServerConfigdespite 3/4 fields overlappingRecommendation: Low priority —
VSCodeMCPServermirrors an external VS Code JSON schema, so full unification isn't required, but consider embeddingBaseMCPServerConfigfor the shared subset.Clusters 6-10 (lower priority — noted for completeness)
DomainAnalysis/FirewallAnalysisboth embedAnalysisBasebut each hand-rolls its ownMarshalJSON/wire struct — worth consolidating the shim logic, not the types. Informational only.RecommendationvsAuditComparisonRecommendation: same word, subset of fields (pkg/cli/audit_report.go:75vspkg/cli/audit_comparison.go:68); a naming pass (AuditComparisonAction) would reduce confusion — low priority.Outcome*naming family is large:OutcomeResult,OutcomeStatus,OutcomeReport,OutcomeSummary,OutcomeEvaluation,OutcomesData,OutcomesConfig,OutcomesHistoryConfig— recommend a naming/consolidation pass alongside fixing Cluster 1.stringin some structs, bareintinAuditLogEntry, and a proper enum (WorkflowStatus,pkg/cli/status_command.go:31) elsewhere — one canonical enum per domain (network status vs. GH Actions run status) would remove several stringly-typed fields.ProgressBar/SpinnerWrapperbuild-tag pairs (pkg/console/progress.go+progress_wasm.go,spinner.go+spinner_wasm.go): expected native/wasm platform split under build tags — not a refactor target, included only to confirm it was checked and correctly skipped.Untyped Usages
Summary Statistics
interface{}occurrences: 16 (all confined topkg/linters/*/testdata/*.gofixtures — not real code, skip)anyoccurrences: ~4,283 (dominant patternmap[string]anyfor dynamic YAML/JSON — mostly justified and already documented project policy)map[string]any/map[string]interface{}: 2,541[]any/[]interface{}: 469The project's own
scratchpad/go-type-patterns.mddocuments itsany-usage philosophy and prior migrations (EngineName,WorkflowID,GitHubToolName,GitHubAllowedTools, etc.), so most bulkanyusage below was deliberately excluded as justified dynamic parsing. The items below are the residual, genuine opportunities.Category 1: Struct fields that are secretly fixed-value enums
Impact: Medium — these are implicit enums hiding behind bare
string, validated only at runtimeSafe-outputs URL policy —
pkg/workflow/safe_outputs_validation.go:16-17definesSafeOutputsURLsPolicyAllowedOnly = "allowed-only"and...AllowedOrCodeRegion = "allowed-or-code-region"as untyped bare consts, used in aswitch config.URLswhereURLs string(pkg/workflow/safe_outputs_config_types.go).Benefit: the compiler rejects a typo like
"allow-only"instead of only failing validation at runtime.Reaction types —
pkg/workflow/reactions.go:12-21hasvalidReactions map[string]boolwith 8 literal keys (+1,-1,laugh,confused,heart,hooray,rocket,eyes), consumed viaAIReaction string(pkg/workflow/workflow_data.go:118).Suggested:
type ReactionType string+ named consts,validReactions map[ReactionType]bool.MCP param type —
pkg/workflow/mcp_scripts_parser.go:60:MCPScriptParam.Type string // JSON schema type (string, number, boolean, array, object)— the comment enumerates exactly 5 legal values but the field stays a barestring.Suggested:
type MCPParamType stringwith 5 named consts — enables exhaustiveswitch-based codegen and rejects typos in tool-schema authoring.Category 2: Mixed-type ID field (JSON number-or-string)
Impact: Medium — real runtime ambiguity, not just style
pkg/cli/logs_models.go:358-359—RunID any/RunNumber anyon theaw_info.json-derived struct, while sibling structs in the same file typeRunID int64(:123,:280) orstring(:329). This stems from${{ github.run_id }}serializing as either a JSON number or a templated string depending on producer.Suggested: a small
NumericIDtype with a customUnmarshalJSONthat accepts both forms and normalizes toint64, removing the scattered%v/fmt.Sprintfformatting at call sites.Category 3: Untyped constants that will need to become enums
Impact: Low-Medium — pre-emptive, avoids future untyped→typed churn
pkg/workflow/frontmatter_types.go:10—const RunnerTopologyArcDind = "arc-dind", compared viaswitch config.Topology(pkg/workflow/runner_config.go:46). Only one legal value exists today, but the switch/error-message pattern ("unsupported runner.topology value %q; supported values: %q") signals it's meant to grow. Suggested: introducetype RunnerTopology stringnow, before a second value arrives.pkg/workflow/behavior_defined_engine.go:19-21—behaviorSecretStrategyUniversalLLMConsumerandbehaviorProviderEnvModeUniversalLLMConsumerare two conceptually distinct bare-string consts that happen to share the literal value"universal-llm-consumer". If one changes, the other silently diverges. Suggested: give each its own named type (BehaviorSecretStrategy,BehaviorProviderEnvMode) even while they share a value today.Minor/low-priority mentions
pkg/console/console_types.go:50—FormField.Value any: zero constructors/usages found anywhere inpkg/exceptform_wasm.go; likely dead or JS-bridge-only code, worth a dead-code check rather than a type fix.pkg/console/layout_wasm.go:16—func LayoutEmphasisBox(content string, color any) string:coloris unused in the function body and there's no non-wasm counterpart; likely an orphaned parameter worth deleting.pkg/workflow/step_shell_validator.go:118—func checkStepGHToken(step any, ...): technically justified since a workflow step can be a string shorthand or a map, but worth an explicit doc comment.Explicitly reviewed and judged JUSTIFIED (skip)
deepCopyAny(v any) any(pkg/workflow/behavior_defined_engine.go:872) — textbook recursive deep-copy helper over arbitraryyaml.Unmarshaloutput.ID anyon JSON-RPC payload structs (pkg/cli/gateway_logs_types.go:179,190) — JSON-RPC 2.0 spec allowsidto be string, number, or null; standard protocol pattern.Engine any,RunsOn any,Checkout any,Imports any,Evals anyinpkg/workflow/frontmatter_types.go— each already carries a doc comment justifying the union-of-YAML-shapes rationale.(value any)/map[string]anyhelpers inpkg/parser/mcp.go,pkg/parser/tools_merger.go,pkg/parser/schema_suggestions.gooperating on raw parsed YAML/JSON Schema before validation — the core "dynamic data" pattern the project's style guide explicitly endorses.Refactoring Recommendations
Priority 1: Consolidate the two outcome enums (Cluster 1)
Steps: 1) Pick a winner between
OutcomeResult/OutcomeStatus(or merge into one enum with all 9 distinct values). 2) UpdateOutcomeReportto carry a single field. 3) Update all switch statements and comparisons. 4) Run tests.Estimated effort: 1-2 hours. Impact: High — removes ambiguity in a package that's actively used for outcome evaluation.
Priority 2: Reuse
pkg/workflowstep/job types incopilot_setup.go(Cluster 4)Steps: 1) Check whether
workflow.WorkflowStep/Jobcan serialize to the subsetcopilot-setup-steps.ymlneeds. 2) ReplaceCopilotWorkflowStep/WorkflowJob/Workflowwith the existing types (or a thin shared package). 3) Update the scaffold-generation call sites. 4) Run tests.Estimated effort: 2-4 hours. Impact: High — removes a full duplicate object model.
Priority 3: Merge overlapping log-entry structs (Cluster 2) and tool-usage structs (Cluster 3)
Steps: extract common base structs, normalize
Statusto a single type, run tests.Estimated effort: 3-5 hours combined. Impact: Medium.
Priority 4: Type the implicit enums (Untyped Usages Categories 1 & 3)
Steps: introduce
SafeOutputsURLsPolicy,ReactionType,MCPParamType,RunnerTopology,BehaviorSecretStrategy/BehaviorProviderEnvModeas named string types with consts; update field types and switch statements.Estimated effort: 3-4 hours combined. Impact: Medium — mostly compile-time safety wins on validation logic that already exists.
Priority 5 (optional cleanup): Investigate two likely-dead
anyfieldsFormField.ValueandLayoutEmphasisBox's unusedcolorparameter — confirm dead/orphaned and remove, or document why they're needed.Implementation Checklist
OutcomeResult/OutcomeStatusinto one enum (Priority 1)copilot_setup.go's bespoke workflow model withpkg/workflowtypes (Priority 2)NetworkLogEntrybase forAccessLogEntry/FirewallLogEntry/AuditLogEntry(Priority 3)ToolUsageSummary/ToolUsageInfointo oneToolUsageStatstype (Priority 3)SafeOutputsURLsPolicy*,ReactionType,MCPParamType,RunnerTopology, behavior-strategy consts (Priority 4)NumericIDtype with customUnmarshalJSONforRunID/RunNumberinlogs_models.goFormField.ValueandLayoutEmphasisBox's unusedcolorparamAnalysis Metadata
pkg/only)anyoccurrences, the vast majority justified)References:
All reactions