diff --git a/DEADCODE.md b/DEADCODE.md index c7280e9726..519afa9f52 100644 --- a/DEADCODE.md +++ b/DEADCODE.md @@ -8,12 +8,25 @@ deadcode ./cmd/... ./internal/tools/... 2>/dev/null **Critical:** Always include `./internal/tools/...` — it covers separate binaries called by the Makefile (e.g. `make actions-build`). Running `./cmd/...` alone gives false positives. +## Correct methodology + +`deadcode` analyses the production binary entry points only. **Test files compile into a separate test binary** and do not keep production code alive. A function flagged by `deadcode` is dead regardless of whether test files call it. + +**Correct approach:** +1. `deadcode` flags `Foo` as unreachable +2. `grep -rn "Foo" --include="*.go"` shows callers only in `*_test.go` files +3. **Delete `Foo` AND any test functions that exclusively test `Foo`** + +**Wrong approach (batch 4 mistake):** treating test-only callers as evidence the function is "live" and skipping it. + +**Exception — `compiler_test_helpers.go`:** the 3 functions there (`containsInNonCommentLines`, `indexInNonCommentLines`, `extractJobSection`) are production-file helpers used by ≥15 test files as shared test infrastructure. They're dead in the production binary but valuable as test utilities. Leave them. + ## Verification after every batch ```bash go build ./... go vet ./... -go vet -tags=integration ./... # catches integration test files invisible without the tag +go vet -tags=integration ./... # catches integration test files invisible without this tag make fmt ``` @@ -23,39 +36,137 @@ make fmt - `compiler.ParseWorkflowString` - `compiler.CompileToYAML` -**Test helpers** — `pkg/workflow/compiler_test_helpers.go` shows 3 dead functions but is used by 15 test files. Don't delete it. +**`compiler_test_helpers.go`** — shows 3 dead functions but serves as shared test infrastructure for ≥15 test files. Do not delete. **Constant/embed rescue** — Some otherwise-dead files contain live constants or `//go:embed` directives. Extract them before deleting the file. --- -## Current dead code (276 functions, as of 2026-02-28) - -Run the command above to regenerate. Top files by dead function count: - -| File | Dead | Notes | -|------|------|-------| -| `pkg/workflow/js.go` | 17 | Get*/bundle stubs; many have no callers anywhere | -| `pkg/workflow/compiler_types.go` | 17 | `With*` option funcs + getters; check WASM first | -| `pkg/workflow/artifact_manager.go` | 14 | Many test callers; do last | -| `pkg/constants/constants.go` | 13 | All `String()`/`IsValid()` on semantic type aliases | -| `pkg/workflow/domains.go` | 10 | Check callers | -| `pkg/workflow/expression_builder.go` | 9 | Check callers | -| `pkg/workflow/validation_helpers.go` | 6 | Check callers | -| `pkg/cli/docker_images.go` | 6 | Check callers | -| `pkg/workflow/permissions_factory.go` | 5 | Check callers | -| `pkg/workflow/map_helpers.go` | 5 | Check callers | -| `pkg/workflow/engine_helpers.go` | 5 | Check callers | -| `pkg/console/console.go` | 5 | Check callers | -| `pkg/workflow/safe_outputs_env.go` | 4 | Check callers | -| `pkg/workflow/expression_nodes.go` | 4 | Check callers | - -~80 additional files have 1–3 dead functions each. - -## Suggested approach - -1. Pick a file with 5+ dead functions. -2. For each dead function, check callers: `grep -rn "FuncName" --include="*.go"`. If only test callers, also remove the tests. -3. Remove the function and any now-unused imports. -4. Run the verification commands above. -5. Commit per logical group, keep PRs small and reviewable. +## Batch plan (248 dead functions as of 2026-02-28) + +Each batch: delete the dead functions, delete the tests that exclusively test them, run verification, commit, open PR. + +### Batch 5 — simple helpers (11 functions) +Files: `pkg/workflow/validation_helpers.go` (6), `pkg/workflow/map_helpers.go` (5) + +Dead functions: +- `ValidateRequired`, `ValidateMaxLength`, `ValidateMinLength`, `ValidateInList`, `ValidatePositiveInt`, `ValidateNonNegativeInt` +- `isEmptyOrNil`, `getMapFieldAsString`, `getMapFieldAsMap`, `getMapFieldAsBool`, `getMapFieldAsInt` + +Tests to remove from `validation_helpers_test.go`: +- `TestValidateRequired`, `TestValidateMaxLength`, `TestValidateMinLength`, `TestValidateInList`, `TestValidatePositiveInt`, `TestValidateNonNegativeInt` +- `TestIsEmptyOrNil`, `TestGetMapFieldAsString`, `TestGetMapFieldAsMap`, `TestGetMapFieldAsBool`, `TestGetMapFieldAsInt` + +### Batch 6 — engine helpers (5 functions) +File: `pkg/workflow/engine_helpers.go` (5) + +Dead functions: `ExtractAgentIdentifier`, `GetHostedToolcachePathSetup`, `GetSanitizedPATHExport`, `GetToolBinsSetup`, `GetToolBinsEnvArg` + +Tests to remove from `engine_helpers_test.go`: +- `TestExtractAgentIdentifier`, `TestGetHostedToolcachePathSetup`, `TestGetHostedToolcachePathSetup_Consistency`, `TestGetHostedToolcachePathSetup_UsesToolBins`, `TestGetToolBinsSetup`, `TestGetToolBinsEnvArg`, `TestGetSanitizedPATHExport`, `TestGetSanitizedPATHExport_ShellExecution` + +### Batch 7 — domain helpers (10 functions) +File: `pkg/workflow/domains.go` (10) + +Dead functions: `mergeDomainsWithNetwork`, `mergeDomainsWithNetworkAndTools`, `GetCopilotAllowedDomains`, `GetCopilotAllowedDomainsWithSafeInputs`, `GetCopilotAllowedDomainsWithTools`, `GetCodexAllowedDomains`, `GetCodexAllowedDomainsWithTools`, `GetClaudeAllowedDomains`, `GetClaudeAllowedDomainsWithSafeInputs`, `GetClaudeAllowedDomainsWithTools` + +Tests to remove from `domains_test.go`, `domains_protocol_test.go`, `domains_sort_test.go`, `safe_inputs_firewall_test.go`, `http_mcp_domains_test.go` — remove only the specific test functions that call these dead helpers; keep tests for live functions in those files. + +### Batch 8 — expression graph (16 functions) +Files: `pkg/workflow/expression_nodes.go` (4), `pkg/workflow/expression_builder.go` (9), `pkg/workflow/known_needs_expressions.go` (3) + +Dead functions in `expression_nodes.go`: `ParenthesesNode.Render`, `NumberLiteralNode.Render`, `TernaryNode.Render`, `ContainsNode.Render` + +Dead functions in `expression_builder.go`: `BuildNumberLiteral`, `BuildContains`, `BuildTernary`, `BuildLabelContains`, `BuildActionEquals`, `BuildRefStartsWith`, `BuildExpressionWithDescription`, `BuildPRCommentCondition`, `AddDetectionSuccessCheck` + +Dead functions in `known_needs_expressions.go`: `getSafeOutputJobNames`, `hasMultipleSafeOutputTypes`, `getCustomJobNames` + +Tests to find and remove: check `expressions_test.go`, `expression_coverage_test.go`, `known_needs_expressions_test.go`. + +### Batch 9 — constants & console (18 functions) +Files: `pkg/constants/constants.go` (13), `pkg/console/console.go` (5) + +Dead functions in `constants.go`: all `String()`/`IsValid()` methods on `LineLength`, `FeatureFlag`, `URL`, `ModelName`, `WorkflowID`, `EngineName`, plus `MCPServerID.IsValid` + +Dead functions in `console.go`: `FormatLocationMessage`, `FormatCountMessage`, `FormatListHeader`, `RenderTree`, `buildLipglossTree` + +Tests to remove: relevant subtests in `constants_test.go`; `TestFormatLocationMessage`, `TestRenderTree`, `TestRenderTreeSimple`, `TestFormatCountMessage`, `TestFormatListHeader` in `console_test.go` and related files. + +### Batch 10 — agent session builder (1 function) +File: `pkg/workflow/create_agent_session.go` + +Dead function: `Compiler.buildCreateOutputAgentSessionJob` + +Find and remove its test(s): `grep -rn "buildCreateOutputAgentSessionJob" --include="*_test.go"`. + +### Batch 11 — safe-outputs & MCP helpers (13 functions) +Files: `pkg/workflow/safe_outputs_env.go` (4), `pkg/workflow/safe_outputs_config_helpers.go` (3), `pkg/workflow/mcp_playwright_config.go` (3), `pkg/workflow/mcp_config_builtin.go` (3) + +Dead functions in `safe_outputs_env.go`: `applySafeOutputEnvToSlice`, `buildTitlePrefixEnvVar`, `buildLabelsEnvVar`, `buildCategoryEnvVar` + +Dead functions in `safe_outputs_config_helpers.go`: `getEnabledSafeOutputToolNamesReflection`, `Compiler.formatDetectionRunsOn`, `GetEnabledSafeOutputToolNames` + +Dead functions in `mcp_playwright_config.go`: `getPlaywrightDockerImageVersion`, `getPlaywrightMCPPackageVersion`, `generatePlaywrightDockerArgs` + +Dead functions in `mcp_config_builtin.go`: `renderSafeOutputsMCPConfig`, `renderSafeOutputsMCPConfigTOML`, `renderAgenticWorkflowsMCPConfigTOML` + +Tests to remove: check `safe_output_helpers_test.go`, `version_field_test.go`, `mcp_benchmark_test.go`, `mcp_config_refactor_test.go`, `mcp_config_shared_test.go`, `threat_detection_test.go`. + +### Batch 12 — small utilities (9 functions) +Files: `pkg/sliceutil/sliceutil.go` (3), `pkg/stringutil/pat_validation.go` (3), `pkg/workflow/error_aggregation.go` (3) + +Dead functions in `sliceutil.go`: `ContainsAny`, `ContainsIgnoreCase`, `FilterMap` + +Dead functions in `pat_validation.go`: `IsFineGrainedPAT`, `IsClassicPAT`, `IsOAuthToken` + +Dead functions in `error_aggregation.go`: `ErrorCollector.HasErrors`, `FormatAggregatedError`, `SplitJoinedErrors` + +### Batch 13 — parser utilities (9 functions) +Files: `pkg/parser/include_expander.go` (3), `pkg/parser/schema_validation.go` (3), `pkg/parser/yaml_error.go` (3) + +Dead functions in `include_expander.go`: `ExpandIncludes`, `ProcessIncludesForEngines`, `ProcessIncludesForSafeOutputs` + +Dead functions in `schema_validation.go`: `ValidateMainWorkflowFrontmatterWithSchema`, `ValidateIncludedFileFrontmatterWithSchema`, `ValidateMCPConfigWithSchema` + +Dead functions in `yaml_error.go`: `ExtractYAMLError`, `extractFromGoccyFormat`, `extractFromStringParsing` + +### Batch 14 — agentic engine & compiler types (16 functions) +Files: `pkg/workflow/agentic_engine.go` (3), `pkg/workflow/compiler_types.go` (10+), `pkg/cli/docker_images.go` (6) + +Dead functions in `agentic_engine.go`: `BaseEngine.convertStepToYAML`, `GenerateSecretValidationStep`, `EngineRegistry.GetAllEngines` + +Dead functions in `compiler_types.go` (check WASM binary first): `WithCustomOutput`, `WithVersion`, `WithSkipValidation`, `WithNoEmit`, `WithStrictMode`, `WithForceRefreshActionPins`, `WithWorkflowIdentifier`, `NewCompilerWithVersion`, `Compiler.GetSharedActionResolverForTest`, `Compiler.GetArtifactManager` + +Dead functions in `docker_images.go`: `isDockerAvailable`, `ResetDockerPullState`, `ValidateMCPServerDockerAvailability`, `SetDockerImageDownloading`, `SetMockImageAvailable`, `PrintDockerPullStatus` + +### Batch 15 — js.go stubs (6 functions) +File: `pkg/workflow/js.go` + +Dead functions: the remaining 6 unreachable `get*Script()` / public `Get*` stubs reported by deadcode. + +### Batch 16 — artifact manager (14 functions) +File: `pkg/workflow/artifact_manager.go` + +Save for last — most complex, with deep coupling to `artifact_manager_integration_test.go`. + +### Remaining (~120 functions) +~80+ files each with 1–3 dead functions. Tackle after the above batches clear the larger clusters. + +--- + +## Per-batch checklist + +For each batch: + +- [ ] Run `deadcode ./cmd/... ./internal/tools/... 2>/dev/null` to confirm current dead list +- [ ] For each dead function, `grep -rn "FuncName" --include="*.go"` to find all callers +- [ ] Delete the function +- [ ] Delete test functions that exclusively call the deleted function (not shared helpers) +- [ ] Check for now-unused imports in edited files +- [ ] `go build ./...` +- [ ] `go vet ./...` +- [ ] `go vet -tags=integration ./...` +- [ ] `make fmt` +- [ ] Run selective tests for touched packages: `go test -v -run "TestAffected" ./pkg/...` +- [ ] Commit with message: `chore: remove dead functions (batch N) — X -> Y dead` +- [ ] Open PR, confirm CI passes before merging diff --git a/pkg/workflow/frontmatter_types.go b/pkg/workflow/frontmatter_types.go index 2b3bd80892..e365f9b8c8 100644 --- a/pkg/workflow/frontmatter_types.go +++ b/pkg/workflow/frontmatter_types.go @@ -514,28 +514,6 @@ func ExtractMapField(frontmatter map[string]any, key string) map[string]any { return make(map[string]any) } -// ExtractStringField is a convenience wrapper for extracting string fields. -// Returns empty string if the key doesn't exist or cannot be converted. -func ExtractStringField(frontmatter map[string]any, key string) string { - var result string - err := unmarshalFromMap(frontmatter, key, &result) - if err != nil { - return "" - } - return result -} - -// ExtractIntField is a convenience wrapper for extracting integer fields. -// Returns 0 if the key doesn't exist or cannot be converted. -func ExtractIntField(frontmatter map[string]any, key string) int { - var result int - err := unmarshalFromMap(frontmatter, key, &result) - if err != nil { - return 0 - } - return result -} - // ToMap converts FrontmatterConfig back to map[string]any for backward compatibility // This allows gradual migration from map[string]any to strongly-typed config func (fc *FrontmatterConfig) ToMap() map[string]any { diff --git a/pkg/workflow/js.go b/pkg/workflow/js.go index 737f4c70d5..1eaa09a651 100644 --- a/pkg/workflow/js.go +++ b/pkg/workflow/js.go @@ -68,7 +68,6 @@ func init() { // All getter functions return empty strings since embedded scripts were removed func getAddCommentScript() string { return "" } -func getAddLabelsScript() string { return "" } func getAssignToAgentScript() string { return "" } func getCreateCodeScanningAlertScript() string { return "" } func getCreateDiscussionScript() string { return "" } diff --git a/pkg/workflow/permissions_factory.go b/pkg/workflow/permissions_factory.go index 5095c625c5..0a7ef8fe1e 100644 --- a/pkg/workflow/permissions_factory.go +++ b/pkg/workflow/permissions_factory.go @@ -90,16 +90,6 @@ func NewPermissionsContentsReadIssuesWritePRWrite() *Permissions { }) } -// NewPermissionsContentsReadIssuesWritePRWriteDiscussionsWrite creates permissions with contents: read, issues: write, pull-requests: write, discussions: write -func NewPermissionsContentsReadIssuesWritePRWriteDiscussionsWrite() *Permissions { - return NewPermissionsFromMap(map[PermissionScope]PermissionLevel{ - PermissionContents: PermissionRead, - PermissionIssues: PermissionWrite, - PermissionPullRequests: PermissionWrite, - PermissionDiscussions: PermissionWrite, - }) -} - // NewPermissionsActionsWrite creates permissions with actions: write // This is required for dispatching workflows via workflow_dispatch func NewPermissionsActionsWrite() *Permissions { @@ -108,17 +98,6 @@ func NewPermissionsActionsWrite() *Permissions { }) } -// NewPermissionsActionsWriteContentsWriteIssuesWritePRWrite creates permissions with actions: write, contents: write, issues: write, pull-requests: write -// This is required for the replaceActorsForAssignable GraphQL mutation used to assign GitHub Copilot coding agent to issues -func NewPermissionsActionsWriteContentsWriteIssuesWritePRWrite() *Permissions { - return NewPermissionsFromMap(map[PermissionScope]PermissionLevel{ - PermissionActions: PermissionWrite, - PermissionContents: PermissionWrite, - PermissionIssues: PermissionWrite, - PermissionPullRequests: PermissionWrite, - }) -} - // NewPermissionsContentsWrite creates permissions with contents: write func NewPermissionsContentsWrite() *Permissions { return NewPermissionsFromMap(map[PermissionScope]PermissionLevel{ @@ -144,13 +123,6 @@ func NewPermissionsContentsWriteIssuesWritePRWrite() *Permissions { }) } -// NewPermissionsDiscussionsWrite creates permissions with discussions: write -func NewPermissionsDiscussionsWrite() *Permissions { - return NewPermissionsFromMap(map[PermissionScope]PermissionLevel{ - PermissionDiscussions: PermissionWrite, - }) -} - // NewPermissionsContentsReadDiscussionsWrite creates permissions with contents: read and discussions: write func NewPermissionsContentsReadDiscussionsWrite() *Permissions { return NewPermissionsFromMap(map[PermissionScope]PermissionLevel{ @@ -202,22 +174,3 @@ func NewPermissionsContentsReadProjectsWrite() *Permissions { PermissionOrganizationProj: PermissionWrite, }) } - -// NewPermissionsContentsWritePRReadIssuesRead creates permissions with contents: write, pull-requests: read, issues: read -func NewPermissionsContentsWritePRReadIssuesRead() *Permissions { - return NewPermissionsFromMap(map[PermissionScope]PermissionLevel{ - PermissionContents: PermissionWrite, - PermissionPullRequests: PermissionRead, - PermissionIssues: PermissionRead, - }) -} - -// NewPermissionsContentsWriteIssuesWritePRWriteDiscussionsWrite creates permissions with contents: write, issues: write, pull-requests: write, discussions: write -func NewPermissionsContentsWriteIssuesWritePRWriteDiscussionsWrite() *Permissions { - return NewPermissionsFromMap(map[PermissionScope]PermissionLevel{ - PermissionContents: PermissionWrite, - PermissionIssues: PermissionWrite, - PermissionPullRequests: PermissionWrite, - PermissionDiscussions: PermissionWrite, - }) -} diff --git a/pkg/workflow/safe_output_parser.go b/pkg/workflow/safe_output_parser.go index 9e479e5ecd..9ed7ad4723 100644 --- a/pkg/workflow/safe_output_parser.go +++ b/pkg/workflow/safe_output_parser.go @@ -76,22 +76,6 @@ func ParseFilterConfig(configMap map[string]any) SafeOutputFilterConfig { return config } -// ParseDiscussionFilterConfig parses filter config plus required-category for discussion operations. -func ParseDiscussionFilterConfig(configMap map[string]any) SafeOutputDiscussionFilterConfig { - config := SafeOutputDiscussionFilterConfig{ - SafeOutputFilterConfig: ParseFilterConfig(configMap), - } - - // Parse required-category - if requiredCategory, exists := configMap["required-category"]; exists { - if categoryStr, ok := requiredCategory.(string); ok { - config.RequiredCategory = categoryStr - } - } - - return config -} - // parseRequiredLabelsFromConfig extracts and validates required-labels from a config map. // Returns a slice of label strings, or nil if not present or invalid. func parseRequiredLabelsFromConfig(configMap map[string]any) []string { @@ -103,66 +87,3 @@ func parseRequiredLabelsFromConfig(configMap map[string]any) []string { func parseRequiredTitlePrefixFromConfig(configMap map[string]any) string { return extractStringFromMap(configMap, "required-title-prefix", safeOutputParserLog) } - -// ParseCloseJobConfig parses common close job fields from a config map. -// Returns the parsed CloseJobConfig and a boolean indicating if there was a validation error. -func ParseCloseJobConfig(configMap map[string]any) (CloseJobConfig, bool) { - config := CloseJobConfig{} - - // Parse target config - targetConfig, isInvalid := ParseTargetConfig(configMap) - if isInvalid { - return config, true - } - config.SafeOutputTargetConfig = targetConfig - - // Parse filter config - config.SafeOutputFilterConfig = ParseFilterConfig(configMap) - - return config, false -} - -// ParseListJobConfig parses common list job fields from a config map. -// Returns the parsed ListJobConfig and a boolean indicating if there was a validation error. -func ParseListJobConfig(configMap map[string]any, allowedKey string) (ListJobConfig, bool) { - config := ListJobConfig{} - - // Parse target config - targetConfig, isInvalid := ParseTargetConfig(configMap) - if isInvalid { - return config, true - } - config.SafeOutputTargetConfig = targetConfig - - // Parse allowed list (using the specified key like "allowed", "reviewers", etc.) - if allowed, exists := configMap[allowedKey]; exists { - // Handle single string format - if allowedStr, ok := allowed.(string); ok { - config.Allowed = []string{allowedStr} - } else if allowedArray, ok := allowed.([]any); ok { - // Handle array format - for _, item := range allowedArray { - if itemStr, ok := item.(string); ok { - config.Allowed = append(config.Allowed, itemStr) - } - } - } - } - - // Parse blocked list - if blocked, exists := configMap["blocked"]; exists { - // Handle single string format - if blockedStr, ok := blocked.(string); ok { - config.Blocked = []string{blockedStr} - } else if blockedArray, ok := blocked.([]any); ok { - // Handle array format - for _, item := range blockedArray { - if itemStr, ok := item.(string); ok { - config.Blocked = append(config.Blocked, itemStr) - } - } - } - } - - return config, false -}