Repository Quality Improvement: Constants Package Domain Segregation #23557
Closed
Replies: 1 comment
-
|
This discussion has been marked as outdated by Repository Quality Improvement Agent. A newer discussion is available at Discussion #23730. |
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.
-
🎯 Repository Quality Improvement Report — Constants Package Domain Segregation
Analysis Date: 2026-03-30
Focus Area: Constants Package Domain Segregation
Strategy Type: Custom (first run, 60% custom probability)
Custom Area: Yes —
pkg/constants/constants.gois a 1087-line monolith mixing 12+ distinct conceptual domains. Splitting it into domain-specific files dramatically improves discoverability and reduces cognitive overhead.Executive Summary
pkg/constants/constants.gohas grown to 1087 lines containing 14 semantic types, 112 const groups, and several largevardeclarations—all in a single file imported by 121 packages. The file mixes completely unrelated concerns: runtime language version defaults, engine option metadata, MCP server IDs, GitHub Actions job names, step IDs, firewall configuration, feature flags, security validation lists, and Docker mount constants.This monolith makes it unnecessarily difficult to locate a specific constant (e.g., "which file defines the default Node.js version?"), increases review noise (every PR that touches any constant shows a diff against this single massive file), and will accumulate further without intervention.
Go's package model fully supports splitting a package across multiple files—all files in the same
pkg/constants/directory remain in the same package, so no import paths change. The refactor is purely organizational and carries zero runtime risk.Separately, several CLI and workflow package files exceed 800–1300 lines, and the
audit_cross_run_render.gofile writes markdown output usingfmt.Println/fmt.Printfto stdout rather than a buffered writer, which mixes concerns and makes testing harder.Full Analysis Report
Focus Area: Constants Package Domain Segregation
Current State Assessment
Metrics Collected:
constants.goline countconstkeywords)constantspkg/constants/fmt.Errorfwith%wwrappinginterface{}instead ofanyFindings
Strengths
interface{}— the codebase already uses the modernanyalias throughoutfmt.Errorfuse proper%wwrappingworkflow_errors.goprovides a rich, structuredWorkflowValidationErrortype with Field/Value/Reason/Suggestion fieldsschema_suggestions.go) include fuzzy matching and "Did you mean?" guidanceAreas for Improvement
gateway_logs.go(1332),constants.go(1087),audit_report_render.go(1045),logs_report.go(1011),trial_command.go(1007)return errand 102+ barereturn nil, errwithout context wrappingfmt.Print*—audit_cross_run_render.gowrites structured markdown to stdout withfmt.Println/Printfrather than accepting anio.Writer, making it hard to testDetailed Analysis
constants.go Domain Breakdown
The file contains these clearly separable domains:
constants_types.goconstants_engines.goconstants_runtimes.goconstants_timeouts.goconstants_mcp.goconstants_infrastructure.goconstants_workflow_jobs.goconstants_features.goconstants_security.goconstants_workflow_schema.goEach resulting file would be 25–210 lines — well within the 200-line guideline.
🤖 Tasks for Copilot Agent
NOTE TO PLANNER AGENT: The following tasks are designed for GitHub Copilot coding agent execution. Please split these into individual work items for the agent to process one at a time.
Improvement Tasks
Task 1: Split
constants.gointo domain-specific files — Part A (Types + Engines + Runtimes)Priority: High
Estimated Effort: Medium
Focus Area: Constants Package Domain Segregation
Description:
The
pkg/constants/constants.gofile is 1087 lines and mixes 12+ unrelated domains. This task covers the first split: extract the semantic type definitions, engine constants, and runtime version defaults into dedicated files. All files remain in thepackage constantspackage, so no import paths change anywhere in the codebase.Acceptance Criteria:
pkg/constants/constants_types.gocontaining all 14 type definitions (LineLength, Version, FeatureFlag, URL, ModelName, JobName, StepID, CommandPrefix, WorkflowID, EngineName, DocURL, MCPServerID, EngineOption struct, SystemSecretSpec struct) with their String()/IsValid() methodspkg/constants/constants_engines.gocontaining engine-related constants:CopilotEngine,ClaudeEngine,CodexEngine,GeminiEngine,AgenticEngines,EngineOptions,SystemSecrets,GetEngineOption(),GetAllEngineSecretNames()pkg/constants/constants_runtimes.gocontaining allDefault*Versionruntime constants (Node, Python, Go, Ruby, DotNet, Java, Elixir, Haskell, Deno, Bun) and image constants (DefaultNodeAlpineLTSImage, etc.)constants.gomake build && make test-unitto confirm no breakageconstants.goshould be noticeably shorter (target <700 lines after this task)Code Region:
pkg/constants/constants.go(lines 1–510 approximately)Task 2: Split
constants.go— Part B (MCP + Infrastructure + Workflow Jobs + Features + Security)Priority: High
Estimated Effort: Medium
Focus Area: Constants Package Domain Segregation
Description:
Continuation of the constants split. Extract the remaining 5 domains from
constants.gointo dedicated files. After this task,constants.goshould contain only the few items that don't fit cleanly into a domain (CLIExtensionPrefix, GetWorkflowDir, MaxSymlinkDepth, SharedWorkflowForbiddenFields, etc.) and be under 150 lines.Acceptance Criteria:
pkg/constants/constants_mcp.gowith MCP server IDs (SafeOutputsMCPServerID, MCPScriptsMCPServerID, AgenticWorkflowsMCPServerID), MCP version constants, DefaultMCPRegistryURL, DefaultMCPGatewayVersion/Container/PayloadDir/PayloadSizeThreshold, DefaultMCPSDKVersion, DefaultGitHubMCPServerVersion, DefaultReadOnlyGitHubTools, DefaultGitHubToolsLocal/Remote, DefaultGitHubTools, DefaultBashTools, CopilotStemCommandspkg/constants/constants_infrastructure.gowith AWF/firewall constants (AWFDefaultCommand, AWFProxyLogsDir, AWFAuditDir, FirewallAuditArtifactName, AWFDefaultLogLevel, AWFExcludeEnvMinVersion, DefaultFirewallVersion, DefaultFirewallRegistry, DefaultAPMActionVersion, DefaultAPMVersion, DefaultGitHubLockdown), container mount constants (GhAwRootDir, GhAwRootDirShell, DefaultGhAwMount, DefaultGhBinaryMount, DefaultTmpGhAwMount, DefaultWorkspaceMount, DefaultActivationJobRunnerImage)pkg/constants/constants_workflow_jobs.gowith all job name constants (AgentJobName, ActivationJobName etc.), step ID constants (CheckMembershipStepID etc.), artifact name constants, output name constants, and rate limit defaultspkg/constants/constants_features.gowith all FeatureFlag const values (MCPScriptsFeatureFlag, MCPGatewayFeatureFlag, DisableXPIAPromptFeatureFlag, CopilotRequestsFeatureFlag, DIFCProxyFeatureFlag)pkg/constants/constants_security.gowith SafeWorkflowEvents, AllowedExpressions, DangerousPropertyNames, DefaultAllowedDomainsconstants.goshould be under 150 lines after both Task 1 and Task 2make build && make test-unitto confirm no breakageCode Region:
pkg/constants/constants.go(lines 510–1087 approximately)Task 3: Add Context to Unwrapped Error Returns in High-Traffic Packages
Priority: Medium
Estimated Effort: Medium
Focus Area: Error Handling Quality
Description:
The codebase has ~340 bare
return err/return nil, errpatterns that pass errors up without adding context. While not all of these are wrong (e.g., immediately after a top-level user-facing function), those in deeply-nested internal functions lose callsite context. This task focuses on thepkg/workflow/package which has the most bare returns in functions that benefit from wrapping context.Acceptance Criteria:
return errinpkg/workflow/(excluding*_test.go)fmt.Errorf("context: %w", err)in internal helper functions where the error's origin would be unclear from the message alonecheckout_manager.go,cache.go, andcompiler_orchestrator_workflow.go— the three largest workflow files"failed to X: %w"formatmake build && make test-unitto confirm no breakageCode Region:
pkg/workflow/checkout_manager.go,pkg/workflow/cache.go,pkg/workflow/compiler_orchestrator_workflow.goTask 4: Refactor
audit_cross_run_render.goto Acceptio.Writerfor TestabilityPriority: Medium
Estimated Effort: Small
Focus Area: Testability & Output Routing
Description:
pkg/cli/audit_cross_run_render.go(360 lines) renders a markdown report by callingfmt.Printlnandfmt.Printfdirectly, hardcoded to stdout. This makes it impossible to unit test the rendering output without capturing stdout, and it prevents callers from redirecting the output. The functions should accept anio.Writerparameter.Acceptance Criteria:
renderCrossRunReportMarkdown(report *CrossRunAuditReport)to accept anio.Writeras first parameter:renderCrossRunReportMarkdown(w io.Writer, report *CrossRunAuditReport)fmt.Println(...)andfmt.Printf(...)calls inside this function (and any helpers it calls within the same file) withfmt.Fprintln(w, ...)andfmt.Fprintf(w, ...)renderCrossRunReportMarkdownto passos.Stdoutaudit_cross_run_render_test.go(create if not exists) that callsrenderCrossRunReportMarkdownwith abytes.Bufferand asserts the output contains expected stringsmake build && make test-unitto confirm no breakageCode Region:
pkg/cli/audit_cross_run_render.go, callers inpkg/cli/audit_cross_run.goto:
Replace all
fmt.Println(x)withfmt.Fprintln(w, x)and allfmt.Printf(format, args...)withfmt.Fprintf(w, format, args...)throughout the function body and any unexported helper functions called exclusively by it within the same file.Add
"io"to the import block.Find all callers of
renderCrossRunReportMarkdowninpkg/cli/and update them to passos.Stdoutas the first argument.Create
pkg/cli/audit_cross_run_render_test.gowith at least one test:Verify:
make build && make test-unitBeta Was this translation helpful? Give feedback.
All reactions