diff --git a/.github/workflows/contribution-check.lock.yml b/.github/workflows/contribution-check.lock.yml index 4d20274cb5c..753246e6e5c 100644 --- a/.github/workflows/contribution-check.lock.yml +++ b/.github/workflows/contribution-check.lock.yml @@ -23,6 +23,9 @@ # For more information: https://github.github.com/gh-aw/introduction/overview/ # # +# Frontmatter env variables: +# - TARGET_REPOSITORY: (main workflow) +# # Secrets used: # - COPILOT_GITHUB_TOKEN # - GH_AW_GITHUB_MCP_SERVER_TOKEN diff --git a/.github/workflows/stale-repo-identifier.lock.yml b/.github/workflows/stale-repo-identifier.lock.yml index a03e617ca2b..bffdaec66bb 100644 --- a/.github/workflows/stale-repo-identifier.lock.yml +++ b/.github/workflows/stale-repo-identifier.lock.yml @@ -32,6 +32,9 @@ # - shared/reporting.md # - shared/trending-charts-simple.md # +# Frontmatter env variables: +# - ORGANIZATION: (main workflow) +# # Secrets used: # - COPILOT_GITHUB_TOKEN # - GH_AW_GITHUB_MCP_SERVER_TOKEN diff --git a/docs/adr/26113-env-field-support-in-shared-workflow-imports.md b/docs/adr/26113-env-field-support-in-shared-workflow-imports.md new file mode 100644 index 00000000000..6a23efbe0b0 --- /dev/null +++ b/docs/adr/26113-env-field-support-in-shared-workflow-imports.md @@ -0,0 +1,92 @@ +# ADR-26113: `env` Field Support in Shared Workflow Imports with Conflict-Error Semantics + +**Date**: 2026-04-14 +**Status**: Draft +**Deciders**: pelikhan, Copilot + +--- + +## Part 1 — Narrative (Human-Friendly) + +### Context + +The GitHub Agentic Workflows (gh-aw) compiler supports shared workflow files that are imported by main workflows. These shared files allow teams to extract reusable steps, tools, permissions, and other configuration into composable fragments. Prior to this change, the `env` field was explicitly listed in `SharedWorkflowForbiddenFields`, meaning any `env:` block in a shared import was silently dropped with a warning. This prevented shared workflows from declaring workflow-level environment variables (e.g., `TARGET_REPOSITORY`, `SHARED_CONFIG`), forcing workflow authors to duplicate those declarations in every consuming main workflow — violating the DRY principle and making shared workflows less self-contained. + +### Decision + +We will lift the `env` restriction from shared workflow imports and implement a three-tier precedence model: (1) the main workflow's `env` vars always win over any imported value, (2) import-import conflicts on the same variable name are a hard compilation error with an actionable message, and (3) when no conflict exists, imported env vars are merged into the compiled lock file. Additionally, the lock file header will list all env vars with source attribution (`(main workflow)` or the import file path) to aid auditability. This model makes the main workflow the single authoritative override point while enforcing explicit conflict resolution between imports. + +### Alternatives Considered + +#### Alternative 1: Last-Write-Wins Between Imports + +Import order could determine precedence when two shared files define the same env var (breadth-first topological order of the import graph). This would avoid surfacing an error but would make behavior silently dependent on the import declaration order, creating subtle, hard-to-debug bugs when shared files are reordered or when a new shared import is added that happens to define the same variable. + +#### Alternative 2: First-Write-Wins Between Imports + +Similar to last-write-wins but more predictable in practice (the first declared import "owns" the variable). Still suffers from the same silent ordering dependency; teams would have no visible signal that two imports conflict, leading to unexpected behavior when the import list is edited. + +#### Alternative 3: Keep `env` Forbidden in Shared Imports (Status Quo) + +Maintaining the current restriction is simple and avoids the complexity of conflict resolution entirely. However, it forces every consumer of a shared workflow to manually redeclare env vars that logically belong to the shared concern, making shared workflows less reusable and increasing the risk of drift between copies. + +#### Alternative 4: Allow All Overrides Without Source Attribution + +Merging could be done without tracking which import contributed which variable. This simplifies the implementation but sacrifices transparency: when a compiled workflow contains an unexpected env var value, there is no way to determine where it came from without re-reading every imported file. + +### Consequences + +#### Positive +- Shared workflow files are more self-contained; env vars that logically belong to a reusable concern can be co-located with the rest of the shared configuration. +- Import-import conflicts fail loudly at compile time with a clear, actionable error message instead of silently producing incorrect behavior. +- Lock file headers now list all env vars with source attribution, improving auditability of compiled workflows. +- The main workflow retains unconditional override authority, preserving the "main workflow is the source of truth" invariant already established for other merged fields. + +#### Negative +- Workflow authors who accidentally duplicated the same env var across two shared imports will now get a compilation error they must resolve before their workflow compiles. +- The `importAccumulator` struct gains two new fields (`envBuilder`, `envSources`) and the `ImportsResult` and `WorkflowData` types each gain new fields, increasing structural complexity. +- The lock file header grows by the number of env vars, which may slightly increase generated file size. + +#### Neutral +- The env merging approach (newline-separated JSON objects) follows the same internal serialisation convention already used for other merged fields (e.g., `MergedJobs`). +- Existing workflows without `env:` in their shared imports are entirely unaffected; no migration is required. +- The `include_processor.go` suppression-list update (adding `"env"` to valid non-workflow frontmatter fields) removes a spurious warning that users would have seen if they had `env:` in an included file. + +--- + +## Part 2 — Normative Specification (RFC 2119) + +> The key words **MUST**, **MUST NOT**, **REQUIRED**, **SHALL**, **SHALL NOT**, **SHOULD**, **SHOULD NOT**, **RECOMMENDED**, **MAY**, and **OPTIONAL** in this section are to be interpreted as described in [RFC 2119](https://www.rfc-editor.org/rfc/rfc2119). + +### Env Field Allowance + +1. Shared workflow imports **MUST** be permitted to declare an `env:` field; the compiler **MUST NOT** treat `env` as a forbidden field in shared workflow files. +2. The `env` key **MUST NOT** appear in `SharedWorkflowForbiddenFields`. +3. The `env` field **MUST** be listed as a valid non-workflow frontmatter field in the include processor to suppress spurious unknown-field warnings. + +### Env Merge Precedence + +1. Env vars declared in the main workflow **MUST** take precedence over any env var from an imported file with the same key; the compiled output **MUST** use the main workflow's value. +2. When two different imported files both declare the same env var key, the compiler **MUST** return a compilation error before producing any output. +3. The compilation error for import-import conflicts **MUST** identify the conflicting variable name and both import file paths, and **SHOULD** include guidance on how to resolve the conflict (e.g., move the variable to the main workflow or remove it from one import). +4. Imported env vars that do not conflict with each other and are not overridden by the main workflow **MUST** be merged into the compiled workflow's `env:` block. + +### Source Attribution + +1. The compiled lock file header **MUST** include an `# Env variables:` section listing every env var that is present in the merged env block. +2. Each entry in the env variables section **MUST** be annotated with its source: `(main workflow)` if the variable originates from the main workflow file, or the import file path (relative to the repo root) if it originates from a shared import. +3. Keys in the env variables header section **MUST** be emitted in sorted (lexicographic ascending) order for deterministic output. + +### Data Model + +1. `ImportsResult` **MUST** expose a `MergedEnv string` field containing the accumulated env var JSON from all imports, and a `MergedEnvSources map[string]string` field mapping each env var key to its originating import path. +2. `WorkflowData` **MUST** expose an `EnvSources map[string]string` field mapping each env var key to its final source label (`(main workflow)` or import path) for use in lock file header generation. +3. Implementations **MUST NOT** store the merged env blob in any format other than newline-separated JSON objects, consistent with the existing convention for other multi-import merged fields. + +### Conformance + +An implementation is considered conformant with this ADR if it satisfies all **MUST** and **MUST NOT** requirements above. Specifically: the `env` field is accepted in shared imports without warning, import-import conflicts cause a hard compilation error with an informative message, main-workflow env vars always override imported values, and the compiled lock file header lists all env vars with source attribution in sorted order. Failure to meet any **MUST** or **MUST NOT** requirement constitutes non-conformance. + +--- + +*This is a DRAFT ADR generated by the [Design Decision Gate](https://github.com/github/gh-aw/actions/runs/24374798953) workflow. The PR author must review, complete, and finalize this document before the PR can merge.* diff --git a/docs/src/content/docs/reference/imports.md b/docs/src/content/docs/reference/imports.md index ae87450486f..5e648f028e2 100644 --- a/docs/src/content/docs/reference/imports.md +++ b/docs/src/content/docs/reference/imports.md @@ -319,6 +319,7 @@ Shared workflow files (without `on:` field) can define: - `permissions:` - GitHub Actions permissions (validated, not merged) - `runtimes:` - Runtime version overrides (node, python, go, etc.) - `secret-masking:` - Secret masking steps +- `env:` - Workflow-level environment variables - `github-app:` - GitHub App credentials for token minting (centralize shared app config) Agent files (`.github/agents/*.md`) can additionally define: @@ -345,6 +346,7 @@ Imports are processed using breadth-first traversal: direct imports first, then | `steps:` | Imported steps prepended to main; concatenated in import order. | | `jobs:` | Not merged — define only in the main workflow. Use `safe-outputs.jobs` for importable jobs. | | `safe-outputs.jobs` | Names must be unique; duplicates fail. Order determined by `needs:` dependencies. | +| `env:` | Main workflow env vars take precedence over imports. Duplicate keys across different imports fail compilation — move to the main workflow to override imported values. | Example — `tools.bash.allowed` merging: diff --git a/pkg/constants/constants.go b/pkg/constants/constants.go index 1b1b034bbb0..6a2ec3cf59e 100644 --- a/pkg/constants/constants.go +++ b/pkg/constants/constants.go @@ -245,7 +245,7 @@ var IgnoredFrontmatterFields = []string{"user-invokable"} // - Workflow triggers: on (defines it as a main workflow) // - Workflow execution: command, run-name, runs-on, concurrency, if, timeout-minutes, timeout_minutes // - Workflow metadata: name, tracker-id, strict -// - Workflow features: container, env, environment, sandbox, features +// - Workflow features: container, environment, sandbox, features // - Access control: roles, github-token // // All other fields defined in main_workflow_schema.json can be used in shared workflows @@ -255,7 +255,6 @@ var SharedWorkflowForbiddenFields = []string{ "command", // Command for workflow execution "concurrency", // Concurrency control "container", // Container configuration - "env", // Environment variables "environment", // Deployment environment "features", // Feature flags "github-token", // GitHub token configuration diff --git a/pkg/parser/import_field_extractor.go b/pkg/parser/import_field_extractor.go index 38c7e0827eb..1972bb714ef 100644 --- a/pkg/parser/import_field_extractor.go +++ b/pkg/parser/import_field_extractor.go @@ -30,8 +30,10 @@ type importAccumulator struct { permissionsBuilder strings.Builder secretMaskingBuilder strings.Builder postStepsBuilder strings.Builder - jobsBuilder strings.Builder // Jobs from imported YAML workflows - observabilityBuilder strings.Builder // observability config (first-wins for OTLP endpoint) + jobsBuilder strings.Builder // Jobs from imported YAML workflows + envBuilder strings.Builder // env vars from imported workflows (JSON, one object per line) + envSources map[string]string // env var name → source import path (for conflict detection and header listing) + observabilityBuilder strings.Builder // observability config (first-wins for OTLP endpoint) engines []string safeOutputs []string mcpScripts []string @@ -67,6 +69,7 @@ func newImportAccumulator() *importAccumulator { skipRolesSet: make(map[string]bool), skipBotsSet: make(map[string]bool), importInputs: make(map[string]any), + envSources: make(map[string]string), } } @@ -339,6 +342,22 @@ func (acc *importAccumulator) extractAllImportFields(content []byte, item import acc.jobsBuilder.WriteString(jobsContent + "\n") } + // Extract env from imported file (append in order; main workflow env takes precedence). + // Conflicts between two imports are disallowed — only the main workflow may override imported vars. + envContent, err := extractFieldJSONFromMap(fm, "env", "{}") + if err == nil && envContent != "" && envContent != "{}" { + var envMap map[string]any + if jsonErr := json.Unmarshal([]byte(envContent), &envMap); jsonErr == nil { + for key := range envMap { + if existingSource, exists := acc.envSources[key]; exists { + return fmt.Errorf("env variable %q is defined in multiple imports: %q and %q; remove the duplicate definition from one of the imports, or move it to the main workflow to override imported values", key, existingSource, item.importPath) + } + acc.envSources[key] = item.importPath + } + acc.envBuilder.WriteString(envContent + "\n") + } + } + // Extract labels from imported file (merge into set to avoid duplicates) labelsContent, err := extractFieldJSONFromMap(fm, "labels", "[]") if err == nil && labelsContent != "" && labelsContent != "[]" { @@ -439,6 +458,8 @@ func (acc *importAccumulator) toImportsResult(topologicalOrder []string) *Import MergedLabels: acc.labels, MergedCaches: acc.caches, MergedJobs: acc.jobsBuilder.String(), + MergedEnv: acc.envBuilder.String(), + MergedEnvSources: acc.envSources, MergedFeatures: acc.features, MergedObservability: acc.observabilityBuilder.String(), ImportedFiles: topologicalOrder, diff --git a/pkg/parser/import_field_extractor_test.go b/pkg/parser/import_field_extractor_test.go index caa06ca19e8..53df2bfc0c5 100644 --- a/pkg/parser/import_field_extractor_test.go +++ b/pkg/parser/import_field_extractor_test.go @@ -202,6 +202,92 @@ imports: assert.Contains(t, importsResult.MergedJobs, "ubuntu-slim", "MergedJobs should contain the job runner") } +// TestEnvFieldExtractedFromMdImport verifies that env: in a shared .md workflow's +// frontmatter is captured into ImportsResult.MergedEnv and merged correctly. +func TestEnvFieldExtractedFromMdImport(t *testing.T) { + tmpDir := t.TempDir() + + // Create a shared .md workflow with an env: section + sharedContent := `--- +env: + TARGET_REPOSITORY: owner/repo + SHARED_VAR: shared-value +--- + +# Shared workflow with env vars +` + sharedDir := filepath.Join(tmpDir, "shared") + require.NoError(t, os.MkdirAll(sharedDir, 0755), "Failed to create shared dir") + require.NoError(t, os.WriteFile(filepath.Join(sharedDir, "target.md"), []byte(sharedContent), 0644), "Failed to write shared file") + + // Create a main .md workflow that imports the shared workflow + mainContent := `--- +name: Main Workflow +on: issue_comment +imports: + - shared/target.md +--- + +# Main Workflow +` + result, err := ExtractFrontmatterFromContent(mainContent) + require.NoError(t, err, "ExtractFrontmatterFromContent should succeed") + + importsResult, err := ProcessImportsFromFrontmatterWithSource(result.Frontmatter, tmpDir, nil, "", "") + require.NoError(t, err, "ProcessImportsFromFrontmatterWithSource should succeed") + + assert.NotEmpty(t, importsResult.MergedEnv, "MergedEnv should be populated from shared .md import") + assert.Contains(t, importsResult.MergedEnv, "TARGET_REPOSITORY", "MergedEnv should contain TARGET_REPOSITORY") + assert.Contains(t, importsResult.MergedEnv, "owner/repo", "MergedEnv should contain the repository value") + assert.Contains(t, importsResult.MergedEnv, "SHARED_VAR", "MergedEnv should contain SHARED_VAR") + assert.Equal(t, "shared/target.md", importsResult.MergedEnvSources["TARGET_REPOSITORY"], "MergedEnvSources should track the import path for TARGET_REPOSITORY") + assert.Equal(t, "shared/target.md", importsResult.MergedEnvSources["SHARED_VAR"], "MergedEnvSources should track the import path for SHARED_VAR") +} + +// TestEnvFieldConflictBetweenImports verifies that defining the same env var in two different +// imports produces a compilation error. +func TestEnvFieldConflictBetweenImports(t *testing.T) { + tmpDir := t.TempDir() + + sharedDir := filepath.Join(tmpDir, "shared") + require.NoError(t, os.MkdirAll(sharedDir, 0755), "Failed to create shared dir") + + // First import defines SHARED_KEY + require.NoError(t, os.WriteFile(filepath.Join(sharedDir, "first.md"), []byte(`--- +env: + SHARED_KEY: value-from-first +--- + +# First shared workflow +`), 0644)) + + // Second import also defines SHARED_KEY (conflict) + require.NoError(t, os.WriteFile(filepath.Join(sharedDir, "second.md"), []byte(`--- +env: + SHARED_KEY: value-from-second +--- + +# Second shared workflow +`), 0644)) + + mainContent := `--- +name: Main Workflow +on: issue_comment +imports: + - shared/first.md + - shared/second.md +--- + +# Main Workflow +` + result, err := ExtractFrontmatterFromContent(mainContent) + require.NoError(t, err, "ExtractFrontmatterFromContent should succeed") + + _, err = ProcessImportsFromFrontmatterWithSource(result.Frontmatter, tmpDir, nil, "", "") + require.Error(t, err, "Should error when two imports define the same env var") + assert.Contains(t, err.Error(), "SHARED_KEY", "Error should mention the conflicting variable name") +} + // TestExtractAllImportFields_BuiltinCacheHit verifies that extractAllImportFields uses the // process-level builtin frontmatter cache for builtin files without inputs. func TestExtractAllImportFields_BuiltinCacheHit(t *testing.T) { diff --git a/pkg/parser/import_processor.go b/pkg/parser/import_processor.go index 31bb85a6f98..d7468270616 100644 --- a/pkg/parser/import_processor.go +++ b/pkg/parser/import_processor.go @@ -14,38 +14,40 @@ var importLog = logger.New("parser:import_processor") // ImportsResult holds the result of processing imports from frontmatter type ImportsResult struct { - MergedTools string // Merged tools configuration from all imports - MergedMCPServers string // Merged mcp-servers configuration from all imports - MergedEngines []string // Merged engine configurations from all imports - MergedSafeOutputs []string // Merged safe-outputs configurations from all imports - MergedMCPScripts []string // Merged mcp-scripts configurations from all imports - MergedMarkdown string // Only contains imports WITH inputs (for compile-time substitution) - ImportPaths []string // List of import file paths for runtime-import macro generation (replaces MergedMarkdown) - MergedSteps string // Merged steps configuration from all imports (excluding copilot-setup-steps) - CopilotSetupSteps string // Steps from copilot-setup-steps.yml (inserted at start) - MergedPreSteps string // Merged pre-steps configuration from all imports (prepended in order) - MergedRuntimes string // Merged runtimes configuration from all imports - MergedRunInstallScripts bool // true if any imported workflow sets run-install-scripts: true (global or node-level) - MergedServices string // Merged services configuration from all imports - MergedNetwork string // Merged network configuration from all imports - MergedPermissions string // Merged permissions configuration from all imports - MergedSecretMasking string // Merged secret-masking steps from all imports - MergedBots []string // Merged bots list from all imports (union of bot names) - MergedSkipRoles []string // Merged skip-roles list from all imports (union of role names) - MergedSkipBots []string // Merged skip-bots list from all imports (union of usernames) - MergedActivationGitHubToken string // GitHub token from on.github-token in first imported workflow that defines it - MergedActivationGitHubApp string // JSON-encoded on.github-app from first imported workflow that defines it - MergedTopLevelGitHubApp string // JSON-encoded top-level github-app from first imported workflow that defines it - MergedPostSteps string // Merged post-steps configuration from all imports (appended in order) - MergedLabels []string // Merged labels from all imports (union of label names) - MergedCaches []string // Merged cache configurations from all imports (appended in order) - MergedJobs string // Merged jobs from imported YAML workflows (JSON format) - MergedFeatures []map[string]any // Merged features configuration from all imports (parsed YAML structures) - MergedObservability string // Observability config (JSON) from first import that defines it (first-wins) - ImportedFiles []string // List of imported file paths (for manifest) - AgentFile string // Path to custom agent file (if imported) - AgentImportSpec string // Original import specification for agent file (e.g., "owner/repo/path@ref") - RepositoryImports []string // List of repository imports (format: "owner/repo@ref") for .github folder merging + MergedTools string // Merged tools configuration from all imports + MergedMCPServers string // Merged mcp-servers configuration from all imports + MergedEngines []string // Merged engine configurations from all imports + MergedSafeOutputs []string // Merged safe-outputs configurations from all imports + MergedMCPScripts []string // Merged mcp-scripts configurations from all imports + MergedMarkdown string // Only contains imports WITH inputs (for compile-time substitution) + ImportPaths []string // List of import file paths for runtime-import macro generation (replaces MergedMarkdown) + MergedSteps string // Merged steps configuration from all imports (excluding copilot-setup-steps) + CopilotSetupSteps string // Steps from copilot-setup-steps.yml (inserted at start) + MergedPreSteps string // Merged pre-steps configuration from all imports (prepended in order) + MergedRuntimes string // Merged runtimes configuration from all imports + MergedRunInstallScripts bool // true if any imported workflow sets run-install-scripts: true (global or node-level) + MergedServices string // Merged services configuration from all imports + MergedNetwork string // Merged network configuration from all imports + MergedPermissions string // Merged permissions configuration from all imports + MergedSecretMasking string // Merged secret-masking steps from all imports + MergedBots []string // Merged bots list from all imports (union of bot names) + MergedSkipRoles []string // Merged skip-roles list from all imports (union of role names) + MergedSkipBots []string // Merged skip-bots list from all imports (union of usernames) + MergedActivationGitHubToken string // GitHub token from on.github-token in first imported workflow that defines it + MergedActivationGitHubApp string // JSON-encoded on.github-app from first imported workflow that defines it + MergedTopLevelGitHubApp string // JSON-encoded top-level github-app from first imported workflow that defines it + MergedPostSteps string // Merged post-steps configuration from all imports (appended in order) + MergedLabels []string // Merged labels from all imports (union of label names) + MergedCaches []string // Merged cache configurations from all imports (appended in order) + MergedJobs string // Merged jobs from imported YAML workflows (JSON format) + MergedEnv string // Merged env configuration from all imports (JSON format) + MergedEnvSources map[string]string // env var name → source import path (for conflict detection and lock file header listing) + MergedFeatures []map[string]any // Merged features configuration from all imports (parsed YAML structures) + MergedObservability string // Observability config (JSON) from first import that defines it (first-wins) + ImportedFiles []string // List of imported file paths (for manifest) + AgentFile string // Path to custom agent file (if imported) + AgentImportSpec string // Original import specification for agent file (e.g., "owner/repo/path@ref") + RepositoryImports []string // List of repository imports (format: "owner/repo@ref") for .github folder merging // ImportInputs uses map[string]any because input values can be different types (string, number, boolean). // This is parsed from YAML frontmatter where the structure is dynamic and not known at compile time. // This is an appropriate use of 'any' for dynamic YAML/JSON data. diff --git a/pkg/parser/include_processor.go b/pkg/parser/include_processor.go index 5aeba9c7e8f..62bec99d5ae 100644 --- a/pkg/parser/include_processor.go +++ b/pkg/parser/include_processor.go @@ -158,6 +158,7 @@ func processIncludedFileWithVisited(filePath, sectionName string, extractTools b validFields := map[string]bool{ "tools": true, "engine": true, + "env": true, "network": true, "mcp-servers": true, "imports": true, diff --git a/pkg/workflow/compiler_orchestrator_workflow.go b/pkg/workflow/compiler_orchestrator_workflow.go index f510cc0a733..8d8c1e23084 100644 --- a/pkg/workflow/compiler_orchestrator_workflow.go +++ b/pkg/workflow/compiler_orchestrator_workflow.go @@ -142,6 +142,35 @@ func (c *Compiler) ParseWorkflowFile(markdownPath string) (*WorkflowData, error) } } + // Merge env from imports (main workflow env vars take precedence over imported env vars) + if engineSetup.importsResult.MergedEnv != "" { + topEnv := ExtractMapField(result.Frontmatter, "env") + mergedEnvMap, err := mergeEnv(topEnv, engineSetup.importsResult.MergedEnv) + if err != nil { + return nil, fmt.Errorf("failed to merge env from imports: %w", err) + } + if len(mergedEnvMap) > 0 { + workflowData.Env = c.extractTopLevelYAMLSection(map[string]any{"env": mergedEnvMap}, "env") + // Build source attribution: imported vars get the import path; main-workflow vars are labelled accordingly + envSources := make(map[string]string, len(mergedEnvMap)) + for key := range mergedEnvMap { + if _, inTop := topEnv[key]; inTop { + envSources[key] = "(main workflow)" + } else if src, ok := engineSetup.importsResult.MergedEnvSources[key]; ok { + envSources[key] = src + } + } + workflowData.EnvSources = envSources + } + } else if topEnv := ExtractMapField(result.Frontmatter, "env"); len(topEnv) > 0 { + // No imports provided env — still label main workflow vars so the header can show them + envSources := make(map[string]string, len(topEnv)) + for key := range topEnv { + envSources[key] = "(main workflow)" + } + workflowData.EnvSources = envSources + } + // Inject OTLP configuration: add endpoint domain to firewall allowlist and // set OTEL env vars in the workflow env block (no-op when not configured). c.injectOTLPConfig(workflowData) diff --git a/pkg/workflow/compiler_types.go b/pkg/workflow/compiler_types.go index fa86b8942da..d34c895058b 100644 --- a/pkg/workflow/compiler_types.go +++ b/pkg/workflow/compiler_types.go @@ -404,6 +404,7 @@ type WorkflowData struct { Concurrency string // workflow-level concurrency configuration RunName string Env string + EnvSources map[string]string // env var name → source ("(main workflow)" or import file path) for lock file header If string TimeoutMinutes string CustomSteps string diff --git a/pkg/workflow/compiler_yaml.go b/pkg/workflow/compiler_yaml.go index 2e818dde561..7eda7b66568 100644 --- a/pkg/workflow/compiler_yaml.go +++ b/pkg/workflow/compiler_yaml.go @@ -193,6 +193,22 @@ func (c *Compiler) generateWorkflowHeader(yaml *strings.Builder, data *WorkflowD yaml.WriteString("# inlined-imports: true\n") } + // Add frontmatter-declared env vars with source attribution. + // Note: programmatically injected env vars (e.g. OTEL_* from OTLP config) are not listed here. + if len(data.EnvSources) > 0 { + yaml.WriteString("#\n") + yaml.WriteString("# Frontmatter env variables:\n") + // Sort keys for deterministic output + keys := make([]string, 0, len(data.EnvSources)) + for k := range data.EnvSources { + keys = append(keys, k) + } + sort.Strings(keys) + for _, k := range keys { + fmt.Fprintf(yaml, "# - %s: %s\n", k, data.EnvSources[k]) + } + } + // Add list of secrets referenced in the workflow if len(secrets) > 0 { yaml.WriteString("#\n") diff --git a/pkg/workflow/compiler_yaml_helpers_test.go b/pkg/workflow/compiler_yaml_helpers_test.go index c2b8c4a9c11..3982d854eaf 100644 --- a/pkg/workflow/compiler_yaml_helpers_test.go +++ b/pkg/workflow/compiler_yaml_helpers_test.go @@ -224,6 +224,20 @@ func TestGenerateWorkflowHeader(t *testing.T) { "# Manual approval required: environment 'production'", }, }, + { + name: "header with env sources from main and import", + data: &WorkflowData{ + EnvSources: map[string]string{ + "MAIN_VAR": "(main workflow)", + "IMPORTED_VAR": ".github/shared/target.md", + }, + }, + expectInStr: []string{ + "# Frontmatter env variables:", + "# - IMPORTED_VAR: .github/shared/target.md", + "# - MAIN_VAR: (main workflow)", + }, + }, { name: "minimal header", data: &WorkflowData{}, diff --git a/pkg/workflow/imports.go b/pkg/workflow/imports.go index 29dd7e8ab2c..d7b34ab5b42 100644 --- a/pkg/workflow/imports.go +++ b/pkg/workflow/imports.go @@ -692,3 +692,36 @@ func (c *Compiler) MergeFeatures(topFeatures map[string]any, importedFeatures [] importsLog.Printf("Successfully merged features: total=%d", len(result)) return result, nil } + +// mergeEnv merges env var configurations from imports with top-level env vars. +// Top-level env vars take precedence over imported env vars. +// Conflicts between imports (same key in two different imported files) are detected +// earlier in the importAccumulator and fail compilation before mergeEnv is called. +func mergeEnv(topEnv map[string]any, importedEnvJSON string) (map[string]any, error) { + importsLog.Printf("Merging env: topEnv=%d", len(topEnv)) + result := make(map[string]any) + + // Merge imported env vars first (newline-separated JSON objects, each from a distinct import) + if importedEnvJSON != "" { + lines := strings.SplitSeq(strings.TrimSpace(importedEnvJSON), "\n") + for line := range lines { + line = strings.TrimSpace(line) + if line == "" || line == "{}" { + continue + } + + var importedEnv map[string]any + if err := json.Unmarshal([]byte(line), &importedEnv); err != nil { + return nil, fmt.Errorf("failed to parse imported env JSON: %w", err) + } + + maps.Copy(result, importedEnv) + } + } + + // Top-level env vars take precedence: copy last so they override any imported values + maps.Copy(result, topEnv) + + importsLog.Printf("Merged %d total env vars", len(result)) + return result, nil +} diff --git a/pkg/workflow/imports_env_test.go b/pkg/workflow/imports_env_test.go new file mode 100644 index 00000000000..f4f16673df2 --- /dev/null +++ b/pkg/workflow/imports_env_test.go @@ -0,0 +1,81 @@ +//go:build !integration + +package workflow + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestMergeEnvWithNoImports(t *testing.T) { + topEnv := map[string]any{ + "KEY1": "value1", + "KEY2": "value2", + } + + result, err := mergeEnv(topEnv, "") + require.NoError(t, err, "mergeEnv should not error with empty imports") + assert.Equal(t, topEnv, result, "Should return top-level env unchanged when no imports") +} + +func TestMergeEnvWithImportedEnvVars(t *testing.T) { + topEnv := map[string]any{} + + importedJSON := `{"TARGET_REPOSITORY":"owner/repo","SHARED_VAR":"shared-value"}` + + result, err := mergeEnv(topEnv, importedJSON) + require.NoError(t, err, "mergeEnv should not error with valid imports") + assert.Equal(t, "owner/repo", result["TARGET_REPOSITORY"], "Should contain imported TARGET_REPOSITORY") + assert.Equal(t, "shared-value", result["SHARED_VAR"], "Should contain imported SHARED_VAR") +} + +func TestMergeEnvTopLevelTakesPrecedence(t *testing.T) { + topEnv := map[string]any{ + "SHARED_KEY": "main-value", + "MAIN_ONLY": "main", + } + + importedJSON := `{"SHARED_KEY":"import-value","IMPORT_ONLY":"imported"}` + + result, err := mergeEnv(topEnv, importedJSON) + require.NoError(t, err, "mergeEnv should not error") + assert.Equal(t, "main-value", result["SHARED_KEY"], "Main workflow env var should override imported") + assert.Equal(t, "main", result["MAIN_ONLY"], "Main-only var should be present") + assert.Equal(t, "imported", result["IMPORT_ONLY"], "Import-only var should be merged in") +} + +func TestMergeEnvWithMultipleImports(t *testing.T) { + topEnv := map[string]any{} + + // Two imports with distinct keys — import-to-import conflicts are caught earlier in the + // importAccumulator before mergeEnv is called (see TestEnvFieldConflictBetweenImports in + // pkg/parser/import_field_extractor_test.go). The JSON passed to mergeEnv is therefore + // guaranteed to have no duplicate keys across different import lines. + importedJSON := `{"KEY1":"val1","KEY2":"val2"} +{"KEY3":"val3","KEY4":"val4"}` + + result, err := mergeEnv(topEnv, importedJSON) + require.NoError(t, err, "mergeEnv should not error with multiple import lines") + assert.Equal(t, "val1", result["KEY1"], "KEY1 from first import should be present") + assert.Equal(t, "val2", result["KEY2"], "KEY2 from first import should be present") + assert.Equal(t, "val3", result["KEY3"], "KEY3 from second import should be present") + assert.Equal(t, "val4", result["KEY4"], "KEY4 from second import should be present") +} + +func TestMergeEnvWithNilTopLevel(t *testing.T) { + importedJSON := `{"IMPORTED_VAR":"imported-value"}` + + result, err := mergeEnv(nil, importedJSON) + require.NoError(t, err, "mergeEnv should not error with nil top-level") + assert.Equal(t, "imported-value", result["IMPORTED_VAR"], "Imported var should be present") +} + +func TestMergeEnvWithInvalidJSON(t *testing.T) { + topEnv := map[string]any{"KEY": "value"} + + _, err := mergeEnv(topEnv, `{invalid json}`) + require.Error(t, err, "mergeEnv should return an error for invalid JSON") + assert.Contains(t, err.Error(), "failed to parse imported env JSON", "Error message should be descriptive") +}