Repository navigation
[typist] 🔤 Typist - Go Type Consistency Analysis #59203
Closed
Replies: 1 comment
|
This discussion was automatically closed because it expired on 2026-09-08T11:42:05.776Z.
|
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.
Analysis of repository: github/gh-aw
Executive Summary
I scanned 1,353 non-test
.gofiles underpkg/(1,225 struct/interface definitions) for duplicated types and untyped (interface{}/any) usage. The good news: exact-name type duplication is rare — of ~1,225 type names, only two represent genuine cross-package duplicates (graderManifestEntry,runtimeImportReference); the rest of the name collisions are either intentional build-tag platform variants (SpinnerWrapper,ProgressBarinpkg/console) or a well-designed sharedvalidationerror.Payloadembedding pattern that other packages deliberately reuse rather than duplicate.The more interesting findings are (a) structurally-duplicated types that were given different names — three generations of "HTTP request log entry" structs in
pkg/cli, and a pair of near-identical signal-aware loop-options structs — and (b) eight concreteinterface{}/anyusages that force avoidable runtime type assertions, several with a schema/validation drift risk similar to the confirmedgraderManifestEntryduplicate. Fixing the top few items (Azure DevOpsTarget anyfields, the OTLPif-missingstring mode, and the two duplicated schema-drift structs) would remove a dozen-plus type assertions and close a couple of real "add a field in one place, forget the other" traps.Full Analysis Report
Duplicated Type Definitions
Summary statistics
pkg/)Cluster 1:
graderManifestEntry— exact-name duplicate with schema driftType: Near-duplicate (same name, diverging fields)
Impact: High — silent data loss risk
Locations:
pkg/workflow/compiler_yaml_graders.go:86— full definition (11 fields:ID, Name, Description, Source, Enabled, Unit, Direction, Threshold, Max, Min, Digest, Run, Config)pkg/cli/audit_report_graders.go:67— partial definition (5 fields:ID, Name, Unit, Direction, Threshold)The
clipackage's copy is missingDescription,Source,Enabled,Max,Min,Digest,Run, andConfig. Since theworkflowpackage writes the manifest andclireads it back for audit reports, any future field added to the writer's struct silently vanishes when parsed by the reader unless someone remembers to update both.Recommendation: Move
graderManifestEntryto a shared package (e.g.pkg/gradersorpkg/types) and have bothworkflowandcliimport it.Estimated effort: 1-2 hours · Benefits: single source of truth for the manifest schema.
Cluster 2:
runtimeImportReference— exact-name duplicateType: Near-exact duplicate (one field renamed)
Impact: Medium — logic duplication risk
Locations:
pkg/workflow/runtime_import_validation.go:53—{ importPath string; startLine int; endLine int }pkg/parser/frontmatter_hash.go:572—{ path string; startLine int; endLine int }Both model a
{{#runtime-import ...}}macro reference with identical shape and purpose, in two different packages, with near-identical parsing regexes nearby. This has the look of a copy-paste during a package split.Recommendation: Consolidate into one shared type (likely
pkg/parser, sinceworkflowalready depends on it) and remove the second implementation.Estimated effort: 1-2 hours · Benefits: one parsing implementation to maintain and test.
Cluster 3: HTTP/firewall request-log entries — 3-4 structs, same concept, different names
Type: Semantic duplicate
Impact: Medium — three "generations" of the same log schema
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, Timestamp float64, Client, Host, Dest, Method, Status int, Decision, URL}pkg/cli/firewall_policy.go:63—EnrichedRequest{Timestamp float64, Host, Status int, RuleID, Action, Reason}AuditLogEntryandFirewallLogEntryin particular cover the same six core concepts (client, destination, method, status, decision, URL) but disagree on field names (Client/ClientIPPort,Dest/DestIPPort,Host/Domain) and types (Status stringvsStatus int,Timestamp stringvsfloat64) — these look like the legacy squid-style access log, the firewall log, and the neweraudit.jsonlschema, each hand-rolled separately.Recommendation: Introduce one canonical "request record" type with format-specific parse functions converting into it, rather than bespoke structs per log generation.
Estimated effort: 3-4 hours · Benefits: one place to fix a parsing/severity bug instead of three.
Cluster 4:
PollOptionsvsRepeatOptions— near-identical signal-aware loop optionsType: Semantic duplicate
Impact: Low-Medium
Locations:
pkg/cli/signal_aware_poll.go:35—PollOptions{Ctx, PollInterval, Timeout, PollFunc, StartMessage, ProgressMessage, SuccessMessage, Verbose}pkg/cli/retry.go:20—RepeatOptions{Ctx, RepeatCount, StartMessage, RepeatMessage, ExecuteFunc, CleanupFunc, UseStderr}Both implement "run a function in a loop with graceful Ctrl-C handling and console status messages," down to a shared verbatim doc-comment line — a strong copy-paste signal. They differ only in the loop-termination mechanism (time-based vs count-based) and message-field naming.
Recommendation: Extract a shared base (
Ctx,StartMessage,Verbose/UseStderr) that both embed, keeping only the two loop-specific fields separate.Estimated effort: 2 hours · Benefits: one place to fix signal-handling bugs.
(A minor, low-value case was also spotted:
permissionEntry/repoEntryinpkg/cli/mcp_server_cache.go:22,27are both trivial{value string; timestamp time.Time}cache records living side by side — not worth a dedicated fix unless a broader cache-layer refactor is already underway.)Several other candidate clusters (
SetupAuthOptions/SetupRepositoryCheckOptions,ValidationResult/PermissionsValidationResult/ChecksResult, the various*Cachetypes) were checked and found to be genuinely distinct in shape and purpose — some even have explicit comments explaining why they're kept separate. No action needed there.Untyped Usages
Summary statistics (excluding
_test.goandpkg/linters/, whoseinterface{}/anyusage is idiomaticgo/asttraversal code)interface{}/anyfindings identified: 81.
Target anyduplicated across 5 Azure DevOps safe-output configs — should bestringpkg/workflow/safe_outputs_azure_devops.go:38,50,55,62,68(UpdateWorkItemConfig.Target,CommentOnWorkItemConfig.Target,AssignWorkItemConfig.Target,LinkWorkItemsConfig.Target,UploadWorkItemAttachmentConfig.Target) and helpersaddAzureDevOpsTarget(builder, target any)/appendAzureDevOpsTargetConstraint(constraints, target any)(lines 139, 238).Targetfield is a plainstring, validated centrally byvalidateTargetValue(configName, target string)inpkg/workflow/safe_outputs_validation.go:86-224. Only the Azure DevOps configs useany, which silently excludes them from that shared validation path — and a test literally asserts the value is the string"42".string.2.
ToolConfig.GetAnyused only to immediately type-assertpkg/workflow/mcp_config_types.go:66,117; call sitespkg/workflow/mcp_config_custom.go:533(→.(map[string]any)) and:627(→.(bool)).ToolConfigalready hasGetString/GetStringArray/GetStringMap. AGetBool(key) (bool, bool)accessor would remove the assertion and make a bad value (e.g.required: "yes") fail fast instead of silently no-oping.3.
ManifestFilesPolicy *string— stringly-typed enum with inconsistent valid-value setspkg/workflow/create_pull_request.go:112,156andpkg/workflow/push_to_pull_request_branch.go:30,188.create_pull_request.go:156allows{"blocked","allowed","fallback-to-issue","request_review"}, whilepush_to_pull_request_branch.go:188only allows{"blocked","allowed","fallback-to-issue"}(missingrequest_review). No shared type means a future value change in one is easy to forget in the other — the same drift pattern as thegraderManifestEntryduplicate above.type ProtectedFilesPolicy stringwith named constants and one sharedIsValid()/validator, reused by both configs.4. OTLP
if-missingmode passed as barestringwith scattered literal comparisonspkg/workflow/observability_otlp.go:303-312(normalizeOTLPIfMissingMode),:316(getOTLPIfMissingModereturnsstring), compared against literals at:893, 935, 938."","error","warn","ignore"), but a typo like"ignroe"silently normalizes away instead of failing to compile.type OTLPIfMissingMode stringwithOTLPIfMissingError/Warn/Ignore/Unsetconstants.5.
WorkflowTrialResult.SafeOutputs map[string]anyfor a fixed JSONL shapepkg/cli/trial_types.go:9,pkg/cli/trial_support.go:20; consumed via a two-level assertion chain inpkg/cli/trial_result_helpers.go:14-35.type SafeOutputsArtifact struct { Items []map[string]any; Errors []string }— onlyErrorsis ever read.6.
MCPGatewayAgentPolicy.AllowOnly map[string]anyhas exactly one producer shapepkg/workflow/tools_types.go:560; only ever constructed atpkg/workflow/enclaves.go:125-128as{"repos": []string, "min-integrity": string}.type MCPGatewayAllowOnlyPolicy struct { Repos []string; MinIntegrity string }.7.
statsJSONResponse.Stats map[string]anyneeds 4 chained assertions for 2 known numeric fieldspkg/workflow/stats_jsonl_logs.go:12,64-84; sibling pattern inpkg/workflow/pi_logs.go:80-84.statsJSONStats{ Models map[string]struct{ InputTokens, OutputTokens float64 }; Tools map[string]any }letsencoding/jsondo the work instead of 4 runtime assertions.8.
copilotEventsJSONLEntryData.Usage map[string]anyduplicates an already-typed sibling structpkg/cli/copilot_events_jsonl.go:76, read via alias-key lookups at:354-355,375-380. The same file already defines a properly typedcopilotUsageMetrics(:103-108) for an analogous event — the untyped map only exists to handle two alternate JSON key spellings, which a customUnmarshalJSONcould resolve just as well.Refactoring Recommendations
Priority 1: Fix the two schema-drift risks (Clusters 1 & 3 above)
graderManifestEntryto a shared location; update both importers.ProtectedFilesPolicyas a shared enum-like type used by both PR safe-output configs.Priority 2: Replace the 5 Azure DevOps
anytargets and the OTLP mode stringPriority 3: Consolidate
runtimeImportReference, the request-log-entry cluster, andPollOptions/RepeatOptionsPriority 4: Replace the remaining
map[string]anyusages (items 2, 5, 6, 7, 8) with typed structs/accessors where a single fixed shape already existsImplementation Checklist
graderManifestEntryinto a shared typeProtectedFilesPolicyenum for manifest-files policyTarget anyfields (+2 helper params) tostringOTLPIfMissingModetyped enumruntimeImportReferencebetweenworkflowandparserpkg/clifirewall/access log structsPollOptions/RepeatOptionsmap[string]anyusages (ToolConfig.GetAny, trial artifact, MCP gateway allow-only policy, stats JSONL, copilot usage map)Analysis Metadata
pkg/)_test.goandpkg/linters/analyzer boilerplate)Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
api.anthropic.comTo allow these domains, add them to the
network.allowedlist in your workflow frontmatter:See Network Configuration for more information.
All reactions