[typist] Typist - Go Type Consistency Analysis #62931
Closed
Replies: 1 comment
|
This discussion was automatically closed because it expired on 2026-09-24T11:45:27.252Z.
|
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 the ~1,379 non-test
.gofiles underpkg/(1,237 type definitions) for duplicated type definitions and weakly-typed (interface{}/any/untyped-constant) code. The headline finds: the{{#import ...}}reference-extraction logic and itsruntimeImportReferencetype are implemented twice, independently, inpkg/parserandpkg/workflow— a real risk of the two parsers drifting apart on the same template syntax. There is also agraderManifestEntrystruct defined separately inpkg/workflow(writer) andpkg/cli(reader) with only a partial field overlap, fiveTarget anyfields in the Azure DevOps safe-outputs config that should just bestring(matching every siblingTargetfield elsewhere in the package), and a couple of untyped constants shadowing enum-like types (GitHubIntegrityLevel, an impliedAWFLogLevel) that already exist nearby.The good news: the codebase already does some of this well —
MCPServerStatsBaseis a shared embedded struct across three health/stats types, and theValidationError/WorkflowValidationErrorpair both build on a commonvalidationerror.Payload. So this is not a systemic problem, just a handful of concrete, fixable spots.Full Analysis Report
Duplicated Type Definitions
Summary Statistics
pkg/)graderManifestEntry,runtimeImportReference)ValidationErrorfamily,MCPServerHealth/Statsfamily)Cluster 1:
runtimeImportReference— duplicated parsing logicType: Exact duplicate (type + logic)
Occurrences: 2 independent implementations
Impact: High — same
{{#import}}/{{#runtime-import}}template syntax parsed by two separate regexes that can silently drift apartLocations:
pkg/parser/frontmatter_hash.go:572—type runtimeImportReference struct { path string; startLine int; endLine int }, extracted viaextractRuntimeImportReferences(frontmatter_hash.go:617) using its ownregexp.MustCompilepkg/workflow/runtime_import_validation.go:38—type runtimeImportReference struct { importPath string; startLine int; endLine int }, extracted via its ownextractRuntimeImportReferences(runtime_import_validation.go:44)Recommendation:
pkg/parser(or a small shared package both already depend on) and havepkg/workflowimport it instead of re-implementing the regex.{{#import}}syntax — currently a syntax change requires remembering to update two regexes in two packages.Cluster 2:
graderManifestEntry— writer/reader split with only partial field overlapType: Exact duplicate (name), near-duplicate (fields)
Occurrences: 2
Impact: Medium-High — the JSON manifest producer and consumer are not type-linked, so a field rename in one silently breaks deserialization in the other without a compiler error
Locations:
pkg/workflow/compiler_yaml_graders.go:86(writer) — 13 fields:ID, Name, Description, Source, Enabled, Unit, Direction, Threshold, Max, Min, Digest, Run, Inline, Configpkg/cli/audit_report_graders.go:67(reader) — 5 fields:ID, Name, Unit, Direction, ThresholdRecommendation:
pkg/gradersor alongsidepkg/validationerror) that bothpkg/workflowandpkg/cliimport, with the reader using the full struct (ignoring unused fields via JSON tags) rather than a hand-maintained subset.Cluster 3:
ValidationError/WorkflowValidationError— acceptable layering, minor cleanup opportunityType: Semantic duplicate
Occurrences: 2
Impact: Low — both already build on the shared
validationerror.Payload, so this is mostly fine as-isLocations:
pkg/parser/validation_error.go—type ValidationError struct { Payload validationerror.Payload }pkg/workflow/workflow_errors.go—type WorkflowValidationError struct { Payload validationerror.Payload; Severity ErrorSeverity; Category string; Timestamp time.Time; File string; Line int; Column int }Recommendation: No urgent action —
pkg/parser.ValidationErroris a thin wrapper andpkg/workflow.WorkflowValidationErrorlegitimately adds workflow-specific fields (severity, timestamp, location). Optionally reconsider whetherparser.ValidationErrorcould just be an alias forvalidationerror.Payloaddirectly, saving one indirection layer.Cluster 4:
MCPServerHealth/Statsfamily — mostly good, wire structs duplicate the baseType: Semantic duplicate
Occurrences: 5 related types
Impact: Medium —
MCPServerStatsBase(pkg/cli/logs_models.go:211) is already correctly embedded byMCPServerStats,MCPServerHealthDetail, andMCPServerCrossRunHealth. But two separate "wire" structs —mcpServerHealthDetailWireandmcpServerCrossRunHealthWire(both inpkg/cli/mcp_schema.go) — re-declare the same fields flatly for JSON schema purposes instead of embedding the base.Recommendation:
MCPServerStatsBase(with matching JSON tags) rather than re-listingServerName/error-count fields by hand, so a future field addition to the base does not need to be manually copied into the wire schema too.Untyped Usages
Summary Statistics
anystruct fields flagged: 6 (5 in one file, 1 elsewhere)interface{}/anyparams or returns flagged: 3map[string]anyis used pervasively and correctly elsewhere for generic YAML/JSON frontmatter parsing, and was deliberately excluded as idiomatic)Category 1:
anystruct fields that should be a concrete typeImpact: High — these force
!= nilchecks instead of the!= ""checks used by every sibling field in the same packageExample: Azure DevOps
Targetfields (pkg/workflow/safe_outputs_azure_devops.go)"42","*"); every consumer (addAzureDevOpsTarget,appendAzureDevOpsTargetConstraint) treats it as a printable string, never doing numeric work on it.Target string `yaml:"target,omitempty"`in all 5 configs; switch the!= nilguards to!= "".Example: two-variant union treated as
any(pkg/workflow/tools_types.go:434)Allow bool,ExemptServers []string) populated during parsing, instead of letting any type pass compilation.Category 2:
interface{}/anyparams/returns with one real call-site typeImpact: Medium
pkg/cli/trial_helpers.go:361—saveTrialResult(filename string, result any, verbose bool) erroris only ever called withWorkflowTrialResultorCombinedTrialResult. Suggested: make it generic —func saveTrialResult[T any](filename string, result T, verbose bool) error.pkg/workflow/cache_integrity.go:141—cacheIntegrityLevelexplicitly converts the existingGitHubIntegrityLevelenum type down tostringviastring(...)and back. Suggested: returnGitHubIntegrityLeveldirectly and drop the conversion.pkg/workflow/awf_command_builder.go:505—string(constants.AWFDefaultLogLevel)conversion is a no-op today because the constant is untyped; see Category 3 below.Category 3: Untyped constants shadowing an existing (or implied) enum type
Impact: Medium
defaultCacheIntegrityLevelpkg/workflow/cache_integrity.go:17"none", default for a field typedGitHubIntegrityLevelconst defaultCacheIntegrityLevel GitHubIntegrityLevel = "none"AWFDefaultLogLevelpkg/constants/constants.go:306"info"; caller already writesstring(...)around it, implying an intended distinct typetype AWFLogLevel stringwith named levels;const AWFDefaultLogLevel AWFLogLevel = "info"defaultCodeCoverageWaitForProcessingTimeoutpkg/workflow/publish_code_coverage.go:18160, comment says "seconds" but no unit in the typeSecondstype if/when other duration constants get the same treatmentRefactoring Recommendations
Priority 1: High — deduplicate the
{{#import}}reference parserConsolidate
runtimeImportReferenceand its extraction regex into one package (pkg/parser) imported bypkg/workflow. Effort: 2-3 hours. Impact: eliminates the risk of the two implementations silently diverging on template syntax.Priority 2: Medium — share the grader manifest type between writer and reader
Extract
graderManifestEntryinto a shared location used by bothpkg/workflow(writer) andpkg/cli(reader). Effort: 1-2 hours. Impact: compiler-enforced consistency between manifest producer and consumer.Priority 3: Medium — fix the Azure DevOps
Target anyfieldsChange all 5
Target anyfields toTarget string, matching every siblingTargetfield in the package. Effort: under 1 hour. Impact: removes an inconsistentnil-check pattern and closes off invalid non-string YAML values at parse time.Priority 4: Low-Medium — tighten the remaining untyped constants and params
Type
defaultCacheIntegrityLevelasGitHubIntegrityLevel, introduceAWFLogLevel, and makesaveTrialResultgeneric. Effort: 2-3 hours combined. Impact: closes a few compiler-blind spots without touching any hot paths.Implementation Checklist
runtimeImportReferenceparsing intopkg/parser, updatepkg/workflowto import itgraderManifestEntrybetweenpkg/workflowandpkg/cliTarget anyfields toTarget stringPrivateToPublicFlowsas an explicit union struct instead ofanydefaultCacheIntegrityLevelasGitHubIntegrityLevel; drop thestring(...)round-trip incacheIntegrityLevelAWFLogLevelforconstants.AWFDefaultLogLevelsaveTrialResultgeneric over its two call-site result typespkg/workflow,pkg/cli,pkg/parserall have relevant test coverage already)Analysis Metadata
pkg/)find_referencing_symbolstool was unavailable in this environment (its Go/TypeScript/Bash language servers require toolchains —go,node— not installed in this sandbox), so reference tracing fell back to targeted Grep searches instead.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