[typist] Typist Go Type Consistency Analysis #52015
Closed
Replies: 1 comment
|
This discussion has been marked as outdated by Typist - Go Type Analysis. A newer discussion is available at Discussion #52283. |
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 all 1,205 non-test
.gofiles underpkg/(1,129 type definitions across ~33 packages), looking for duplicated type definitions and untyped (interface{}/any/untyped-constant) usage. The good news first: this codebase is already quite disciplined about avoiding type duplication. Most same-named types across packages (ActionPin,ContainerPin,InputDefinition,SanitizeOptions,LogMetrics,ToolCallInfo, etc.) turned out to be intentionaltype X = otherpkg.Xre-export aliases pointing at one canonical definition, not copy-pasted redundancy — so I excluded those from the findings below.The real duplication is small but concrete, concentrated in
pkg/cli:JobStep/JobStepDataare 100%-identical structs that require an explicit conversion (JobStepData(step)) every time data crosses between them, and four independently-defined "log entry" structs (AccessLogEntry,FirewallLogEntry,AuditLogEntry,GatewayLogEntry) all model the same concept — a parsed log line — with no shared base type. On the untyped-usage side, the most actionable findings aren'tinterface{}(that's rare here, and appropriately reserved for genuine YAML/JSON tree-walking) but a handful of untyped numeric/duration constants inpkg/constantswhose units (bytes, minutes, hours, credits) are documented only in comments, not enforced by the compiler — plus a couple ofany-returning helper functions inpkg/parser/schema_suggestions.gothat always return one concrete type.Full Analysis Report
Duplicated Type Definitions
Summary Statistics
.gounderpkg/)ActionYAMLInput,ActionPin,ActionPinsData,ContainerPin,SHAResolver,InputDefinition,SanitizeOptions,LogMetrics,ToolCallInfo) — each is atype X = otherpkg.Xalias inpkg/workflow/pkg/clipointing at one canonical definition inpkg/actionpins,pkg/types, orpkg/stringutil. Verified directly, e.g.pkg/workflow/action_pins.go:25is literallytype ActionPin = actionpins.ActionPin.JobStep/JobStepData)JobInfo/JobData), 1 questionable (ExperimentInfo/ExperimentData— see note)Cluster 1:
JobStep/JobStepData— exact duplicateImpact: Medium — same shape defined twice, forcing a manual conversion at the one place they meet
Locations:
pkg/cli/logs_models.go—type JobStep struct { Name, Status, Conclusion string }pkg/cli/audit_report.go—type JobStepData struct { Name, Status, Conclusion string }Evidence of the pain point (
pkg/cli/audit_report.go:385):Because the two types are structurally identical but nominally distinct,
buildAuditJobshas to map over every step just to do a bare struct conversion — this line of code exists purely to satisfy the type system, not because the data differs.Recommendation: Drop
JobStepDataand useJobStep(with its existingjsontags, if any need adding) inaudit_report.godirectly. Delete thesliceutil.Map/conversion call.Cluster 2:
JobInfo/JobData— near duplicate (Info/Data synonym)Impact: Medium —
JobInfois the more widely used and embedded type (JobInfoWithDurationembeds it; referenced inlogs_github_api.go,logs_summary.go, and 8+ test files), whileJobDatainaudit_report.goduplicatesName/Status/Conclusionbut adds aDuration stringfield (pre-formatted) instead of theStartedAt/CompletedAt time.TimepairJobInfouses, and nests[]JobStepDatainstead of[]JobStep.Recommendation: Since
JobDatais really "aJobInforendered for the audit report, with duration pre-formatted as a string," consider deriving it fromJobInfovia a constructor function (func toJobData(j JobInfo) JobData) rather than an independently maintained struct, or add aFormattedDuration() stringmethod toJobInfoand dropJobDataentirely onceJobStepData(Cluster 1) is removed.Cluster 3:⚠️
ExperimentInfo/ExperimentData— name-only near duplicateNote: this pair was flagged by the automated name-synonym pass (
Info/Datasuffix match) but the fields don't actually overlap:ExperimentInfo(pkg/cli/experiments_command.go) is{WorkflowID, Branch string; Experiments, TotalRuns int; LastRun string}, whileExperimentData(pkg/cli/audit_report_experiments.go) is{Assignments map[string]string; CumulativeCounts map[string]map[string]int}— a completely different shape for a different purpose (variant-assignment bookkeeping vs. a list-view summary). This is very likely just naming-convention coincidence, not duplication. No action needed beyond, optionally, renaming one of them to avoid the misleading impression of overlap (e.g.ExperimentAssignmentData).Cluster 4: Log-entry structs — semantic duplicate
Impact: Low/Medium — four structs, one per log source, all modeling "a parsed line from a log," with no shared base and largely overlapping intent:
pkg/cli/access_log.go—AccessLogEntry {Timestamp, Duration, ClientIP, Status, Size, Method, URL, User, Hierarchy, Type string}pkg/cli/firewall_log.go—FirewallLogEntry {Timestamp, ClientIPPort, Domain, DestIPPort, Proto, Method, Status, Decision, URL, UserAgent string}pkg/cli/firewall_policy.go—AuditLogEntry {Schema string, Timestamp float64, Client, Host, Dest, Method, Decision, URL string, Status int}pkg/cli/gateway_logs_types.go—GatewayLogEntry(19 fields — timestamp, level, type, event, server/tool identifiers, duration, sizes, status, error, tags, author info)Recommendation: These genuinely differ in source format (access log vs. firewall log vs. audit JSON vs. gateway JSONL) so full unification isn't warranted — but a shared
LogEntryCommon { Timestamp string; Method, URL, Status string }embed (mirroring the pattern the codebase already uses forAnalysisBase/DomainBucketsinpkg/cli/firewall_policy.go, which the team's own comment says exists to "eliminate code duplication") would cut the repeated field declarations and give parsers/renderers a common interface to work against.Untyped Usages
Summary Statistics
interface{}usages: rare and mostly confined to test/linter-testdata files (no high-value production hits found)anyusages requiring attention: 5 (4 function signatures, 1 struct field)pkg/constantsCategory 1:
any-returning helpers that always return one concrete typeImpact: Medium — these discard compile-time guarantees for callers, in code that's otherwise fully within the maintainers' control (not truly polymorphic).
generateStringExample—pkg/parser/schema_suggestions.go:386Every return path is a
string. Fix:func generateStringExample(schema map[string]any) string.generateArrayExample—pkg/parser/schema_suggestions.go:405Both return paths produce
[]any. Fix:func generateArrayExample(schema map[string]any) []any.generateObjectExample—pkg/parser/schema_suggestions.go:417Always constructs and returns
map[string]any. Fix:func generateObjectExample(schema map[string]any) map[string]any.LayoutEmphasisBox—pkg/console/layout_wasm.go:16The
colorparameter is typedanybut is never read in the function body — it's dead. Fix: remove the parameter, or type it concretely (e.g.lipgloss.Color) and actually apply it.Category 2:
any-typed struct field with unclear contractFormField.Value—pkg/console/console_types.go:50The comment implies a small closed set of pointer kinds, but the field is
any, and it appears to have no live (non-wasm-stub) consumer that unwraps it via type assertion. Fix: either delete the unused path, or replace with a small tagged interface (FormValueimplemented by*StringValue/*BoolValue) keyed off the existingTypediscriminator.Category 3: Untyped constants with an implied unit
Impact: Medium — the unit (bytes, minutes, hours, credits) is documented only in a comment, not enforced by the type system, so a future mix-up (e.g. passing a byte count where a duration is expected) would compile silently.
DefaultMCPGatewayPayloadSizeThresholdpkg/constants/constants.go:245= 524288ByteSize = 524288DefaultMaxDailyAICreditspkg/constants/constants.go:329= "5000"(string!)int64, format to string only at the template-embedding site — note its siblingsDefaultMaxAICredits/DefaultDetectionMaxAICreditsare already typedint64DefaultMaxRunspkg/constants/constants.go:332= 500int64 = 500(match sibling constants)MaxSymlinkDepthpkg/constants/constants.go:615= 5int = 5DefaultRateLimitWindowpkg/constants/job_constants.go:260= 60(minutes, bare int)time.Duration = 60 * time.Minute, matching howDefaultToolTimeoutalready usestime.Durationa few dozen lines earlierDefaultActionFailureIssueExpiresHourspkg/workflow/repo_config.go:63= 24 * 7(hours)time.Duration = 24 * 7 * time.HourmaxScannerBufferSizepkg/cli/gateway_logs_types.go:15= 1024 * 1024(reused in 6+ files)int = 1024 * 1024, consider promoting topkg/constantsgiven its reusemaxRedirectDepthpkg/cli/update_redirects.go:18= 20int = 20; same recursion-depth-limit pattern asMaxSymlinkDepthAlso worth noting:
defaultCacheIntegrityLevelinpkg/workflow/cache_integrity.go:17is a bare"none"string standing in for a proper enum type (GitHubIntegrityLevel) that already exists in the same package with typed constants likeGitHubIntegrityApproved. Because it's untyped, two call sites dostring(github.MinIntegrity)conversions just to compare against it. Adding aGitHubIntegrityNonetyped constant would remove both conversions.Refactoring Recommendations
Priority 1: Remove the
JobStep/JobStepDataconversion (Cluster 1)Delete
JobStepData, useJobStepdirectly inaudit_report.go, drop thesliceutil.Mapconversion.Estimated effort: 30–60 minutes · Impact: Medium
Priority 2: Fix
any-returning schema-suggestion helpersNarrow
generateStringExample/generateArrayExample/generateObjectExamplereturn types inpkg/parser/schema_suggestions.go; remove the deadcolor anyparameter fromLayoutEmphasisBox.Estimated effort: 1–2 hours · Impact: Medium (mostly clarity + removes dead code)
Priority 3: Type the constants in
pkg/constantsthat carry an implied unitIntroduce
ByteSize/reusetime.Durationfor the eight constants listed above; fixDefaultMaxDailyAICreditsspecifically since it's a string standing in for a number.Estimated effort: 2–3 hours (small diffs, but touches call sites doing manual parsing/formatting) · Impact: Medium — prevents unit-confusion bugs at compile time
Priority 4 (optional, lower confidence): Consolidate
JobInfo/JobDataand give the four log-entry structs a shared baseEstimated effort: 4–6 hours combined · Impact: Low/Medium — nice-to-have consistency, not urgent
Implementation Checklist
JobStepData, useJobStepdirectly (Priority 1)generateStringExample/generateArrayExample/generateObjectExample; remove deadcolor anyparam fromLayoutEmphasisBox(Priority 2)ByteSize,time.Duration) to the 8 flagged constants inpkg/constants/pkg/workflow/repo_config.go/pkg/cli(Priority 3)defaultCacheIntegrityLevelto use the existingGitHubIntegrityLevelenumJobInfo/JobData; add a shared base for the 4 log-entry structsExperimentDataif theInfo/Datanaming coincidence is considered confusing (no structural change needed)Analysis Metadata
pkg/)anyusages, 8 untyped constants)get_symbols_overview,find_referencing_symbols) + targeted pattern matching, cross-referenced to confirm real vs. alias duplicationAll reactions