diff --git a/.serena/project.yml b/.serena/project.yml index 7b46044fc19..8da1c65e531 100644 --- a/.serena/project.yml +++ b/.serena/project.yml @@ -105,3 +105,16 @@ default_modes: # fixed set of tools to use as the base tool set (if non-empty), replacing Serena's default set of tools. # This cannot be combined with non-empty excluded_tools or included_optional_tools. fixed_tools: [] + +# time budget (seconds) per tool call for the retrieval of additional symbol information +# such as docstrings or parameter information. +# This overrides the corresponding setting in the global configuration; see the documentation there. +# If null or missing, use the setting from the global configuration. +symbol_info_budget: + +# The language backend to use for this project. +# If not set, the global setting from serena_config.yml is used. +# Valid values: LSP, JetBrains +# Note: the backend is fixed at startup. If a project with a different backend +# is activated post-init, an error will be returned. +language_backend: diff --git a/DEADCODE.md b/DEADCODE.md index 29afbcea069..5cce9747fba 100644 --- a/DEADCODE.md +++ b/DEADCODE.md @@ -44,14 +44,15 @@ make fmt --- -## Batch plan (107 dead functions as of 2026-03-02) +## Batch plan (85 dead functions as of 2026-03-02, after phases 5–8) -Batches 1–4 have been completed. The original batches 5–16 are superseded by this plan; -many of those functions were removed during prior work, and the remainder (plus newly -discovered dead code) are redistributed below into 30 focused phases. +Phases 5–8 are complete. The original phases 9–34 are superseded by this revised plan, +which groups work by domain, includes LOC estimates, and skips functions too small to +be worth the disruption. Each phase: delete the dead functions, delete tests that exclusively test them, -run verification, commit, open PR. +run verification, commit, open PR. Branches are stacked: dc-9 based on dc-8, dc-10 +based on dc-9, etc. **WASM false positives (do not delete):** - `Compiler.CompileToYAML` (`compiler_string_api.go:15`) — used by `cmd/gh-aw-wasm` @@ -60,6 +61,19 @@ run verification, commit, open PR. **Shared test infrastructure (do not delete):** - `containsInNonCommentLines`, `indexInNonCommentLines`, `extractJobSection` (`compiler_test_helpers.go`) — used by ≥15 test files +**Compiler option test infrastructure (do not delete):** +- `WithCustomOutput`, `WithVersion`, `WithSkipValidation`, `WithNoEmit`, `WithStrictMode`, `WithForceRefreshActionPins`, `WithWorkflowIdentifier` (`compiler_types.go`) — used at 50+ call sites in test files; removing would require massive test refactoring for minimal gain +- `NewCompilerWithVersion` (`compiler_types.go:160`) — primary test entry point for compiler construction +- `Compiler.GetSharedActionResolverForTest` (`compiler_types.go:305`) — test helper + +**Not worth deleting (< 10 lines, isolated):** +`envVarPrefix` (2), `GenerateSafeInputGoToolScriptForInspector` (2), `MapToolConfig.GetAny` (3), +`HasErrors` (5), `extractMapFromFrontmatter` (5), `GetDefaultMaxForType` (5), +`GetValidationConfigForType` (6), `getPlaywrightMCPPackageVersion` (6), `RunGit` (6), +`ExecGHWithOutput` (7), `convertGoPatternToJavaScript` (7), `ValidateExpressionSafetyPublic` (7), +`IsSafeInputsHTTPMode` (7), `HasSafeJobsEnabled` (7), `renderSafeOutputsMCPConfig` (8), +`renderPlaywrightMCPConfig` (8), `SecurityFinding.String` (8), `GetAllEngines` (11) + --- ### Phase 5 — CLI git helpers (3 functions) @@ -112,312 +126,138 @@ Note: If `ImportError` struct has no remaining methods, consider deleting the ty Tests to check: `import_error_test.go`, `import_processor_test.go`. -### Phase 9 — compiler option functions part 1 (5 functions) -File: `pkg/workflow/compiler_types.go` - -| Function | Line | -|----------|------| -| `WithCustomOutput` | 26 | -| `WithVersion` | 31 | -| `WithSkipValidation` | 36 | -| `WithNoEmit` | 41 | -| `WithStrictMode` | 46 | - -Tests to check: `compiler_types_test.go`, `compiler_test.go` — remove tests for these `With*` option constructors. - -### Phase 10 — compiler option functions part 2 (5 functions) -File: `pkg/workflow/compiler_types.go` - -| Function | Line | -|----------|------| -| `WithForceRefreshActionPins` | 56 | -| `WithWorkflowIdentifier` | 61 | -| `NewCompilerWithVersion` | 160 | -| `Compiler.GetSharedActionResolverForTest` | 305 | -| `Compiler.GetArtifactManager` | 333 | - -Note: `GetSharedActionResolverForTest` may be used only in tests — delete it AND any test callers. -After this phase, clean up the `CompilerOption` type if no live `With*` functions remain. - -### Phase 11 — agentic engine (3 functions) -File: `pkg/workflow/agentic_engine.go` - -| Function | Line | -|----------|------| -| `BaseEngine.convertStepToYAML` | 333 | -| `GenerateSecretValidationStep` | 430 | -| `EngineRegistry.GetAllEngines` | 502 | - -Tests to check: `agentic_engine_test.go`. - -### Phase 12 — error handling utilities (5 functions) -Files: `pkg/workflow/error_aggregation.go` (3), `pkg/workflow/error_helpers.go` (2) - -| Function | File | Line | -|----------|------|------| -| `ErrorCollector.HasErrors` | `error_aggregation.go` | 92 | -| `FormatAggregatedError` | `error_aggregation.go` | 144 | -| `SplitJoinedErrors` | `error_aggregation.go` | 174 | -| `EnhanceError` | `error_helpers.go` | 165 | -| `WrapErrorWithContext` | `error_helpers.go` | 187 | - -Tests to check: `error_aggregation_test.go`, `error_helpers_test.go`. - -### Phase 13 — safe outputs env vars (4 functions) -File: `pkg/workflow/safe_outputs_env.go` - -| Function | Line | -|----------|------| -| `applySafeOutputEnvToSlice` | 47 | -| `buildTitlePrefixEnvVar` | 311 | -| `buildLabelsEnvVar` | 321 | -| `buildCategoryEnvVar` | 332 | - -Tests to check: `safe_outputs_env_test.go`, `safe_output_helpers_test.go`. - -### Phase 14 — safe outputs config helpers (3 functions) -File: `pkg/workflow/safe_outputs_config_helpers.go` - -| Function | Line | -|----------|------| -| `getEnabledSafeOutputToolNamesReflection` | 85 | -| `Compiler.formatDetectionRunsOn` | 127 | -| `GetEnabledSafeOutputToolNames` | 216 | - -Tests to check: `safe_outputs_config_helpers_test.go`, `threat_detection_test.go`. - -### Phase 15 — Playwright MCP config (3 functions) -File: `pkg/workflow/mcp_playwright_config.go` - -| Function | Line | -|----------|------| -| `getPlaywrightDockerImageVersion` | 15 | -| `getPlaywrightMCPPackageVersion` | 26 | -| `generatePlaywrightDockerArgs` | 32 | - -Tests to check: `mcp_playwright_config_test.go`. - -### Phase 16 — MCP config builtins (3 functions) -File: `pkg/workflow/mcp_config_builtin.go` - -| Function | Line | -|----------|------| -| `renderSafeOutputsMCPConfig` | 113 | -| `renderSafeOutputsMCPConfigTOML` | 295 | -| `renderAgenticWorkflowsMCPConfigTOML` | 308 | - -Tests to check: `mcp_config_builtin_test.go`, `mcp_config_refactor_test.go`, `mcp_config_shared_test.go`. - -### Phase 17 — MCP config miscellaneous (4 functions) -Files: `pkg/workflow/mcp_config_custom.go` (1), `mcp_config_playwright_renderer.go` (1), `mcp_config_types.go` (1), `mcp_config_validation.go` (1) - -| Function | File | Line | -|----------|------|------| -| `renderCustomMCPConfigWrapper` | `mcp_config_custom.go` | 21 | -| `renderPlaywrightMCPConfig` | `mcp_config_playwright_renderer.go` | 71 | -| `MapToolConfig.GetAny` | `mcp_config_types.go` | 99 | -| `getTypeString` | `mcp_config_validation.go` | 176 | - -Tests to check: `mcp_config_custom_test.go`, `mcp_config_playwright_renderer_test.go`, `mcp_config_types_test.go`, `mcp_config_validation_test.go`. - -### Phase 18 — safe inputs system (4 functions) -Files: `pkg/workflow/safe_inputs_generator.go` (1), `safe_inputs_parser.go` (2), `safe_inputs_renderer.go` (1) - -| Function | File | Line | -|----------|------|------| -| `GenerateSafeInputGoToolScriptForInspector` | `safe_inputs_generator.go` | 391 | -| `IsSafeInputsHTTPMode` | `safe_inputs_parser.go` | 64 | -| `ParseSafeInputs` | `safe_inputs_parser.go` | 210 | -| `getSafeInputsEnvVars` | `safe_inputs_renderer.go` | 14 | - -Tests to check: `safe_inputs_generator_test.go`, `safe_inputs_parser_test.go`, `safe_inputs_renderer_test.go`. - -### Phase 19 — safe outputs validation & safe jobs (3 functions) -Files: `pkg/workflow/safe_output_validation_config.go` (2), `safe_jobs.go` (1) - -| Function | File | Line | -|----------|------|------| -| `GetValidationConfigForType` | `safe_output_validation_config.go` | 409 | -| `GetDefaultMaxForType` | `safe_output_validation_config.go` | 415 | -| `HasSafeJobsEnabled` | `safe_jobs.go` | 34 | - -Tests to check: `safe_output_validation_config_test.go`, `safe_jobs_test.go`. - -### Phase 20 — safe output job builders: comments & discussions (4 functions) -Files: `pkg/workflow/add_comment.go` (1), `create_code_scanning_alert.go` (1), `create_discussion.go` (1), `create_pr_review_comment.go` (1) - -| Function | File | Line | -|----------|------|------| -| `Compiler.buildCreateOutputAddCommentJob` | `add_comment.go` | 34 | -| `Compiler.buildCreateOutputCodeScanningAlertJob` | `create_code_scanning_alert.go` | 21 | -| `Compiler.buildCreateOutputDiscussionJob` | `create_discussion.go` | 132 | -| `Compiler.buildCreateOutputPullRequestReviewCommentJob` | `create_pr_review_comment.go` | 24 | - -Note: If these are the only functions in their files, consider deleting the entire file. - -Tests to check: `add_comment_test.go`, `create_code_scanning_alert_test.go`, `create_discussion_test.go`, `create_pr_review_comment_test.go`. - -### Phase 21 — safe output job builders: sessions & missing data (3 functions) -Files: `pkg/workflow/create_agent_session.go` (1), `missing_data.go` (1), `missing_tool.go` (1) - -| Function | File | Line | -|----------|------|------| -| `Compiler.buildCreateOutputAgentSessionJob` | `create_agent_session.go` | 88 | -| `Compiler.buildCreateOutputMissingDataJob` | `missing_data.go` | 12 | -| `Compiler.buildCreateOutputMissingToolJob` | `missing_tool.go` | 12 | - -Note: If the file contains only the dead function, delete the entire file. - -Tests to check: `create_agent_session_test.go`, `missing_data_test.go`, `missing_tool_test.go`. - -### Phase 22 — safe output compilation (3 functions) -Files: `pkg/workflow/compiler_safe_outputs.go` (2), `compiler_safe_outputs_specialized.go` (1) - -| Function | File | Line | -|----------|------|------| -| `Compiler.generateJobName` | `compiler_safe_outputs.go` | 185 | -| `Compiler.mergeSafeJobsFromIncludes` | `compiler_safe_outputs.go` | 219 | -| `Compiler.buildCreateProjectStepConfig` | `compiler_safe_outputs_specialized.go` | 139 | - -Tests to check: `compiler_safe_outputs_test.go`, `compiler_safe_outputs_specialized_test.go`. - -### Phase 23 — issue reporting (2 functions) -File: `pkg/workflow/missing_issue_reporting.go` - -| Function | Line | -|----------|------| -| `Compiler.buildIssueReportingJob` | 48 | -| `envVarPrefix` | 175 | - -Note: If these are the only non-trivial functions in the file, consider deleting it entirely. - -Tests to check: `missing_issue_reporting_test.go`. - -### Phase 24 — checkout manager (2 functions) -File: `pkg/workflow/checkout_manager.go` - -| Function | Line | -|----------|------| -| `CheckoutManager.GetCurrentRepository` | 186 | -| `getCurrentCheckoutRepository` | 553 | - -Tests to check: `checkout_manager_test.go`. - -### Phase 25 — expression processing (3 functions) -Files: `pkg/workflow/expression_extraction.go` (1), `expression_parser.go` (1), `expression_validation.go` (1) - -| Function | File | Line | -|----------|------|------| -| `ExpressionExtractor.GetMappings` | `expression_extraction.go` | 239 | -| `NormalizeExpressionForComparison` | `expression_parser.go` | 463 | -| `ValidateExpressionSafetyPublic` | `expression_validation.go` | 359 | - -Tests to check: `expression_extraction_test.go`, `expression_parser_test.go`, `expression_validation_test.go`. - -### Phase 26 — frontmatter extraction (3 functions) -Files: `pkg/workflow/frontmatter_extraction_metadata.go` (1), `frontmatter_extraction_yaml.go` (1), `frontmatter_types.go` (1) +### Phase 9 — Output job builders (~480 LOC, 6 functions) +Files: `pkg/workflow/add_comment.go`, `create_code_scanning_alert.go`, `create_discussion.go`, `create_pr_review_comment.go`, `create_agent_session.go`, `missing_issue_reporting.go` -| Function | File | Line | -|----------|------|------| -| `extractMapFromFrontmatter` | `frontmatter_extraction_metadata.go` | 246 | -| `Compiler.extractYAMLValue` | `frontmatter_extraction_yaml.go` | 18 | -| `unmarshalFromMap` | `frontmatter_types.go` | 196 | - -Tests to check: `frontmatter_extraction_metadata_test.go`, `frontmatter_extraction_yaml_test.go`, `frontmatter_types_test.go`. - -### Phase 27 — git, GitHub CLI & shell helpers (4 functions) -Files: `pkg/workflow/git_helpers.go` (2), `github_cli.go` (1), `shell.go` (1) - -| Function | File | Line | -|----------|------|------| -| `GetCurrentGitTag` | `git_helpers.go` | 69 | -| `RunGit` | `git_helpers.go` | 119 | -| `ExecGHWithOutput` | `github_cli.go` | 84 | -| `shellEscapeCommandString` | `shell.go` | 82 | - -Tests to check: `git_helpers_test.go`, `github_cli_test.go`, `shell_test.go`. +Dead `buildCreateOutput*Job` methods — the primary job-assembly step for each safe-output type +is not reachable from any live code path. -### Phase 28 — config & concurrency validation (4 functions) -Files: `pkg/workflow/config_helpers.go` (2), `concurrency_validation.go` (1), `permissions_validation.go` (1) +| Function | File | LOC | +|----------|------|-----| +| `Compiler.buildCreateOutputAddCommentJob` | `add_comment.go` | 116 | +| `Compiler.buildCreateOutputDiscussionJob` | `create_discussion.go` | 78 | +| `Compiler.buildCreateOutputCodeScanningAlertJob` | `create_code_scanning_alert.go` | 73 | +| `Compiler.buildCreateOutputPullRequestReviewCommentJob` | `create_pr_review_comment.go` | 63 | +| `Compiler.buildIssueReportingJob` | `missing_issue_reporting.go` | 61 | +| `Compiler.buildCreateOutputAgentSessionJob` | `create_agent_session.go` | 53 | -| Function | File | Line | -|----------|------|------| -| `parseParticipantsFromConfig` | `config_helpers.go` | 131 | -| `ParseIntFromConfig` | `config_helpers.go` | 218 | -| `extractGroupExpression` | `concurrency_validation.go` | 289 | -| `GetToolsetsData` | `permissions_validation.go` | 77 | - -Tests to check: `config_helpers_test.go`, `concurrency_validation_test.go`, `permissions_validation_test.go`. - -### Phase 29 — security & error types (4 functions) -Files: `pkg/workflow/markdown_security_scanner.go` (1), `secrets_validation.go` (1), `tools_validation.go` (1), `shared_workflow_error.go` (1) +Note: The `parse*Config` helpers in the same files are **live** (called from `safe_outputs_config.go`). Only delete the `buildCreateOutput*` methods. -| Function | File | Line | -|----------|------|------| -| `SecurityFinding.String` | `markdown_security_scanner.go` | 64 | -| `validateSecretReferences` | `secrets_validation.go` | 31 | -| `isGitToolAllowed` | `tools_validation.go` | 31 | -| `NewSharedWorkflowError` | `shared_workflow_error.go` | 21 | +Tests to check: no dedicated test files were found for these builders specifically. -Note: If `SharedWorkflowError` has no remaining constructor, consider deleting the type entirely. +### Phase 10 — Safe output compilation helpers (~250 LOC, 6 functions) +Files: `pkg/workflow/compiler_safe_outputs.go`, `compiler_safe_outputs_specialized.go`, `unified_prompt_step.go`, `missing_data.go`, `missing_tool.go` -Tests to check: `markdown_security_scanner_test.go`, `secrets_validation_test.go`, `tools_validation_test.go`, `shared_workflow_error_test.go`. +| Function | File | LOC | +|----------|------|-----| +| `Compiler.generateUnifiedPromptStep` | `unified_prompt_step.go` | 118 | +| `Compiler.buildCreateProjectStepConfig` | `compiler_safe_outputs_specialized.go` | 41 | +| `Compiler.generateJobName` | `compiler_safe_outputs.go` | 34 | +| `Compiler.mergeSafeJobsFromIncludes` | `compiler_safe_outputs.go` | 24 | +| `Compiler.buildCreateOutputMissingDataJob` | `missing_data.go` | 17 | +| `Compiler.buildCreateOutputMissingToolJob` | `missing_tool.go` | 17 | -### Phase 30 — step & job types (3 functions) -Files: `pkg/workflow/step_types.go` (2), `jobs.go` (1) +Note: `unified_prompt_step.go` may be entirely removable if `generateUnifiedPromptStep` is its only function. -| Function | File | Line | -|----------|------|------| -| `WorkflowStep.IsRunStep` | `step_types.go` | 36 | -| `WorkflowStep.ToYAML` | `step_types.go` | 171 | -| `JobManager.GetTopologicalOrder` | `jobs.go` | 412 | +### Phase 11 — Large compiler infrastructure helpers (~195 LOC, 3 functions) +Files: `pkg/workflow/compiler_yaml_helpers.go`, `repo_memory.go`, `jobs.go` -Tests to check: `step_types_test.go`, `jobs_test.go`. +| Function | File | LOC | +|----------|------|-----| +| `generateRepoMemoryPushSteps` | `repo_memory.go` | 87 | +| `Compiler.generateCheckoutGitHubFolder` | `compiler_yaml_helpers.go` | 55 | +| `JobManager.GetTopologicalOrder` | `jobs.go` | 52 | -### Phase 31 — utilities cleanup (4 functions) -Files: `pkg/sliceutil/sliceutil.go` (1), `pkg/workflow/semver.go` (1), `repository_features_validation.go` (1), `compiler_yaml_ai_execution.go` (1) +Tests to check: `repo_memory_test.go`, `compiler_yaml_helpers_test.go`, `jobs_test.go`. -| Function | File | Line | -|----------|------|------| -| `FilterMap` | `pkg/sliceutil/sliceutil.go` | 49 | -| `extractMajorVersion` | `semver.go` | 41 | -| `ClearRepositoryFeaturesCache` | `repository_features_validation.go` | 83 | -| `Compiler.convertGoPatternToJavaScript` | `compiler_yaml_ai_execution.go` | 116 | - -Tests to check: `sliceutil_test.go`, `semver_test.go`, `repository_features_validation_test.go`, `compiler_yaml_ai_execution_test.go`. - -### Phase 32 — compiler helpers (3 functions) -Files: `pkg/workflow/compiler_yaml_helpers.go` (1), `unified_prompt_step.go` (1), `repo_memory.go` (1) - -| Function | File | Line | -|----------|------|------| -| `Compiler.generateCheckoutGitHubFolder` | `compiler_yaml_helpers.go` | 221 | -| `Compiler.generateUnifiedPromptStep` | `unified_prompt_step.go` | 30 | -| `generateRepoMemoryPushSteps` | `repo_memory.go` | 520 | - -Note: If `unified_prompt_step.go` contains only the dead function, delete the entire file. +### Phase 12 — Engine helpers (~57 LOC, 2 functions) +File: `pkg/workflow/agentic_engine.go` -Tests to check: `compiler_yaml_helpers_test.go`, `unified_prompt_step_test.go`, `repo_memory_test.go`. +| Function | LOC | +|----------|-----| +| `GenerateSecretValidationStep` | 31 | +| `BaseEngine.convertStepToYAML` | 15 | -### Phase 33 — metrics extraction (2 functions) -File: `pkg/workflow/metrics.go` +Note: `EngineRegistry.GetAllEngines` (11 LOC) is also dead but below the threshold — skip. -| Function | Line | -|----------|------| -| `ExtractFirstMatch` | 39 | -| `ExtractMCPServer` | 274 | +Tests to check: `agentic_engine_test.go`. -Tests to check: `metrics_test.go`. +### Phase 13 — Error handling cluster (~85 LOC, 4 functions) +Files: `pkg/workflow/error_aggregation.go`, `error_helpers.go` -### Phase 34 — WASM string API audit (0 deletions) -File: `pkg/workflow/compiler_string_api.go` +| Function | File | LOC | +|----------|------|-----| +| `FormatAggregatedError` | `error_aggregation.go` | 30 | +| `EnhanceError` | `error_helpers.go` | 22 | +| `SplitJoinedErrors` | `error_aggregation.go` | 21 | +| `WrapErrorWithContext` | `error_helpers.go` | 12 | -Functions: `Compiler.CompileToYAML` (line 15), `Compiler.ParseWorkflowString` (line 52) +Note: `ErrorCollector.HasErrors` (5 LOC) is also dead but below the threshold — skip. -**Action:** Do not delete. Verify that `cmd/gh-aw-wasm/main.go` still calls these functions. -If WASM binary is ever removed, both functions become deletable. +Tests to check: `error_aggregation_test.go`, `error_helpers_test.go`. -Run: `grep -rn "CompileToYAML\|ParseWorkflowString" cmd/gh-aw-wasm/` +### Phase 14 — Safe outputs env & config helpers (~129 LOC, 7 functions) +Files: `pkg/workflow/safe_outputs_env.go`, `safe_outputs_config_helpers.go` + +| Function | File | LOC | +|----------|------|-----| +| `getEnabledSafeOutputToolNamesReflection` | `safe_outputs_config_helpers.go` | 31 | +| `Compiler.formatDetectionRunsOn` | `safe_outputs_config_helpers.go` | 30 | +| `applySafeOutputEnvToSlice` | `safe_outputs_env.go` | 25 | +| `GetEnabledSafeOutputToolNames` | `safe_outputs_config_helpers.go` | 12 | +| `buildLabelsEnvVar` | `safe_outputs_env.go` | 11 | +| `buildTitlePrefixEnvVar` | `safe_outputs_env.go` | 10 | +| `buildCategoryEnvVar` | `safe_outputs_env.go` | 10 | + +Tests to check: `safe_outputs_env_test.go`, `safe_outputs_config_helpers_test.go`. + +### Phase 15 — MCP config rendering (~106 LOC, 4 functions) +Files: `pkg/workflow/mcp_config_builtin.go`, `mcp_config_custom.go`, `mcp_config_validation.go`, `mcp_playwright_config.go` + +| Function | File | LOC | +|----------|------|-----| +| `renderAgenticWorkflowsMCPConfigTOML` | `mcp_config_builtin.go` | 67 | +| `renderCustomMCPConfigWrapper` | `mcp_config_custom.go` | 26 | +| `getTypeString` | `mcp_config_validation.go` | 25 | +| `renderSafeOutputsMCPConfigTOML` | `mcp_config_builtin.go` | 13 | +| `generatePlaywrightDockerArgs` | `mcp_playwright_config.go` | 12 | + +Note: `renderSafeOutputsMCPConfig` (8 LOC), `renderPlaywrightMCPConfig` (8 LOC), `getPlaywrightDockerImageVersion` (11 LOC) are also dead — include them in this phase for completeness. + +Tests to check: `mcp_config_builtin_test.go`, `mcp_config_custom_test.go`, `mcp_playwright_config_test.go`. + +### Phase 16 — Validation & git helpers (~140 LOC, 4 functions) +Files: `pkg/workflow/tools_validation.go`, `concurrency_validation.go`, `config_helpers.go`, `git_helpers.go`, `permissions_validation.go` + +| Function | File | LOC | +|----------|------|-----| +| `isGitToolAllowed` | `tools_validation.go` | 45 | +| `extractGroupExpression` | `concurrency_validation.go` | 39 | +| `parseParticipantsFromConfig` | `config_helpers.go` | 32 | +| `GetCurrentGitTag` | `git_helpers.go` | 24 | +| `GetToolsetsData` | `permissions_validation.go` | 19 | + +Tests to check: `tools_validation_test.go`, `concurrency_validation_test.go`, `config_helpers_test.go`, `git_helpers_test.go`. + +### Phase 17 — Diverse medium functions (~210 LOC, 10 functions) +Various files that each have one dead function of moderate size (12–26 LOC). + +| Function | File | LOC | +|----------|------|-----| +| `getSafeInputsEnvVars` | `safe_inputs_renderer.go` | 26 | +| `ExtractMCPServer` | `metrics.go` | 24 | +| `ClearRepositoryFeaturesCache` | `repository_features_validation.go` | 25 | +| `unmarshalFromMap` | `frontmatter_types.go` | 23 | +| `Compiler.extractYAMLValue` | `frontmatter_extraction_yaml.go` | 22 | +| `GetMappings` | `expression_extraction.go` | 20 | +| `ParseSafeInputs` | `safe_inputs_parser.go` | 20 | +| `validateSecretReferences` | `secrets_validation.go` | 18 | +| `WorkflowStep.ToYAML` | `step_types.go` | 14 | +| `FilterMap` | `pkg/sliceutil/sliceutil.go` | 13 | + +Note: `CheckoutManager.GetCurrentRepository` (12 LOC), `getCurrentCheckoutRepository` (19 LOC), `ParseIntFromConfig` (19 LOC), `NormalizeExpressionForComparison` (12 LOC), and `mergeSafeJobsFromIncludes` may also be included if still dead after prior phases. + +Tests to check: per-file test files as named. --- @@ -425,18 +265,17 @@ Run: `grep -rn "CompileToYAML\|ParseWorkflowString" cmd/gh-aw-wasm/` | Metric | Value | |--------|-------| -| Total dead functions reported | 107 | +| Dead functions after phases 5–8 | 85 | | WASM false positives (skip) | 2 | | Shared test infrastructure (skip) | 3 | -| Functions to delete | **102** | -| Phases with deletions | 29 | -| Audit-only phases | 1 | -| Average functions per phase | 3.4 | - -**Estimated effort per phase:** 15–30 minutes (delete, test, verify, commit). -**Estimated total effort:** ~10–15 hours across all 30 phases. - -**Recommended execution order:** Phases are designed to be executed top-to-bottom. Phases within the same domain (e.g., MCP phases 15–17, safe output phases 13–14, 19–22) can be combined into larger PRs if velocity is high. +| Compiler option test infrastructure (skip) | 9 | +| Functions < 10 LOC (skip) | ~18 | +| **Functions to delete across phases 9–17** | **~53** | +| Phases remaining | 9 | +| Estimated LOC to remove | ~1,650 | + +**Estimated effort per phase:** 20–45 minutes (larger phases have many test updates). +**Recommended execution order:** Phases 9–17 top-to-bottom; phases are stacked (dc-N based on dc-(N-1)). --- diff --git a/pkg/workflow/add_comment.go b/pkg/workflow/add_comment.go index f97ae0369b6..e39efd9104d 100644 --- a/pkg/workflow/add_comment.go +++ b/pkg/workflow/add_comment.go @@ -1,10 +1,6 @@ package workflow import ( - "encoding/json" - "errors" - "fmt" - "github.com/github/gh-aw/pkg/logger" ) @@ -30,122 +26,6 @@ type AddCommentsConfig struct { Footer *string `yaml:"footer,omitempty"` // Controls whether AI-generated footer is added. When false, visible footer is omitted but XML markers are kept. } -// buildCreateOutputAddCommentJob creates the add_comment job -func (c *Compiler) buildCreateOutputAddCommentJob(data *WorkflowData, mainJobName string, createIssueJobName string, createDiscussionJobName string, createPullRequestJobName string) (*Job, error) { - addCommentLog.Printf("Building add_comment job: target=%s, discussion=%v", data.SafeOutputs.AddComments.Target, data.SafeOutputs.AddComments.Discussion != nil && *data.SafeOutputs.AddComments.Discussion) - if data.SafeOutputs == nil || data.SafeOutputs.AddComments == nil { - return nil, errors.New("safe-outputs.add-comment configuration is required") - } - - // Build pre-steps for debugging output - var preSteps []string - preSteps = append(preSteps, " - name: Debug agent outputs\n") - preSteps = append(preSteps, " env:\n") - preSteps = append(preSteps, fmt.Sprintf(" AGENT_OUTPUT: ${{ needs.%s.outputs.output }}\n", mainJobName)) - preSteps = append(preSteps, fmt.Sprintf(" AGENT_OUTPUT_TYPES: ${{ needs.%s.outputs.output_types }}\n", mainJobName)) - preSteps = append(preSteps, " run: |\n") - preSteps = append(preSteps, " echo \"Output: $AGENT_OUTPUT\"\n") - preSteps = append(preSteps, " echo \"Output types: $AGENT_OUTPUT_TYPES\"\n") - - // Build custom environment variables specific to add-comment - var customEnvVars []string - - // Pass the comment target configuration - if data.SafeOutputs.AddComments.Target != "" { - customEnvVars = append(customEnvVars, fmt.Sprintf(" GH_AW_COMMENT_TARGET: %q\n", data.SafeOutputs.AddComments.Target)) - } - // Pass the discussion flag configuration - if data.SafeOutputs.AddComments.Discussion != nil && *data.SafeOutputs.AddComments.Discussion { - customEnvVars = append(customEnvVars, " GITHUB_AW_COMMENT_DISCUSSION: \"true\"\n") - } - // Pass the hide-older-comments flag configuration - customEnvVars = append(customEnvVars, buildTemplatableBoolEnvVar("GH_AW_HIDE_OLDER_COMMENTS", data.SafeOutputs.AddComments.HideOlderComments)...) - // Pass the allowed-reasons list configuration - if len(data.SafeOutputs.AddComments.AllowedReasons) > 0 { - reasonsJSON, err := json.Marshal(data.SafeOutputs.AddComments.AllowedReasons) - if err == nil { - customEnvVars = append(customEnvVars, fmt.Sprintf(" GH_AW_ALLOWED_REASONS: %q\n", string(reasonsJSON))) - } - } - // Add environment variables for the URLs from other safe output jobs if they exist - if createIssueJobName != "" { - customEnvVars = append(customEnvVars, fmt.Sprintf(" GH_AW_CREATED_ISSUE_URL: ${{ needs.%s.outputs.issue_url }}\n", createIssueJobName)) - customEnvVars = append(customEnvVars, fmt.Sprintf(" GH_AW_CREATED_ISSUE_NUMBER: ${{ needs.%s.outputs.issue_number }}\n", createIssueJobName)) - customEnvVars = append(customEnvVars, fmt.Sprintf(" GH_AW_TEMPORARY_ID_MAP: ${{ needs.%s.outputs.temporary_id_map }}\n", createIssueJobName)) - } - if createDiscussionJobName != "" { - customEnvVars = append(customEnvVars, fmt.Sprintf(" GH_AW_CREATED_DISCUSSION_URL: ${{ needs.%s.outputs.discussion_url }}\n", createDiscussionJobName)) - customEnvVars = append(customEnvVars, fmt.Sprintf(" GH_AW_CREATED_DISCUSSION_NUMBER: ${{ needs.%s.outputs.discussion_number }}\n", createDiscussionJobName)) - } - if createPullRequestJobName != "" { - customEnvVars = append(customEnvVars, fmt.Sprintf(" GH_AW_CREATED_PULL_REQUEST_URL: ${{ needs.%s.outputs.pull_request_url }}\n", createPullRequestJobName)) - customEnvVars = append(customEnvVars, fmt.Sprintf(" GH_AW_CREATED_PULL_REQUEST_NUMBER: ${{ needs.%s.outputs.pull_request_number }}\n", createPullRequestJobName)) - } - - // Add standard environment variables (metadata + staged/target repo) - customEnvVars = append(customEnvVars, c.buildStandardSafeOutputEnvVars(data, data.SafeOutputs.AddComments.TargetRepoSlug)...) - - // Create outputs for the job - outputs := map[string]string{ - "comment_id": "${{ steps.add_comment.outputs.comment_id }}", - "comment_url": "${{ steps.add_comment.outputs.comment_url }}", - } - - // Build job condition with event check if target is not specified - jobCondition := BuildSafeOutputType("add_comment") - if data.SafeOutputs.AddComments != nil && data.SafeOutputs.AddComments.Target == "" { - var eventTerms []ConditionNode - if data.SafeOutputs.AddComments.Issues == nil || *data.SafeOutputs.AddComments.Issues { - eventTerms = append(eventTerms, BuildPropertyAccess("github.event.issue.number")) - } - if data.SafeOutputs.AddComments.PullRequests == nil || *data.SafeOutputs.AddComments.PullRequests { - eventTerms = append(eventTerms, BuildPropertyAccess("github.event.pull_request.number")) - } - if data.SafeOutputs.AddComments.Discussions == nil || *data.SafeOutputs.AddComments.Discussions { - eventTerms = append(eventTerms, BuildPropertyAccess("github.event.discussion.number")) - } - if len(eventTerms) > 0 { - eventCondition := &DisjunctionNode{Terms: eventTerms} - jobCondition = BuildAnd(jobCondition, eventCondition) - } - } - - // Build the needs list - always depend on mainJobName, and conditionally on the other jobs - needs := []string{mainJobName} - if createIssueJobName != "" { - needs = append(needs, createIssueJobName) - } - if createDiscussionJobName != "" { - needs = append(needs, createDiscussionJobName) - } - if createPullRequestJobName != "" { - needs = append(needs, createPullRequestJobName) - } - - // Determine permissions based on Issues, PullRequests, and Discussions fields. - // Issues: nil or true → issues:write (default: true) - // PullRequests: nil or true → pull-requests:write (default: true) - // Discussions: nil or true → discussions:write (default: true) - permissions := buildAddCommentPermissions(data.SafeOutputs.AddComments) - - // Use the shared builder function to create the job - return c.buildSafeOutputJob(data, SafeOutputJobConfig{ - JobName: "add_comment", - StepName: "Add Issue Comment", - StepID: "add_comment", - MainJobName: mainJobName, - CustomEnvVars: customEnvVars, - ScriptName: "add_comment", - Permissions: permissions, - Outputs: outputs, - Condition: jobCondition, - Needs: needs, - PreSteps: preSteps, - Token: data.SafeOutputs.AddComments.GitHubToken, - TargetRepoSlug: data.SafeOutputs.AddComments.TargetRepoSlug, - }) -} - // parseCommentsConfig handles add-comment configuration func (c *Compiler) parseCommentsConfig(outputMap map[string]any) *AddCommentsConfig { // Check if the key exists diff --git a/pkg/workflow/add_comment_dependencies_test.go b/pkg/workflow/add_comment_dependencies_test.go deleted file mode 100644 index 1f20690e089..00000000000 --- a/pkg/workflow/add_comment_dependencies_test.go +++ /dev/null @@ -1,183 +0,0 @@ -//go:build !integration - -package workflow - -import ( - "slices" - "strings" - "testing" -) - -func TestAddCommentJobDependencies(t *testing.T) { - tests := []struct { - name string - createIssueJobName string - createDiscussionJobName string - createPullRequestJobName string - expectedNeeds []string - expectedEnvVars []string - expectIssueEnvVars bool - expectDiscussionEnvVars bool - expectPullRequestEnvVars bool - }{ - { - name: "No dependencies", - createIssueJobName: "", - createDiscussionJobName: "", - createPullRequestJobName: "", - expectedNeeds: []string{"main"}, - expectedEnvVars: []string{}, - expectIssueEnvVars: false, - expectDiscussionEnvVars: false, - expectPullRequestEnvVars: false, - }, - { - name: "Only create_issue dependency", - createIssueJobName: "create_issue", - createDiscussionJobName: "", - createPullRequestJobName: "", - expectedNeeds: []string{"main", "create_issue"}, - expectedEnvVars: []string{ - "GH_AW_CREATED_ISSUE_URL", - "GH_AW_CREATED_ISSUE_NUMBER", - }, - expectIssueEnvVars: true, - expectDiscussionEnvVars: false, - expectPullRequestEnvVars: false, - }, - { - name: "Only create_discussion dependency", - createIssueJobName: "", - createDiscussionJobName: "create_discussion", - createPullRequestJobName: "", - expectedNeeds: []string{"main", "create_discussion"}, - expectedEnvVars: []string{ - "GH_AW_CREATED_DISCUSSION_URL", - "GH_AW_CREATED_DISCUSSION_NUMBER", - }, - expectIssueEnvVars: false, - expectDiscussionEnvVars: true, - expectPullRequestEnvVars: false, - }, - { - name: "Only create_pull_request dependency", - createIssueJobName: "", - createDiscussionJobName: "", - createPullRequestJobName: "create_pull_request", - expectedNeeds: []string{"main", "create_pull_request"}, - expectedEnvVars: []string{ - "GH_AW_CREATED_PULL_REQUEST_URL", - "GH_AW_CREATED_PULL_REQUEST_NUMBER", - }, - expectIssueEnvVars: false, - expectDiscussionEnvVars: false, - expectPullRequestEnvVars: true, - }, - { - name: "All dependencies", - createIssueJobName: "create_issue", - createDiscussionJobName: "create_discussion", - createPullRequestJobName: "create_pull_request", - expectedNeeds: []string{"main", "create_issue", "create_discussion", "create_pull_request"}, - expectedEnvVars: []string{ - "GH_AW_CREATED_ISSUE_URL", - "GH_AW_CREATED_ISSUE_NUMBER", - "GH_AW_CREATED_DISCUSSION_URL", - "GH_AW_CREATED_DISCUSSION_NUMBER", - "GH_AW_CREATED_PULL_REQUEST_URL", - "GH_AW_CREATED_PULL_REQUEST_NUMBER", - }, - expectIssueEnvVars: true, - expectDiscussionEnvVars: true, - expectPullRequestEnvVars: true, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - compiler := &Compiler{} - workflowData := &WorkflowData{ - Name: "Test Workflow", - SafeOutputs: &SafeOutputsConfig{ - AddComments: &AddCommentsConfig{ - BaseSafeOutputConfig: BaseSafeOutputConfig{}, - }, - }, - } - - job, err := compiler.buildCreateOutputAddCommentJob( - workflowData, - "main", - tt.createIssueJobName, - tt.createDiscussionJobName, - tt.createPullRequestJobName, - ) - if err != nil { - t.Fatalf("Failed to build safe_outputs job: %v", err) - } - - // Check job dependencies (needs) - if len(job.Needs) != len(tt.expectedNeeds) { - t.Errorf("Expected %d needs, got %d", len(tt.expectedNeeds), len(job.Needs)) - } - for _, expectedNeed := range tt.expectedNeeds { - found := slices.Contains(job.Needs, expectedNeed) - if !found { - t.Errorf("Expected need '%s' not found in job.Needs: %v", expectedNeed, job.Needs) - } - } - - // Convert steps to string to check for environment variables - stepsStr := strings.Join(job.Steps, "") - - // Check for expected environment variables - for _, envVar := range tt.expectedEnvVars { - if !strings.Contains(stepsStr, envVar) { - t.Errorf("Expected environment variable '%s' not found in job steps", envVar) - } - } - - // Check for issue-specific environment variables - if tt.expectIssueEnvVars { - if !strings.Contains(stepsStr, "GH_AW_CREATED_ISSUE_URL: ${{ needs.create_issue.outputs.issue_url }}") { - t.Error("Expected issue_url environment variable declaration not found in job steps") - } - if !strings.Contains(stepsStr, "GH_AW_CREATED_ISSUE_NUMBER: ${{ needs.create_issue.outputs.issue_number }}") { - t.Error("Expected issue_number environment variable declaration not found in job steps") - } - } else { - if strings.Contains(stepsStr, "GH_AW_CREATED_ISSUE_URL: ${{ needs.create_issue.outputs.issue_url }}") { - t.Error("Unexpected GH_AW_CREATED_ISSUE_URL environment variable declaration found in job steps") - } - } - - // Check for discussion-specific environment variables - if tt.expectDiscussionEnvVars { - if !strings.Contains(stepsStr, "GH_AW_CREATED_DISCUSSION_URL: ${{ needs.create_discussion.outputs.discussion_url }}") { - t.Error("Expected discussion_url environment variable declaration not found in job steps") - } - if !strings.Contains(stepsStr, "GH_AW_CREATED_DISCUSSION_NUMBER: ${{ needs.create_discussion.outputs.discussion_number }}") { - t.Error("Expected discussion_number environment variable declaration not found in job steps") - } - } else { - if strings.Contains(stepsStr, "GH_AW_CREATED_DISCUSSION_URL: ${{ needs.create_discussion.outputs.discussion_url }}") { - t.Error("Unexpected GH_AW_CREATED_DISCUSSION_URL environment variable declaration found in job steps") - } - } - - // Check for pull request-specific environment variables - if tt.expectPullRequestEnvVars { - if !strings.Contains(stepsStr, "GH_AW_CREATED_PULL_REQUEST_URL: ${{ needs.create_pull_request.outputs.pull_request_url }}") { - t.Error("Expected pull_request_url environment variable declaration not found in job steps") - } - if !strings.Contains(stepsStr, "GH_AW_CREATED_PULL_REQUEST_NUMBER: ${{ needs.create_pull_request.outputs.pull_request_number }}") { - t.Error("Expected pull_request_number environment variable declaration not found in job steps") - } - } else { - if strings.Contains(stepsStr, "GH_AW_CREATED_PULL_REQUEST_URL: ${{ needs.create_pull_request.outputs.pull_request_url }}") { - t.Error("Unexpected GH_AW_CREATED_PULL_REQUEST_URL environment variable declaration found in job steps") - } - } - }) - } -} diff --git a/pkg/workflow/create_agent_session.go b/pkg/workflow/create_agent_session.go index d920eb674cf..df54858d347 100644 --- a/pkg/workflow/create_agent_session.go +++ b/pkg/workflow/create_agent_session.go @@ -1,9 +1,6 @@ package workflow import ( - "errors" - "fmt" - "github.com/github/gh-aw/pkg/logger" ) @@ -83,59 +80,3 @@ func (c *Compiler) parseAgentSessionConfig(outputMap map[string]any) *CreateAgen return nil } - -// buildCreateOutputAgentSessionJob creates the create_agent_session job -func (c *Compiler) buildCreateOutputAgentSessionJob(data *WorkflowData, mainJobName string) (*Job, error) { - if data.SafeOutputs == nil || data.SafeOutputs.CreateAgentSessions == nil { - return nil, errors.New("safe-outputs.create-agent-session configuration is required") - } - - createAgentSessionLog.Printf("Building create-agent-session job: workflow=%s, main_job=%s, base=%s", - data.Name, mainJobName, data.SafeOutputs.CreateAgentSessions.Base) - - var preSteps []string - - // Step 1: Checkout repository for gh CLI to work - preSteps = append(preSteps, " - name: Checkout repository for gh CLI\n") - preSteps = append(preSteps, fmt.Sprintf(" uses: %s\n", GetActionPin("actions/checkout"))) - preSteps = append(preSteps, " with:\n") - preSteps = append(preSteps, " persist-credentials: false\n") - - // Build custom environment variables specific to create-agent-session - customEnvVars := []string{ - fmt.Sprintf(" GITHUB_AW_WORKFLOW_NAME: %q\n", data.Name), - } - - // Pass custom base branch only if explicitly configured; JS will resolve dynamically otherwise - if data.SafeOutputs.CreateAgentSessions.Base != "" { - customEnvVars = append(customEnvVars, fmt.Sprintf(" GITHUB_AW_AGENT_SESSION_BASE: %q\n", data.SafeOutputs.CreateAgentSessions.Base)) - } - - // Add standard environment variables (metadata + staged/target repo) - customEnvVars = append(customEnvVars, c.buildStandardSafeOutputEnvVars(data, data.SafeOutputs.CreateAgentSessions.TargetRepoSlug)...) - - // Create outputs for the job - outputs := map[string]string{ - "session_number": "${{ steps.create_agent_session.outputs.session_number }}", - "session_url": "${{ steps.create_agent_session.outputs.session_url }}", - } - - jobCondition := BuildSafeOutputType("create_agent_session") - - // Use the shared builder function to create the job - return c.buildSafeOutputJob(data, SafeOutputJobConfig{ - JobName: "create_agent_session", - StepName: "Create Agent Session", - StepID: "create_agent_session", - MainJobName: mainJobName, - CustomEnvVars: customEnvVars, - Script: "const { main } = require('/opt/gh-aw/actions/create_agent_session.cjs'); await main();", - Permissions: NewPermissionsContentsWriteIssuesWritePRWrite(), - Outputs: outputs, - Condition: jobCondition, - PreSteps: preSteps, - Token: data.SafeOutputs.CreateAgentSessions.GitHubToken, - UseCopilotRequestsToken: true, // Use Copilot token preference for agent session creation - TargetRepoSlug: data.SafeOutputs.CreateAgentSessions.TargetRepoSlug, - }) -} diff --git a/pkg/workflow/create_agent_session_test.go b/pkg/workflow/create_agent_session_test.go index 7fa4ce65ee5..e8dcf90a53e 100644 --- a/pkg/workflow/create_agent_session_test.go +++ b/pkg/workflow/create_agent_session_test.go @@ -99,59 +99,6 @@ func TestParseAgentTaskConfig(t *testing.T) { } } -func TestBuildCreateOutputAgentTaskJob(t *testing.T) { - compiler := NewCompiler() - workflowData := &WorkflowData{ - Name: "Test Workflow", - SafeOutputs: &SafeOutputsConfig{ - CreateAgentSessions: &CreateAgentSessionConfig{ - BaseSafeOutputConfig: BaseSafeOutputConfig{ - Max: strPtr("1"), - }, - Base: "main", - TargetRepoSlug: "owner/repo", - }, - }, - } - - job, err := compiler.buildCreateOutputAgentSessionJob(workflowData, "main_job") - if err != nil { - t.Fatalf("buildCreateOutputAgentSessionJob() error = %v", err) - } - - if job == nil { - t.Fatal("buildCreateOutputAgentSessionJob() returned nil job") - } - - if job.Name != "create_agent_session" { - t.Errorf("buildCreateOutputAgentSessionJob().Name = %v, want 'create_agent_session'", job.Name) - } - - if job.TimeoutMinutes != 10 { - t.Errorf("buildCreateOutputAgentSessionJob().TimeoutMinutes = %v, want 10", job.TimeoutMinutes) - } - - if len(job.Outputs) != 2 { - t.Errorf("buildCreateOutputAgentSessionJob().Outputs length = %v, want 2", len(job.Outputs)) - } - - if _, ok := job.Outputs["session_number"]; !ok { - t.Error("buildCreateOutputAgentSessionJob().Outputs missing 'session_number'") - } - - if _, ok := job.Outputs["session_url"]; !ok { - t.Error("buildCreateOutputAgentSessionJob().Outputs missing 'session_url'") - } - - if len(job.Steps) == 0 { - t.Error("buildCreateOutputAgentSessionJob().Steps is empty") - } - - if len(job.Needs) != 1 || job.Needs[0] != "main_job" { - t.Errorf("buildCreateOutputAgentSessionJob().Needs = %v, want ['main_job']", job.Needs) - } -} - func TestExtractSafeOutputsConfigWithAgentTask(t *testing.T) { compiler := NewCompiler() frontmatter := map[string]any{ diff --git a/pkg/workflow/create_code_scanning_alert.go b/pkg/workflow/create_code_scanning_alert.go index 783fe2ccc36..56ea1b004a4 100644 --- a/pkg/workflow/create_code_scanning_alert.go +++ b/pkg/workflow/create_code_scanning_alert.go @@ -1,9 +1,6 @@ package workflow import ( - "errors" - "fmt" - "github.com/github/gh-aw/pkg/logger" ) @@ -17,79 +14,6 @@ type CreateCodeScanningAlertsConfig struct { AllowedRepos []string `yaml:"allowed-repos,omitempty"` // List of additional repositories in format "owner/repo" that code scanning alerts can be created in } -// buildCreateOutputCodeScanningAlertJob creates the create_code_scanning_alert job -func (c *Compiler) buildCreateOutputCodeScanningAlertJob(data *WorkflowData, mainJobName string, workflowFilename string) (*Job, error) { - if data.SafeOutputs == nil || data.SafeOutputs.CreateCodeScanningAlerts == nil { - return nil, errors.New("safe-outputs.create-code-scanning-alert configuration is required") - } - - // Build custom environment variables specific to create-code-scanning-alert - var customEnvVars []string - if maxVal := templatableIntValue(data.SafeOutputs.CreateCodeScanningAlerts.Max); maxVal > 0 { - customEnvVars = append(customEnvVars, fmt.Sprintf(" GH_AW_SECURITY_REPORT_MAX: %d\n", maxVal)) - } else { - customEnvVars = append(customEnvVars, buildTemplatableIntEnvVar("GH_AW_SECURITY_REPORT_MAX", data.SafeOutputs.CreateCodeScanningAlerts.Max)...) - } - // Pass the driver configuration, defaulting to frontmatter name - driverName := data.SafeOutputs.CreateCodeScanningAlerts.Driver - if driverName == "" { - if data.FrontmatterName != "" { - driverName = data.FrontmatterName - } else { - driverName = data.Name // fallback to H1 header name - } - } - createCodeScanningAlertLog.Printf("Building create_code_scanning_alert job: driver=%s, max=%d", driverName, data.SafeOutputs.CreateCodeScanningAlerts.Max) - customEnvVars = append(customEnvVars, fmt.Sprintf(" GH_AW_SECURITY_REPORT_DRIVER: %s\n", driverName)) - // Pass the workflow filename for rule ID prefix - customEnvVars = append(customEnvVars, fmt.Sprintf(" GH_AW_WORKFLOW_FILENAME: %s\n", workflowFilename)) - - // Add workflow metadata (name, source, and tracker-id) for consistency - customEnvVars = append(customEnvVars, buildWorkflowMetadataEnvVarsWithTrackerID(data.Name, data.Source, data.TrackerID)...) - - // Build post-steps for SARIF artifact upload - var postSteps []string - // Add step to upload SARIF artifact - postSteps = append(postSteps, " - name: Upload SARIF artifact\n") - postSteps = append(postSteps, " if: steps.create_code_scanning_alert.outputs.sarif_file\n") - postSteps = append(postSteps, fmt.Sprintf(" uses: %s\n", GetActionPin("actions/upload-artifact"))) - postSteps = append(postSteps, " with:\n") - postSteps = append(postSteps, " name: code-scanning-alert.sarif\n") - postSteps = append(postSteps, " path: ${{ steps.create_code_scanning_alert.outputs.sarif_file }}\n") - - // Add step to upload SARIF to GitHub Code Scanning - postSteps = append(postSteps, " - name: Upload SARIF to GitHub Security\n") - postSteps = append(postSteps, " if: steps.create_code_scanning_alert.outputs.sarif_file\n") - postSteps = append(postSteps, fmt.Sprintf(" uses: %s\n", GetActionPin("github/codeql-action/upload-sarif"))) - postSteps = append(postSteps, " with:\n") - postSteps = append(postSteps, " sarif_file: ${{ steps.create_code_scanning_alert.outputs.sarif_file }}\n") - - // Create outputs for the job - outputs := map[string]string{ - "sarif_file": "${{ steps.create_code_scanning_alert.outputs.sarif_file }}", - "findings_count": "${{ steps.create_code_scanning_alert.outputs.findings_count }}", - "artifact_uploaded": "${{ steps.create_code_scanning_alert.outputs.artifact_uploaded }}", - "codeql_uploaded": "${{ steps.create_code_scanning_alert.outputs.codeql_uploaded }}", - } - - jobCondition := BuildSafeOutputType("create_code_scanning_alert") - - // Use the shared builder function to create the job - return c.buildSafeOutputJob(data, SafeOutputJobConfig{ - JobName: "create_code_scanning_alert", - StepName: "Create Code Scanning Alert", - StepID: "create_code_scanning_alert", - MainJobName: mainJobName, - CustomEnvVars: customEnvVars, - Script: "", - Permissions: NewPermissionsContentsReadSecurityEventsWriteActionsRead(), - Outputs: outputs, - Condition: jobCondition, - PostSteps: postSteps, - Token: data.SafeOutputs.CreateCodeScanningAlerts.GitHubToken, - }) -} - // parseCodeScanningAlertsConfig handles create-code-scanning-alert configuration func (c *Compiler) parseCodeScanningAlertsConfig(outputMap map[string]any) *CreateCodeScanningAlertsConfig { if _, exists := outputMap["create-code-scanning-alert"]; !exists { diff --git a/pkg/workflow/create_discussion.go b/pkg/workflow/create_discussion.go index 65c2684bd1a..b4b8ef52aca 100644 --- a/pkg/workflow/create_discussion.go +++ b/pkg/workflow/create_discussion.go @@ -1,7 +1,6 @@ package workflow import ( - "errors" "fmt" "os" "strings" @@ -128,84 +127,6 @@ func (c *Compiler) parseDiscussionsConfig(outputMap map[string]any) *CreateDiscu return &config } -// buildCreateOutputDiscussionJob creates the create_discussion job -func (c *Compiler) buildCreateOutputDiscussionJob(data *WorkflowData, mainJobName string, createIssueJobName string) (*Job, error) { - discussionLog.Printf("Building create_discussion job for workflow: %s", data.Name) - - if data.SafeOutputs == nil || data.SafeOutputs.CreateDiscussions == nil { - return nil, errors.New("safe-outputs.create-discussion configuration is required") - } - - // Build custom environment variables specific to create-discussion using shared helpers - var customEnvVars []string - customEnvVars = append(customEnvVars, buildTitlePrefixEnvVar("GH_AW_DISCUSSION_TITLE_PREFIX", data.SafeOutputs.CreateDiscussions.TitlePrefix)...) - customEnvVars = append(customEnvVars, buildCategoryEnvVar("GH_AW_DISCUSSION_CATEGORY", data.SafeOutputs.CreateDiscussions.Category)...) - customEnvVars = append(customEnvVars, buildLabelsEnvVar("GH_AW_DISCUSSION_LABELS", data.SafeOutputs.CreateDiscussions.Labels)...) - customEnvVars = append(customEnvVars, buildLabelsEnvVar("GH_AW_DISCUSSION_ALLOWED_LABELS", data.SafeOutputs.CreateDiscussions.AllowedLabels)...) - customEnvVars = append(customEnvVars, buildAllowedReposEnvVar("GH_AW_ALLOWED_REPOS", data.SafeOutputs.CreateDiscussions.AllowedRepos)...) - - // Add close-older-discussions flag if set - customEnvVars = append(customEnvVars, buildTemplatableBoolEnvVar("GH_AW_CLOSE_OLDER_DISCUSSIONS", data.SafeOutputs.CreateDiscussions.CloseOlderDiscussions)...) - - // Add expires value if set - if data.SafeOutputs.CreateDiscussions.Expires > 0 { - customEnvVars = append(customEnvVars, fmt.Sprintf(" GH_AW_DISCUSSION_EXPIRES: \"%d\"\n", data.SafeOutputs.CreateDiscussions.Expires)) - } - - // Add fallback-to-issue flag - ftiVal := data.SafeOutputs.CreateDiscussions.FallbackToIssue - if ftiVal != nil { - customEnvVars = append(customEnvVars, fmt.Sprintf(" GH_AW_DISCUSSION_FALLBACK_TO_ISSUE: \"%t\"\n", *ftiVal)) - } else { - customEnvVars = append(customEnvVars, " GH_AW_DISCUSSION_FALLBACK_TO_ISSUE: \"true\"\n") - } - - // Add footer flag if explicitly set to false - if data.SafeOutputs.CreateDiscussions.Footer != nil && *data.SafeOutputs.CreateDiscussions.Footer == "false" { - customEnvVars = append(customEnvVars, " GH_AW_FOOTER: \"false\"\n") - discussionLog.Print("Footer disabled - XML markers will be included but visible footer content will be omitted") - } - - // Add environment variable for temporary ID map from create_issue job - if createIssueJobName != "" { - customEnvVars = append(customEnvVars, fmt.Sprintf(" GH_AW_TEMPORARY_ID_MAP: ${{ needs.%s.outputs.temporary_id_map }}\n", createIssueJobName)) - } - - discussionLog.Printf("Configured %d custom environment variables for discussion creation", len(customEnvVars)) - - // Add standard environment variables (metadata + staged/target repo) - customEnvVars = append(customEnvVars, c.buildStandardSafeOutputEnvVars(data, data.SafeOutputs.CreateDiscussions.TargetRepoSlug)...) - - // Create outputs for the job - outputs := map[string]string{ - "discussion_number": "${{ steps.create_discussion.outputs.discussion_number }}", - "discussion_url": "${{ steps.create_discussion.outputs.discussion_url }}", - } - - // Build the needs list - always depend on mainJobName, and conditionally on create_issue - needs := []string{mainJobName} - if createIssueJobName != "" { - needs = append(needs, createIssueJobName) - } - - // Use the shared builder function to create the job - return c.buildSafeOutputJob(data, SafeOutputJobConfig{ - JobName: "create_discussion", - StepName: "Create Output Discussion", - StepID: "create_discussion", - MainJobName: mainJobName, - CustomEnvVars: customEnvVars, - ScriptName: "create_discussion", - Permissions: NewPermissionsContentsReadIssuesWriteDiscussionsWrite(), - Outputs: outputs, - Needs: needs, - Token: data.SafeOutputs.CreateDiscussions.GitHubToken, - TargetRepoSlug: data.SafeOutputs.CreateDiscussions.TargetRepoSlug, - }) -} - -// normalizeDiscussionCategory normalizes discussion category to lowercase -// and provides warnings about naming conventions // Returns normalized category (or original if it's a category ID) func normalizeDiscussionCategory(category string, log *logger.Logger, markdownPath string) string { // Empty category is allowed (GitHub Discussions will use default) diff --git a/pkg/workflow/create_discussion_dependencies_test.go b/pkg/workflow/create_discussion_dependencies_test.go index 403797b037a..b0512fe6452 100644 --- a/pkg/workflow/create_discussion_dependencies_test.go +++ b/pkg/workflow/create_discussion_dependencies_test.go @@ -3,89 +3,12 @@ package workflow import ( - "slices" - "strings" "testing" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) -func TestCreateDiscussionJobDependencies(t *testing.T) { - tests := []struct { - name string - createIssueJobName string - expectedNeeds []string - expectTempIDEnvVar bool - }{ - { - name: "No create_issue dependency", - createIssueJobName: "", - expectedNeeds: []string{"main"}, - expectTempIDEnvVar: false, - }, - { - name: "With create_issue dependency", - createIssueJobName: "create_issue", - expectedNeeds: []string{"main", "create_issue"}, - expectTempIDEnvVar: true, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - compiler := &Compiler{} - workflowData := &WorkflowData{ - Name: "Test Workflow", - SafeOutputs: &SafeOutputsConfig{ - CreateDiscussions: &CreateDiscussionsConfig{ - BaseSafeOutputConfig: BaseSafeOutputConfig{ - Max: strPtr("1"), - }, - Category: "general", - }, - }, - } - - job, err := compiler.buildCreateOutputDiscussionJob( - workflowData, - "main", - tt.createIssueJobName, - ) - if err != nil { - t.Fatalf("Failed to build create_discussion job: %v", err) - } - - // Check job dependencies (needs) - if len(job.Needs) != len(tt.expectedNeeds) { - t.Errorf("Expected %d needs, got %d: %v", len(tt.expectedNeeds), len(job.Needs), job.Needs) - } - for _, expectedNeed := range tt.expectedNeeds { - found := slices.Contains(job.Needs, expectedNeed) - if !found { - t.Errorf("Expected need '%s' not found in job.Needs: %v", expectedNeed, job.Needs) - } - } - - // Convert steps to string to check for environment variables - stepsStr := strings.Join(job.Steps, "") - - // Check for temporary ID map environment variable declaration - // Use the exact syntax pattern to avoid matching the bundled script content - envVarDeclaration := "GH_AW_TEMPORARY_ID_MAP: ${{ needs.create_issue.outputs.temporary_id_map }}" - if tt.expectTempIDEnvVar { - if !strings.Contains(stepsStr, envVarDeclaration) { - t.Error("Expected GH_AW_TEMPORARY_ID_MAP environment variable declaration not found in job steps") - } - } else { - if strings.Contains(stepsStr, envVarDeclaration) { - t.Error("Unexpected GH_AW_TEMPORARY_ID_MAP environment variable declaration found in job steps") - } - } - }) - } -} - func TestParseDiscussionsConfigDefaultExpiration(t *testing.T) { tests := []struct { name string diff --git a/pkg/workflow/create_pr_review_comment.go b/pkg/workflow/create_pr_review_comment.go index ba2999b539f..5b7ce22f653 100644 --- a/pkg/workflow/create_pr_review_comment.go +++ b/pkg/workflow/create_pr_review_comment.go @@ -1,9 +1,6 @@ package workflow import ( - "errors" - "fmt" - "github.com/github/gh-aw/pkg/logger" ) @@ -18,72 +15,6 @@ type CreatePullRequestReviewCommentsConfig struct { AllowedRepos []string `yaml:"allowed-repos,omitempty"` // List of additional repositories that PR review comments can be added to (additionally to the target-repo) } -// buildCreateOutputPullRequestReviewCommentJob creates the create_pr_review_comment job -// -//nolint:unused // Only used in integration tests -func (c *Compiler) buildCreateOutputPullRequestReviewCommentJob(data *WorkflowData, mainJobName string) (*Job, error) { - createPRReviewCommentLog.Printf("Building PR review comment job: main_job=%s", mainJobName) - - if data.SafeOutputs == nil || data.SafeOutputs.CreatePullRequestReviewComments == nil { - return nil, errors.New("safe-outputs.create-pull-request-review-comment configuration is required") - } - - // Log configuration details - side := data.SafeOutputs.CreatePullRequestReviewComments.Side - target := data.SafeOutputs.CreatePullRequestReviewComments.Target - createPRReviewCommentLog.Printf("Configuration: side=%s, target=%s", side, target) - - // Build custom environment variables specific to create-pull-request-review-comment - var customEnvVars []string - - // Pass the side configuration - if data.SafeOutputs.CreatePullRequestReviewComments.Side != "" { - customEnvVars = append(customEnvVars, fmt.Sprintf(" GH_AW_PR_REVIEW_COMMENT_SIDE: %q\n", data.SafeOutputs.CreatePullRequestReviewComments.Side)) - } - // Pass the target configuration - if data.SafeOutputs.CreatePullRequestReviewComments.Target != "" { - customEnvVars = append(customEnvVars, fmt.Sprintf(" GH_AW_PR_REVIEW_COMMENT_TARGET: %q\n", data.SafeOutputs.CreatePullRequestReviewComments.Target)) - } - - // Add standard environment variables (metadata + staged/target repo) - customEnvVars = append(customEnvVars, c.buildStandardSafeOutputEnvVars(data, data.SafeOutputs.CreatePullRequestReviewComments.TargetRepoSlug)...) - - // Create outputs for the job - outputs := map[string]string{ - "review_comment_id": "${{ steps.create_pr_review_comment.outputs.review_comment_id }}", - "review_comment_url": "${{ steps.create_pr_review_comment.outputs.review_comment_url }}", - } - - var jobCondition = BuildSafeOutputType("create_pull_request_review_comment") - if data.SafeOutputs.CreatePullRequestReviewComments != nil && data.SafeOutputs.CreatePullRequestReviewComments.Target == "" { - issueWithPR := &AndNode{ - Left: &ExpressionNode{Expression: "github.event.issue.number"}, - Right: &ExpressionNode{Expression: "github.event.issue.pull_request"}, - } - eventCondition := BuildOr( - issueWithPR, - BuildPropertyAccess("github.event.pull_request"), - ) - jobCondition = BuildAnd(jobCondition, eventCondition) - } - - // Use the shared builder function to create the job - return c.buildSafeOutputJob(data, SafeOutputJobConfig{ - JobName: "create_pr_review_comment", - StepName: "Create PR Review Comment", - StepID: "create_pr_review_comment", - MainJobName: mainJobName, - CustomEnvVars: customEnvVars, - ScriptName: "create_pr_review_comment", - Permissions: NewPermissionsContentsReadPRWrite(), - Outputs: outputs, - Condition: jobCondition, - Token: data.SafeOutputs.CreatePullRequestReviewComments.GitHubToken, - TargetRepoSlug: data.SafeOutputs.CreatePullRequestReviewComments.TargetRepoSlug, - }) -} - -// parsePullRequestReviewCommentsConfig handles create-pull-request-review-comment configuration func (c *Compiler) parsePullRequestReviewCommentsConfig(outputMap map[string]any) *CreatePullRequestReviewCommentsConfig { if _, exists := outputMap["create-pull-request-review-comment"]; !exists { createPRReviewCommentLog.Printf("Configuration not found") diff --git a/pkg/workflow/missing_data.go b/pkg/workflow/missing_data.go index b34f05d386e..c81eb32a155 100644 --- a/pkg/workflow/missing_data.go +++ b/pkg/workflow/missing_data.go @@ -1,31 +1,11 @@ package workflow import ( - "errors" - "github.com/github/gh-aw/pkg/logger" ) var missingDataLog = logger.New("workflow:missing_data") -// buildCreateOutputMissingDataJob creates the missing_data job -func (c *Compiler) buildCreateOutputMissingDataJob(data *WorkflowData, mainJobName string) (*Job, error) { - if data.SafeOutputs == nil || data.SafeOutputs.MissingData == nil { - return nil, errors.New("safe-outputs.missing-data configuration is required") - } - - return c.buildIssueReportingJob(data, mainJobName, issueReportingJobParams{ - kind: "missing_data", - envPrefix: envVarPrefix("missing_data"), - defaultTitle: "[missing data]", - outputKey: "data_reported", - stepName: "Record Missing Data", - config: data.SafeOutputs.MissingData, - log: missingDataLog, - }) -} - -// parseMissingDataConfig handles missing-data configuration func (c *Compiler) parseMissingDataConfig(outputMap map[string]any) *MissingDataConfig { return c.parseIssueReportingConfig(outputMap, "missing-data", "[missing data]", missingDataLog) } diff --git a/pkg/workflow/missing_data_test.go b/pkg/workflow/missing_data_test.go index 8f3c3c8f970..c46f5af836b 100644 --- a/pkg/workflow/missing_data_test.go +++ b/pkg/workflow/missing_data_test.go @@ -211,79 +211,3 @@ func TestMissingDataConfigParsing(t *testing.T) { }) } } - -func TestBuildCreateOutputMissingDataJob(t *testing.T) { - tests := []struct { - name string - config *MissingDataConfig - expectErr bool - }{ - { - name: "Basic config without issue creation", - config: &MissingDataConfig{ - BaseSafeOutputConfig: BaseSafeOutputConfig{Max: strPtr("0")}, - CreateIssue: false, - TitlePrefix: "[missing data]", - Labels: []string{}, - }, - expectErr: false, - }, - { - name: "Config with issue creation", - config: &MissingDataConfig{ - BaseSafeOutputConfig: BaseSafeOutputConfig{Max: strPtr("5")}, - CreateIssue: true, - TitlePrefix: "[data needed]", - Labels: []string{"data", "blocked"}, - }, - expectErr: false, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - compiler := NewCompiler() - - data := &WorkflowData{ - Name: "Test Workflow", - Source: "test.md", - SafeOutputs: &SafeOutputsConfig{ - MissingData: tt.config, - }, - } - - job, err := compiler.buildCreateOutputMissingDataJob(data, "agent") - - if tt.expectErr { - if err == nil { - t.Error("Expected error but got nil") - } - return - } - - if err != nil { - t.Errorf("Unexpected error: %v", err) - return - } - - if job == nil { - t.Fatal("Expected job but got nil") - } - - // Verify job name - if job.Name != "missing_data" { - t.Errorf("Expected job name 'missing_data', got %q", job.Name) - } - - // Verify depends on agent job - if len(job.Needs) != 1 || job.Needs[0] != "agent" { - t.Errorf("Expected job to depend on 'agent', got %v", job.Needs) - } - - // Verify condition contains missing_data type check - if job.If == "" { - t.Error("Expected job condition but got empty string") - } - }) - } -} diff --git a/pkg/workflow/missing_issue_reporting.go b/pkg/workflow/missing_issue_reporting.go index 8ac6522743f..264ce96dcf5 100644 --- a/pkg/workflow/missing_issue_reporting.go +++ b/pkg/workflow/missing_issue_reporting.go @@ -1,10 +1,6 @@ package workflow import ( - "encoding/json" - "fmt" - "strings" - "github.com/github/gh-aw/pkg/logger" ) @@ -43,69 +39,6 @@ type issueReportingJobParams struct { log *logger.Logger } -// buildIssueReportingJob constructs the GitHub Actions job for a missing-data or missing-tool -// safe-output type. The two callers differ only in the params they supply. -func (c *Compiler) buildIssueReportingJob(data *WorkflowData, mainJobName string, p issueReportingJobParams) (*Job, error) { - p.log.Printf("Building %s job for workflow: %s", p.kind, data.Name) - - var customEnvVars []string - - if p.config.Max != nil { - p.log.Printf("Setting max %s limit: %s", p.kind, *p.config.Max) - customEnvVars = append(customEnvVars, buildTemplatableIntEnvVar(p.envPrefix+"_MAX", p.config.Max)...) - } - - if p.config.CreateIssue { - customEnvVars = append(customEnvVars, fmt.Sprintf(" %s_CREATE_ISSUE: \"true\"\n", p.envPrefix)) - p.log.Printf("create-issue enabled for %s", p.kind) - } - - if p.config.TitlePrefix != "" { - customEnvVars = append(customEnvVars, fmt.Sprintf(" %s_TITLE_PREFIX: %q\n", p.envPrefix, p.config.TitlePrefix)) - p.log.Printf("title-prefix: %s", p.config.TitlePrefix) - } - - if len(p.config.Labels) > 0 { - labelsJSON, err := json.Marshal(p.config.Labels) - if err == nil { - customEnvVars = append(customEnvVars, fmt.Sprintf(" %s_LABELS: %q\n", p.envPrefix, string(labelsJSON))) - p.log.Printf("labels: %v", p.config.Labels) - } - } - - customEnvVars = append(customEnvVars, buildWorkflowMetadataEnvVarsWithTrackerID(data.Name, data.Source, data.TrackerID)...) - - outputs := map[string]string{ - p.outputKey: fmt.Sprintf("${{ steps.%s.outputs.%s }}", p.kind, p.outputKey), - "total_count": fmt.Sprintf("${{ steps.%s.outputs.total_count }}", p.kind), - } - - jobCondition := BuildSafeOutputType(p.kind) - - permissions := NewPermissionsContentsRead() - if p.config.CreateIssue { - permissions.Set(PermissionIssues, PermissionWrite) - p.log.Printf("Added issues:write permission for create-issue functionality") - } - - script := fmt.Sprintf("const { main } = require('/opt/gh-aw/actions/%s.cjs'); await main();", p.kind) - - return c.buildSafeOutputJob(data, SafeOutputJobConfig{ - JobName: p.kind, - StepName: p.stepName, - StepID: p.kind, - MainJobName: mainJobName, - CustomEnvVars: customEnvVars, - Script: script, - Permissions: permissions, - Outputs: outputs, - Condition: jobCondition, - Token: p.config.GitHubToken, - }) -} - -// parseIssueReportingConfig is the shared parsing implementation for missing-data and -// missing-tool configuration blocks. The caller supplies the YAML key and default title. func (c *Compiler) parseIssueReportingConfig(outputMap map[string]any, yamlKey, defaultTitle string, log *logger.Logger) *IssueReportingConfig { configData, exists := outputMap[yamlKey] if !exists { @@ -169,9 +102,3 @@ func (c *Compiler) parseIssueReportingConfig(outputMap map[string]any, yamlKey, return cfg } - -// envVarPrefix converts a snake_case kind (e.g. "missing_data") to its env-var prefix -// (e.g. "GH_AW_MISSING_DATA"). -func envVarPrefix(kind string) string { - return "GH_AW_" + strings.ToUpper(kind) -} diff --git a/pkg/workflow/missing_tool.go b/pkg/workflow/missing_tool.go index 9a4b63c91f9..196644f53ea 100644 --- a/pkg/workflow/missing_tool.go +++ b/pkg/workflow/missing_tool.go @@ -1,31 +1,11 @@ package workflow import ( - "errors" - "github.com/github/gh-aw/pkg/logger" ) var missingToolLog = logger.New("workflow:missing_tool") -// buildCreateOutputMissingToolJob creates the missing_tool job -func (c *Compiler) buildCreateOutputMissingToolJob(data *WorkflowData, mainJobName string) (*Job, error) { - if data.SafeOutputs == nil || data.SafeOutputs.MissingTool == nil { - return nil, errors.New("safe-outputs.missing-tool configuration is required") - } - - return c.buildIssueReportingJob(data, mainJobName, issueReportingJobParams{ - kind: "missing_tool", - envPrefix: envVarPrefix("missing_tool"), - defaultTitle: "[missing tool]", - outputKey: "tools_reported", - stepName: "Record Missing Tool", - config: data.SafeOutputs.MissingTool, - log: missingToolLog, - }) -} - -// parseMissingToolConfig handles missing-tool configuration func (c *Compiler) parseMissingToolConfig(outputMap map[string]any) *MissingToolConfig { return c.parseIssueReportingConfig(outputMap, "missing-tool", "[missing tool]", missingToolLog) } diff --git a/pkg/workflow/missing_tool_test.go b/pkg/workflow/missing_tool_test.go index e43e3ead149..02c2fa5a9c7 100644 --- a/pkg/workflow/missing_tool_test.go +++ b/pkg/workflow/missing_tool_test.go @@ -111,31 +111,6 @@ func TestMissingToolSafeOutput(t *testing.T) { t.Error("Expected MissingTool config to be nil, but it was not") } } - - // Test job creation - if tt.expectJob { - if safeOutputs == nil || safeOutputs.MissingTool == nil { - t.Error("Expected SafeOutputs and MissingTool config to exist for job creation test") - } else { - job, err := compiler.buildCreateOutputMissingToolJob(&WorkflowData{ - SafeOutputs: safeOutputs, - }, "main-job") - if err != nil { - t.Errorf("Failed to build missing tool job: %v", err) - } - if job == nil { - t.Error("Expected job to be created, but it was nil") - } - if job != nil { - if job.Name != "missing_tool" { - t.Errorf("Expected job name to be 'missing_tool', got '%s'", job.Name) - } - if len(job.Needs) != 1 || job.Needs[0] != "main-job" { - t.Errorf("Expected job to depend on 'main-job', got %v", job.Needs) - } - } - } - } }) } } diff --git a/pkg/workflow/safe_output_refactor_test.go b/pkg/workflow/safe_output_refactor_test.go index 1f1d7211577..a066e58b157 100644 --- a/pkg/workflow/safe_output_refactor_test.go +++ b/pkg/workflow/safe_output_refactor_test.go @@ -11,183 +11,6 @@ import ( "github.com/github/gh-aw/pkg/testutil" ) -// TestCreatePRReviewCommentUsesHelper verifies that create_pr_review_comment.go -// uses the buildSafeOutputJobEnvVars helper correctly -func TestCreatePRReviewCommentUsesHelper(t *testing.T) { - c := NewCompiler() - - workflowData := &WorkflowData{ - Name: "test-workflow", - SafeOutputs: &SafeOutputsConfig{ - Staged: true, - CreatePullRequestReviewComments: &CreatePullRequestReviewCommentsConfig{ - BaseSafeOutputConfig: BaseSafeOutputConfig{Max: strPtr("10")}, - TargetRepoSlug: "owner/target-repo", - }, - }, - } - - job, err := c.buildCreateOutputPullRequestReviewCommentJob(workflowData, "main_job") - if err != nil { - t.Fatalf("Unexpected error building PR review comment job: %v", err) - } - - // Convert steps to a single string for testing - stepsContent := strings.Join(job.Steps, "") - - // Verify that GH_AW_SAFE_OUTPUTS_STAGED is present - if !strings.Contains(stepsContent, " GH_AW_SAFE_OUTPUTS_STAGED: \"true\"\n") { - t.Error("Expected GH_AW_SAFE_OUTPUTS_STAGED to be set in create-pull-request-review-comment job") - } - - // Verify that GH_AW_TARGET_REPO_SLUG is present with the correct value - if !strings.Contains(stepsContent, " GH_AW_TARGET_REPO_SLUG: \"owner/target-repo\"\n") { - t.Error("Expected GH_AW_TARGET_REPO_SLUG to be set correctly in create-pull-request-review-comment job") - } -} - -// TestCreateDiscussionUsesHelper verifies that create_discussion.go -// uses the buildSafeOutputJobEnvVars helper correctly (standalone job still uses env vars) -func TestCreateDiscussionUsesHelper(t *testing.T) { - c := NewCompiler() - - workflowData := &WorkflowData{ - Name: "test-workflow", - SafeOutputs: &SafeOutputsConfig{ - Staged: true, - CreateDiscussions: &CreateDiscussionsConfig{ - BaseSafeOutputConfig: BaseSafeOutputConfig{Max: strPtr("1")}, - Category: "12345", - TargetRepoSlug: "owner/target-repo", - }, - }, - } - - job, err := c.buildCreateOutputDiscussionJob(workflowData, "main_job", "") - if err != nil { - t.Fatalf("Unexpected error building discussion job: %v", err) - } - - // Convert steps to a single string for testing - stepsContent := strings.Join(job.Steps, "") - - // Verify that GH_AW_SAFE_OUTPUTS_STAGED is present - if !strings.Contains(stepsContent, " GH_AW_SAFE_OUTPUTS_STAGED: \"true\"\n") { - t.Error("Expected GH_AW_SAFE_OUTPUTS_STAGED to be set in create-discussion standalone job") - } - - // Standalone jobs still use env var for target-repo (not handler config) - // This is expected for backward compatibility with non-consolidated jobs - // The handler manager version would use config, but this is the standalone job -} - -// TestTrialModeWithoutTargetRepo verifies that trial mode without explicit -// target-repo config uses the trial repo slug -func TestTrialModeWithoutTargetRepo(t *testing.T) { - c := NewCompiler() - c.SetTrialMode(true) - c.SetTrialLogicalRepoSlug("owner/trial-repo") - - workflowData := &WorkflowData{ - Name: "test-workflow", - TrialMode: true, - TrialLogicalRepo: "owner/trial-repo", - SafeOutputs: &SafeOutputsConfig{ - CreateDiscussions: &CreateDiscussionsConfig{ - BaseSafeOutputConfig: BaseSafeOutputConfig{Max: strPtr("1")}, - Category: "12345", - }, - }, - } - - job, err := c.buildCreateOutputDiscussionJob(workflowData, "main_job", "") - if err != nil { - t.Fatalf("Unexpected error building discussion job: %v", err) - } - - // Convert steps to a single string for testing - stepsContent := strings.Join(job.Steps, "") - - // Verify that GH_AW_SAFE_OUTPUTS_STAGED is present (trial mode sets this) - if !strings.Contains(stepsContent, " GH_AW_SAFE_OUTPUTS_STAGED: \"true\"\n") { - t.Error("Expected GH_AW_SAFE_OUTPUTS_STAGED to be set in trial mode") - } - - // Verify that GH_AW_TARGET_REPO_SLUG uses trial repo slug - if !strings.Contains(stepsContent, " GH_AW_TARGET_REPO_SLUG: \"owner/trial-repo\"\n") { - t.Error("Expected GH_AW_TARGET_REPO_SLUG to use trial repo slug in trial mode") - } -} - -// TestNoStagedNorTrialMode verifies that neither staged flag nor target repo slug -// are added when not configured -func TestNoStagedNorTrialMode(t *testing.T) { - c := NewCompiler() - - workflowData := &WorkflowData{ - Name: "test-workflow", - SafeOutputs: &SafeOutputsConfig{ - CreatePullRequestReviewComments: &CreatePullRequestReviewCommentsConfig{ - BaseSafeOutputConfig: BaseSafeOutputConfig{Max: strPtr("10")}, - }, - }, - } - - job, err := c.buildCreateOutputPullRequestReviewCommentJob(workflowData, "main_job") - if err != nil { - t.Fatalf("Unexpected error building PR review comment job: %v", err) - } - - // Convert steps to a single string for testing - stepsContent := strings.Join(job.Steps, "") - - // Verify that GH_AW_SAFE_OUTPUTS_STAGED is NOT present - if strings.Contains(stepsContent, "GH_AW_SAFE_OUTPUTS_STAGED:") { - t.Error("Expected GH_AW_SAFE_OUTPUTS_STAGED to not be set when staged is false") - } - - // Verify that GH_AW_TARGET_REPO_SLUG is NOT present - if strings.Contains(stepsContent, "GH_AW_TARGET_REPO_SLUG:") { - t.Error("Expected GH_AW_TARGET_REPO_SLUG to not be set when not configured") - } -} - -// TestTargetRepoOverridesTrialRepo verifies that explicit target-repo config -// takes precedence over trial mode repo slug -func TestTargetRepoOverridesTrialRepo(t *testing.T) { - c := NewCompiler() - c.SetTrialMode(true) - c.SetTrialLogicalRepoSlug("owner/trial-repo") - - workflowData := &WorkflowData{ - Name: "test-workflow", - SafeOutputs: &SafeOutputsConfig{ - CreatePullRequestReviewComments: &CreatePullRequestReviewCommentsConfig{ - BaseSafeOutputConfig: BaseSafeOutputConfig{Max: strPtr("10")}, - TargetRepoSlug: "owner/explicit-target", - }, - }, - } - - job, err := c.buildCreateOutputPullRequestReviewCommentJob(workflowData, "main_job") - if err != nil { - t.Fatalf("Unexpected error building PR review comment job: %v", err) - } - - // Convert steps to a single string for testing - stepsContent := strings.Join(job.Steps, "") - - // Verify that GH_AW_TARGET_REPO_SLUG uses explicit target, not trial repo - if !strings.Contains(stepsContent, " GH_AW_TARGET_REPO_SLUG: \"owner/explicit-target\"\n") { - t.Error("Expected GH_AW_TARGET_REPO_SLUG to use explicit target-repo, not trial repo") - } - - // Verify that trial repo slug is NOT used - if strings.Contains(stepsContent, " GH_AW_TARGET_REPO_SLUG: \"owner/trial-repo\"\n") { - t.Error("Expected trial repo slug to be overridden by explicit target-repo") - } -} - // TestSafeOutputJobBuilderRefactor validates that the refactored safe output job builders // produce the expected job structure and maintain consistency across different output types func TestSafeOutputJobBuilderRefactor(t *testing.T) { diff --git a/pkg/workflow/security_reports_test.go b/pkg/workflow/security_reports_test.go index 55559032db6..f171ec814bc 100644 --- a/pkg/workflow/security_reports_test.go +++ b/pkg/workflow/security_reports_test.go @@ -3,7 +3,6 @@ package workflow import ( - "strings" "testing" ) @@ -99,136 +98,6 @@ func TestCodeScanningAlertsConfig(t *testing.T) { } } -// TestBuildCreateOutputCodeScanningAlertJob tests the creation of code scanning alert job -func TestBuildCreateOutputCodeScanningAlertJob(t *testing.T) { - compiler := NewCompiler() - - // Test valid configuration - data := &WorkflowData{ - SafeOutputs: &SafeOutputsConfig{ - CreateCodeScanningAlerts: &CreateCodeScanningAlertsConfig{BaseSafeOutputConfig: BaseSafeOutputConfig{Max: nil}}, - }, - } - - job, err := compiler.buildCreateOutputCodeScanningAlertJob(data, "main_job", "test-workflow") - if err != nil { - t.Fatalf("Expected no error, got: %v", err) - } - - if job.Name != "create_code_scanning_alert" { - t.Errorf("Expected job name 'create_code_scanning_alert', got '%s'", job.Name) - } - - if job.TimeoutMinutes != 10 { - t.Errorf("Expected timeout 10 minutes, got %d", job.TimeoutMinutes) - } - - if len(job.Needs) != 1 || job.Needs[0] != "main_job" { - t.Errorf("Expected dependency on 'main_job', got %v", job.Needs) - } - - // Check that job has necessary permissions - if !strings.Contains(job.Permissions, "security-events: write") { - t.Errorf("Expected security-events: write permission in job, got: %s", job.Permissions) - } - - // Check that steps include SARIF upload - stepsStr := strings.Join(job.Steps, "") - if !strings.Contains(stepsStr, "Upload SARIF") { - t.Errorf("Expected SARIF upload steps in job") - } - - if !strings.Contains(stepsStr, "codeql-action/upload-sarif") { - t.Errorf("Expected CodeQL SARIF upload action in job") - } - - // Test with max configuration - dataWithMax := &WorkflowData{ - SafeOutputs: &SafeOutputsConfig{ - CreateCodeScanningAlerts: &CreateCodeScanningAlertsConfig{BaseSafeOutputConfig: BaseSafeOutputConfig{Max: strPtr("25")}}, - }, - } - - jobWithMax, err := compiler.buildCreateOutputCodeScanningAlertJob(dataWithMax, "main_job", "test-workflow") - if err != nil { - t.Fatalf("Expected no error, got: %v", err) - } - - stepsWithMaxStr := strings.Join(jobWithMax.Steps, "") - if !strings.Contains(stepsWithMaxStr, "GH_AW_SECURITY_REPORT_MAX: 25") { - t.Errorf("Expected max configuration in environment variables") - } - - // Test with driver configuration - dataWithDriver := &WorkflowData{ - Name: "My Security Workflow", - FrontmatterName: "My Security Workflow", - SafeOutputs: &SafeOutputsConfig{ - CreateCodeScanningAlerts: &CreateCodeScanningAlertsConfig{Driver: "Custom Scanner"}, - }, - } - - jobWithDriver, err := compiler.buildCreateOutputCodeScanningAlertJob(dataWithDriver, "main_job", "my-workflow") - if err != nil { - t.Fatalf("Expected no error, got: %v", err) - } - - stepsWithDriverStr := strings.Join(jobWithDriver.Steps, "") - if !strings.Contains(stepsWithDriverStr, "GH_AW_SECURITY_REPORT_DRIVER: Custom Scanner") { - t.Errorf("Expected driver configuration in environment variables") - } - - // Test with no driver configuration - should default to frontmatter name - dataNoDriver := &WorkflowData{ - Name: "Security Analysis Workflow", - FrontmatterName: "Security Analysis Workflow", - SafeOutputs: &SafeOutputsConfig{ - CreateCodeScanningAlerts: &CreateCodeScanningAlertsConfig{BaseSafeOutputConfig: BaseSafeOutputConfig{Max: nil}}, // No driver specified - }, - } - - jobNoDriver, err := compiler.buildCreateOutputCodeScanningAlertJob(dataNoDriver, "main_job", "security-analysis") - if err != nil { - t.Fatalf("Expected no error, got: %v", err) - } - - stepsNoDriverStr := strings.Join(jobNoDriver.Steps, "") - if !strings.Contains(stepsNoDriverStr, "GH_AW_SECURITY_REPORT_DRIVER: Security Analysis Workflow") { - t.Errorf("Expected frontmatter name as default driver in environment variables, got: %s", stepsNoDriverStr) - } - - // Test with no driver and no frontmatter name - should fallback to H1 name - dataFallback := &WorkflowData{ - Name: "Security Analysis", - FrontmatterName: "", // No frontmatter name - SafeOutputs: &SafeOutputsConfig{ - CreateCodeScanningAlerts: &CreateCodeScanningAlertsConfig{BaseSafeOutputConfig: BaseSafeOutputConfig{Max: nil}}, // No driver specified - }, - } - - jobFallback, err := compiler.buildCreateOutputCodeScanningAlertJob(dataFallback, "main_job", "security-analysis") - if err != nil { - t.Fatalf("Expected no error, got: %v", err) - } - - stepsFallbackStr := strings.Join(jobFallback.Steps, "") - if !strings.Contains(stepsFallbackStr, "GH_AW_SECURITY_REPORT_DRIVER: Security Analysis") { - t.Errorf("Expected H1 name as fallback driver in environment variables, got: %s", stepsFallbackStr) - } - - // Check that workflow filename is passed - if !strings.Contains(stepsWithDriverStr, "GH_AW_WORKFLOW_FILENAME: my-workflow") { - t.Errorf("Expected workflow filename in environment variables") - } - - // Test error case - no configuration - dataNoConfig := &WorkflowData{SafeOutputs: nil} - _, err = compiler.buildCreateOutputCodeScanningAlertJob(dataNoConfig, "main_job", "test-workflow") - if err == nil { - t.Errorf("Expected error when no SafeOutputs config provided") - } -} - // TestParseCodeScanningAlertsConfig tests the parsing function directly func TestParseCodeScanningAlertsConfig(t *testing.T) { compiler := NewCompiler()