diff --git a/docs/adr/29212-expression-inputs-for-list-constraints.md b/docs/adr/29212-expression-inputs-for-list-constraints.md new file mode 100644 index 00000000000..36a6ef56b64 --- /dev/null +++ b/docs/adr/29212-expression-inputs-for-list-constraints.md @@ -0,0 +1,79 @@ +# ADR-29212: Allow GitHub Actions Expression Inputs for List-Valued Safe-Output Constraints + +**Date**: 2026-04-30 +**Status**: Draft +**Deciders**: pelikhan, Copilot + +--- + +## Part 1 — Narrative (Human-Friendly) + +### Context + +Safe-output fields such as `labels`, `allowed-repos`, and `allowed-base-branches` define list-valued runtime constraints on safe-output actions (`push-to-pull-request-branch`, `create-pull-request`, `add-comment`). These fields previously required literal YAML arrays, which worked well for single-repository workflows but made reusable `workflow_call` workflows impossible to parameterize: callers cannot inject list values at runtime because the parser only accepted static YAML sequences. The demand for reusable workflows that delegate constraint configuration to their callers surfaces this limitation as a hard blocker. + +### Decision + +We will extend the six affected list-valued safe-output fields to accept either a literal YAML array (existing behavior, unchanged) or a single GitHub Actions expression string (e.g., `${{ inputs.required-labels }}`). At build time the compiler wraps a detected expression in a single-element `[]string` slice so the existing struct fields survive YAML unmarshal; it then emits the expression as a raw JSON string into the generated config so that GitHub Actions evaluates it inside the heredoc before `config.json` is written. Non-expression bare strings are rejected with an actionable error. No changes are required to the JavaScript handlers because they already accept comma-separated strings. + +### Alternatives Considered + +#### Alternative 1: Keep Requiring Literal Arrays (Status Quo) + +Literal arrays remain the only accepted value form. This was rejected because it forces workflow authors to hardcode list constraints, making reusable `workflow_call` workflows impossible to parameterize — the direct trigger for this PR. + +#### Alternative 2: Add New Expression-Specific Sibling Fields + +Introduce new fields (e.g., `labels-expr`, `allowed-repos-expr`) that accept expression strings, leaving the existing array fields untouched. This was rejected because it doubles the number of schema fields, creates confusing dual-field semantics, and requires callers to learn a new naming convention for every parameterizable list field. + +#### Alternative 3: Resolve Expressions at Compile Time + +Attempt to evaluate `workflow_call` input defaults at compile time to produce a concrete array. This was rejected because `workflow_call` inputs are only known at runtime when the calling workflow passes arguments; compile-time resolution is not feasible for this class of values. + +### Consequences + +#### Positive +- Reusable `workflow_call` workflows can now parameterize any of the six list-valued constraint fields, unblocking a common workflow composition pattern. +- Literal YAML array behavior is fully preserved; no migration is required for existing workflows. +- JavaScript runtime handlers require no changes because they already parse comma-separated strings. + +#### Negative +- JSON schema for the six fields changes from `type: "array"` to `oneOf: [{type: "array"}, {type: "string"}]`, reducing compile-time strictness: a mis-typed bare string now produces a runtime error rather than a parse error. +- Expression correctness (e.g., whether the referenced input exists) is validated only at runtime, not at compile time. +- The new `preprocessStringArrayFieldAsTemplatable` helper adds a pre-pass over `configData` that must be kept in sync whenever new list-valued fields are added. + +#### Neutral +- The PR number is used as the ADR number (`29212`), consistent with the project's ADR numbering convention. +- Six fields are affected across three parsers: `labels` and `allowed-repos` in `push_to_pull_request_branch.go`, `labels`, `allowed-repos`, and `allowed-base-branches` in `create_pull_request.go`, and `allowed-repos` in `add_comment.go`. + +--- + +## 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). + +### Expression Detection and Preprocessing + +1. Implementations **MUST** accept either a YAML sequence (literal array) or a single GitHub Actions expression string of the form `${{ ... }}` as the value of any list-valued safe-output constraint field (`labels`, `allowed-repos`, `allowed-base-branches`). +2. Implementations **MUST NOT** accept a bare string value that is not a valid GitHub Actions expression (i.e., does not match `^\$\{\{.*\}\}$`); such values **MUST** produce a descriptive parse error. +3. Implementations **MUST** detect expression strings in `configData` before YAML unmarshal and wrap them in a single-element `[]string` slice so that existing `[]string` struct fields survive unmarshalling unchanged. +4. Implementations **MUST NOT** attempt to evaluate or validate the content of an expression at compile time. + +### Compiler Emission + +1. Implementations **MUST** emit a detected single-element expression slice as a raw JSON string (not a JSON array) in the generated config heredoc, so that GitHub Actions evaluates the expression before `config.json` is written. +2. Implementations **MUST** emit a literal multi-element slice or a slice containing no expression as a JSON array. +3. Implementations **SHOULD** use `AddTemplatableStringSlice` (or an equivalent abstraction) for all six affected fields when constructing the handler config, rather than calling `AddStringSlice` directly. + +### JSON Schema + +1. The JSON Schema entry for each of the six affected fields **MUST** be changed from `type: "array"` to `oneOf: [{type: "array"}, {type: "string"}]` to accept both literal arrays and expression strings. +2. The array variant within `oneOf` **MUST** retain the same `items` sub-schema that was present before this change. + +### Conformance + +An implementation is considered conformant with this ADR if it satisfies all **MUST** and **MUST NOT** requirements above. 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/25140707660) workflow. The PR author must review, complete, and finalize this document before the PR can merge.* diff --git a/pkg/parser/schema_test.go b/pkg/parser/schema_test.go index b8bb7c330bf..b4397a0fbfb 100644 --- a/pkg/parser/schema_test.go +++ b/pkg/parser/schema_test.go @@ -500,11 +500,33 @@ func TestMainWorkflowSchema_CreatePullRequestAllowedBaseBranches(t *testing.T) { t.Fatal("'allowed-base-branches' not found under safe-outputs.create-pull-request") } - if gotType, _ := allowedBaseBranches["type"].(string); gotType != "array" { - t.Fatalf("'allowed-base-branches' should be type array, got: %v", allowedBaseBranches["type"]) + // The field accepts either a literal array or an expression string (oneOf). + // Validate that the array variant is present and has the right structure. + var arraySchema map[string]any + if oneOf, hasOneOf := allowedBaseBranches["oneOf"].([]any); hasOneOf { + // New schema: oneOf[array, string-expression] + for _, candidate := range oneOf { + candidateMap, ok := candidate.(map[string]any) + if !ok { + continue + } + if t2, _ := candidateMap["type"].(string); t2 == "array" { + arraySchema = candidateMap + break + } + } + if arraySchema == nil { + t.Fatal("'allowed-base-branches' oneOf does not include an array variant") + } + } else { + // Legacy schema: direct type:array + if gotType, _ := allowedBaseBranches["type"].(string); gotType != "array" { + t.Fatalf("'allowed-base-branches' should be type array (or oneOf with array), got: %v", allowedBaseBranches["type"]) + } + arraySchema = allowedBaseBranches } - items, ok := allowedBaseBranches["items"].(map[string]any) + items, ok := arraySchema["items"].(map[string]any) if !ok { t.Fatal("'allowed-base-branches.items' not found in schema") } diff --git a/pkg/parser/schemas/main_workflow_schema.json b/pkg/parser/schemas/main_workflow_schema.json index 5e05504750d..aa0eb4db4ae 100644 --- a/pkg/parser/schemas/main_workflow_schema.json +++ b/pkg/parser/schemas/main_workflow_schema.json @@ -553,7 +553,7 @@ { "type": "array", "minItems": 1, - "description": "Array of label names \u2014 any of these labels will trigger the workflow.", + "description": "Array of label names — any of these labels will trigger the workflow.", "items": { "type": "string", "minLength": 1, @@ -574,7 +574,7 @@ { "type": "array", "minItems": 1, - "description": "Array of label names \u2014 any of these labels will trigger the workflow.", + "description": "Array of label names — any of these labels will trigger the workflow.", "items": { "type": "string", "minLength": 1, @@ -1725,7 +1725,7 @@ "oneOf": [ { "type": "null", - "description": "Bare key with no value \u2014 equivalent to true. Skips workflow execution if any CI checks on the target branch are currently failing." + "description": "Bare key with no value — equivalent to true. Skips workflow execution if any CI checks on the target branch are currently failing." }, { "type": "boolean", @@ -1799,12 +1799,12 @@ "description": "Skip workflow execution for specific GitHub users. Useful for preventing workflows from running for specific accounts (e.g., bots, specific team members)." }, "roles": { - "description": "Repository access roles required to trigger agentic workflows. Defaults to ['admin', 'maintainer', 'write'] for security. Use 'all' to allow any authenticated user (\u26a0\ufe0f security consideration).", + "description": "Repository access roles required to trigger agentic workflows. Defaults to ['admin', 'maintainer', 'write'] for security. Use 'all' to allow any authenticated user (⚠️ security consideration).", "oneOf": [ { "type": "string", "enum": ["admin", "maintainer", "maintain", "write", "triage", "read", "all"], - "description": "Single repository permission level that can trigger the workflow. Use 'all' to allow any authenticated user (\u26a0\ufe0f disables permission checking entirely - use with caution)" + "description": "Single repository permission level that can trigger the workflow. Use 'all' to allow any authenticated user (⚠️ disables permission checking entirely - use with caution)" }, { "type": "array", @@ -1829,7 +1829,7 @@ } }, "labels": { - "description": "Filter workflows triggered by pull_request_target (or other labeled events) to only fire when the triggering label matches one of these names. Generates a job-level if: condition on the pre-activation job so unmatched label events show as Skipped (\u2298) rather than Failed (\u274c).", + "description": "Filter workflows triggered by pull_request_target (or other labeled events) to only fire when the triggering label matches one of these names. Generates a job-level if: condition on the pre-activation job so unmatched label events show as Skipped (⊘) rather than Failed (❌).", "oneOf": [ { "$ref": "#/$defs/non_empty_string", @@ -1896,7 +1896,22 @@ ], "default": "eyes", "description": "AI reaction to add/remove on triggering item. Scalar form accepts one of: +1, -1, laugh, confused, heart, hooray, rocket, eyes, none. Object form implies enabled reactions and supports optional `issues`, `pull-requests`, and `discussions` fields to control trigger groups independently; use `type` to choose the reaction emoji (defaults to `eyes` when omitted). Use 'none' to disable reactions.", - "examples": ["eyes", "rocket", "+1", 1, -1, "none", { "type": "rocket", "pull-requests": false }, { "issues": false, "discussions": true }] + "examples": [ + "eyes", + "rocket", + "+1", + 1, + -1, + "none", + { + "type": "rocket", + "pull-requests": false + }, + { + "issues": false, + "discussions": true + } + ] }, "status-comment": { "oneOf": [ @@ -1923,7 +1938,24 @@ } ], "description": "Whether to post status comments (started/completed) on the triggering item. Boolean form enables/disables status comments globally. Object form implies enabled status comments and supports optional `issues`, `pull-requests`, and `discussions` fields to control trigger groups independently. Automatically enabled for slash_command and label_command triggers when not explicitly configured.", - "examples": [true, false, { "issues": false }, { "pull-requests": false }, { "discussions": false }, { "issues": false, "pull-requests": true, "discussions": true }] + "examples": [ + true, + false, + { + "issues": false + }, + { + "pull-requests": false + }, + { + "discussions": false + }, + { + "issues": false, + "pull-requests": true, + "discussions": true + } + ] }, "github-token": { "type": "string", @@ -2918,7 +2950,7 @@ }, "network": { "$comment": "Strict mode requirements: When strict=true, the 'network' field must be present (not null/undefined) and cannot contain standalone wildcard '*' in allowed domains (but patterns like '*.example.com' ARE allowed). This is validated in Go code (pkg/workflow/strict_mode_validation.go) via validateStrictNetwork().", - "description": "Network access control for AI engines using ecosystem identifiers and domain allowlists. Supports wildcard patterns like '*.example.com' to match any subdomain. Controls web fetch and search capabilities. IMPORTANT: For workflows that build/install/test code, always include the language ecosystem identifier alongside 'defaults' \u2014 'defaults' alone only covers basic infrastructure, not package registries. Key ecosystem identifiers by runtime: 'dotnet' (.NET/NuGet), 'python' (pip/PyPI), 'node' (npm/yarn), 'go' (go modules), 'java' (Maven/Gradle), 'ruby' (Bundler), 'rust' (Cargo), 'swift' (Swift PM). Example: a .NET project needs network: { allowed: [defaults, dotnet] }.", + "description": "Network access control for AI engines using ecosystem identifiers and domain allowlists. Supports wildcard patterns like '*.example.com' to match any subdomain. Controls web fetch and search capabilities. IMPORTANT: For workflows that build/install/test code, always include the language ecosystem identifier alongside 'defaults' — 'defaults' alone only covers basic infrastructure, not package registries. Key ecosystem identifiers by runtime: 'dotnet' (.NET/NuGet), 'python' (pip/PyPI), 'node' (npm/yarn), 'go' (go modules), 'java' (Maven/Gradle), 'ruby' (Bundler), 'rust' (Cargo), 'swift' (Swift PM). Example: a .NET project needs network: { allowed: [defaults, dotnet] }.", "examples": [ "defaults", { @@ -3400,7 +3432,7 @@ [ { "name": "Verify Post-Steps Execution", - "run": "echo \"\u2705 Post-steps are executing correctly\"\necho \"This step runs after the AI agent completes\"\n" + "run": "echo \"✅ Post-steps are executing correctly\"\necho \"This step runs after the AI agent completes\"\n" }, { "name": "Upload Test Results", @@ -5663,11 +5695,21 @@ "description": "Target repository in format 'owner/repo' for cross-repository comments. Takes precedence over trial target repo settings." }, "allowed-repos": { - "type": "array", - "items": { - "type": "string" - }, - "description": "List of additional repositories in format 'owner/repo' that comments can be created in. When specified, the agent can use a 'repo' field in the output to specify which repository to create the comment in. The target repository (current or target-repo) is always implicitly allowed." + "description": "List of additional repositories in format 'owner/repo' that comments can be created in. When specified, the agent can use a 'repo' field in the output to specify which repository to create the comment in. The target repository (current or target-repo) is always implicitly allowed. Accepts an array or a GitHub Actions expression resolving to a comma-separated list (e.g. '${{ inputs[\\'allowed-repos\\'] }}').", + "oneOf": [ + { + "type": "array", + "description": "Array of repository slugs in 'owner/repo' format", + "items": { + "type": "string" + } + }, + { + "type": "string", + "pattern": "^\\$\\{\\{.*\\}\\}$", + "description": "GitHub Actions expression resolving to a comma-separated list of repository slugs (e.g. '${{ inputs[\\'allowed-repos\\'] }}')" + } + ] }, "hide-older-comments": { "type": "boolean", @@ -5753,11 +5795,21 @@ "description": "Optional prefix for the pull request title" }, "labels": { - "type": "array", - "description": "Optional list of labels to attach to the pull request", - "items": { - "type": "string" - } + "description": "Optional list of labels to attach to the pull request. Accepts an array of label names or a GitHub Actions expression resolving to a comma-separated list (e.g. '${{ inputs.labels }}').", + "oneOf": [ + { + "type": "array", + "description": "Array of label names", + "items": { + "type": "string" + } + }, + { + "type": "string", + "pattern": "^\\$\\{\\{.*\\}\\}$", + "description": "GitHub Actions expression resolving to a comma-separated list of label names (e.g. '${{ inputs.labels }}')" + } + ] }, "allowed-labels": { "type": "array", @@ -5844,11 +5896,21 @@ "description": "Target repository in format 'owner/repo' for cross-repository pull request creation. Takes precedence over trial target repo settings." }, "allowed-repos": { - "type": "array", - "items": { - "type": "string" - }, - "description": "List of additional repositories in format 'owner/repo' that pull requests can be created in. When specified, the agent can use a 'repo' field in the output to specify which repository to create the pull request in. The target repository (current or target-repo) is always implicitly allowed." + "description": "List of additional repositories in format 'owner/repo' that pull requests can be created in. When specified, the agent can use a 'repo' field in the output to specify which repository to create the pull request in. The target repository (current or target-repo) is always implicitly allowed. Accepts an array or a GitHub Actions expression resolving to a comma-separated list (e.g. '${{ inputs[\\'allowed-repos\\'] }}').", + "oneOf": [ + { + "type": "array", + "description": "Array of repository slugs in 'owner/repo' format", + "items": { + "type": "string" + } + }, + { + "type": "string", + "pattern": "^\\$\\{\\{.*\\}\\}$", + "description": "GitHub Actions expression resolving to a comma-separated list of repository slugs (e.g. '${{ inputs[\\'allowed-repos\\'] }}')" + } + ] }, "github-token": { "$ref": "#/$defs/github_token", @@ -5879,11 +5941,21 @@ "description": "Base branch for the pull request. Defaults to the workflow's branch (github.ref_name) if not specified. Useful for cross-repository PRs targeting non-default branches (e.g., 'vnext', 'release/v1.0')." }, "allowed-base-branches": { - "type": "array", - "items": { - "type": "string" - }, - "description": "Optional list of allowed base branch patterns (glob syntax, e.g. 'main', 'release/*'). When configured, the agent may provide a `base` field in create_pull_request output to override base-branch for a single run, but only if it matches one of these patterns." + "description": "Optional list of allowed base branch patterns (glob syntax, e.g. 'main', 'release/*'). When configured, the agent may provide a `base` field in create_pull_request output to override base-branch for a single run, but only if it matches one of these patterns. Accepts an array or a GitHub Actions expression resolving to a comma-separated list (e.g. '${{ inputs[\\'allowed-base-branches\\'] }}').", + "oneOf": [ + { + "type": "array", + "description": "Array of base branch patterns (glob syntax supported)", + "items": { + "type": "string" + } + }, + { + "type": "string", + "pattern": "^\\$\\{\\{.*\\}\\}$", + "description": "GitHub Actions expression resolving to a comma-separated list of base branch patterns (e.g. '${{ inputs[\\'allowed-base-branches\\'] }}')" + } + ] }, "footer": { "type": "boolean", @@ -5927,7 +5999,9 @@ }, "exclude": { "type": "array", - "items": { "type": "string" }, + "items": { + "type": "string" + }, "description": "List of filenames or path prefixes to remove from the default protected-file set. Items are matched by basename (e.g. \"AGENTS.md\") or path prefix (e.g. \".agents/\"). Use this to allow the agent to modify specific files that are otherwise blocked by default.", "examples": [["AGENTS.md"], ["AGENTS.md", ".agents/"]] } @@ -5943,7 +6017,7 @@ "items": { "type": "string" }, - "description": "Exclusive allowlist of glob patterns. When set, every file in the patch must match at least one pattern \u2014 files outside the list are always refused, including normal source files. This is a restriction, not an exception: setting allowed-files: [\".github/workflows/*\"] blocks all other files. To allow multiple sets of files, list all patterns explicitly. Acts independently of the protected-files policy; both checks must pass. To modify a protected file, it must both match allowed-files and be permitted by protected-files (e.g. protected-files: allowed). Supports * (any characters except /) and ** (any characters including /)." + "description": "Exclusive allowlist of glob patterns. When set, every file in the patch must match at least one pattern — files outside the list are always refused, including normal source files. This is a restriction, not an exception: setting allowed-files: [\".github/workflows/*\"] blocks all other files. To allow multiple sets of files, list all patterns explicitly. Acts independently of the protected-files policy; both checks must pass. To modify a protected file, it must both match allowed-files and be permitted by protected-files (e.g. protected-files: allowed). Supports * (any characters except /) and ** (any characters including /)." }, "preserve-branch-name": { "type": "boolean", @@ -6208,7 +6282,7 @@ "oneOf": [ { "type": "object", - "description": "Configuration for resolving review threads on pull requests. Resolution is scoped to the triggering PR only \u2014 threads on other PRs cannot be resolved.", + "description": "Configuration for resolving review threads on pull requests. Resolution is scoped to the triggering PR only — threads on other PRs cannot be resolved.", "properties": { "max": { "description": "Maximum number of review threads to resolve (default: 10) Supports integer or GitHub Actions expression (e.g. '${{ inputs.max }}').", @@ -7154,11 +7228,21 @@ "description": "Required prefix for pull request title. Only pull requests with this prefix will be accepted." }, "labels": { - "type": "array", - "description": "Required labels for pull request validation. Only pull requests with all these labels will be accepted.", - "items": { - "type": "string" - } + "description": "Required labels for pull request validation. Only pull requests with all these labels will be accepted. Accepts an array of label names or a GitHub Actions expression resolving to a comma-separated list of labels (e.g. '${{ inputs[\\'required-labels\\'] }}').", + "oneOf": [ + { + "type": "array", + "description": "Array of label names", + "items": { + "type": "string" + } + }, + { + "type": "string", + "pattern": "^\\$\\{\\{.*\\}\\}$", + "description": "GitHub Actions expression resolving to a comma-separated list of label names (e.g. '${{ inputs[\\'required-labels\\'] }}')" + } + ] }, "if-no-changes": { "type": "string", @@ -7197,11 +7281,21 @@ "description": "Target repository in format 'owner/repo' for cross-repository push to pull request branch. Takes precedence over trial target repo settings." }, "allowed-repos": { - "type": "array", - "items": { - "type": "string" - }, - "description": "List of additional repositories in format 'owner/repo' that push to pull request branch can target. When specified, the agent can use a 'repo' field in the output to specify which repository to push to. The target repository (current or target-repo) is always implicitly allowed." + "description": "List of additional repositories in format 'owner/repo' that push to pull request branch can target. When specified, the agent can use a 'repo' field in the output to specify which repository to push to. The target repository (current or target-repo) is always implicitly allowed. Accepts an array or a GitHub Actions expression resolving to a comma-separated list (e.g. '${{ inputs[\\'allowed-repos\\'] }}').", + "oneOf": [ + { + "type": "array", + "description": "Array of repository slugs in 'owner/repo' format", + "items": { + "type": "string" + } + }, + { + "type": "string", + "pattern": "^\\$\\{\\{.*\\}\\}$", + "description": "GitHub Actions expression resolving to a comma-separated list of repository slugs (e.g. '${{ inputs[\\'allowed-repos\\'] }}')" + } + ] }, "protected-files": { "oneOf": [ @@ -7222,7 +7316,9 @@ }, "exclude": { "type": "array", - "items": { "type": "string" }, + "items": { + "type": "string" + }, "description": "List of filenames or path prefixes to remove from the default protected-file set. Items are matched by basename (e.g. \"AGENTS.md\") or path prefix (e.g. \".agents/\"). Use this to allow the agent to modify specific files that are otherwise blocked by default.", "examples": [["AGENTS.md"], ["AGENTS.md", ".agents/"]] } @@ -7238,7 +7334,7 @@ "items": { "type": "string" }, - "description": "Exclusive allowlist of glob patterns. When set, every file in the patch must match at least one pattern \u2014 files outside the list are always refused, including normal source files. This is a restriction, not an exception: setting allowed-files: [\".github/workflows/*\"] blocks all other files. To allow multiple sets of files, list all patterns explicitly. Acts independently of the protected-files policy; both checks must pass. To modify a protected file, it must both match allowed-files and be permitted by protected-files (e.g. protected-files: allowed). Supports * (any characters except /) and ** (any characters including /)." + "description": "Exclusive allowlist of glob patterns. When set, every file in the patch must match at least one pattern — files outside the list are always refused, including normal source files. This is a restriction, not an exception: setting allowed-files: [\".github/workflows/*\"] blocks all other files. To allow multiple sets of files, list all patterns explicitly. Acts independently of the protected-files policy; both checks must pass. To modify a protected file, it must both match allowed-files and be permitted by protected-files (e.g. protected-files: allowed). Supports * (any characters except /) and ** (any characters including /)." }, "excluded-files": { "type": "array", @@ -8244,7 +8340,7 @@ }, "scripts": { "type": "object", - "description": "Inline JavaScript script handlers that run inside the consolidated safe-outputs job handler loop. Unlike 'jobs' (which create separate GitHub Actions jobs), scripts execute in-process alongside the built-in handlers. Users write only the body of the main function \u2014 the compiler wraps it with 'async function main(config = {}) { ... }' and 'module.exports = { main };' automatically. Script names containing dashes will be automatically normalized to underscores (e.g., 'post-slack-message' becomes 'post_slack_message').", + "description": "Inline JavaScript script handlers that run inside the consolidated safe-outputs job handler loop. Unlike 'jobs' (which create separate GitHub Actions jobs), scripts execute in-process alongside the built-in handlers. Users write only the body of the main function — the compiler wraps it with 'async function main(config = {}) { ... }' and 'module.exports = { main };' automatically. Script names containing dashes will be automatically normalized to underscores (e.g., 'post-slack-message' becomes 'post_slack_message').", "patternProperties": { "^[a-zA-Z_][a-zA-Z0-9_-]*$": { "type": "object", @@ -8302,7 +8398,7 @@ }, "script": { "type": "string", - "description": "JavaScript handler body. Write only the code that runs inside the handler for each item \u2014 the compiler generates the full outer wrapper including config input destructuring (`const { channel, message } = config;`) and the handler function (`return async function handleX(item, resolvedTemporaryIds) { ... }`). The body has access to `item` (runtime message with input values), `resolvedTemporaryIds` (map of temporary IDs), and config-destructured local variables for each declared input." + "description": "JavaScript handler body. Write only the code that runs inside the handler for each item — the compiler generates the full outer wrapper including config input destructuring (`const { channel, message } = config;`) and the handler function (`return async function handleX(item, resolvedTemporaryIds) { ... }`). The body has access to `item` (runtime message with input values), `resolvedTemporaryIds` (map of temporary IDs), and config-destructured local variables for each declared input." } }, "required": ["script"], @@ -8337,8 +8433,8 @@ }, "staged-title": { "type": "string", - "description": "Custom title template for staged mode preview. Available placeholders: {operation}. Example: '\ud83c\udfad Preview: {operation}'", - "examples": ["\ud83c\udfad Preview: {operation}", "## Staged Mode: {operation}"] + "description": "Custom title template for staged mode preview. Available placeholders: {operation}. Example: '🎭 Preview: {operation}'", + "examples": ["🎭 Preview: {operation}", "## Staged Mode: {operation}"] }, "staged-description": { "type": "string", @@ -8352,18 +8448,18 @@ }, "run-success": { "type": "string", - "description": "Custom message template for successful workflow completion. Available placeholders: {workflow_name}, {run_url}. Default: '\u2705 Agentic [{workflow_name}]({run_url}) completed successfully.'", - "examples": ["\u2705 Agentic [{workflow_name}]({run_url}) completed successfully.", "\u2705 [{workflow_name}]({run_url}) finished."] + "description": "Custom message template for successful workflow completion. Available placeholders: {workflow_name}, {run_url}. Default: '✅ Agentic [{workflow_name}]({run_url}) completed successfully.'", + "examples": ["✅ Agentic [{workflow_name}]({run_url}) completed successfully.", "✅ [{workflow_name}]({run_url}) finished."] }, "run-failure": { "type": "string", - "description": "Custom message template for failed workflow. Available placeholders: {workflow_name}, {run_url}, {status}. Default: '\u274c Agentic [{workflow_name}]({run_url}) {status} and wasn't able to produce a result.'", - "examples": ["\u274c Agentic [{workflow_name}]({run_url}) {status} and wasn't able to produce a result.", "\u274c [{workflow_name}]({run_url}) {status}."] + "description": "Custom message template for failed workflow. Available placeholders: {workflow_name}, {run_url}, {status}. Default: '❌ Agentic [{workflow_name}]({run_url}) {status} and wasn't able to produce a result.'", + "examples": ["❌ Agentic [{workflow_name}]({run_url}) {status} and wasn't able to produce a result.", "❌ [{workflow_name}]({run_url}) {status}."] }, "detection-failure": { "type": "string", - "description": "Custom message template for detection job failure. Available placeholders: {workflow_name}, {run_url}. Default: '\u26a0\ufe0f Security scanning failed for [{workflow_name}]({run_url}). Review the logs for details.'", - "examples": ["\u26a0\ufe0f Security scanning failed for [{workflow_name}]({run_url}). Review the logs for details.", "\u26a0\ufe0f Detection job failed in [{workflow_name}]({run_url})."] + "description": "Custom message template for detection job failure. Available placeholders: {workflow_name}, {run_url}. Default: '⚠️ Security scanning failed for [{workflow_name}]({run_url}). Review the logs for details.'", + "examples": ["⚠️ Security scanning failed for [{workflow_name}]({run_url}). Review the logs for details.", "⚠️ Detection job failed in [{workflow_name}]({run_url})."] }, "agent-failure-issue": { "type": "string", @@ -9135,7 +9231,7 @@ }, "github-token": { "type": "string", - "description": "GitHub token expression to authenticate APM with private package repositories. Uses cascading fallback (GH_AW_PLUGINS_TOKEN \u2192 GH_AW_GITHUB_TOKEN \u2192 GITHUB_TOKEN) when not specified. Takes effect unless github-app is also configured (which takes precedence).", + "description": "GitHub token expression to authenticate APM with private package repositories. Uses cascading fallback (GH_AW_PLUGINS_TOKEN → GH_AW_GITHUB_TOKEN → GITHUB_TOKEN) when not specified. Takes effect unless github-app is also configured (which takes precedence).", "examples": ["${{ secrets.MY_TOKEN }}", "${{ secrets.GH_AW_GITHUB_TOKEN }}"] } }, @@ -9915,7 +10011,7 @@ }, "auth": { "type": "array", - "description": "Authentication bindings \u2014 maps logical roles (e.g. 'api-key') to GitHub Actions secret names", + "description": "Authentication bindings — maps logical roles (e.g. 'api-key') to GitHub Actions secret names", "items": { "type": "object", "properties": { diff --git a/pkg/workflow/add_comment.go b/pkg/workflow/add_comment.go index e2d4a998c55..adaecaa8fae 100644 --- a/pkg/workflow/add_comment.go +++ b/pkg/workflow/add_comment.go @@ -51,6 +51,12 @@ func (c *Compiler) parseCommentsConfig(outputMap map[string]any) *AddCommentsCon return nil } + // Pre-process list fields that also accept a GitHub Actions expression string. + if err := preprocessStringArrayFieldAsTemplatable(configData, "allowed-repos", addCommentLog); err != nil { + addCommentLog.Printf("Invalid allowed-repos value: %v", err) + return nil + } + config := parseConfigScaffold(outputMap, "add-comment", addCommentLog, func(err error) *AddCommentsConfig { addCommentLog.Printf("Failed to unmarshal config: %v", err) // For backward compatibility, handle nil/empty config diff --git a/pkg/workflow/compiler_safe_outputs_builder.go b/pkg/workflow/compiler_safe_outputs_builder.go index 1df303bc144..c2a5ac15981 100644 --- a/pkg/workflow/compiler_safe_outputs_builder.go +++ b/pkg/workflow/compiler_safe_outputs_builder.go @@ -40,6 +40,29 @@ func (b *handlerConfigBuilder) AddStringSlice(key string, value []string) *handl return b } +// AddTemplatableStringSlice adds a string slice field that may contain a GitHub Actions +// expression. When the slice has exactly one element and that element is a GitHub Actions +// expression (as produced by preprocessStringArrayFieldAsTemplatable or +// ParseStringArrayOrExprFromConfig), the expression string is stored as a plain JSON string +// rather than a JSON array. This allows GitHub Actions to evaluate the expression at +// runtime when the config.json file is written via heredoc expansion. +// +// For all other non-empty slices the field is stored as a JSON array, matching the +// behaviour of AddStringSlice. +func (b *handlerConfigBuilder) AddTemplatableStringSlice(key string, value []string) *handlerConfigBuilder { + if len(value) == 0 { + return b + } + // A single-element expression slice is the canonical representation produced by + // preprocessing – store as a string so GitHub Actions evaluates it at runtime. + if len(value) == 1 && isExpression(value[0]) { + b.config[key] = value[0] + return b + } + b.config[key] = value + return b +} + // AddBoolPtr adds a boolean pointer field only if the pointer is not nil func (b *handlerConfigBuilder) AddBoolPtr(key string, value *bool) *handlerConfigBuilder { if value != nil { diff --git a/pkg/workflow/compiler_safe_outputs_handlers.go b/pkg/workflow/compiler_safe_outputs_handlers.go index 65c39172bbd..e789ec78510 100644 --- a/pkg/workflow/compiler_safe_outputs_handlers.go +++ b/pkg/workflow/compiler_safe_outputs_handlers.go @@ -37,7 +37,7 @@ var handlerRegistry = map[string]handlerBuilder{ AddIfNotEmpty("target", c.Target). AddTemplatableBool("hide_older_comments", c.HideOlderComments). AddIfNotEmpty("target-repo", c.TargetRepoSlug). - AddStringSlice("allowed_repos", c.AllowedRepos). + AddTemplatableStringSlice("allowed_repos", c.AllowedRepos). AddIfNotEmpty("github-token", c.GitHubToken). AddTemplatableBool("footer", getEffectiveFooterForTemplatable(c.Footer, cfg.Footer)). AddIfTrue("staged", c.Staged). @@ -388,7 +388,7 @@ var handlerRegistry = map[string]handlerBuilder{ builder := newHandlerConfigBuilder(). AddTemplatableInt("max", c.Max). AddIfNotEmpty("title_prefix", c.TitlePrefix). - AddStringSlice("labels", c.Labels). + AddTemplatableStringSlice("labels", c.Labels). AddStringSlice("fallback_labels", c.FallbackLabels). AddStringSlice("reviewers", c.Reviewers). AddStringSlice("team_reviewers", c.TeamReviewers). @@ -399,8 +399,8 @@ var handlerRegistry = map[string]handlerBuilder{ AddTemplatableBool("auto_merge", c.AutoMerge). AddIfPositive("expires", c.Expires). AddIfNotEmpty("target-repo", c.TargetRepoSlug). - AddStringSlice("allowed_repos", c.AllowedRepos). - AddStringSlice("allowed_base_branches", c.AllowedBaseBranches). + AddTemplatableStringSlice("allowed_repos", c.AllowedRepos). + AddTemplatableStringSlice("allowed_base_branches", c.AllowedBaseBranches). AddDefault("max_patch_size", maxPatchSize). AddDefault("max_patch_files", maxPatchFiles). AddIfNotEmpty("github-token", c.GitHubToken). @@ -434,13 +434,13 @@ var handlerRegistry = map[string]handlerBuilder{ AddTemplatableInt("max", c.Max). AddIfNotEmpty("target", c.Target). AddIfNotEmpty("title_prefix", c.TitlePrefix). - AddStringSlice("labels", c.Labels). + AddTemplatableStringSlice("labels", c.Labels). AddIfNotEmpty("if_no_changes", c.IfNoChanges). AddIfTrue("ignore_missing_branch_failure", c.IgnoreMissingBranchFailure). AddIfNotEmpty("commit_title_suffix", c.CommitTitleSuffix). AddDefault("max_patch_size", maxPatchSize). AddIfNotEmpty("target-repo", c.TargetRepoSlug). - AddStringSlice("allowed_repos", c.AllowedRepos). + AddTemplatableStringSlice("allowed_repos", c.AllowedRepos). AddIfNotEmpty("github-token", c.GitHubToken). AddIfTrue("staged", c.Staged). AddStringPtr("protected_files_policy", c.ManifestFilesPolicy). diff --git a/pkg/workflow/config_helpers.go b/pkg/workflow/config_helpers.go index 2c1c8a9ffb4..6afb3d7adde 100644 --- a/pkg/workflow/config_helpers.go +++ b/pkg/workflow/config_helpers.go @@ -83,6 +83,46 @@ func extractStringSliceFromConfig(config map[string]any, key string) []string { return parseStringSliceAny(raw, nil) } +// ParseStringArrayOrExprFromConfig is like ParseStringArrayFromConfig but also accepts a +// GitHub Actions expression string as a valid value. When the raw value is an expression +// (starts with "${{" and ends with "}}") it is returned as []string{exprString} so that +// the handler config builder can later detect and re-emit it as a JSON string rather than +// a JSON array. +// +// Non-expression bare strings are treated as invalid and nil is returned. +func ParseStringArrayOrExprFromConfig(m map[string]any, key string, log *logger.Logger) []string { + if value, exists := m[key]; exists { + if log != nil { + log.Printf("Parsing %s from config", key) + } + // Accept a GitHub Actions expression string: wrap it in a single-element slice. + if s, ok := value.(string); ok { + if isExpression(s) { + if log != nil { + log.Printf("Field %s is a GitHub Actions expression, wrapping in single-element array", key) + } + return []string{s} + } + // Non-expression string is invalid for an array field. + if log != nil { + log.Printf("Field %q must be an array or a GitHub Actions expression, ignoring non-expression string: %q", key, s) + } + return nil + } + // Handle arrays (existing logic). + if strings := parseStringSliceAny(value, log); strings != nil { + if len(strings) == 0 && log != nil { + log.Printf("No valid %s strings found, returning empty array", key) + } + if log != nil { + log.Printf("Parsed %d %s from config", len(strings), key) + } + return strings + } + } + return nil +} + // parseLabelsFromConfig extracts and validates labels from a config map // Returns a slice of label strings, or nil if labels is not present or invalid func parseLabelsFromConfig(configMap map[string]any) []string { diff --git a/pkg/workflow/config_parsing_helpers_test.go b/pkg/workflow/config_parsing_helpers_test.go index 12450635c59..88c2f9e5a9d 100644 --- a/pkg/workflow/config_parsing_helpers_test.go +++ b/pkg/workflow/config_parsing_helpers_test.go @@ -4,6 +4,7 @@ package workflow import ( "maps" + "strings" "testing" ) @@ -935,3 +936,827 @@ func TestUnmarshalConfig(t *testing.T) { }) } } + +// TestParseStringArrayOrExprFromConfig verifies that the expression-aware array helper +// accepts GitHub Actions expression strings in addition to the usual array types. +func TestParseStringArrayOrExprFromConfig(t *testing.T) { + tests := []struct { + name string + input map[string]any + key string + expected []string + }{ + { + name: "nil map returns nil", + input: nil, + key: "labels", + expected: nil, + }, + { + name: "missing key returns nil", + input: map[string]any{}, + key: "labels", + expected: nil, + }, + { + name: "valid []string array", + input: map[string]any{ + "labels": []string{"bug", "enhancement"}, + }, + key: "labels", + expected: []string{"bug", "enhancement"}, + }, + { + name: "valid []any array", + input: map[string]any{ + "labels": []any{"bug", "enhancement"}, + }, + key: "labels", + expected: []string{"bug", "enhancement"}, + }, + { + name: "empty array returns empty slice", + input: map[string]any{ + "labels": []string{}, + }, + key: "labels", + expected: []string{}, + }, + { + name: "GitHub Actions expression string wrapped in single-element slice", + input: map[string]any{ + "labels": "${{ inputs['required-labels'] }}", + }, + key: "labels", + expected: []string{"${{ inputs['required-labels'] }}"}, + }, + { + name: "expression with extra whitespace is still valid", + input: map[string]any{ + "allowed-repos": "${{ inputs['allowed-repos'] }}", + }, + key: "allowed-repos", + expected: []string{"${{ inputs['allowed-repos'] }}"}, + }, + { + name: "non-expression bare string returns nil", + input: map[string]any{ + "labels": "not-an-expression", + }, + key: "labels", + expected: nil, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := ParseStringArrayOrExprFromConfig(tt.input, tt.key, nil) + if tt.expected == nil { + if result != nil { + t.Errorf("expected nil, got %v", result) + } + return + } + if result == nil { + t.Errorf("expected %v, got nil", tt.expected) + return + } + if len(result) != len(tt.expected) { + t.Errorf("expected length %d, got %d: %v", len(tt.expected), len(result), result) + return + } + for i, v := range tt.expected { + if result[i] != v { + t.Errorf("element[%d]: expected %q, got %q", i, v, result[i]) + } + } + }) + } +} + +// TestPreprocessStringArrayFieldAsTemplatable verifies that the expression-aware +// preprocessor wraps expression strings and rejects non-expression strings. +func TestPreprocessStringArrayFieldAsTemplatable(t *testing.T) { + tests := []struct { + name string + configData map[string]any + fieldName string + wantErr bool + wantErrContains []string // substrings the error message must contain + wantWrapped bool // true when the value should be wrapped in []string{expr} + }{ + { + name: "nil configData is a no-op", + configData: nil, + fieldName: "labels", + wantErr: false, + wantWrapped: false, + }, + { + name: "missing field is a no-op", + configData: map[string]any{}, + fieldName: "labels", + wantErr: false, + wantWrapped: false, + }, + { + name: "array value is left unchanged", + configData: map[string]any{ + "labels": []string{"bug"}, + }, + fieldName: "labels", + wantErr: false, + wantWrapped: false, + }, + { + name: "expression string is wrapped", + configData: map[string]any{ + "labels": "${{ inputs.labels }}", + }, + fieldName: "labels", + wantErr: false, + wantWrapped: true, + }, + { + name: "non-expression string returns error with actionable message", + configData: map[string]any{ + "labels": "automation", + }, + fieldName: "labels", + wantErr: true, + wantErrContains: []string{"labels", "array", "expression"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := preprocessStringArrayFieldAsTemplatable(tt.configData, tt.fieldName, nil) + if tt.wantErr { + if err == nil { + t.Error("expected error, got nil") + return + } + for _, substr := range tt.wantErrContains { + if !strings.Contains(err.Error(), substr) { + t.Errorf("error message %q does not contain expected substring %q", err.Error(), substr) + } + } + return + } + if err != nil { + t.Errorf("unexpected error: %v", err) + return + } + if tt.wantWrapped { + val, exists := tt.configData[tt.fieldName] + if !exists { + t.Errorf("field %q disappeared from configData", tt.fieldName) + return + } + wrapped, ok := val.([]string) + if !ok { + t.Errorf("expected []string, got %T: %v", val, val) + return + } + if len(wrapped) != 1 { + t.Errorf("expected single-element slice, got len=%d: %v", len(wrapped), wrapped) + return + } + if !isExpression(wrapped[0]) { + t.Errorf("expected expression in wrapped slice, got %q", wrapped[0]) + } + } + }) + } +} + +// TestAddTemplatableStringSliceBuilder verifies the handler config builder method. +func TestAddTemplatableStringSliceBuilder(t *testing.T) { + tests := []struct { + name string + value []string + wantKey bool + wantAsArray bool // true if stored as []string; false if stored as string + wantValue string // expected string when stored as expression + }{ + { + name: "nil slice omits key", + value: nil, + wantKey: false, + }, + { + name: "empty slice omits key", + value: []string{}, + wantKey: false, + }, + { + name: "literal slice stored as array", + value: []string{"bug", "enhancement"}, + wantKey: true, + wantAsArray: true, + }, + { + name: "expression single-element stored as string", + value: []string{"${{ inputs.labels }}"}, + wantKey: true, + wantAsArray: false, + wantValue: "${{ inputs.labels }}", + }, + { + name: "multi-element slice with expression-like string stored as array", + value: []string{"${{ inputs.labels }}", "bug"}, + wantKey: true, + wantAsArray: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + b := newHandlerConfigBuilder() + b.AddTemplatableStringSlice("field", tt.value) + cfg := b.Build() + + if !tt.wantKey { + if _, exists := cfg["field"]; exists { + t.Errorf("expected key to be absent, but it was present: %v", cfg["field"]) + } + return + } + + val, exists := cfg["field"] + if !exists { + t.Error("expected key to be present, but it was absent") + return + } + + if tt.wantAsArray { + arr, ok := val.([]string) + if !ok { + t.Errorf("expected []string, got %T: %v", val, val) + return + } + if len(arr) != len(tt.value) { + t.Errorf("array length mismatch: expected %d, got %d", len(tt.value), len(arr)) + } + } else { + s, ok := val.(string) + if !ok { + t.Errorf("expected string (expression), got %T: %v", val, val) + return + } + if s != tt.wantValue { + t.Errorf("expression value: expected %q, got %q", tt.wantValue, s) + } + } + }) + } +} + +// TestParsePullRequestsConfigExpressionFields verifies that create-pull-request +// accepts GitHub Actions expression strings for labels, allowed-repos, and +// allowed-base-branches. +func TestParsePullRequestsConfigExpressionFields(t *testing.T) { + tests := []struct { + name string + field string + expr string + getField func(*CreatePullRequestsConfig) []string + }{ + { + name: "labels as expression", + field: "labels", + expr: "${{ inputs.labels }}", + getField: func(c *CreatePullRequestsConfig) []string { return c.Labels }, + }, + { + name: "allowed-repos as expression", + field: "allowed-repos", + expr: "${{ inputs['allowed-repos'] }}", + getField: func(c *CreatePullRequestsConfig) []string { return c.AllowedRepos }, + }, + { + name: "allowed-base-branches as expression", + field: "allowed-base-branches", + expr: "${{ inputs['allowed-base-branches'] }}", + getField: func(c *CreatePullRequestsConfig) []string { return c.AllowedBaseBranches }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + compiler := &Compiler{} + outputMap := map[string]any{ + "create-pull-request": map[string]any{ + tt.field: tt.expr, + }, + } + + result := compiler.parsePullRequestsConfig(outputMap) + if result == nil { + t.Fatal("expected non-nil result") + } + + got := tt.getField(result) + if len(got) != 1 { + t.Fatalf("expected single-element slice, got %v", got) + } + if got[0] != tt.expr { + t.Errorf("expected expression %q, got %q", tt.expr, got[0]) + } + }) + } +} + +// TestParseAddCommentConfigExpressionFields verifies that add-comment accepts a +// GitHub Actions expression string for allowed-repos. +func TestParseAddCommentConfigExpressionFields(t *testing.T) { + compiler := &Compiler{} + expr := "${{ inputs['allowed-repos'] }}" + outputMap := map[string]any{ + "add-comment": map[string]any{ + "allowed-repos": expr, + }, + } + + result := compiler.parseCommentsConfig(outputMap) + if result == nil { + t.Fatal("expected non-nil result") + } + + if len(result.AllowedRepos) != 1 { + t.Fatalf("expected single-element AllowedRepos, got %v", result.AllowedRepos) + } + if result.AllowedRepos[0] != expr { + t.Errorf("expected expression %q, got %q", expr, result.AllowedRepos[0]) + } +} + +// TestParsePushToPullRequestBranchExpressionFields verifies that +// push-to-pull-request-branch accepts GitHub Actions expression strings for +// labels and allowed-repos. +func TestParsePushToPullRequestBranchExpressionFields(t *testing.T) { + tests := []struct { + name string + field string + expr string + getField func(*PushToPullRequestBranchConfig) []string + }{ + { + name: "labels as expression", + field: "labels", + expr: "${{ inputs['required-labels'] }}", + getField: func(c *PushToPullRequestBranchConfig) []string { return c.Labels }, + }, + { + name: "allowed-repos as expression", + field: "allowed-repos", + expr: "${{ inputs['allowed-repos'] }}", + getField: func(c *PushToPullRequestBranchConfig) []string { return c.AllowedRepos }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + compiler := &Compiler{} + outputMap := map[string]any{ + "push-to-pull-request-branch": map[string]any{ + tt.field: tt.expr, + }, + } + + result := compiler.parsePushToPullRequestBranchConfig(outputMap) + if result == nil { + t.Fatal("expected non-nil result") + } + + got := tt.getField(result) + if len(got) != 1 { + t.Fatalf("expected single-element slice, got %v", got) + } + if got[0] != tt.expr { + t.Errorf("expected expression %q, got %q", tt.expr, got[0]) + } + }) + } +} + +// TestHandlerConfigExpressionFields verifies that the handler config builder +// emits expression strings as JSON strings (not arrays) when a single-element +// expression slice is provided. +func TestHandlerConfigExpressionFields(t *testing.T) { + tests := []struct { + name string + safeOuts *SafeOutputsConfig + handler string + configKey string + wantValue string // expected string in config; empty means wantArray=true + wantArray bool + }{ + { + name: "create_pull_request labels expression stored as string", + safeOuts: &SafeOutputsConfig{ + CreatePullRequests: &CreatePullRequestsConfig{ + Labels: []string{"${{ inputs.labels }}"}, + }, + }, + handler: "create_pull_request", + configKey: "labels", + wantValue: "${{ inputs.labels }}", + }, + { + name: "create_pull_request labels literal stored as array", + safeOuts: &SafeOutputsConfig{ + CreatePullRequests: &CreatePullRequestsConfig{ + Labels: []string{"bug", "enhancement"}, + }, + }, + handler: "create_pull_request", + configKey: "labels", + wantArray: true, + }, + { + name: "create_pull_request allowed_repos expression stored as string", + safeOuts: &SafeOutputsConfig{ + CreatePullRequests: &CreatePullRequestsConfig{ + AllowedRepos: []string{"${{ inputs['allowed-repos'] }}"}, + }, + }, + handler: "create_pull_request", + configKey: "allowed_repos", + wantValue: "${{ inputs['allowed-repos'] }}", + }, + { + name: "create_pull_request allowed_base_branches expression stored as string", + safeOuts: &SafeOutputsConfig{ + CreatePullRequests: &CreatePullRequestsConfig{ + AllowedBaseBranches: []string{"${{ inputs['allowed-base-branches'] }}"}, + }, + }, + handler: "create_pull_request", + configKey: "allowed_base_branches", + wantValue: "${{ inputs['allowed-base-branches'] }}", + }, + { + name: "add_comment allowed_repos expression stored as string", + safeOuts: &SafeOutputsConfig{ + AddComments: &AddCommentsConfig{ + AllowedRepos: []string{"${{ inputs['allowed-repos'] }}"}, + }, + }, + handler: "add_comment", + configKey: "allowed_repos", + wantValue: "${{ inputs['allowed-repos'] }}", + }, + { + name: "push_to_pull_request_branch labels expression stored as string", + safeOuts: &SafeOutputsConfig{ + PushToPullRequestBranch: &PushToPullRequestBranchConfig{ + Labels: []string{"${{ inputs['required-labels'] }}"}, + }, + }, + handler: "push_to_pull_request_branch", + configKey: "labels", + wantValue: "${{ inputs['required-labels'] }}", + }, + { + name: "push_to_pull_request_branch allowed_repos expression stored as string", + safeOuts: &SafeOutputsConfig{ + PushToPullRequestBranch: &PushToPullRequestBranchConfig{ + AllowedRepos: []string{"${{ inputs['allowed-repos'] }}"}, + }, + }, + handler: "push_to_pull_request_branch", + configKey: "allowed_repos", + wantValue: "${{ inputs['allowed-repos'] }}", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + builder, exists := handlerRegistry[tt.handler] + if !exists { + t.Fatalf("handler %q not found in registry", tt.handler) + } + + cfg := builder(tt.safeOuts) + if cfg == nil { + t.Fatal("handler returned nil config") + } + + val, exists := cfg[tt.configKey] + if !exists { + t.Fatalf("key %q not found in handler config; config: %v", tt.configKey, cfg) + } + + if tt.wantArray { + _, ok := val.([]string) + if !ok { + t.Errorf("expected []string, got %T: %v", val, val) + } + } else { + s, ok := val.(string) + if !ok { + t.Errorf("expected string (expression), got %T: %v", val, val) + return + } + if s != tt.wantValue { + t.Errorf("expected %q, got %q", tt.wantValue, s) + } + } + }) + } +} + +// TestExpressionFieldsRejectedForInvalidStrings verifies that non-expression bare strings +// are rejected (return nil / return error) rather than silently accepted. +func TestExpressionFieldsRejectedForInvalidStrings(t *testing.T) { + // create-pull-request: non-expression bare string returns nil config + compiler := &Compiler{} + + for _, field := range []string{"labels", "allowed-repos", "allowed-base-branches"} { + t.Run("create-pull-request/"+field+"/non-expression", func(t *testing.T) { + outputMap := map[string]any{ + "create-pull-request": map[string]any{ + field: "not-an-expression", + }, + } + result := compiler.parsePullRequestsConfig(outputMap) + // Must return nil (invalid input is rejected) + if result != nil { + t.Errorf("expected nil result for invalid bare string in %q, got %+v", field, result) + } + }) + } + + // add-comment: non-expression bare string returns nil config + t.Run("add-comment/allowed-repos/non-expression", func(t *testing.T) { + outputMap := map[string]any{ + "add-comment": map[string]any{ + "allowed-repos": "not-an-expression", + }, + } + result := compiler.parseCommentsConfig(outputMap) + if result != nil { + t.Errorf("expected nil result for invalid bare string in allowed-repos, got %+v", result) + } + }) +} + +// TestAllListEncodingForms verifies all supported list encodings for the templatable +// string-array safe-output fields, covering both compile-time (literal arrays) and +// runtime (expression strings) forms. +// +// Compile-time forms (supported by the Go parser): +// - Literal array with multiple items +// - Literal array with a single item +// - Empty literal array (→ omitted from handler config) +// - GitHub Actions expression string (bracket notation required for names with hyphens) +// +// Runtime forms (resolved by GitHub Actions before config.json is written, then parsed +// by JS handlers using comma-splitting): +// - The expression resolves to a comma-separated string: "bug,enhancement" +// - The expression resolves to a single value: "bug" +// - The expression resolves to a value with spaces: "bug, enhancement" +// +// This test covers the Go compile-time pipeline only. The JS runtime forms are +// covered by the existing JS handler tests (parseAllowedRepos, parseStringListConfig, +// parseAllowedBaseBranches). +func TestAllListEncodingForms(t *testing.T) { + type listFieldCase struct { + name string + value any // raw value as it would appear in the config map + wantSlice []string // expected []string in the parsed config struct; nil = field absent + } + + // Helper: run a single field case through parsePullRequestsConfig.labels and + // verify the Labels field. + runPRLabelsCase := func(t *testing.T, tc listFieldCase) { + t.Helper() + compiler := &Compiler{} + outputMap := map[string]any{ + "create-pull-request": map[string]any{ + "labels": tc.value, + }, + } + result := compiler.parsePullRequestsConfig(outputMap) + if tc.wantSlice == nil { + // Invalid input — parser should reject (nil config) + if result != nil && len(result.Labels) > 0 { + t.Errorf("expected empty/absent Labels, got %v", result.Labels) + } + return + } + if result == nil { + t.Fatal("expected non-nil config") + } + if len(result.Labels) != len(tc.wantSlice) { + t.Fatalf("Labels length: expected %d, got %d (%v)", len(tc.wantSlice), len(result.Labels), result.Labels) + } + for i, v := range tc.wantSlice { + if result.Labels[i] != v { + t.Errorf("Labels[%d]: expected %q, got %q", i, v, result.Labels[i]) + } + } + } + + // Helper: run a case through the handler builder and check the JSON output. + runHandlerLabelsCase := func(t *testing.T, tc listFieldCase, wantKey bool, wantArray bool, wantExprValue string) { + t.Helper() + labels, ok := tc.value.([]string) + if !ok { + t.Skip("handler test requires []string input") + } + builder, exists := handlerRegistry["create_pull_request"] + if !exists { + t.Fatal("create_pull_request handler not found") + } + cfg := builder(&SafeOutputsConfig{ + CreatePullRequests: &CreatePullRequestsConfig{Labels: labels}, + }) + if cfg == nil { + t.Fatal("handler returned nil config") + } + val, exists := cfg["labels"] + if !wantKey { + if exists { + t.Errorf("expected 'labels' key absent, got %v", val) + } + return + } + if !exists { + t.Errorf("expected 'labels' key present, but absent") + return + } + if wantArray { + if _, ok := val.([]string); !ok { + t.Errorf("expected []string, got %T: %v", val, val) + } + } else { + s, ok := val.(string) + if !ok { + t.Errorf("expected string (expression), got %T: %v", val, val) + return + } + if s != wantExprValue { + t.Errorf("expected expression %q, got %q", wantExprValue, s) + } + } + } + + t.Run("parser/literal_multi_item_array", func(t *testing.T) { + runPRLabelsCase(t, listFieldCase{ + value: []any{"bug", "enhancement"}, + wantSlice: []string{"bug", "enhancement"}, + }) + }) + + t.Run("parser/literal_single_item_array", func(t *testing.T) { + runPRLabelsCase(t, listFieldCase{ + value: []any{"automation"}, + wantSlice: []string{"automation"}, + }) + }) + + t.Run("parser/literal_empty_array", func(t *testing.T) { + runPRLabelsCase(t, listFieldCase{ + value: []any{}, + wantSlice: []string{}, + }) + }) + + t.Run("parser/expression_with_bracket_notation", func(t *testing.T) { + expr := "${{ inputs['required-labels'] }}" + runPRLabelsCase(t, listFieldCase{ + value: expr, + wantSlice: []string{expr}, + }) + }) + + t.Run("parser/expression_vars_reference", func(t *testing.T) { + expr := "${{ vars.PR_LABELS }}" + runPRLabelsCase(t, listFieldCase{ + value: expr, + wantSlice: []string{expr}, + }) + }) + + t.Run("parser/expression_with_fallback_operator", func(t *testing.T) { + expr := "${{ inputs.labels || 'automation' }}" + runPRLabelsCase(t, listFieldCase{ + value: expr, + wantSlice: []string{expr}, + }) + }) + + t.Run("parser/expression_env_reference", func(t *testing.T) { + expr := "${{ env.DEFAULT_LABELS }}" + runPRLabelsCase(t, listFieldCase{ + value: expr, + wantSlice: []string{expr}, + }) + }) + + // Handler builder forms — verify JSON config output. + t.Run("builder/multi_item_array_stored_as_array", func(t *testing.T) { + runHandlerLabelsCase(t, listFieldCase{value: []string{"bug", "enhancement"}}, true, true, "") + }) + + t.Run("builder/single_item_literal_stored_as_array", func(t *testing.T) { + runHandlerLabelsCase(t, listFieldCase{value: []string{"automation"}}, true, true, "") + }) + + t.Run("builder/empty_array_omits_key", func(t *testing.T) { + runHandlerLabelsCase(t, listFieldCase{value: []string{}}, false, false, "") + }) + + t.Run("builder/expression_string_stored_as_string", func(t *testing.T) { + expr := "${{ inputs['required-labels'] }}" + runHandlerLabelsCase(t, listFieldCase{value: []string{expr}}, true, false, expr) + }) + + t.Run("builder/vars_expression_stored_as_string", func(t *testing.T) { + expr := "${{ vars.PR_LABELS }}" + runHandlerLabelsCase(t, listFieldCase{value: []string{expr}}, true, false, expr) + }) + + // ParseStringArrayOrExprFromConfig — all resolved string forms. + // These represent what the expression might resolve to at runtime (passed through + // the Go helper when config is read from a pre-expanded config.json at startup time). + t.Run("parseHelper/comma_separated_string_not_accepted_at_compile_time", func(t *testing.T) { + // A comma-separated bare string is rejected at compile time (not an expression). + // At runtime the expression resolves before Go code runs, so this case never + // occurs in practice — but we document it explicitly. + result := ParseStringArrayOrExprFromConfig(map[string]any{ + "labels": "automation,bot", + }, "labels", nil) + if result != nil { + t.Errorf("expected nil for non-expression bare string, got %v", result) + } + }) + + t.Run("parseHelper/expression_string_accepted", func(t *testing.T) { + expr := "${{ inputs.labels }}" + result := ParseStringArrayOrExprFromConfig(map[string]any{ + "labels": expr, + }, "labels", nil) + if len(result) != 1 || result[0] != expr { + t.Errorf("expected [%q], got %v", expr, result) + } + }) + + t.Run("parseHelper/string_array_all_forms", func(t *testing.T) { + cases := []struct { + name string + value any + wantLen int + wantVals []string + }{ + {"single-string-array", []string{"automation"}, 1, []string{"automation"}}, + {"multi-string-array", []string{"automation", "bot"}, 2, []string{"automation", "bot"}}, + {"any-array", []any{"automation", "bot"}, 2, []string{"automation", "bot"}}, + {"empty-array", []string{}, 0, []string{}}, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + result := ParseStringArrayOrExprFromConfig(map[string]any{"labels": c.value}, "labels", nil) + if len(result) != c.wantLen { + t.Fatalf("expected len %d, got %d: %v", c.wantLen, len(result), result) + } + for i, v := range c.wantVals { + if result[i] != v { + t.Errorf("[%d]: expected %q, got %q", i, v, result[i]) + } + } + }) + } + }) + + // Error message content for hyphenated vs non-hyphenated field names. + t.Run("errorMessage/hyphenated_field_uses_bracket_notation", func(t *testing.T) { + err := preprocessStringArrayFieldAsTemplatable( + map[string]any{"allowed-repos": "not-an-expr"}, + "allowed-repos", nil, + ) + if err == nil { + t.Fatal("expected error, got nil") + } + msg := err.Error() + if !strings.Contains(msg, "inputs['allowed-repos']") { + t.Errorf("error message should use bracket notation, got: %q", msg) + } + }) + + t.Run("errorMessage/non_hyphenated_field_uses_dot_notation", func(t *testing.T) { + err := preprocessStringArrayFieldAsTemplatable( + map[string]any{"labels": "not-an-expr"}, + "labels", nil, + ) + if err == nil { + t.Fatal("expected error, got nil") + } + msg := err.Error() + if !strings.Contains(msg, "inputs.labels") { + t.Errorf("error message should use dot notation, got: %q", msg) + } + }) +} diff --git a/pkg/workflow/create_pull_request.go b/pkg/workflow/create_pull_request.go index 4823fe04cc0..dab0bbf430c 100644 --- a/pkg/workflow/create_pull_request.go +++ b/pkg/workflow/create_pull_request.go @@ -128,6 +128,17 @@ func (c *Compiler) parsePullRequestsConfig(outputMap map[string]any) *CreatePull return nil } + // Pre-process list fields that also accept a GitHub Actions expression string. + // An expression is wrapped in a single-element []string so the []string struct field + // can receive it after YAML unmarshaling; the handler config builder later re-emits it + // as a JSON string for runtime evaluation. + for _, field := range []string{"labels", "allowed-repos", "allowed-base-branches"} { + if err := preprocessStringArrayFieldAsTemplatable(configData, field, createPRLog); err != nil { + createPRLog.Printf("Invalid %s value: %v", field, err) + return nil + } + } + config := parseConfigScaffold(outputMap, "create-pull-request", createPRLog, func(err error) *CreatePullRequestsConfig { createPRLog.Printf("Failed to unmarshal config: %v", err) // For backward compatibility, handle nil/empty config diff --git a/pkg/workflow/push_to_pull_request_branch.go b/pkg/workflow/push_to_pull_request_branch.go index 5003973f1ea..bc58b6b50cf 100644 --- a/pkg/workflow/push_to_pull_request_branch.go +++ b/pkg/workflow/push_to_pull_request_branch.go @@ -126,8 +126,8 @@ func (c *Compiler) parsePushToPullRequestBranchConfig(outputMap map[string]any) // Parse title-prefix using shared helper pushToBranchConfig.TitlePrefix = parseTitlePrefixFromConfig(configMap) - // Parse labels using shared helper - pushToBranchConfig.Labels = parseLabelsFromConfig(configMap) + // Parse labels using expression-aware shared helper + pushToBranchConfig.Labels = ParseStringArrayOrExprFromConfig(configMap, "labels", pushToPullRequestBranchLog) // Parse commit-title-suffix (optional) if commitTitleSuffix, exists := configMap["commit-title-suffix"]; exists { @@ -147,8 +147,8 @@ func (c *Compiler) parsePushToPullRequestBranchConfig(outputMap map[string]any) // Parse target-repo for cross-repository push pushToBranchConfig.TargetRepoSlug = parseTargetRepoFromConfig(configMap) - // Parse allowed-repos for cross-repository push - pushToBranchConfig.AllowedRepos = parseAllowedReposFromConfig(configMap) + // Parse allowed-repos for cross-repository push (expression-aware) + pushToBranchConfig.AllowedRepos = ParseStringArrayOrExprFromConfig(configMap, "allowed-repos", pushToPullRequestBranchLog) // Parse protected-files: supports string enum OR object form {policy, exclude}. exclude := preprocessProtectedFilesField(configMap, pushToPullRequestBranchLog) diff --git a/pkg/workflow/templatables.go b/pkg/workflow/templatables.go index 450f64797e0..3ac05512f56 100644 --- a/pkg/workflow/templatables.go +++ b/pkg/workflow/templatables.go @@ -36,6 +36,7 @@ import ( "encoding/json" "fmt" "strconv" + "strings" "github.com/github/gh-aw/pkg/logger" ) @@ -253,6 +254,47 @@ func preprocessIntFieldAsString(configData map[string]any, fieldName string, log return nil } +// preprocessStringArrayFieldAsTemplatable handles a string-array config field that also +// accepts a GitHub Actions expression string (e.g. "${{ inputs.labels }}"). +// +// When the field value is an expression string it is wrapped in a single-element []string +// so that existing YAML struct-unmarshal code (which expects []string) continues to work +// unchanged. The handler config builder then detects this single-element expression slice +// and stores it as a JSON string rather than a JSON array, allowing GitHub Actions to +// evaluate the expression at runtime before the config.json file is written. +// +// Free-form strings that are not GitHub Actions expressions are rejected with an error. +// Array values ([]string, []any) are left untouched for the normal YAML unmarshal path. +func preprocessStringArrayFieldAsTemplatable(configData map[string]any, fieldName string, log *logger.Logger) error { + if configData == nil { + return nil + } + if val, exists := configData[fieldName]; exists { + if s, ok := val.(string); ok { + if !isExpression(s) { + // Build an example expression that is syntactically valid for fieldNames + // containing hyphens: dot-notation (e.g. inputs.foo) is invalid for those, + // so use bracket notation (e.g. inputs['foo']) instead. + var exampleExpr string + if strings.Contains(fieldName, "-") { + exampleExpr = fmt.Sprintf("${{ inputs['%s'] }}", fieldName) + } else { + exampleExpr = fmt.Sprintf("${{ inputs.%s }}", fieldName) + } + return fmt.Errorf("field %q must be an array of strings or a GitHub Actions expression (e.g. '%s'), got string %q", fieldName, exampleExpr, s) + } + // Wrap the expression in a single-element slice so the []string struct field + // can receive it after YAML marshaling/unmarshaling. + configData[fieldName] = []string{s} + if log != nil { + log.Printf("Wrapped %s expression string in single-element array before unmarshaling", fieldName) + } + } + // Arrays ([]string, []any) are left unchanged for YAML unmarshal to handle. + } + return nil +} + // buildTemplatableIntEnvVar returns a YAML environment variable entry for a // templatable integer field. If value is a GitHub Actions expression it is // embedded unquoted so that GitHub Actions can evaluate it at runtime;