🧹 Remove dead code functions across multiple packages

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

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 {

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 "" }

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,
-	})
-}

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
-}