From 06cc28543e69062a0b5a63cff3318c787ac1ced4 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 3 Jun 2026 17:55:34 +0000 Subject: [PATCH 1/4] Initial plan From 29d04e1ed9b98b3043dbfbbc69d2b1d82194657a Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 3 Jun 2026 18:03:01 +0000 Subject: [PATCH 2/4] Plan: refactor buildCustomJobs with helpers Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com> --- .github/workflows/deep-report.lock.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/deep-report.lock.yml b/.github/workflows/deep-report.lock.yml index 4859162e9ae..cbfbc570d8e 100644 --- a/.github/workflows/deep-report.lock.yml +++ b/.github/workflows/deep-report.lock.yml @@ -1,4 +1,4 @@ -# gh-aw-metadata: {"schema_version":"v4","frontmatter_hash":"b1378e3a94dc80144c9098262e28b1643aa88cc03737e623786dd73e87d8319f","body_hash":"d9aeb7d1ad73b83d345a7b34a53f3878ebc8fb846420f8e7cf5bb129711c0a4e","strict":true,"agent_id":"claude"} +# gh-aw-metadata: {"schema_version":"v4","frontmatter_hash":"b1378e3a94dc80144c9098262e28b1643aa88cc03737e623786dd73e87d8319f","body_hash":"998dc32188e283fc86b37ca07bef72bd4c957f0282c50af482395434a8b2cb66","strict":true,"agent_id":"claude"} # gh-aw-manifest: {"version":1,"secrets":["ANTHROPIC_API_KEY","GH_AW_GITHUB_MCP_SERVER_TOKEN","GH_AW_GITHUB_TOKEN","GH_AW_OTEL_GRAFANA_AUTHORIZATION","GH_AW_OTEL_GRAFANA_ENDPOINT","GH_AW_OTEL_SENTRY_AUTHORIZATION","GH_AW_OTEL_SENTRY_ENDPOINT","GITHUB_TOKEN"],"actions":[{"repo":"actions/cache/restore","sha":"27d5ce7f107fe9357f9df03efb73ab90386fccae","version":"v5.0.5"},{"repo":"actions/cache/save","sha":"27d5ce7f107fe9357f9df03efb73ab90386fccae","version":"v5.0.5"},{"repo":"actions/checkout","sha":"de0fac2e4500dabe0009e67214ff5f5447ce83dd","version":"v6.0.2"},{"repo":"actions/download-artifact","sha":"3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c","version":"v8.0.1"},{"repo":"actions/github-script","sha":"3a2844b7e9c422d3c10d287c895573f7108da1b3","version":"v9.0.0"},{"repo":"actions/setup-go","sha":"4a3601121dd01d1626a1e23e37211e3254c1c06c","version":"v6.4.0"},{"repo":"actions/setup-node","sha":"48b55a011bda9f5d6aeb4c2d9c7362e8dae4041e","version":"v6.4.0"},{"repo":"actions/upload-artifact","sha":"043fb46d1a93c77aae656e7c1c64a875d1fc6a0a","version":"v7.0.1"},{"repo":"docker/build-push-action","sha":"f9f3042f7e2789586610d6e8b85c8f03e5195baf","version":"v7.2.0"},{"repo":"docker/setup-buildx-action","sha":"d7f5e7f509e45cec5c76c4d5afdd7de93d0b3df5","version":"v4.1.0"}],"containers":[{"image":"ghcr.io/github/gh-aw-firewall/agent:0.25.58"},{"image":"ghcr.io/github/gh-aw-firewall/api-proxy:0.25.58"},{"image":"ghcr.io/github/gh-aw-firewall/cli-proxy:0.25.58"},{"image":"ghcr.io/github/gh-aw-firewall/squid:0.25.58"},{"image":"ghcr.io/github/gh-aw-mcpg:v0.3.22"},{"image":"ghcr.io/github/github-mcp-server:v1.1.0"},{"image":"node:lts-alpine","digest":"sha256:2bdb65ed1dab192432bc31c95f94155ca5ad7fc1392fb7eb7526ab682fa5bf14","pinned_image":"node:lts-alpine@sha256:2bdb65ed1dab192432bc31c95f94155ca5ad7fc1392fb7eb7526ab682fa5bf14"}]} # ___ _ _ # / _ \ | | (_) From 1ab63312b3772a2cb93b67808e99dbcd5c5bb66f Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 3 Jun 2026 18:06:40 +0000 Subject: [PATCH 3/4] Refactor buildCustomJobs into helper methods Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com> --- pkg/workflow/compiler_jobs.go | 772 ++++++++++++++++++++-------------- 1 file changed, 455 insertions(+), 317 deletions(-) diff --git a/pkg/workflow/compiler_jobs.go b/pkg/workflow/compiler_jobs.go index 261236f45c7..9269307b83f 100644 --- a/pkg/workflow/compiler_jobs.go +++ b/pkg/workflow/compiler_jobs.go @@ -514,6 +514,40 @@ func (c *Compiler) extractJobsFromFrontmatter(frontmatter map[string]any) map[st func (c *Compiler) buildCustomJobs(data *WorkflowData, activationJobCreated bool) error { compilerJobsLog.Printf("Building %d custom jobs", len(data.Jobs)) + promptReferencedJobs, onNeedsJobs := c.getCustomJobDependencySets(data) + + for jobName, jobConfig := range data.Jobs { + if c.shouldSkipCustomJob(jobName) { + continue + } + configMap, ok := jobConfig.(map[string]any) + if !ok { + continue + } + + job, err := c.buildCustomJob( + jobName, + configMap, + data, + activationJobCreated, + promptReferencedJobs, + onNeedsJobs, + ) + if err != nil { + return err + } + + if err := c.jobManager.AddJob(job); err != nil { + return fmt.Errorf("failed to add custom job '%s': %w", jobName, err) + } + compilerJobsLog.Printf("Successfully added custom job '%s' with %d needs dependencies", jobName, len(job.Needs)) + } + + compilerJobsLog.Print("Completed building all custom jobs") + return nil +} + +func (c *Compiler) getCustomJobDependencySets(data *WorkflowData) (map[string]bool, map[string]bool) { // Pre-compute jobs referenced in the markdown body with no explicit needs. // These run before activation (not after), so we must not auto-add activation to them. promptReferencedJobsSlice := c.getCustomJobsReferencedInPromptWithNoActivationDep(data) @@ -521,361 +555,465 @@ func (c *Compiler) buildCustomJobs(data *WorkflowData, activationJobCreated bool for _, j := range promptReferencedJobsSlice { promptReferencedJobs[j] = true } + onNeedsJobs := make(map[string]bool, len(data.OnNeeds)) for _, j := range data.OnNeeds { onNeedsJobs[j] = true } - for jobName, jobConfig := range data.Jobs { - // Skip jobs.pre-activation (or pre_activation) as it's handled specially in buildPreActivationJob - if jobName == string(constants.PreActivationJobName) || jobName == "pre-activation" { - compilerJobsLog.Printf("Skipping jobs.%s (handled in buildPreActivationJob)", jobName) - continue + return promptReferencedJobs, onNeedsJobs +} + +func (c *Compiler) shouldSkipCustomJob(jobName string) bool { + // Skip jobs.pre-activation (or pre_activation) as it's handled specially in buildPreActivationJob + if jobName == string(constants.PreActivationJobName) || jobName == "pre-activation" { + compilerJobsLog.Printf("Skipping jobs.%s (handled in buildPreActivationJob)", jobName) + return true + } + + // Built-in jobs are already created before buildCustomJobs; treat jobs. + // entries as customization-only and do not create duplicate jobs. + if _, exists := c.jobManager.GetJob(jobName); exists { + compilerJobsLog.Printf("Skipping jobs.%s (built-in job already exists)", jobName) + return true + } + + return false +} + +func (c *Compiler) buildCustomJob( + jobName string, + configMap map[string]any, + data *WorkflowData, + activationJobCreated bool, + promptReferencedJobs map[string]bool, + onNeedsJobs map[string]bool, +) (*Job, error) { + job := &Job{Name: jobName} + + hasExplicitNeeds := extractCustomJobNeeds(job, configMap) + c.applyAutomaticActivationDependency(job, jobName, hasExplicitNeeds, activationJobCreated, promptReferencedJobs, onNeedsJobs) + + if err := c.extractCustomJobProperties(job, jobName, configMap); err != nil { + return nil, err + } + + if err := c.configureCustomJobExecution(job, jobName, configMap, data); err != nil { + return nil, err + } + + return job, nil +} + +func extractCustomJobNeeds(job *Job, configMap map[string]any) bool { + needs, hasNeeds := configMap["needs"] + if !hasNeeds { + return false + } + + if needsList, ok := needs.([]any); ok { + for _, need := range needsList { + if needStr, ok := need.(string); ok { + job.Needs = append(job.Needs, needStr) + } } + } else if needStr, ok := needs.(string); ok { + // Single dependency as string + job.Needs = append(job.Needs, needStr) + } - // Built-in jobs are already created before buildCustomJobs; treat jobs. - // entries as customization-only and do not create duplicate jobs. - if _, exists := c.jobManager.GetJob(jobName); exists { - compilerJobsLog.Printf("Skipping jobs.%s (built-in job already exists)", jobName) - continue + return true +} + +func (c *Compiler) applyAutomaticActivationDependency( + job *Job, + jobName string, + hasExplicitNeeds bool, + activationJobCreated bool, + promptReferencedJobs map[string]bool, + onNeedsJobs map[string]bool, +) { + // If no explicit needs and activation job exists, automatically add activation as dependency + // This ensures custom jobs wait for workflow validation before executing. + // Exception: jobs whose outputs are referenced in the markdown body run before activation + // (so the activation job can include their outputs in the prompt). + isReferencedInMarkdown := promptReferencedJobs[jobName] + isOnNeedsDependency := onNeedsJobs[jobName] + + if !hasExplicitNeeds && activationJobCreated && !isReferencedInMarkdown && !isOnNeedsDependency { + job.Needs = append(job.Needs, string(constants.ActivationJobName)) + compilerJobsLog.Printf("Added automatic dependency: custom job '%s' now depends on '%s'", jobName, string(constants.ActivationJobName)) + } else if !hasExplicitNeeds && isReferencedInMarkdown { + compilerJobsLog.Printf("Custom job '%s' referenced in markdown body runs before activation (no auto-added dependency)", jobName) + } else if !hasExplicitNeeds && isOnNeedsDependency { + compilerJobsLog.Printf("Custom job '%s' listed in on.needs runs before activation (no auto-added dependency)", jobName) + } +} + +func (c *Compiler) extractCustomJobProperties(job *Job, jobName string, configMap map[string]any) error { + if err := c.extractCustomJobCoreProperties(job, jobName, configMap); err != nil { + return err + } + extractCustomJobOutputs(job, jobName, configMap) + return nil +} + +func (c *Compiler) extractCustomJobCoreProperties(job *Job, jobName string, configMap map[string]any) error { + if err := c.extractCustomJobRunsOn(job, jobName, configMap); err != nil { + return err + } + + if ifCond, hasIf := configMap["if"]; hasIf { + if ifStr, ok := ifCond.(string); ok { + job.If = c.extractExpressionFromIfString(ifStr) } + } - if configMap, ok := jobConfig.(map[string]any); ok { - job := &Job{ - Name: jobName, + if permissions, hasPermissions := configMap["permissions"]; hasPermissions { + if permsMap, ok := permissions.(map[string]any); ok { + formattedPerms, err := formatIndentedYAMLField("permissions", permsMap, false) + if err != nil { + return fmt.Errorf("failed to convert permissions to YAML for job '%s': %w", jobName, err) } + job.Permissions = formattedPerms + } + } - // Extract job dependencies - hasExplicitNeeds := false - if needs, hasNeeds := configMap["needs"]; hasNeeds { - hasExplicitNeeds = true - if needsList, ok := needs.([]any); ok { - for _, need := range needsList { - if needStr, ok := need.(string); ok { - job.Needs = append(job.Needs, needStr) - } - } - } else if needStr, ok := needs.(string); ok { - // Single dependency as string - job.Needs = append(job.Needs, needStr) - } + if strategy, hasStrategy := configMap["strategy"]; hasStrategy { + if strategyMap, ok := strategy.(map[string]any); ok { + formattedStrategy, err := formatIndentedYAMLField("strategy", strategyMap, false) + if err != nil { + return fmt.Errorf("failed to convert strategy to YAML for job '%s': %w", jobName, err) } + job.Strategy = formattedStrategy + } + } - // If no explicit needs and activation job exists, automatically add activation as dependency - // This ensures custom jobs wait for workflow validation before executing. - // Exception: jobs whose outputs are referenced in the markdown body run before activation - // (so the activation job can include their outputs in the prompt). - isReferencedInMarkdown := promptReferencedJobs[jobName] - isOnNeedsDependency := onNeedsJobs[jobName] - if !hasExplicitNeeds && activationJobCreated && !isReferencedInMarkdown && !isOnNeedsDependency { - job.Needs = append(job.Needs, string(constants.ActivationJobName)) - compilerJobsLog.Printf("Added automatic dependency: custom job '%s' now depends on '%s'", jobName, string(constants.ActivationJobName)) - } else if !hasExplicitNeeds && isReferencedInMarkdown { - compilerJobsLog.Printf("Custom job '%s' referenced in markdown body runs before activation (no auto-added dependency)", jobName) - } else if !hasExplicitNeeds && isOnNeedsDependency { - compilerJobsLog.Printf("Custom job '%s' listed in on.needs runs before activation (no auto-added dependency)", jobName) - } + // Extract name (display name) for custom jobs + if name, hasName := configMap["name"]; hasName { + if nameStr, ok := name.(string); ok { + job.DisplayName = nameStr + } + } - // Extract other job properties - if runsOn, hasRunsOn := configMap["runs-on"]; hasRunsOn { - if runsOnStr, ok := runsOn.(string); ok { - job.RunsOn = "runs-on: " + runsOnStr - } else { - // Array or object form: marshal the value and build indented YAML snippet - yamlBytes, err := yaml.Marshal(runsOn) - if err != nil { - return fmt.Errorf("failed to convert runs-on to YAML for job '%s': %w", jobName, err) - } - lines := strings.Split(strings.TrimSpace(string(yamlBytes)), "\n") - var b strings.Builder - b.WriteString("runs-on:\n") - for _, line := range lines { - b.WriteString(" " + line + "\n") - } - job.RunsOn = strings.TrimSuffix(b.String(), "\n") - } - } + if err := extractCustomJobTimeoutMinutes(job, jobName, configMap); err != nil { + return err + } - if ifCond, hasIf := configMap["if"]; hasIf { - if ifStr, ok := ifCond.(string); ok { - job.If = c.extractExpressionFromIfString(ifStr) - } - } + if err := extractCustomJobConcurrency(job, jobName, configMap); err != nil { + return err + } - // Extract permissions - if permissions, hasPermissions := configMap["permissions"]; hasPermissions { - if permsMap, ok := permissions.(map[string]any); ok { - // Use goccy/go-yaml to marshal permissions - yamlBytes, err := yaml.Marshal(permsMap) - if err != nil { - return fmt.Errorf("failed to convert permissions to YAML for job '%s': %w", jobName, err) - } - // Indent the YAML properly for job-level permissions - permsYAML := string(yamlBytes) - lines := strings.Split(strings.TrimSpace(permsYAML), "\n") - var formattedPerms strings.Builder - formattedPerms.WriteString("permissions:\n") - for _, line := range lines { - formattedPerms.WriteString(" " + line + "\n") - } - job.Permissions = formattedPerms.String() - } - } + extractCustomJobEnv(job, configMap) - // Extract strategy for custom jobs - if strategy, hasStrategy := configMap["strategy"]; hasStrategy { - if strategyMap, ok := strategy.(map[string]any); ok { - // Use goccy/go-yaml to marshal strategy - yamlBytes, err := yaml.Marshal(strategyMap) - if err != nil { - return fmt.Errorf("failed to convert strategy to YAML for job '%s': %w", jobName, err) - } - // Indent the YAML properly for job-level strategy - strategyYAML := string(yamlBytes) - lines := strings.Split(strings.TrimSpace(strategyYAML), "\n") - var formattedStrategy strings.Builder - formattedStrategy.WriteString("strategy:\n") - for _, line := range lines { - formattedStrategy.WriteString(" " + line + "\n") - } - job.Strategy = formattedStrategy.String() - } - } + if err := extractCustomJobContainer(job, jobName, configMap); err != nil { + return err + } + if err := extractCustomJobServices(job, jobName, configMap); err != nil { + return err + } + extractCustomJobContinueOnError(job, configMap) - // Extract name (display name) for custom jobs - if name, hasName := configMap["name"]; hasName { - if nameStr, ok := name.(string); ok { - job.DisplayName = nameStr - } - } + if err := extractCustomJobEnvironment(job, jobName, configMap); err != nil { + return err + } - // Extract timeout-minutes for custom jobs - if timeout, hasTimeout := configMap["timeout-minutes"]; hasTimeout { - switch v := timeout.(type) { - case int: - job.TimeoutMinutes = v - case uint64: - if v <= uint64(^uint(0)>>1) { - job.TimeoutMinutes = int(v) - } - case float64: - job.TimeoutMinutes = int(v) - case string: - // isExpression validates full GitHub Actions expression syntax (${{ - // ... }}) and is defined in expression_patterns.go. - if isExpression(v) { - job.TimeoutMinutesExpression = v - } else { - return fmt.Errorf( - "job '%s' timeout-minutes must be an integer or a GitHub Actions expression (e.g. '${{ inputs.timeout }}'), got %q", - jobName, - v, - ) - } - } - } + return nil +} - // Extract concurrency for custom jobs - if concurrency, hasConcurrency := configMap["concurrency"]; hasConcurrency { - switch v := concurrency.(type) { - case string: - job.Concurrency = "concurrency: " + v - case map[string]any: - // Default cancel-in-progress to false for non-agent jobs if not explicitly set. - // This prevents accidental cancellation of queued runs when multiple agents - // are running the same workflow concurrently. - if _, hasCancelInProgress := v["cancel-in-progress"]; !hasCancelInProgress { - v["cancel-in-progress"] = false - } - yamlBytes, err := yaml.Marshal(v) - if err != nil { - return fmt.Errorf("failed to convert concurrency to YAML for job '%s': %w", jobName, err) - } - lines := strings.Split(strings.TrimSpace(string(yamlBytes)), "\n") - var formattedConcurrency strings.Builder - formattedConcurrency.WriteString("concurrency:\n") - for _, line := range lines { - formattedConcurrency.WriteString(" " + line + "\n") - } - job.Concurrency = formattedConcurrency.String() - } - } +func (c *Compiler) extractCustomJobRunsOn(job *Job, jobName string, configMap map[string]any) error { + runsOn, hasRunsOn := configMap["runs-on"] + if !hasRunsOn { + return nil + } + if runsOnStr, ok := runsOn.(string); ok { + job.RunsOn = "runs-on: " + runsOnStr + return nil + } - // Extract env for custom jobs - if env, hasEnv := configMap["env"]; hasEnv { - if envMap, ok := env.(map[string]any); ok { - job.Env = make(map[string]string) - for key, val := range envMap { - if valStr, ok := val.(string); ok { - job.Env[key] = valStr - } else if val != nil { - // Arrays and maps are serialized as JSON so that shell consumers - // (e.g. jq --argjson) receive valid JSON. - job.Env[key] = marshalEnvValue(val) - } - } - } - } + // Array or object form: marshal the value and build indented YAML snippet + formattedRunsOn, err := formatIndentedYAMLField("runs-on", runsOn, true) + if err != nil { + return fmt.Errorf("failed to convert runs-on to YAML for job '%s': %w", jobName, err) + } + job.RunsOn = formattedRunsOn + return nil +} - // Extract container for custom jobs - if container, hasContainer := configMap["container"]; hasContainer { - switch v := container.(type) { - case string: - job.Container = "container: " + v - case map[string]any: - yamlBytes, err := yaml.Marshal(v) - if err != nil { - return fmt.Errorf("failed to convert container to YAML for job '%s': %w", jobName, err) - } - lines := strings.Split(strings.TrimSpace(string(yamlBytes)), "\n") - var formattedContainer strings.Builder - formattedContainer.WriteString("container:\n") - for _, line := range lines { - formattedContainer.WriteString(" " + line + "\n") - } - job.Container = formattedContainer.String() - } - } +func extractCustomJobTimeoutMinutes(job *Job, jobName string, configMap map[string]any) error { + timeout, hasTimeout := configMap["timeout-minutes"] + if !hasTimeout { + return nil + } - // Extract services for custom jobs - if services, hasServices := configMap["services"]; hasServices { - if servicesMap, ok := services.(map[string]any); ok { - yamlBytes, err := yaml.Marshal(servicesMap) - if err != nil { - return fmt.Errorf("failed to convert services to YAML for job '%s': %w", jobName, err) - } - lines := strings.Split(strings.TrimSpace(string(yamlBytes)), "\n") - var formattedServices strings.Builder - formattedServices.WriteString("services:\n") - for _, line := range lines { - formattedServices.WriteString(" " + line + "\n") - } - job.Services = formattedServices.String() - } - } + switch v := timeout.(type) { + case int: + job.TimeoutMinutes = v + case uint64: + if v <= uint64(^uint(0)>>1) { + job.TimeoutMinutes = int(v) + } + case float64: + job.TimeoutMinutes = int(v) + case string: + // isExpression validates full GitHub Actions expression syntax (${{ + // ... }}) and is defined in expression_patterns.go. + if isExpression(v) { + job.TimeoutMinutesExpression = v + } else { + return fmt.Errorf( + "job '%s' timeout-minutes must be an integer or a GitHub Actions expression (e.g. '${{ inputs.timeout }}'), got %q", + jobName, + v, + ) + } + } - // Extract continue-on-error for custom jobs - if continueOnError, hasCOE := configMap["continue-on-error"]; hasCOE { - if coeVal, ok := continueOnError.(bool); ok { - job.ContinueOnError = &coeVal - } - } + return nil +} - // Extract environment for custom jobs - if environment, hasEnvironment := configMap["environment"]; hasEnvironment { - switch v := environment.(type) { - case string: - job.Environment = "environment: " + v - case map[string]any: - yamlBytes, err := yaml.Marshal(v) - if err != nil { - return fmt.Errorf("failed to convert environment to YAML for job '%s': %w", jobName, err) - } - lines := strings.Split(strings.TrimSpace(string(yamlBytes)), "\n") - var formattedEnvironment strings.Builder - formattedEnvironment.WriteString("environment:\n") - for _, line := range lines { - formattedEnvironment.WriteString(" " + line + "\n") - } - job.Environment = strings.TrimSuffix(formattedEnvironment.String(), "\n") - } - } +func extractCustomJobConcurrency(job *Job, jobName string, configMap map[string]any) error { + concurrency, hasConcurrency := configMap["concurrency"] + if !hasConcurrency { + return nil + } - // Extract outputs for custom jobs - if outputs, hasOutputs := configMap["outputs"]; hasOutputs { - if outputsMap, ok := outputs.(map[string]any); ok { - job.Outputs = make(map[string]string) - for key, val := range outputsMap { - if valStr, ok := val.(string); ok { - job.Outputs[key] = valStr - } else { - compilerJobsLog.Printf("Warning: output '%s' in job '%s' has non-string value (type: %T), ignoring", key, jobName, val) - } - } - } - } + switch v := concurrency.(type) { + case string: + job.Concurrency = "concurrency: " + v + case map[string]any: + // Default cancel-in-progress to false for non-agent jobs if not explicitly set. + // This prevents accidental cancellation of queued runs when multiple agents + // are running the same workflow concurrently. + if _, hasCancelInProgress := v["cancel-in-progress"]; !hasCancelInProgress { + v["cancel-in-progress"] = false + } - // Check if this is a reusable workflow call - if uses, hasUses := configMap["uses"]; hasUses { - if usesStr, ok := uses.(string); ok { - compilerJobsLog.Printf("Custom job '%s' is a reusable workflow call: %s", jobName, usesStr) - job.Uses = usesStr - - // Extract with parameters for reusable workflow - if with, hasWith := configMap["with"]; hasWith { - if withMap, ok := with.(map[string]any); ok { - job.With = withMap - } - } + formattedConcurrency, err := formatIndentedYAMLField("concurrency", v, false) + if err != nil { + return fmt.Errorf("failed to convert concurrency to YAML for job '%s': %w", jobName, err) + } + job.Concurrency = formattedConcurrency + } - // Extract secrets for reusable workflow - if secrets, hasSecrets := configMap["secrets"]; hasSecrets { - switch sv := secrets.(type) { - case string: - if sv == "inherit" { - job.SecretsInherit = true - } - case map[string]any: - job.Secrets = make(map[string]string) - for key, val := range sv { - if valStr, ok := val.(string); ok { - // Validate that the secret value is a proper GitHub Actions expression - // Note: We don't pass the key to validateSecretsExpression to prevent - // CodeQL from detecting sensitive data flow to error messages/logs - if err := validateSecretsExpression(valStr); err != nil { - return err - } - job.Secrets[key] = valStr - } - } - } - } - } - } else { - // Add basic steps if specified (only for non-reusable workflow jobs). - // `pre-steps` are inserted after setup-injected steps and before the - // regular `steps` list (including any checkout step it may contain). - var preSteps []string - var regularSteps []string - _, hasPreStepsField := configMap["pre-steps"] - _, hasStepsField := configMap["steps"] - if hasPreStepsField { - var err error - preSteps, err = c.extractPinnedJobSteps("pre-steps", jobName, configMap, data) - if err != nil { - return fmt.Errorf("failed to process pre-steps for job '%s': %w", jobName, err) - } - } - if hasStepsField { - var err error - regularSteps, err = c.extractPinnedJobSteps("steps", jobName, configMap, data) - if err != nil { - return fmt.Errorf("failed to process steps for job '%s': %w", jobName, err) - } - } + return nil +} + +func extractCustomJobEnv(job *Job, configMap map[string]any) { + env, hasEnv := configMap["env"] + if !hasEnv { + return + } + envMap, ok := env.(map[string]any) + if !ok { + return + } + + job.Env = make(map[string]string) + for key, val := range envMap { + if valStr, ok := val.(string); ok { + job.Env[key] = valStr + } else if val != nil { + // Arrays and maps are serialized as JSON so that shell consumers + // (e.g. jq --argjson) receive valid JSON. + job.Env[key] = marshalEnvValue(val) + } + } +} + +func extractCustomJobContainer(job *Job, jobName string, configMap map[string]any) error { + container, hasContainer := configMap["container"] + if !hasContainer { + return nil + } + + switch v := container.(type) { + case string: + job.Container = "container: " + v + case map[string]any: + formattedContainer, err := formatIndentedYAMLField("container", v, false) + if err != nil { + return fmt.Errorf("failed to convert container to YAML for job '%s': %w", jobName, err) + } + job.Container = formattedContainer + } + + return nil +} + +func extractCustomJobServices(job *Job, jobName string, configMap map[string]any) error { + services, hasServices := configMap["services"] + if !hasServices { + return nil + } + servicesMap, ok := services.(map[string]any) + if !ok { + return nil + } - if hasPreStepsField || hasStepsField { - // Prepend GH_HOST configuration step for GHES/GHEC compatibility. - // Custom frontmatter jobs run as independent GitHub Actions jobs that - // don't inherit GITHUB_ENV from the agent job, so the gh CLI won't - // know which host to target without this step. - job.Steps = append(job.Steps, generateGHESHostConfigurationStep()) - job.Steps = append(job.Steps, preSteps...) - job.Steps = append(job.Steps, regularSteps...) + formattedServices, err := formatIndentedYAMLField("services", servicesMap, false) + if err != nil { + return fmt.Errorf("failed to convert services to YAML for job '%s': %w", jobName, err) + } + job.Services = formattedServices + return nil +} + +func extractCustomJobContinueOnError(job *Job, configMap map[string]any) { + continueOnError, hasCOE := configMap["continue-on-error"] + if !hasCOE { + return + } + if coeVal, ok := continueOnError.(bool); ok { + job.ContinueOnError = &coeVal + } +} + +func extractCustomJobEnvironment(job *Job, jobName string, configMap map[string]any) error { + environment, hasEnvironment := configMap["environment"] + if !hasEnvironment { + return nil + } + + switch v := environment.(type) { + case string: + job.Environment = "environment: " + v + case map[string]any: + formattedEnvironment, err := formatIndentedYAMLField("environment", v, true) + if err != nil { + return fmt.Errorf("failed to convert environment to YAML for job '%s': %w", jobName, err) + } + job.Environment = formattedEnvironment + } + + return nil +} + +func extractCustomJobOutputs(job *Job, jobName string, configMap map[string]any) { + outputs, hasOutputs := configMap["outputs"] + if !hasOutputs { + return + } + outputsMap, ok := outputs.(map[string]any) + if !ok { + return + } + + job.Outputs = make(map[string]string) + for key, val := range outputsMap { + if valStr, ok := val.(string); ok { + job.Outputs[key] = valStr + } else { + compilerJobsLog.Printf("Warning: output '%s' in job '%s' has non-string value (type: %T), ignoring", key, jobName, val) + } + } +} + +func (c *Compiler) configureCustomJobExecution(job *Job, jobName string, configMap map[string]any, data *WorkflowData) error { + uses, hasUses := configMap["uses"] + if hasUses { + if usesStr, ok := uses.(string); ok { + return configureCustomReusableWorkflow(job, jobName, usesStr, configMap) + } + } + + return c.configureCustomJobSteps(job, jobName, configMap, data) +} + +func configureCustomReusableWorkflow(job *Job, jobName string, usesStr string, configMap map[string]any) error { + compilerJobsLog.Printf("Custom job '%s' is a reusable workflow call: %s", jobName, usesStr) + job.Uses = usesStr + + // Extract with parameters for reusable workflow + if with, hasWith := configMap["with"]; hasWith { + if withMap, ok := with.(map[string]any); ok { + job.With = withMap + } + } + + // Extract secrets for reusable workflow + if secrets, hasSecrets := configMap["secrets"]; hasSecrets { + switch sv := secrets.(type) { + case string: + if sv == "inherit" { + job.SecretsInherit = true + } + case map[string]any: + job.Secrets = make(map[string]string) + for key, val := range sv { + if valStr, ok := val.(string); ok { + // Validate that the secret value is a proper GitHub Actions expression + // Note: We don't pass the key to validateSecretsExpression to prevent + // CodeQL from detecting sensitive data flow to error messages/logs + if err := validateSecretsExpression(valStr); err != nil { + return err + } + job.Secrets[key] = valStr } } + } + } - if err := c.jobManager.AddJob(job); err != nil { - return fmt.Errorf("failed to add custom job '%s': %w", jobName, err) - } - compilerJobsLog.Printf("Successfully added custom job '%s' with %d needs dependencies", jobName, len(job.Needs)) + return nil +} + +func (c *Compiler) configureCustomJobSteps(job *Job, jobName string, configMap map[string]any, data *WorkflowData) error { + // Add basic steps if specified (only for non-reusable workflow jobs). + // `pre-steps` are inserted after setup-injected steps and before the + // regular `steps` list (including any checkout step it may contain). + var preSteps []string + var regularSteps []string + _, hasPreStepsField := configMap["pre-steps"] + _, hasStepsField := configMap["steps"] + + if hasPreStepsField { + var err error + preSteps, err = c.extractPinnedJobSteps("pre-steps", jobName, configMap, data) + if err != nil { + return fmt.Errorf("failed to process pre-steps for job '%s': %w", jobName, err) + } + } + if hasStepsField { + var err error + regularSteps, err = c.extractPinnedJobSteps("steps", jobName, configMap, data) + if err != nil { + return fmt.Errorf("failed to process steps for job '%s': %w", jobName, err) } } - compilerJobsLog.Print("Completed building all custom jobs") + if hasPreStepsField || hasStepsField { + // Prepend GH_HOST configuration step for GHES/GHEC compatibility. + // Custom frontmatter jobs run as independent GitHub Actions jobs that + // don't inherit GITHUB_ENV from the agent job, so the gh CLI won't + // know which host to target without this step. + job.Steps = append(job.Steps, generateGHESHostConfigurationStep()) + job.Steps = append(job.Steps, preSteps...) + job.Steps = append(job.Steps, regularSteps...) + } + return nil } +func formatIndentedYAMLField(fieldName string, value any, trimTrailingNewline bool) (string, error) { + yamlBytes, err := yaml.Marshal(value) + if err != nil { + return "", err + } + + lines := strings.Split(strings.TrimSpace(string(yamlBytes)), "\n") + var b strings.Builder + b.WriteString(fieldName + ":\n") + for _, line := range lines { + b.WriteString(" " + line + "\n") + } + + formatted := b.String() + if trimTrailingNewline { + return strings.TrimSuffix(formatted, "\n"), nil + } + return formatted, nil +} + func (c *Compiler) applyBuiltinJobPreSteps(data *WorkflowData) error { if data == nil || data.Jobs == nil { return nil From c8296234c7b4d43b747ede52cba60b7e768e7cfd Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Wed, 3 Jun 2026 20:34:12 +0000 Subject: [PATCH 4/4] docs(adr): add draft ADR-36694 for buildCustomJobs decomposition Co-Authored-By: Claude Opus 4.8 (1M context) --- ...-decompose-buildcustomjobs-into-helpers.md | 39 +++++++++++++++++++ 1 file changed, 39 insertions(+) create mode 100644 docs/adr/36694-decompose-buildcustomjobs-into-helpers.md diff --git a/docs/adr/36694-decompose-buildcustomjobs-into-helpers.md b/docs/adr/36694-decompose-buildcustomjobs-into-helpers.md new file mode 100644 index 00000000000..c0efe905f34 --- /dev/null +++ b/docs/adr/36694-decompose-buildcustomjobs-into-helpers.md @@ -0,0 +1,39 @@ +# ADR-36694: Decompose `buildCustomJobs` into single-responsibility helpers + +**Date**: 2026-06-03 +**Status**: Draft + +## Context + +`buildCustomJobs` in `pkg/workflow/compiler_jobs.go` had grown into a 365-line method that combined many concerns: iterating frontmatter jobs, skipping built-in/pre-activation jobs, computing dependency sets, applying implicit activation-dependency rules, extracting roughly a dozen job properties (runs-on, if, permissions, strategy, timeout, concurrency, env, container, services, continue-on-error, environment, outputs), and configuring two distinct execution modes (reusable-workflow vs. step-based). The method was a cyclomatic-complexity hotspot, and the per-property YAML handling repeated the same marshal/trim/indent boilerplate seven times. This made the orchestration flow hard to follow and individual property rules hard to test or modify in isolation, while the behavior itself was correct and needed to be preserved exactly. + +## Decision + +We decided to decompose `buildCustomJobs` into a thin orchestrator that delegates to focused, single-responsibility helpers, while preserving existing output and behavior byte-for-byte. The orchestrator now only iterates jobs, skips those that should not produce a custom job, builds each job, and adds it to the job manager. Detailed logic moves into named helpers grouped by concern — dependency-set precomputation (`getCustomJobDependencySets`), skip rules (`shouldSkipCustomJob`), per-job assembly (`buildCustomJob`), needs/activation handling, property extraction (one helper per property), and execution-mode configuration (`configureCustomReusableWorkflow` vs. `configureCustomJobSteps`). A shared `formatIndentedYAMLField` helper replaces the repeated marshal/indent blocks, parameterized by field name and whether to trim the trailing newline. + +## Alternatives Considered + +### Alternative 1: Leave the method as-is (documentation only) +We could have left the monolithic method in place and only added comments or section markers. This avoids churn and review risk, but does nothing to reduce the cyclomatic complexity, keeps the duplicated YAML boilerplate, and leaves the property rules untestable in isolation. Rejected because it does not address the underlying maintainability problem. + +### Alternative 2: Introduce a dedicated `CustomJobBuilder` type +We could have extracted the logic into a separate builder struct that holds the job, config map, and compiler context, exposing methods like `withProperties()` and `withExecution()`. This would centralize state but introduces a new type and lifecycle, diverges from the surrounding free-function/method style used elsewhere in `compiler_jobs.go`, and is a larger structural change than the problem warrants. Rejected in favor of plain helper functions/methods that match existing conventions. + +## Consequences + +### Positive +- Cyclomatic complexity of the orchestrator drops sharply (target under ~150 lines), making the high-level flow readable at a glance. +- Each property extractor and the two execution-mode configurators can now be reasoned about and tested independently. +- The shared `formatIndentedYAMLField` helper removes ~7 copies of marshal/trim/indent boilerplate, reducing the risk of inconsistent indentation bugs. + +### Negative +- The number of functions in `compiler_jobs.go` increases substantially, so navigation now requires jumping between many small functions rather than scrolling one method. +- Behavior-preserving refactors of this size carry regression risk in subtle output formatting (e.g., the `trimTrailingNewline` flag must match each field's previous behavior exactly); this relies on existing tests to catch divergence. + +### Neutral +- Generated workflow YAML output shape is unchanged; this is purely an internal restructuring with no user-visible or schema impact. +- The activation-dependency exception rules (prompt-referenced and `on.needs` jobs running before activation) are preserved verbatim, now isolated in `applyAutomaticActivationDependency`. + +--- + +*This is a DRAFT ADR generated by the [Design Decision Gate](https://github.com/github/gh-aw/actions/runs/26909802176) workflow. The PR author must review, complete, and finalize this document before the PR can merge.*