[typist] Typist - Go Type Consistency Analysis #31934
Closed
Replies: 1 comment
-
|
This discussion was automatically closed because it expired on 2026-05-14T12:11:06.227Z.
|
Beta Was this translation helpful? Give feedback.
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.
-
Executive Summary
Analyzed 786 non-test Go files across
pkg/containing roughly 550 struct/interface definitions and ~470 function signatures touchingmap[string]any. The codebase is generally well-typed: shared base types (pkg/types/BaseMCPServerConfig,pkg/workflow/SafeOutputTargetConfig,pkg/workflow/BaseSafeOutputConfig), semantic aliases inpkg/constants(LineLength,CommandPrefix,WorkflowID), and a dedicatedpkg/typeutilfor safeany-conversions are all in place. The findings below are mostly inconsistencies — duplicate field declarations where an existing embed type already exists — plus one exact-duplicate interface and a deprecated empty struct.There is no widespread
interface{}/anyabuse: the ~2,839map[string]anyoccurrences are concentrated in YAML/JSON parsing entry points (frontmatter map[string]any,configMap map[string]any) where dynamic typing is intentional and documented.Critical Findings
TargetRepoSlug/AllowedReposinstead of embedding existingSafeOutputTargetConfigactionpins.SHAResolverandworkflow.ActionSHAResolverworkflow.AddCommentConfigis an empty deprecated struct with no remaining referencesFooter *string,TitlePrefix string,Labels []string,CloseOlderKey stringare re-declared across 5–11 safe-output configsFrontmatterConfigfields are explicitly marked deprecated but still kept:MCPServers,Runtimes,Permissions(allmap[string]any)Full Analysis Report
Duplicated Type Definitions
Summary Statistics
.gofiles underpkg/Cluster 1: SHAResolver / ActionSHAResolver (exact duplicate interface)
Type: Exact duplicate (different name, identical method set)
Impact: Medium — two interfaces with identical contracts force callers to choose; test doubles must be written twice.
Locations:
pkg/actionpins/actionpins.go:59—type SHAResolver interface { ResolveSHA(ctx context.Context, repo, version string) (string, error) }pkg/workflow/action_resolver.go:16—type ActionSHAResolver interface { ResolveSHA(ctx context.Context, repo, version string) (string, error) }Both interfaces are satisfied by the same concrete resolver, and
pkg/workflow/maintenance_workflow.go:22,67,143pluspkg/cli/copilot_setup.go:22,47,178,265,336use theworkflow.ActionSHAResolverform whilepkg/cli/copilot_setup_test.godefines amockSHAResolverthat satisfies both.Recommendation: Promote the interface to
pkg/actionpins(or a neutralpkg/types/resolver.go) and havepkg/workflowimport/use it directly. Go interface satisfaction is structural — only one definition is needed.Cluster 2: AddCommentConfig (dead empty struct)
Type: Deprecated empty placeholder
Impact: Low — code smell, no runtime impact.
Location:
pkg/workflow/add_comment.go:9-12A grep across
pkg/returns only this declaration — no other code (production or test) references the type.Recommendation: Delete the type. The replacement
AddCommentsConfigis already used everywhere.Cluster 3: Safe-output target/repo fields (near-duplicate field declarations)
Type: Near duplicate — same field, same tag, same comment, declared inline in 11 types when an embeddable base already exists.
Existing base (
pkg/workflow/safe_outputs_parser.go:9-13):Already adopted in 18 types (good):
AssignMilestoneConfig,UpdateEntityConfig,HideCommentConfig,ReplyToPullRequestReviewCommentConfig,AssignToAgentConfig,LinkSubIssueConfig,ResolvePullRequestReviewThreadConfig,AddLabelsConfig,RemoveLabelsConfig,SetIssueFieldConfig,MarkPullRequestAsReadyForReviewConfig,SetIssueTypeConfig,UnassignFromUserConfig,CloseEntityConfig,SubmitPullRequestReviewConfig,AssignToUserConfig,CloseJobConfig,ListJobConfig,AddReviewerConfig.Still inlining the fields manually (11 places — should embed instead):
pkg/workflow/create_issue.go:19-20CreateIssuesConfigTargetRepoSlug,AllowedRepospkg/workflow/create_discussion.go:21-22CreateDiscussionsConfigTargetRepoSlug,AllowedRepospkg/workflow/create_pull_request.go:30-31CreatePullRequestsConfigTargetRepoSlug,AllowedRepospkg/workflow/create_pr_review_comment.go:14-15CreatePullRequestReviewCommentsConfigTargetRepoSlug,AllowedRepospkg/workflow/create_code_scanning_alert.go:18-19CreateCodeScanningAlertsConfigTargetRepoSlug,AllowedRepospkg/workflow/create_agent_session.go:13-14CreateAgentSessionConfigTargetRepoSlug,AllowedRepospkg/workflow/add_comment.go:17-19AddCommentsConfigTarget,TargetRepoSlug,AllowedRepospkg/workflow/push_to_pull_request_branch.go:22-23PushToPullRequestBranchConfigTargetRepoSlug,AllowedRepospkg/workflow/comment_memory.go:11-12CommentMemoryConfigTargetRepoSlug,AllowedRepospkg/workflow/update_project.go:29-30UpdateProjectConfigTargetRepoSlug,AllowedRepospkg/workflow/dispatch_workflow.go:15DispatchWorkflowConfigTargetRepoSlugRecommendation:
YAML serialization is preserved (same tag names). Callsites that read
cfg.TargetRepoSlugcontinue to work via field promotion. Effort: ~30 min per type plus testing. Impact: Removes ~25 lines of duplicated field declarations and ensures future changes to target semantics apply uniformly.Cluster 4: Other recurring safe-output fields
These fields are declared verbatim across 5+ types. They are not yet captured in
SafeOutputTargetConfig/SafeOutputFilterConfigand could form a new shared embed if duplication grows:Footer *string \yaml:"footer,omitempty"``TitlePrefix string \yaml:"title-prefix,omitempty"``Labels []string \yaml:"labels,omitempty"``CloseOlderKey string \yaml:"close-older-key,omitempty"``Recommendation: Defer until Cluster 3 lands. More embeds add cognitive overhead; only worth it if these field sets continue to grow.
Cluster 5: Intentional build-tag pairs (do NOT consolidate)
These show up as same-name duplicates but are governed by
//go:build !js && !wasmvs//go:build js || wasmand represent platform-specific implementations:RepositoryFeaturespkg/workflow/repository_features_validation.go:57pkg/workflow/repository_features_validation_wasm.go:5ProgressBarpkg/console/progress.go:31pkg/console/progress_wasm.go:7SpinnerWrapperpkg/console/spinner.go:96pkg/console/spinner_wasm.go:10These are correct — listed here only so future scanners don't churn on them.
Untyped Usages
Summary Statistics
interface{}literals in non-test files: ~2 (one test fixture)anykeyword occurrences: ~2,665 across 300 files (concentrated in YAML/JSON parsing — mostly idiomatic)map[string]anyoccurrences: ~2,839 (predominantly intentional)The codebase has clearly invested in dynamic-typing safely:
pkg/typeutilprovidesParseIntValue,ConvertToInt,ConvertToFloat,ParseBoolfor moving fromanyto concrete types, andpkg/constantsdefines semantic aliases (LineLength,CommandPrefix,WorkflowID) plus typedfs.FileModeconstants for file permissions.Category 1: Deprecated
map[string]anyfields kept for compatibilityLocation:
pkg/workflow/frontmatter_types.go:258-269Each has a typed replacement (
Tools *ToolsConfig,RuntimesTyped *RuntimesConfig,PermissionsTyped *PermissionsConfig) already in the struct.Recommendation: Plan a strict-mode migration that
map[string]anyfield if present,Category 2: GitHub Actions YAML model uses
anyfor polymorphismLocation:
pkg/cli/copilot_setup.go:139-159Envis plausibly alwaysmap[string]string.Permissionscould be a typed enum (read-all,write-all, or a map).RunsOnis genuinely polymorphic (string or list) — keep asanywith a custom unmarshaler or document why.Recommendation: Narrow
CopilotWorkflowStep.Envtomap[string]string. LeaveRunsOn/Onpolymorphic but add doc comments explaining the variance.Category 3: Function parameters typed as
frontmatter map[string]anyRoughly 470 functions in
pkg/workflowacceptmap[string]anyparameters. The vast majority sit at the YAML-parse boundary and are correct. A handful of internal helpers (e.g. inner pass-through functions that never read the map directly) could accept a typed wrapper, but no bulk refactor is justified.Recommendation: Address opportunistically when editing nearby code.
Category 4: Untyped constants — mostly OK
Sampled untyped constants like
const maxTagPeelDepth = 10,const maxFuzzyMatchSuggestions = 7,const defaultArtifactMaxUploads = 1are function-local or file-local and used in arithmetic where Go's untyped-constant rules give the right type automatically. No refactor needed.Where semantic types already exist (
pkg/constants/constants.go:LineLength,CommandPrefix,WorkflowID, file-permissionfs.FileModeconstants), they are a good template if new domain types arise.Refactoring Recommendations (prioritized)
Priority 1 — Adopt
SafeOutputTargetConfigembed in remaining 11 typesSteps:
Target/TargetRepoSlug/AllowedReposdeclarations withSafeOutputTargetConfig \yaml:",inline"``.go build ./... && go test ./pkg/workflow/...to confirm field-promotion access still resolves.Estimated effort: 3–4 hours total. Impact: Removes ~25 lines of duplicated declarations; single source of truth for safe-output target semantics.
Priority 2 — Consolidate the duplicate SHAResolver interface
Steps:
pkg/actionpins.SHAResolverand havepkg/workflowimport it directly, or move to a neutralpkg/types.pkg/workflow/maintenance_workflow.go,pkg/cli/copilot_setup*.go, and test doubles.Estimated effort: 1–2 hours. Impact: One less concept for new contributors; test doubles satisfy a single interface.
Priority 3 — Delete dead
AddCommentConfigSteps: Remove
pkg/workflow/add_comment.go:9-12. Build to confirm no references.Estimated effort: 5 minutes.
Priority 4 — Drop deprecated
map[string]anylegacy fieldsOnly do this once strict-mode rollout for typed replacements is complete:
FrontmatterConfig.MCPServers→ToolsFrontmatterConfig.Runtimes→RuntimesTypedFrontmatterConfig.Permissions→PermissionsTypedEstimated effort: ~1 day including warning-period migration. Impact: Smaller parser surface, fewer code paths to keep in sync.
Priority 5 (optional) — Extract a second safe-output content embed
Only worthwhile if more fields beyond Cluster 4's (Footer/TitlePrefix/Labels) start being added to multiple types. Premature otherwise.
Implementation Checklist
SafeOutputTargetConfigin 11 named types (Priority 1)SHAResolver/ActionSHAResolverinto one interface (Priority 2)AddCommentConfig(Priority 3)map[string]anyfrontmatter fields (Priority 4)CopilotWorkflowStep.Envfrommap[string]anytomap[string]stringAnalysis Metadata
References: §25797721831
Beta Was this translation helpful? Give feedback.
All reactions