[typist] Typist: Go Type Consistency Analysis (pkg/) #60252
Closed
Replies: 1 comment
|
This discussion was automatically closed because it expired on 2026-09-12T11:39:50.112Z.
|
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 · scope:
pkg/**/*.go, excluding_test.goExecutive Summary
I scanned roughly 1,480 non-test type declarations under
pkg/and found 11 duplicate/near-duplicate type clusters (10 real candidates for consolidation, 1 that turned out to be an intentionalwasm-build-tag split). The clearest win ispkg/cli/mcp_schema.go, which has independently hand-copied at least five "wire" structs (domainAnalysisWireSchema,mcpServerHealthDetailWire,mcpServerCrossRunHealthWire,mcpFailureSummaryWire,toolUsageSummaryWire) that already exist as real types or anonymousMarshalJSONstructs elsewhere in the same package — any field added to the real type silently won't show up in the schema doc, and vice versa.On the untyped-usage side, the good news is the codebase has already migrated off
interface{}almost entirely (16 hits total, nearly all in linter testdata fixtures, not production code). The real opportunity is amap[string]any/anypattern used for a handful of closed-shape config fields (Engine,RunsOn,RunsOnSlim,Checkout,Importsinpkg/workflow/frontmatter_types.go) whose comments already document the exact 2-3 accepted shapes — these are strong candidates for dedicated sum types with customUnmarshalYAML. I also found a couple of untyped numeric constants (DefaultRateLimitWindow,workflowCompletionWaitTimeoutMinutes) that encode a time unit only in their comment/identifier rather than their type, which is exactly the kind of thingtime.Durationexists to prevent.Serena's semantic reference tracing (
find_referencing_symbols) could not run for this analysis — the sandbox has no Go toolchain installed, so Serena'sgopls-backed language server failed to initialize. Findings below were cross-checked with targetedgrep-based reference searches instead, which confirmed the flagged duplicate structs are each self-contained (no shared references between the two copies), i.e. genuinely independent, drift-prone definitions rather than one referencing the other.Full Analysis Report
Duplicated Type Definitions
Summary Statistics
Cluster 1:
runtimeImportReference— near duplicateOccurrences: 2 · Impact: Medium
pkg/workflow/runtime_import_validation.go:38—type runtimeImportReference struct { importPath string; startLine int; endLine int }pkg/parser/frontmatter_hash.go:572—type runtimeImportReference struct { path string; startLine int; endLine int }Same unexported name in two packages, same 3-field shape (only the path field is renamed). Both files also independently maintain their own regex for parsing
{{#runtime-import ...}}macros — this looks like the whole macro-reference parsing logic was copy-pasted betweenpkg/parserandpkg/workflow.Recommendation: extract a shared
RuntimeImportReferencetype (and its parsing regex) into a common package (e.g.pkg/parserifpkg/workflowalready depends on it, or a new small shared package) and have both call sites use it.Cluster 2:
graderManifestEntry— near duplicate (schema drift risk)Occurrences: 2 · Impact: High
pkg/workflow/compiler_yaml_graders.go:86— 13 fields (ID, Name, Description, Source, Enabled, Unit, Direction, Threshold, Max, Min, Digest, Run, Config)pkg/cli/audit_report_graders.go:67— 5 fields (ID, Name, Unit, Direction, Threshold)The
pkg/clicopy is a hand-maintained subset of the manifest struct thatpkg/workflowwrites to disk. Confirmed via grep: neither file imports the other's type, so these are two independently maintained mirrors of the same on-disk JSON schema.Recommendation: move the canonical
graderManifestEntrydefinition to a shared location (e.g.pkg/workflowexports it, or both import from a smallpkg/graders/pkg/typespackage) so the reader can't silently fall out of sync with the writer.Cluster 3:
domainAnalysisWire/domainAnalysisWireSchema— exact duplicateOccurrences: 2, same package · Impact: High
pkg/cli/access_log.go:44—domainAnalysisWire(realMarshalJSONoutput type)pkg/cli/mcp_schema.go:213—domainAnalysisWireSchema(hand-copied twin for schema docs)Field-for-field, tag-for-tag identical, both in
pkg/cli. Confirmed via grep: the two names appear in disjoint files with no cross-reference.Recommendation: have
mcp_schema.goreferencedomainAnalysisWiredirectly (or generate the schema doc struct from it) instead of maintaining a byte-for-byte copy.Clusters 4–6 & 10: the
mcp_schema.go"wire twin" patternOccurrences: 4 separate instances of the same pattern · Impact: High (systemic)
pkg/cli/mcp_schema.gorepeatedly re-declares a private struct that duplicates an anonymous struct literal used inline in another file'sMarshalJSON/UnmarshalJSON:mcp_schema.go)mcpServerHealthDetailWire(:185)MCPServerHealthDetail.MarshalJSON,pkg/cli/audit_expanded.go:102mcpServerCrossRunHealthWire(:196)MCPServerCrossRunHealth.MarshalJSON,pkg/cli/audit_cross_run.go:96mcpFailureSummaryWire(:206)MCPFailureSummary.MarshalJSON,pkg/cli/logs_models.go:226toolUsageSummaryWire(:177)ToolUsageSummary(built on sharedToolUsageStatsBase),pkg/cli/logs_report_tools.go:46Recommendation: this is the single highest-leverage fix in the report. Either (a) have each
...MarshalJSONmethod use a package-level named struct instead of an anonymous literal, and pointmcp_schema.goat that same named type, or (b) generatemcp_schema.go's doc types from the real types via reflection/codegen so the two can never diverge.Cluster 7:
MCPToolCall/cachedLogsJSONLMCPToolCall— near duplicateOccurrences: 2, same package · Impact: Medium
pkg/cli/audit_report.go:212— 10 fields includingErrorpkg/cli/logs_cached_json.go:69— same 9 fields, missingErrorRecommendation: embed
MCPToolCallin the cache struct (or reuse it directly) rather than re-declaring 9 of its 10 fields.Cluster 8:
JobInfo/cachedLogsJSONLJobData— near duplicateOccurrences: 2, same package · Impact: Medium
cachedLogsJSONLJobData(pkg/cli/logs_cached_json.go:54) is a manually trimmed subset ofJobInfo(pkg/cli/logs_models.go:316) redefined for the on-disk cache format.Recommendation: embed
JobInfo(or the subset of fields as a shared sub-struct) instead of hand-copying field names/types.Cluster 9:
AuditLogEntry/FirewallLogEntry/AccessLogEntry/GatewayLogEntry— semantic duplicatesOccurrences: 4, same package (
pkg/cli) · Impact: Mediumpkg/cli/firewall_policy.go:50,pkg/cli/firewall_log.go:118,pkg/cli/access_log.go:21,pkg/cli/gateway_logs_types.go:18Four independently hand-rolled structs, each representing "one parsed line of a
*.jsonl/log file describing a network or tool request." Each has its ownTimestamp, aStatus, aMethod/Type, an outcome field (Decision), and a destination field (URL/Host/Domain) — different log sources (audit log, firewall proxy log, access log, gateway log) but similar enough shape that a shared base could plausibly be factored out, similar to the existingToolUsageStatsBase/AnalysisBasepattern already used elsewhere inpkg/cli.Recommendation: lower priority than clusters 3–6/10 since these are genuinely different log sources — but worth a
LogRecordBase{ Timestamp, Status, Decision }embed if these types grow more fields in the future.Cluster 11:
WasmStyle/SpinnerWrapper/ProgressBar— not a real duplicateSame type name declared twice in the same package, gated by
(go/redacted):build wasmvs. default build tags (pkg/console/spinner.go+spinner_wasm.go,progress.go+progress_wasm.go,pkg/styles/theme_wasm.go). This is the standard, idiomatic Go platform-split pattern, not a bug — flagged here only for completeness/transparency, no action needed.Untyped Usages
Summary Statistics
interface{}usages: 16 total, almost all inpkg/linters/**/testdata/fixtures rather than production code — the codebase has already migrated toanyany-typed params/returns/fields/map-values flagged as improvable: ~18 (listed below)const X = ...declarations repo-wide; most are fine as-is, these stood out as unit-ambiguous)Category 1: Closed-shape config fields typed
any(highest value)pkg/workflow/frontmatter_types.gohas several fields whose doc comments already spell out a closed set of 2–3 accepted YAML shapes, making them ideal candidates for a custom sum type withUnmarshalYAMLinstead ofany+ runtime type assertions at every call site:Enginefrontmatter_types.go:356EngineSetting{ Name string; Config *EngineSettingObject }RunsOn/RunsOnSlimfrontmatter_types.go:409RunsOnValuesum typeCheckoutfrontmatter_types.go:465false/ object / arrayCheckoutValuesum typeImportsfrontmatter_types.go:422normalizeStringOrStringSlice(frontmatter_trigger_helpers.go:37) as a realStringOrStringSlicetypeRawPermissionspkg/workflow/safe_jobs.go:27"write-all"/"read-all"ormap[string]stringPermissionsValue, mirroringFrontmatterConfig.PermissionsID(JSON-RPC)pkg/cli/gateway_logs_types.go:179,190RPCIDper JSON-RPC 2.0 conventionRunID/RunIDOrURLpkg/cli/mcp_tools_privileged.go:430RunIdentifier{ ID int64; URL string }with customUnmarshalJSONCategory 2:
anyparams/returns used only for a fixed set of concrete kindspkg/typeutil/convert.go:49,96,121—ParseIntValue/ConvertToInt/ConvertToFloatall takeanybut only ever handle YAML-decodedint/int64/float64/string; a type-switch helper over a named constraint would make the accepted set explicit.pkg/workflow/engine_config_parser.go:160—decodeEngineConfig(config map[string]any, target any) errorre-marshals intotarget; a Go genericfunc decodeEngineConfig[T any](config map[string]any, target *T) errorgives callers a compile-time-checked destination.pkg/workflow/observability_otlp.go:303,316—normalizeOTLPIfMissingMode/getOTLPIfMissingModenormalize into exactly"warn"/"ignore"/"", compared with==later in the same file; atype OTLPIfMissingMode stringenum would replace the string-literal comparisons.pkg/workflow/sandbox_validation.go:433andfrontmatter_trigger_helpers.go:15—getFeatureValueCaseInsensitive/extractOnTriggerValuereturn(any, bool)for values that are really always bool/string or string/map/nil.Category 3:
map[string]anyused where a decode-to-struct would remove hand-written extraction codepkg/workflow/cache_config.go:129(parseCacheMemoryEntry) and 6 sibling functions in the same file manually pull known keys (id,description,retention-days,scope,allowed-extensions,restore-only) out of a raw map — a singleyaml.Unmarshalinto a tagged struct would replace all 7 extraction functions.pkg/workflow/evals_config.go:139(parseEvalDefinition) — same pattern;EvalDefinitionalready exists as the typed result, so decoding directly into it (or a raw mirror struct) is a small change.pkg/workflow/role_checks.go:259(extractRateLimitInt+ friends) —pkg/constants/job_constants.goalready definesDefaultRateLimitMax/DefaultRateLimitWindowas the defaults for this exact config; aRateLimitConfig{ Max, Window int }struct would unify the constants with the map-based parsing.Category 4: Untyped constants missing a semantic unit
Similarly,
pkg/cli/add_interactive_engine.go:238(authMethodCopilotRequests/authMethodPAT) andpkg/constants/constants.go:316(AWFDefaultLogLevel) are small closed enums (auth method, log level) currently expressed as bare string constants — a named string type (type AuthMethod string,type LogLevel string) would let function signatures reject unknown values at compile time rather than accepting arbitrary strings.What I deliberately did not flag
deepCopyAny(v any) any(pkg/workflow/behavior_defined_engine.go:881) andrecursivelyOrderYAMLValue(value any) any(pkg/workflow/yaml.go:398) — generic recursive tree operations over already-parsed arbitrary YAML, idiomatic use ofany.Default any(pkg/workflow/mcp_scripts_parser.go:70) — mirrors JSON-Schema's polymorphicdefaultkeyword, whose shape is dictated by a sibling typedType MCPParamTypefield.Value any(pkg/console/console_types.go:50) — a reflect-style generic destination pointer, similar toflag.Value.map[string]anyusage acrosspkg/workflow's frontmatter/tools parsing andpkg/cli's codemod files — this is the standard, defensible Go pattern for ingesting loosely-typed YAML/JSON before conversion to typed structs; only the specific closed-shape fields above are called out as worth tightening.Recommendations, prioritized
Priority 1 —
pkg/cli/mcp_schema.gowire-type duplication (Clusters 3–6, 10)Consolidate the 5 duplicated "wire" structs to reference the real types/named structs instead of hand-copying them. Highest risk of silent drift since these back user-facing schema docs. Estimated effort: 3-4 hours (mostly mechanical).
Priority 2 — Cross-package near-duplicates (Clusters 1, 2, 7, 8)
Share
runtimeImportReferencebetweenpkg/parser/pkg/workflow; share/embedgraderManifestEntry,MCPToolCall,JobInfobetween the write side and the cache/read side inpkg/cli. Estimated effort: 2-3 hours each.Priority 3 — Closed-shape
anyfields in frontmatter parsingIntroduce sum types with custom
UnmarshalYAMLforEngine,RunsOn/RunsOnSlim,Checkout,Imports,RawPermissions. These are documented today only in code comments; making them real types moves that contract into the compiler. Estimated effort: 4-6 hours (touches several call sites per field).Priority 4 — Unit-bearing constants
Convert
DefaultRateLimitWindowandworkflowCompletionWaitTimeoutMinutestotime.Duration; addAuthMethod/LogLevelstring types. Estimated effort: 1-2 hours.Implementation Checklist
mcp_schema.gowire-twin structs onto the real types (Priority 1)runtimeImportReferencebetweenpkg/parserandpkg/workflow(Priority 2)graderManifestEntry,MCPToolCall,JobInfoinstead of re-declaring subsets (Priority 2)UnmarshalYAMLfor the closed-shape frontmatter fields (Priority 3)time.Duration/ named enum string types (Priority 4)Analysis Metadata
pkg/**/*.go, excluding_test.goanyusages + 5 constants), plus 16 rawinterface{}hits (mostly testdata)grep-based enumeration + targetedReadcomparison, cross-checked withgrep-based reference search (Serena's Go semantic reference tracing was unavailable - no Go toolchain in this sandbox)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